Thanks for choosing Gwenview for your first contribution ;) You even used Arcanist to upload the patch, which is great! If you have any questions or encounter any problems, just ask. Note we also have https://phabricator.kde.org/T7116 if you want to provide any feedback in that direction.

Turns out we even have a wishlist item for the feature your patch adds, see https://bugs.kde.org/show_bug.cgi?id=164847. Please append FEATURE: 164847 to the summary, so the Bugzilla item will get closed automatically when your patch is committed. Also note that the summary will become the commit message.

Now for a first review (I'll look at the code later): Your patch disables more than just the overlay buttons, e.g. changing the mouse cursor on hover and expanding the filename in case it is too long to display. Do you think you could change that?

The overlay buttons are not only used in Browse mode, but also in other places:

• In the thumbnail strips, where they are still there. I'm not sure what we should do: The buttons may be annoying for switching images, on the other hand they provide a way to access the "lighttable" function, i.e. selecting multiple images to view them side by side. Thoughts?
• In Gwenview's importer tool (issue gwenview_importer . to test it), where they are now missing. As selecting images is the only thing you can do there, it might make sense to keep the buttons there. Opinions?

I guess we'd have to decide whether we want to consistently remove the buttons everywhere, or special case some parts for good reasons.

Regarding the config dialog:

• I'm not sure a technical term such as "Hover UI" should appear there and is understood by users. We'd need to come up with something better. @ngraham Any ideas?
• "Show image actions on hover": Perhaps "Show image actions when hovering over thumbnails"? This would make it a bit more clear what the option is about.
• The option feels quite lonely on its new tab. As it is used in more places than Browse mode too, we might just move it to the General tab?

Thanks so much for the contribution! For user-visible features like these, it's also nice to include a screenshot in the Summary section, so folks can see it in action.

In general, it's not the best UI to have labels on both the left and right of a checkbox. There should only be one, to its right. With that in mind, how about this:

• Show quick edit buttons over image thumbnails

No need to mention "hover", I think, since it will be pretty obvious what the label is talking about given how obvious the buttons are once you start using Gwenview for anything.

Also I think it might be overkill to add a new page to hold only one UI control. For now, how about putting this on the General page? It is pretty generally applicable since there are thumbnails in multiple views that show these buttons.

In D10421#204220, @ngraham wrote:

Thanks so much for the contribution! For user-visible features like these, it's also nice to include a screenshot in the Summary section, so folks can see it in action.

@huoni With a before/after screenshot (or perhaps a GIF), you might get featured in Nate's blog.

In general, it's not the best UI to have labels on both the left and right of a checkbox.

That might be better in the general case, however all the other config tabs in Gwenview kind of disagree here, so let's just go with the flow for now ;) Besides that, our HIG allows it too for grouping.

How about this (perhaps right as the first item on the General tab):

Thumbnails: [] Show image actions on hover

(I find "action" more appropriate, because going fullscreen and selecting something are not editing operations, while rotating could be classified as an action too. Also, with putting "Thumbnails" in front we lose "over", so we'd need the "hover" again to be clear enough.)

Thanks for the warm welcome. Bear with me as I'm not familiar with the code base so I'm still getting my head around it.

In the thumbnail strips, where they are still there. I'm not sure what we should do: The buttons may be annoying for switching images, on the other hand they provide a way to access the "lighttable" function, i.e. selecting multiple images to view them side by side. Thoughts?
In Gwenview's importer tool (issue gwenview_importer . to test it), where they are now missing. As selecting images is the only thing you can do there, it might make sense to keep the buttons there. Opinions?

Perhaps giving the user more options is the solution. For example, there could be three options:

1. Show everything (current behaviour, default)
2. Hide only the fullscreen and rotate buttons in Browse View
3. Hide all buttons in all places including the Thumbnail Bar and the importer

This would accommodate users that like having the selection buttons but don't care for the others. It would also keep consistency - having selection buttons in some places but not others I don't think is good design.

The UI for this could be something like:

Thumbnail butons: () Show all
                  () Show selection button only (only affects Browse View)
                  () Hide all

As for the config UI - agree with feedback here, especially if it affects more than the Browse View. Will move it into the General page.

@rkflx thoughts?

Bear with me as I'm not familiar with the code base so I'm still getting my head around it.

I'm discovering new parts each day myself, so sometimes it takes me some time to react as well ;) No worries…

The UI for this could be something like:

That's a great idea and solves the dilemma quite nicely. I don't really get what "only affects Browse View" for the second option means, but the following could work and would be easy to understand for most users IMO:

Thumbnail actions: () All buttons
                   () Show selection button only
                   () None

If you look around, there should be some code in the kcfg and in cpp files already using enums, which you could take inspiration from.

I don't really get what "only affects Browse View" for the second option means

Well the extra buttons only appear in Browse View. The Thumbnail Bar and Import tool already only have the selection button, therefore the intention was to let the user know that setting only has an effect on Browse View.
But perhaps it's not necessary, and could be confusing now that I think about it (your confusion is case in point!).

In D10421#204651, @huoni wrote:

I don't really get what "only affects Browse View" for the second option means

But perhaps it's not necessary, and could be confusing now that I think about it (your confusion is case in point!).

Yup. I know which buttons are where, but I don't think it should be pointed out in the dialog.

I don't think we can include a "show selection button only" item, since that's actually controlled in Dolphin, of all places. If you turn off Selection markers there, they turn off everywhere.

In D10421#204653, @ngraham wrote:

I don't think we can include a "show selection button only" item, since that's actually controlled in Dolphin, of all places. If you turn off Selection markers there, they turn off everywhere.

Hmh. Cannot reproduce, and I even restarted my session. I guess in Gwenview they are different enough.

In D10421#204654, @rkflx wrote:

In D10421#204653, @ngraham wrote:

I don't think we can include a "show selection button only" item, since that's actually controlled in Dolphin, of all places. If you turn off Selection markers there, they turn off everywhere.

Hmh. Cannot reproduce, and I even restarted my session. I guess in Gwenview they are different enough.

I also cannot reproduce this, "show selection marker" in Dolphin doens't appear to have an affect on Gwenview.

In D10421#204664, @huoni wrote:

In D10421#204654, @rkflx wrote:

In D10421#204653, @ngraham wrote:

I don't think we can include a "show selection button only" item, since that's actually controlled in Dolphin, of all places. If you turn off Selection markers there, they turn off everywhere.

Hmh. Cannot reproduce, and I even restarted my session. I guess in Gwenview they are different enough.

I also cannot reproduce this, "show selection marker" in Dolphin doens't appear to have an affect on Gwenview.

What am I saying!? I must be on crack. You two are right, of course. Ignore me!

@rkflx @ngraham

I've implemented the three options. There's one issue I noticed with this implementation - in the Thumbnail Bar (when option set to None), there is no visual feedback when hovering over a thumbnail, like there is in the other two thumbnail views. Thoughts?

I've also recorded a video demo:

EDIT: GfyCat of above video

In D10421#204708, @huoni wrote:

Implement three display options for thumbnail hover buttons

• Show all (default)
• Show only selection buttons
• Hide all
  
  This setting affects Browse View, Thumbnail Bar (in Image View), and the Importer (gwenview_importer)

That works superb, and the settings dialog looks great too!

Thanks for your thorough review!

I have fixed the full screen issue, and everything else you've commented on.

I will start looking into highlighting the thumbnail on hover in the Thumbnail Bar. EDIT: see Diff 27036

Yay, code looks cleaner now, most fullscreen bugs are fixed and even hovering magically works. We are nearly there.

Thanks, I'll look at it tomorrow.

In D10421#205904, @huoni wrote:

• Fix BG colour changing when saving config in fullscreen

I'm okay with including the hover bugfix here as it is slightly related, but please open a separate Diff for the other (obvious ;) problem you fixed. This way you'll have two patches in your name in no time…

magically

Not sure about that :)

Speaking of the hover highlight in the Thumbnail Bar - I noticed that the effect isn't very noticeable in the fullscreen mode thumbnail bar. Should we try to adjust it there?

I'm okay with including the hover bugfix here as it is slightly related, but please open a separate Diff for the other (obvious ;) problem you fixed. This way you'll have two patches in your name in no time…

Answered my question already! Have reverted that and will submit a new diff.

In general, it's better to open new Diffs for unrelated/new issues. This makes it much easier to review and (most importantly) ensures a somewhat tidy Git history (in case you have to understand a change from the past, or revert something, or bisect a bug etc.).

Searching for bugs in Bugzilla before working on something is good (to be able to close the bug and get hints about the implementation/requirements), but in KDE it's not required to have a tracking bug for every Diff.

As for the unnoticeable fullscreen hover: Yeah, this bugs me too (and the normal thumbnail grid is also not that great in that regard), especially when compared to the neon-coloured sidebar frame.

In summary: create two new Diffs…

Perfect, works great now. I'll land it on your behalf in a bit.

Thanks again for contributing! Let us know if you need ideas for what to work on next if you like it so far (either Gwenview or other components).

Awesome! Thanks so much for your help!