BUG: 383059
For the inline context buttons, use the standard QToolButton widget which respects theming so that icons are always visible
Details
Details
- Reviewers
vtasoulas broulik rkflx - Group Reviewers
KDE Applications - Commits
- R260:b09b216fe7b4: Use standard QToolButtons so that their icons use the right colors
Tested in KDE Neon with Breeze light and dark, Oxygen, and all default color schemes. Big improvement.
Current with Breeze:
Patched version with Breeze:
Patched version with Breeze Dark:
Diff Detail
Diff Detail
- Repository
- R260 Gwenview
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Nice work. I feel it needs just a little bit more work:
- now using "Breeze Dark" as color scheme results in nearly invisible icons
- in fullscreen (!= slideshow) mode, the problem still exists (except for the "+"-icon, which already inverts its color correctly)
I wonder if we should just display a standard icon-only toolbutton, as the color inversion should be already solved for that. Alternatively, as Breeze icons are pretty much monochrome compared to Oxygen icons, we could implement support for color scheme dependent icon colors. Not sure if that is what's been done for the "+"-icon? (Did not look at the code yet.)
Comment Actions
@mart Is there a way to tamper into KIconLoader's coloring, so we can override the color palette used for icons in this case?
Comment Actions
Yes, a much better solution would be to use real button widgets instead of drawing custom buttons that don't respect any theming rules. I'll see if I can do that.
Comment Actions
This latest change switches to using plain old QToolButtons, which are smart enough to change the color appropriately with dark themes. There is one remaining issue: with a light theme, the rotate button icons are inappropriately black in Full Screen mode when using a light theme:
Interestingly enough, they are correctly white in Full Screen mode with a dark theme:
Not sure if that's a bug in Gwenview from trying to be too smart by managing color inversion itself in Full Screen, or something with KIconLoader.
If anyone has any ideas on that, I'm all ears. Gwenview's styling has been a challenge for me to wrap my mind around.
Comment Actions
Thanks, much improved. I also tested with the Oxygen widget style and Oxygen icons, this works like it should, too.
In my book this counts as a bugfix, so please rebase this on the Applications/17.08 branch (and merge to master, once accepted and landed), if you agree. Let's see if we can solve the issue in full screen mode in time for 17.08.2. If not, we could commit as is and do the rest in a followup patch later.
@broulik: Do you know whether we guarantee source compatibility for gwenview/lib/*? If not, we could try to remove the ContextBarButton right now. Otherwise we should add a note to contextbarbutton.h to remove the class for a Qt6-based Gwenview.
| lib/thumbnailview/previewitemdelegate.cpp | ||
|---|---|---|
| 613 | d->mToggleSelectionButton = new QToolButton; ← Do it like this for every occurrence. | |
| 642 | QToolButton(d->mView->viewport()), otherwise the save button gets its own window when rotating an image (you have not tested this properly, do you? :) | |
| 643 | Left-over? | |
Comment Actions
Re-based on the Applications 17.08 branch and added warnings to the contaxtbarbutton files.
Comment Actions
I've re-based the diff from the 17.08 branch, added a warning into the contextbarbutton files, addressed your comments, and tested everything (including the save button this time :) ).
| lib/thumbnailview/previewitemdelegate.cpp | ||
|---|---|---|
| 642 | Oops. how embarrassing. I tested other things... but not this. Fixing. | |
Comment Actions
Yay, I solved the last remaining problem!
The hint was already in the "+"-icon: This one gets updated (manually by us via setIcon) in fullscreen mode and therefore works, the others aren't updated and therefore don't work. The root cause is QAbstractButton, which just holds onto a pixmap and has no way of knowing KIconLoader would return a different icon if the stars align differently (here: the colour palette changed). Thus, to work around this bug, we need to:
- factor out the existing calls to setIcon(...) into a (single) separate function in PreviewItemDelegatePrivate
- call this function when initializing in ::PreviewItemDelegate
- call this function when switching modes (not sure where yet, I tested updateImageButtons but this is definitely called too often and thus not the right place – please find something more appropriate)
That's just a rough sketch, but you can give it a try. If you get stuck, I can try it myself in a separate followup patch.
Comment Actions
Great sleuthing, Henrik! However, I'm afraid that implementing your idea is a little beyond my current C++ abilities. Would you mind doing it in a follow-up patch, and then I can see how you did it for my own future reference?
Comment Actions
No problem, will do (but probably not today anymore).
Just had a final look at the patch, there's just one small thing I discovered, please fix it and then you should be ready to go.
| lib/thumbnailview/thumbnailbarview.cpp | ||
|---|---|---|
| 83 | I missed this earlier and have no idea what's the consequence of leaving mView->viewport() out, but I think it's better to just keep / re-add it like above. | |
Comment Actions
Done. Thanks again for all your help. Will land this on the 17.08 branch and merge to master.
Comment Actions
I've got an arc question. I'm doing arc land --onto Applications/17.08 --squash --preview, but it shows the following:
These commits will be landed:
- ba51498 Address comments and resolve improper save button placement
- 6eaa478 Use standard QToolButtons so that their icons use the right colors
Shouldn't this go in as a single commit? What did I do wrong slash how do I squash them together properly? --squash didn't seem to have any effect.
Comment Actions
You can probably leave out --onto... if you are already on the right branch, and --squash should be the default and thus unneeded, too. Read arc help land again and you'll see that --preview stops before squashing, you probably want --hold.
Comment Actions
Uh oh, that's wrong (sorry, never had to use it before). Just keep it then ;)
Edit: To be more precise, you need --onto if you are on arcpatch-... which is based on Applications/..., you do not need it if you are directly on Applications/.... Always learning something…
Comment Actions
The followup patch is in D8031.
I found a much more elegant way to do this, and it even fixes the original problem. However, it is also reverting your patch. Please don't feel discouraged by that, your contributions are appreciated very much. It's just that sometimes a better solution only emerges if one works on something for a while for oneself. After all, it was me who accepted your patch. Without you picking up the bug, this would still sit in my things-to-fix folder today. Thanks again, sorry for not noticing the better solution earlier and I hope you can understand!