Thanks for the patch! Before-and-after screenshots are always appreciated with a subtle visual change like this. I'm having a hard time telling the difference after I apply your patch. And have you tested with other color themes to make sure it still looks good?

In D10564#207461, @ngraham wrote:

Thanks for the patch! Before-and-after screenshots are always appreciated with a subtle visual change like this. I'm having a hard time telling the difference after I apply your patch. And have you tested with other color themes to make sure it still looks good?

Whoops, have added screenshots. Still a bit hard to see, perhaps I'll make a gif...
EDIT: Added video which I believe shows it clearly

I've tested with Breeze and Breeze Dark. Gwenview seems to use a (fixed) dark palette for these areas anyway, so it is unaffected by the system theme.

Wow, now that's what I was talking about!

However, I almost wonder if the new effect goes far enough. It's still pretty subtle. Perhaps we could follow Dolphin's lead here and have the whole image slightly lighten on hover, and then just continue to use the standard highlight color for the background on the selected item.

Perhaps we could follow Dolphin's lead here and have the whole image slightly lighten on hover, and then just continue to use the standard highlight color for the background on the selected item.

Hmm, if we do that I think we'd need to make that consistent everywhere (in Browse as well). Alternatively, in Browse hover actually changes the border color as well, which helps. Perhaps we could copy that?
To be honest, I wouldn't be a fan of the dolphin-style. I can imagine getting annoyed when trying to study the thumbnails and they keep getting brigher on hover.

It was just a thought; I'm not wedded to that.

Looking at this again, I'm not a fan of how the normal view css looks when applied to the fullscreen thumbnail bar.

I'm going to rethink this...

Updated. The fullscreen bar looks better now, although I notice when I set the stylesheet, the scrollbar background gets lighter and I don't know why...

In D10564#207464, @huoni wrote:

Gwenview seems to use a (fixed) dark palette for these areas anyway, so it is unaffected by the system theme.

Are you sure? Selecting "Honeycomb", there's quite a difference.

In D10564#207490, @ngraham wrote:

Perhaps we could follow Dolphin's lead here and have the whole image slightly lighten on hover, and then just continue to use the standard highlight color for the background on the selected item.

I'm with @huoni here, as in Gwenview the accurate display of the image is somewhat important ;) In other situations this effect would be nice, of course.

@huoni There is something off with your previous patch which I noticed only now. The left edge is more pronounced than the right edge (bottom bar):

the scrollbar background gets lighter

Not sure where this happens, but maybe you change too much somewhere: Look at the hover buttons (top bar), those are the light variant for me now (tip: don't use Breeze dark when working on this), and also the background behind the thumbnails suddenly is white. When reviewing the other patch, this would happen (in a different place, though) when I set NormalPalette instead of NormalViewPalette.

(BTW: Staring at all 4 cursors in the summary moving at once but slightly out of sync is quite addictive, I must say ;)

In D10564#208023, @rkflx wrote:

In D10564#207464, @huoni wrote:

Gwenview seems to use a (fixed) dark palette for these areas anyway, so it is unaffected by the system theme.

Are you sure? Selecting "Honeycomb", there's quite a difference.

I'm a bit confused. If I change to Breeze (non-dark), the image background in View is still dark. Similarly, the Thumbnail Bar is dark.
Fullscreen in View is also dark themed on normal Breeze theme.

If I select the Honeycomb color scheme, the background is still dark in both areas.

In D10564#207490, @ngraham wrote:

Perhaps we could follow Dolphin's lead here and have the whole image slightly lighten on hover, and then just continue to use the standard highlight color for the background on the selected item.

I'm with @huoni here, as in Gwenview the accurate display of the image is somewhat important ;) In other situations this effect would be nice, of course.

---

@huoni There is something off with your previous patch which I noticed only now. The left edge is more pronounced than the right edge (bottom bar):

I copied the existing border styles for the hover effect. I will try without setting the border style at all.

the scrollbar background gets lighter

Not sure where this happens, but maybe you change too much somewhere: Look at the hover buttons (top bar), those are the light variant for me now (tip: don't use Breeze dark when working on this), and also the background behind the thumbnails suddenly is white. When reviewing the other patch, this would happen (in a different place, though) when I set NormalPalette instead of NormalViewPalette.

I evidently don't understand how this works. Setting the stylesheet seems to change at least the button to light theme for some reason.

No change, it's all dark themed:

Setting palette only, still dark

QPalette pal = mGvCore->palette(GvCore::FullScreenViewPalette);
mThumbnailBar->setPalette(pal);

Setting stylesheet but no palette - white??

QString style = 
    "QListView::item:hover {"
    "  background-color: red;"
    "}";
mThumbnailBar->setStyleSheet(style);

Setting stylesheet with palette - buttons still white?

QPalette pal = mGvCore->palette(GvCore::FullScreenViewPalette);
mThumbnailBar->setPalette(pal);
QString style = 
    "QListView::item:hover {"
    "  background-color: red;"
    "}";
mThumbnailBar->setStyleSheet(style);

And this is only with the top bar, not the bottom bar, and I can't see how they are different, except the top bar is instantiated with a parent.

---

(BTW: Staring at all 4 cursors in the summary moving at once but slightly out of sync is quite addictive, I must say ;)

It's quite hypnotising now that I look at it :)

It seems to me, setting the stylesheet resets the existing theme/palette, then applies the styles.

So mainwindow.cpp:1276 is setting everything to a dark theme when entering fullscreen. Setting the stylesheet of the bar must then reset it's palette/theme/whatever, and then apply the styles.

What doesn't make sense to me is why manually setting the palette to e.g. FullScreenPalette doesn't fix this, and the context buttons stay white. Obviously I don't understand how the palettes work.

I've had somewhat of a breakthrough (sort of). Through experimentation, it appears calling setStyleSheet() on a widget 'saves' the current application palette in some capacity along with the styles. This means that subsequent changes to the application palette do not update the widget styles, resulting in the white button in my last example above.
Furthermore, calling setStyleSheet() again after the application palette has changed will update the widget style based on the new palette.

I can't figure out exactly what's happening here, but I can 'fix' the problem by putting:

mThumbnailBar->setStyleSheet(mThumbnailBar->styleSheet());

in FullScreenContent::setFullScreenMode

@rkflx do you have any insight?

In D10564#208036, @huoni wrote:

In D10564#208023, @rkflx wrote:

In D10564#207464, @huoni wrote:

Gwenview seems to use a (fixed) dark palette for these areas anyway, so it is unaffected by the system theme.

Are you sure? Selecting "Honeycomb", there's quite a difference.

I'm a bit confused. If I change to Breeze (non-dark), the image background in View is still dark. Similarly, the Thumbnail Bar is dark.
Fullscreen in View is also dark themed on normal Breeze theme.

The colour of the thumbnail grid seems to be the same everywhere, in every mode and independent from the colour scheme. Users can set it in Configure Gwenview → General → Background Colour.

If I select the Honeycomb color scheme, the background is still dark in both areas.

Did you restart Gwenview (to recompute colours) after changing the colour theme? For me it looks like this for the thumbnail strip:

Anyway, I did not detect a problem yet. Still, it makes sense to keep different colour themes in mind when testing.

I evidently don't understand how this works.

I'm sure you'll figure it out eventually, just take your time and read the docs ;) And always consider the possibility that there's a bug somewhere else… At this point you may know more than I do about all those palette/style code.

@rkflx do you have any insight?

Not yet, but I lack the time to debug it in depth ATM. I'll look at it if you are totally stuck, but it seems you are making progress ;)

In D10564#208056, @rkflx wrote:

Did you restart Gwenview (to recompute colours) after changing the colour theme? For me it looks like this for the thumbnail strip:

BTW, I think it's a bug that some parts get the "Honeycomb" treatment (i.e. everything coming from Breeze widget style), while some parts are still blueish (i.e. everything where Gwenview invented its own widget style, like the thumbnail hover frames in the thumbnail grid).

Anyway, that would be material for a different patch. Mentioning it only to emphasize that indeed Gwenview should respect the user's colour theme if possible.

In D10564#208054, @huoni wrote:

• 'Fix' thumbnail bar colors with light system color scheme
• Remove hover border styling on bottom bar

Wow, that's great. For me it works perfectly now.

I'll try to understand and review the code over the weekend, but so far the behaviour seems fine.

In D10564#208058, @rkflx wrote:

In D10564#208054, @huoni wrote:

• 'Fix' thumbnail bar colors with light system color scheme
• Remove hover border styling on bottom bar

Wow, that's great. For me it works perfectly now.

I'll try to understand and review the code over the weekend, but so far the behaviour seems fine.

Thanks! Much appreciated.

I'm not convinced my fix is the right way to do it, but I'll wait for your thoughts on the code.

Note: the hover on the bottom bar doesn't adjust when changing the background color (doesn't suit white). Will work on making this dynamic.

Visually, this is much nicer now! +1.

Looks good in even more situations now, and the code is also much more straightforward. Only some minor things.

In D10564#209171, @rkflx wrote:

Yay! I'll commit this tomorrow (unless any other reviewer still finds a problem).

Awesome, thanks.

Next patch: Colour-theme dependent hues for the fullscreen Browse thumbnail grid frames? Just joking, of course you can pick anything you want if you like it here…

I'm on it! Keep the suggestions coming, makes it easy to find the next challenge :)

Looks great to me. A clear improvement. Thanks for your patches @huoni, and @rkflx, for your 100% amazing code reviews.

In D10564#209191, @ngraham wrote:

and @rkflx, for your 100% amazing code reviews.

Wholeheartedly agree!

Committing this to master only for now, because it seems like a (slight) change in behaviour to me and it's only really important once you turn off the selection hover buttons (which is possible in master only anyway).

@huoni I'll review all of your patches over the weekend (unless you add even more ;)

In D10564#209180, @huoni wrote:

Keep the suggestions coming, makes it easy to find the next challenge :)

