Skip to content

Make TableView rendering consistent with what Qt native does#84

Open
dunkelstern wants to merge 2 commits into
oclero:devfrom
dunkelstern:j.schriewer/tableview-rendering
Open

Make TableView rendering consistent with what Qt native does#84
dunkelstern wants to merge 2 commits into
oclero:devfrom
dunkelstern:j.schriewer/tableview-rendering

Conversation

@dunkelstern
Copy link
Copy Markdown

This Pull request changes a few things on rendering ItemViews and HeaderSections:

  1. The rendering code respects the Qt::BackgroundRole background color role for table view cells. It falls back to qlementine's own method if it cannot pull a QStyleOptionViewItem from the optItem.
  2. Size calculation uses the fontMetrics of the QStyleOption. This means it honors Qt::FontRole of the item model and makes auto-sizing of table cells and headers possible again.

I am not sure if the changed size calculation breaks something, I for one could not see anything strange happening.

Why the change? It allows something like this:
image

@sonarqubecloud
Copy link
Copy Markdown

@oclero
Copy link
Copy Markdown
Owner

oclero commented Jan 22, 2025

Thanks, I'll have a you at your PR :)

const auto active = getActiveState(itemState);
const auto& color = listItemBackgroundColor(mouse, selection, focus, active, optItem->index, w);
p->fillRect(rect, color);
if (const QStyleOptionViewItem *vopt = qstyleoption_cast<const QStyleOptionViewItem*>(opt)) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use auto as much as possible 😊

Suggested change
if (const QStyleOptionViewItem *vopt = qstyleoption_cast<const QStyleOptionViewItem*>(opt)) {
if (const auto *vopt = qstyleoption_cast<const QStyleOptionViewItem*>(opt)) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, why do you cast again opt? It's already made a few lines above?
So the code in the else statement will never be called... So you broke the styling made by Qlementine.

Comment on lines +867 to +868
if (cg == QPalette::Normal && !(vopt->state & QStyle::State_Active))
cg = QPalette::Inactive;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use brackets 😊

Suggested change
if (cg == QPalette::Normal && !(vopt->state & QStyle::State_Active))
cg = QPalette::Inactive;
if (cg == QPalette::Normal && !(vopt->state & QStyle::State_Active)) {
cg = QPalette::Inactive;
}

@oclero
Copy link
Copy Markdown
Owner

oclero commented Feb 18, 2025

I made some small code style remarks. I'll build your branch later and check if anything is broken visually or not. And if it works, I'll merge it :) thanks again!

@oclero
Copy link
Copy Markdown
Owner

oclero commented Feb 19, 2025

I investigated what you did and I have some interrogations.
The code that calls QlementineStyle::listItemBackgroundColor will never be called. If you want to use the QPalette to draw the background, please adapt your code to how Qlementine does things.

@oclero
Copy link
Copy Markdown
Owner

oclero commented Feb 19, 2025

Also, please follow the lib's coding style, use clang-format to format the files. (You may run scripts/format.sh)

@dunkelstern
Copy link
Copy Markdown
Author

Will update the PR in the next days, thanks for the feedback :)

@oclero oclero self-requested a review June 19, 2025 16:31
@oclero
Copy link
Copy Markdown
Owner

oclero commented Jun 19, 2025

Hello @dunkelstern any news?

@oclero
Copy link
Copy Markdown
Owner

oclero commented Jan 12, 2026

@dunkelstern any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants