Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TableView rendering consistent with what Qt native does #84

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

dunkelstern
Copy link

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

@oclero
Copy link
Owner

oclero commented Jan 22, 2025

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

@@ -861,8 +861,31 @@ void QlementineStyle::drawPrimitive(PrimitiveElement pe, const QStyleOption* opt
const auto focus =
widgetHasFocus && selection == SelectionState::Selected ? FocusState::Focused : FocusState::NotFocused;
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
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
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
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
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
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
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
Author

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

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