-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1e4f9e1
to
d79cdeb
Compare
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.
The Tk checkbutton 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? |
6b53724
to
3f88047
Compare
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. |
That's exactly what I meant, thanks! In that case since the theme doesn't change the foreground colour for the |
3f88047
to
2b03be1
Compare
@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. |
2b03be1
to
c4c7152
Compare
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 some special logic for the selected state, rather than using |
3cc9172
to
68b3479
Compare
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.
68b3479
to
d8d4b03
Compare
d8d4b03
to
dc9818d
Compare
de2005b
to
ff60b28
Compare
Rebased 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 in bf3c651
matplotlib/lib/matplotlib/backends/backend_wx.py
Line 1128 in bf3c651