Here are more ideas (you heard them here first until I find time to file them properly on Bugzilla…), but I guess in the meantime you may have found enough material to work on by yourself ;)

• Headers like File Time: in Sidebar → Information → Meta Information sometimes are hardly readable. This happens either when starting Gwenview normally and switching to fullscreen, or when starting in fullscreen (gwenview -f) and switching to normal mode.
• When starting Gwenview in Browse mode, to the left and to the right of the URL bar there is a small margin so it does not sit flush with the dark background below. After pressing F11, the problem is gone. Use GammaRay's widget picker to identify what's happening.
• In View mode, open the Thumbnail Bar (Bug does not trigger for closing), press F11 and Ctrl+Q. After a restart, Gwenview has forgotten the Thumbnail Bar state.
• Opening the context menu for an image in Browse mode works only after the second click.
• Apply https://phabricator.kde.org/D10329 also to the folders sidebar.
• Very challenging: Finish HiDPI work (https://phabricator.kde.org/D7581 and https://phabricator.kde.org/D9078).

One more thing: It has been only two weeks so I probably shouldn't mention this yet, but if you like it here (KDE) and plan to keep submitting patches regularly over the coming months, you could think about applying for commit access.

In D10564#212376, @rkflx wrote:

@huoni I'll review all of your patches over the weekend (unless you add even more ;)

Much appreciated!

In D10564#209180, @huoni wrote:

Keep the suggestions coming, makes it easy to find the next challenge :)

Here are more ideas (you heard them here first until I find time to file them properly on Bugzilla…), but I guess in the meantime you may have found enough material to work on by yourself ;)

• Headers like File Time: in Sidebar → Information → Meta Information sometimes are hardly readable. This happens either when starting Gwenview normally and switching to fullscreen, or when starting in fullscreen (gwenview -f) and switching to normal mode.
• When starting Gwenview in Browse mode, to the left and to the right of the URL bar there is a small margin so it does not sit flush with the dark background below. After pressing F11, the problem is gone. Use GammaRay's widget picker to identify what's happening.
• In View mode, open the Thumbnail Bar (Bug does not trigger for closing), press F11 and Ctrl+Q. After a restart, Gwenview has forgotten the Thumbnail Bar state.
• Opening the context menu for an image in Browse mode works only after the second click.
• Apply https://phabricator.kde.org/D10329 also to the folders sidebar.
• Very challenging: Finish HiDPI work (https://phabricator.kde.org/D7581 and https://phabricator.kde.org/D9078).

These should keep me occupied for a while, thanks!

GammaRay's widget picker

Wish I knew about this sooner! Sounds extremely useful.

---

One more thing: It has been only two weeks so I probably shouldn't mention this yet, but if you like it here (KDE) and plan to keep submitting patches regularly over the coming months, you could think about applying for commit access.

I'll definitely consider it. I have lots of free time at the moment, but that may change soon, so I don't want to apply for an account then stop (or drastically reduce) contributing a few weeks later.
Might revisit this in a few weeks :)

In D10564#212376, @rkflx wrote:

• Headers like File Time: in Sidebar → Information → Meta Information sometimes are hardly readable. This happens either when starting Gwenview normally and switching to fullscreen, or when starting in fullscreen (gwenview -f) and switching to normal mode.

Been looking into this, and I can see the problem, but unsure of the best fix.

Problem

Those headings, like many places, have a custom palette set where the text alpha is modified. Therefore when the palette is changed (going to fullscreen and back), the palette isn't update.
Now, whenever the sidebar is toggled, AND when you switch between pages, these headings along with the rest of the content are cleared and recreated, fixing the issue.
So the issue is only present when the sidebar is open, and one switches to/from fullscreen.

Solution

I think the simplest solution is to find a way to update the contents when switching to/from fullscreen. Since this update happens every time you switch to the Information tab anyway, I don't think this is a waste of resources. (Rather than separating out the palette code and building some 'update palette' methods/signals/slots.)

But the issue with this solution is due to the structure of the code. Somehow we need to get from MainWindow::toggleFullScreen to InfoContextManagerItem::updateSideBarContent, which isn't simple, and would I believe require some mechanism similar to the selectionChanged signal.
On the other hand, we can simply call selectionChanged on mContextManager and achieve the result we want, but it's a bit hacky.

So either:

1. Build some generic mechanism for MainWindow to be able to manually update sidebar content
2. Just pretend the selection was changed to achieve the same result

What do you think? Hopefully I'm making sense.

In D10564#212376, @rkflx wrote:

• Opening the context menu for an image in Browse mode works only after the second click.

I can't replicate this :(

• Apply https://phabricator.kde.org/D10329 also to the folders sidebar.

Will work on this

• Very challenging: Finish HiDPI work (https://phabricator.kde.org/D7581 and https://phabricator.kde.org/D9078).

I think I'll leave this one for the experts :)

Let's use Gwenview's workboard instead of discussing in the comments of a random Diff ;)

T8123: Context menu and sidebar sometimes populated incorrectly
T8124: Unreadable headers in information sidebar