Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Change colour of Tk toolbar icons on dark backgrounds#22163
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The icons now check the colours for all possible states of the button, and I've added a unit test to keep codecov happy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uh oh!
There was an error while loading.Please reload this page.
daniilS commentedJan 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The Tkcheckbutton does allow setting a different image for the selected state using Also, can I ask if you're using any library to set the colours in your example, or are you setting them yourself? |
timhoffm commentedJan 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes.
You mean the sceenshot? That's just the standard KDE "breeze dark" theme. |
Alright, that should be easy to do. Will update soon.
Ah, I've never used KDE myself so not familiar with how Tk reads its theme files. Out of interest, how does a regular |
You mean from:
Seems it doesn't play well with the theme either. |
daniilS commentedJan 10, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That's exactly what I meant, thanks! In that case since the theme doesn't change the foreground colour for the |
daniilS commentedJan 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@timhoffm this should now cover all relevant cases, could you confirm that it also works with your themes please? Should also be more readable, at the cost of storing both images even when not needed. |
Of course, Test should hopefully be fixed now. |
@daniilS there's a difference in the selected buttons depending on mouse hover. To keep it simple I'd use the same image for hovered and unhovered. It's important that both are readable. It's not so important to indicate the hover state. |
@timhoffm Tk lets you specify the background colour for the hover state, but only differentiates between images depending on whether the button is selected or not, so the same image is already being used. I think the actual issue might be that on X11, Tk has somespecial logic for the selected state, rather than using |
daniilS commentedJan 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The background colour of the hover state is set by the theme, not this PR. Looks like the colours of the icons are now working on X11 too then. I've squashed the commit with the X11 fix, so this is ready for review/merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Modulo adding a comment for clarification.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
de2005b
toff60b28
CompareRebased and updated for the new test test scheme (twice). |
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Thanks@tacaswell, anything else I need to do on this or good to go now? |
@daniilS If you could confirm that I did not break this while rebasing I'll go ahead and merge it! |
@tacaswell all working so should be good to merge! |
PR Summary
Closes#22150
Threshold taken from
matplotlib/lib/matplotlib/backends/backend_qt.py
Line 694 inbf3c651
matplotlib/lib/matplotlib/backends/backend_wx.py
Line 1128 inbf3c651