Skip to content

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

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

daniilS
Copy link
Contributor

@daniilS daniilS commented Jan 8, 2022

PR Summary

Closes #22150

Threshold taken from

if self.palette().color(self.backgroundRole()).value() < 128:
Icon colouring taken from
black_mask = (image[..., :3] == 0).all(axis=-1)

@daniilS
Copy link
Contributor Author

daniilS commented Jan 9, 2022

The icons now check the colours for all possible states of the button, and I've added a unit test to keep codecov happy.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good.

The only issue is still with toggle buttons which have a light-grey icon on a white background:
grafik

@daniilS
Copy link
Contributor Author

daniilS commented Jan 9, 2022

The Tk checkbutton does allow setting a different image for the selected state using -selectimage, but I don't think it supports different text colours for the selected state, so I assumed whatever is set as the text colour must work for both states. Should I just default to the black icon if selectcolor isn't dark then?

Also, can I ask if you're using any library to set the colours in your example, or are you setting them yourself?

@daniilS daniilS force-pushed the tk_toolbar_colours branch from 6b53724 to 3f88047 Compare January 9, 2022 22:29
@timhoffm
Copy link
Member

timhoffm commented Jan 9, 2022

Should I just default to the black icon if selectcolor isn't dark then?

Yes.

Also, can I ask if you're using any library to set the colours in your example, or are you setting them yourself?

You mean the sceenshot?

That's just the standard KDE "breeze dark" theme.

@daniilS
Copy link
Contributor Author

daniilS commented Jan 9, 2022

Yes.

Alright, that should be easy to do. Will update soon.

That's just the standard KDE "breeze dark" theme.

Ah, I've never used KDE myself so not familiar with how Tk reads its theme files. Out of interest, how does a regular tk.Checkbutton(master, indicatoron=False, text="sometext") look in the default and selected states? If it does somehow set a different foreground colour for the selected state, it should be possible to retrieve and apply that to the icon.

@timhoffm
Copy link
Member

You mean from:

import tkinter as tk
 
app = tk.Tk() 
app.geometry('150x100')

bt1 = tk.Checkbutton(app, text='Check Box', indicatoron=False) 
bt1.grid(column=0, row=0)

bt2 = tk.Checkbutton(app, text='Check Box', indicatoron=False) 
bt2.grid(column=0, row=1)

app.mainloop()

grafik

Seems it doesn't play well with the theme either.

@daniilS
Copy link
Contributor Author

daniilS commented Jan 10, 2022

That's exactly what I meant, thanks! In that case since the theme doesn't change the foreground colour for the selected state, it should be okay to just use the default icon colour if the background is light.

@daniilS daniilS force-pushed the tk_toolbar_colours branch from 3f88047 to 2b03be1 Compare January 11, 2022 23:22
@daniilS daniilS requested a review from timhoffm January 11, 2022 23:23
@daniilS
Copy link
Contributor Author

daniilS commented Jan 11, 2022

@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.

@daniilS daniilS force-pushed the tk_toolbar_colours branch from 2b03be1 to c4c7152 Compare January 11, 2022 23:59
@daniilS
Copy link
Contributor Author

daniilS commented Jan 12, 2022

Of course, ToolbarTk calls _Button with itself as an argument, so all new functions either have to be nested or called explicitly from the class...

Test should hopefully be fixed now.

@timhoffm
Copy link
Member

@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.

Unselected:
grafik

Selected with mouse hover:
grafik

Selected without mouse hover:
grafik

@daniilS
Copy link
Contributor Author

daniilS commented Jan 17, 2022

@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 selectcolor directly. Could you try the latest commit to see if that fixes it? (On Windows, I can't reproduce your screenshots: )
Unselected:
unselected
Unselected, hover/click:
unselected-active
Selected:
selected
Selected, hover/click:
selected-active

@timhoffm
Copy link
Member

Unselected:
grafik

Unselected, hover/click:
grafik

Selected:
grafik

Selected, hover/click:
grafik

There's now hover-change for unselected, but that's ok by me. It's all readable.

@daniilS daniilS force-pushed the tk_toolbar_colours branch from 3cc9172 to 68b3479 Compare January 19, 2022 22:34
@daniilS
Copy link
Contributor Author

daniilS commented Jan 19, 2022

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.

Copy link
Member

@timhoffm timhoffm left a 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.

@daniilS daniilS force-pushed the tk_toolbar_colours branch from 68b3479 to d8d4b03 Compare January 20, 2022 22:34
@tacaswell tacaswell force-pushed the tk_toolbar_colours branch from d8d4b03 to dc9818d Compare March 23, 2022 04:35
@tacaswell tacaswell added this to the v3.6.0 milestone Mar 23, 2022
@tacaswell tacaswell force-pushed the tk_toolbar_colours branch 2 times, most recently from de2005b to ff60b28 Compare March 23, 2022 23:03
@tacaswell
Copy link
Member

Rebased and updated for the new test test scheme (twice).

Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@daniilS
Copy link
Contributor Author

daniilS commented Mar 25, 2022

Thanks @tacaswell, anything else I need to do on this or good to go now?

@tacaswell
Copy link
Member

@daniilS If you could confirm that I did not break this while rebasing I'll go ahead and merge it!

@daniilS
Copy link
Contributor Author

daniilS commented Mar 26, 2022

@tacaswell all working so should be good to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: Tool icons are hardly visible in Tk when using a dark theme
6 participants