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

Fix highlighted piece color #20971

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jagannatharjun
Copy link
Member

@jagannatharjun jagannatharjun commented Jun 18, 2024

Much better contrast with download piece and selected pieces. Previously same color was used for both

New Behaviour

capture-2024-06-18.11-02PM.mp4
capture-2024-06-18.11-08PM.mp4

@glassez glassez requested a review from a team June 18, 2024 18:11
@glassez glassez added the Look and feel Affect UI "Look and feel" only without changing the logic label Jun 18, 2024
@thalieht
Copy link
Contributor

I don't have the best monitor so don't rely on my opinion.
Tested only in KDE with light and dark breeze themes, IMO it looks better in dark and not better or worse in light:
Up is master and down is this PR:

light1
light

dark1
dark

thalieht
thalieht previously approved these changes Jun 18, 2024
@xavier2k6
Copy link
Member

Haven't tested yet but how does this behave in light/dark "app" mode on Windows when "accent" color is changed?

@jagannatharjun
Copy link
Member Author

Haven't tested yet but how does this behave in light/dark "app" mode on Windows when "accent" color is changed?

Piece colour is always based on the highlight color of QPalatte, which is always constant on Windows and not related to accent. I prefer we keep the scope limited to largest use case i.e default settings, you can't test or make this work on all colors. The attached screenshot in PR description is with default light/dark mode on Windows.

@xavier2k6
Copy link
Member

Piece colour is always based on the highlight color of QPalatte, which is always constant on Windows and not related to accent.

Not completely accurate anymore as of Qt 6.5+ "dark mode"

When we read the dark system palette on Windows, then we use the theme's accent color for the QPalette::Highlight color role. This is however not quite correct, as the Accent is used in other places as well, and some controls use different colors (i.e. on Windows 11's "red" dark theme, the highlight is bright red, while the focus frames are in a paler orange).

https://www.qt.io/blog/dark-mode-on-windows-11-with-qt-6.5

That's why I asked, in any case - this PR does appear to enhance the contrast.

xavier2k6
xavier2k6 previously approved these changes Jun 19, 2024
@jagannatharjun
Copy link
Member Author

jagannatharjun commented Jun 20, 2024

Not completely accurate anymore as of Qt 6.5+ "dark mode"

QPalette::Acent is different from QPalette:Highlight, which is (always been) used here, The screenshot attached in PR description is with QT 6.5 dark/light mode (oopsie I reread your comment but slowly 😅)

@Chocobo1 Chocobo1 requested a review from glassez August 11, 2024 08:10
@Chocobo1
Copy link
Member

@jagannatharjun
FYI it has merge conflicts. It could have once more with #21183.

@glassez
Copy link
Member

glassez commented Aug 11, 2024

Looks good for me 👍

@jagannatharjun
Copy link
Member Author

rebased

@glassez glassez requested a review from a team August 27, 2024 11:17
@glassez glassez added this to the 5.0 milestone Aug 27, 2024
@xavier2k6
Copy link
Member

re-tested! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Look and feel Affect UI "Look and feel" only without changing the logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants