@rkflx - This does match your description for bug fixes that should be based off the stable branch, however I felt it goes with D10564: Improve selection and hover colors in the thumbnail bars which fixes essentially the same problem for the Image View fullscreen thumbnails.

Clearly an improvement, although there are still many blue elements to find. I'll make a list and open a task on Gwenview's workboard once I find the time, so anybody who enjoys fixing a colour scheme bug once in a while can pick an item.

For this patch, my only wish besides the inline comments would be to make the zoom slider stand out a bit less, and perhaps slightly tone down the hover effect you get for overly long filenames. Note I tested with Honeycomb. Would something like this help?

Ideally, every theme would provide a dark variant on its own, but as this is not the case, Gwenview kind of has to invent its own dark theme variant each, by using a dark background and modifying the highlight colours slightly. So far this was hardcoded to blue, now my hope is to make this more flexible.

In D10649#209286, @huoni wrote:

@rkflx - This does match your description for bug fixes that should be based off the stable branch

After D10651#209593 turned into a larger discussion, let me elaborate a bit:

Whether a given change is strictly a bug fix or already a change in behaviour is often a difficult decision to make. For stable releases, I assume users expect no surprises at all, i.e. no regressions, no new crashes, things should work just as before, no workflow adaptations are necessary and only actual problems are fixed. Here we have both: For some users it might improve visual consistency, other might find the change unexpected, and rare cases might even have weird colours where it now looks worse than before. I tend to err on the side of caution.

Helpful heuristics: Are there bug reports or user complaints? If not, it might be a feature and not a bug. What's the risk? How grave is the problem? What does the user-level documentation promise, and what is the application doing for real? Using git blame, what was the intention of the original author?

In the end, it is often the maintainer who has to decide, and if there is no maintainer like in Gwenview, ideally a consensus should be reached among the reviewers.

Any reason you change it only here?

You're right @rkflx - I've now changed it at the source! :)

I'd just like to point out, that the hover color for the fullscreen URL navigator in Browse for some reason uses the Inactive version of the Highlight role.
It was this arguably incorrect use of the color groups that made me think I need to change the colors for all color groups.

This is what I'm referring to:

Thanks, will look at your updates later. At least they sound very promising ;)

In D10649#210589, @huoni wrote:

In D10501#207202, @rkflx wrote:

Oh noes, another item uncovered from my secret list of Gwenview improvements! Just joking, send a patch ;) This has been bothering me for a long time.

Haha, you start to find the obvious things. I guess this has to be fixed in KUrlNavigator, because you can observe the same in Dolphin. Just make sure to search Bugzilla beforehand (I'm not sure whether this might be intentional, but I doubt it).

Edit: In Dolphin, compare the breadcrumb hover colour to the places sidebar hover colour, I guess they should look the same (currently the breadcrumb colour is too light).

In D10649#210591, @rkflx wrote:

Haha, you start to find the obvious things. I guess this has to be fixed in KUrlNavigator, because you can observe the same in Dolphin. Just make sure to search Bugzilla beforehand (I'm not sure whether this might be intentional, but I doubt it).

It could be intentional if none of the Normal (or Active) group colors were suitable. Instead of modifying an existing color, they chose to use an existing color from a different group?

Sorry for the delay, getting a bit swamped with new patches these days…

First reaction: Impressive work! I could cross out all items but one (titlebar of dialog windows, e.g. Rename) on my list of blue elements I found in your first iteration, and also the toned down accent colours are quite nice.

I'll test more in depth in a bit and poke at the code.

Unrelated to your patch: In Fullscreen View mode, the bottom bar of Crop has an incorrect background colour.

In D10649#211908, @rkflx wrote:

Sorry for the delay, getting a bit swamped with new patches these days…

Not a problem!

First reaction: Impressive work! I could cross out all items but one (titlebar of dialog windows, e.g. Rename) on my list of blue elements I found in your first iteration, and also the toned down accent colours are quite nice.

This is why we have reviewers. I have titlebars disabled!

This actually has nothing to do with the palettes. We are modifying the palette, but not the color scheme. The following line in mainwindow.cpp:1315 sets the app color scheme to the blue one, so KWin uses that.

qApp->setProperty("KDE_COLOR_SCHEME_PATH", d->mGvCore->fullScreenPaletteName());

If we comment out this line, KWin uses the normal theme, however since it hasn't been adjusted like the palettes, it's super bright yellow (in the case of Honeycomb).

The only solution I can think of is instead of modifying the palette after it's been created from the color scheme, modify the color scheme directly (then create the fullscreen palette from it). We'd probably need to save the modified scheme somewhere (/var?) in order to set KDE_COLOR_SCHEME_PATH.
However after reading the KColorScheme API, I don't see any way of converting a QPalette back into a color scheme, so this would maybe require text parsing, which doesn't seem like a good idea to me.

As I see it, we have three options:

1. Leave it as is with blue titlebars
2. Use the correct colors but not adjusted therefore bright
3. Try to modify the color scheme somehow and save it as a .color file

Thoughts?

Unrelated to your patch: In Fullscreen View mode, the bottom bar of Crop has an incorrect background colour.

I'm not sure what Crop refers to. Can't see anything called Crop in Fullscreen View.

In D10649#211937, @huoni wrote:

I'm not sure what Crop refers to. Can't see anything called Crop in Fullscreen View.

Press F4 and switch to the Operations tab, or use the ⇧+C shortcut for Crop. (A bit complicated, I guess that's why no one fixed this yet…)

In D10649#211938, @rkflx wrote:

In D10649#211937, @huoni wrote:

I'm not sure what Crop refers to. Can't see anything called Crop in Fullscreen View.

Press F4 and switch to the Operations tab, or use the ⇧+C shortcut for Crop. (A bit complicated, I guess that's why no one fixed this yet…)

It only took me 7 hours to figure out, but it's fixed. With one line of course :).
D10763: Fix background color for image view toolbar (crop/red-eye)

Reviews done now, took slightly longer because there was one extra patch ;) Did not get around to look into the titebar colour issue yet (thanks for the investigation!), but let's not overload the patch with that.

This is a remarkable improvement, thanks so much again!

As for the test plan: It's not strictly necessary in this case, but perhaps @ngraham will appreciate some screenshots for his blog, maybe before/after for fullscreen honeycomb?

Yes, screenshots definitely increase the likelihood of being featured. :)

It's not strictly necessary in this case, but perhaps @ngraham will appreciate some screenshots for his blog, maybe before/after for fullscreen honeycomb?

Yes, screenshots definitely increase the likelihood of being featured. :)

I've added screenshots, but it's only really noticeable in Browse fullscreen, since D10564: Improve selection and hover colors in the thumbnail bars fixed most of it for View already.

EDIT: Added better screenshots to Test Plan. @rkflx - is that where I should put them? I would have assumed just in the summary.

Whoops, I missed the inline comments. I have addressed them now.

but take your time as I've got enough other reviews to work on ;)

My bad, can't help myself!