Fix thumbnail hover icon show-in-fullscreen action
ClosedPublic

Authored by muhlenpfordt on Feb 19 2018, 9:34 AM.

Details

Summary

In non-fullscreen browse mode the thumbnails display four hover icons.
Clicking on the "view in fullscreen" icon switches to fullscreen mode
but not to view mode as expected.
This patch switches to fullscreen view mode.

Test Plan

Open Gwenview in normal browse mode and hover over thumbnail icons
to see the icons. Click on second icon to view this image
in fullscreen mode.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Feb 19 2018, 9:34 AM
muhlenpfordt created this revision.
muhlenpfordt added inline comments.
lib/thumbnailview/previewitemdelegate.cpp
616 ↗(On Diff #27524)

Better descriptions welcome. ;)

rkflx added a subscriber: rkflx.Feb 19 2018, 1:13 PM

+2 on changing the fullscreen button to switch to View mode at the same time.


As for the tooltips, I'm not really a fan. The intention to describe what the buttons are doing is not bad, but in this case it also conflicts a bit with the thumbnails: For the first tooltip to appear I have to wait for quite a bit (which is good), but the subsequent tooltips now show immediately and also after moving the mouse to a different image. They obscure the thumbnail, which is what I'm primarily interested in. For toolbar buttons the tooltips are less annoying, because they don't obscure so much and also there I'm not hovering for a while to ponder whether to click-select an image or not.

Granted, for the thumbnail strips we already have tooltips, but those provide some actual value in form of the filename. Also, Dolphin has switched off tooltips over items by default since some time for similar reasons.

Considering every angle, I feel that leaving out the tooltips would not be too bad, as the icons are good enough and learning the buttons after a while is not that hard. Better avoid the long-term annoyance for everyone than to help a few new users understanding the buttons faster. Most of the icons are even explained in the toolbar…

Big IMHO, happy to discuss if others feel strongly the tooltips are needed. Back in the days applications had a text label in the statusbar to display such information (e.g. when going over menu options), but those are almost extinct today…

I don't cling to the tooltips and agree with your arguments.
If nobody votes for them fast enough I remove this part later. ;)

Tooltips should go in another patch regardless.

I'm split on the matter. On one hands, yeah, it adds some visual clutter, slightly impeding the primary use case of looking through your thumbnails. On the other hand, tiny, icon-less, contextual buttons have low usability for average user, and tooltips may help people actually use the buttons more.

rkflx added a comment.EditedFeb 19 2018, 2:03 PM

I'm split on the matter. On one hands, yeah, it adds some visual clutter, slightly impeding the primary use case of looking through your thumbnails. On the other hand, tiny, icon-less, contextual buttons have low usability for average user, and tooltips may help people actually use the buttons more.

That's true in general, but here I fear we might get complaints (I would file a bug, at least ;). Do we have any bug reports regarding the hover buttons, so we might get more insights?


A bit off-topic, but here is a crazy idea related to the topic: In a lot of places (e.g. toolbar, sidebar etc. – I'm not talking about the hover buttons…) the text of the tooltip is just the same as the text on the button. What if Breeze (?) would hide the tooltip if !text.isHidden && text == tooltip?

I'm split on the matter. On one hands, yeah, it adds some visual clutter, slightly impeding the primary use case of looking through your thumbnails. On the other hand, tiny, icon-less, contextual buttons have low usability for average user, and tooltips may help people actually use the buttons more.

That's true in general, but here I fear we might get complaints (I would file a bug, at least ;). Do we have any bug reports regarding the hover buttons, so we might get more insights?

I'm not aware of any such reports.

A bit off-topic, but here is a crazy idea related to the topic: In a lot of places (e.g. toolbar, sidebar etc. – I'm not talking about the hover buttons…) the text of the tooltip is just the same as the text on the button. What if Breeze (?) would hide the tooltip if !text.isHidden && text == tooltip?

I wouldn't object!

muhlenpfordt retitled this revision from Fix thumbnail hover icon show-in-fullscreen action and add icon tooltips to Fix thumbnail hover icon show-in-fullscreen action.
muhlenpfordt edited the summary of this revision. (Show Details)
muhlenpfordt edited the test plan for this revision. (Show Details)

Removed tooltips

ngraham accepted this revision.Feb 19 2018, 3:34 PM

Fixes the issue, thanks!

Since this is a minor bugfix, I think it can go into the stable branch, FYI. No need to branch from master. If you re-branch from Applications/5.12, using arc land will be much less error-prone.

This revision is now accepted and ready to land.Feb 19 2018, 3:34 PM
rkflx added a comment.EditedFeb 19 2018, 3:36 PM

Since this is a minor bugfix, I think it can go into the stable branch, FYI.

No, to me it seems like a major change in behaviour if the button suddenly does something differently. The current behaviour is not "wrong" per se, because you can double-click to do the other action. We only change the default, if you will.

Distros will never ship point releases if you do stuff like this.

Even if the change in behavior makes the button do what it was supposed to do all along?

rkflx added a comment.EditedFeb 19 2018, 4:09 PM

Even if the change in behavior makes the button do what it was supposed to do all along?

We don't know at this point what the button was supposed to do. 1cd78ec8e6dd clearly does not trigger switching to View, and also the docbook says "enter Fullscreen Mode" when talking about the hover button and later "Switches to Full Screen Mode" when talking about the toolbar button (both do not switch to View automatically when going fullscreen). I don't think "enter" vs. "switch" is enough to conclude this was about "view" vs. "browse".

The difference between 17.12.3 and 18.04 is only a single month anyway… Why would we risk annoyed users of the "stable" branch for something which has been that way for over 10 years!?


On the other hand, tiny, icon-less, contextual buttons have low usability for average user, and tooltips may help people actually use the buttons more.

What about adding more pictures/examples and a better explanation to the docbook?

Even if the change in behavior makes the button do what it was supposed to do all along?

We don't know at this point what the button was supposed to do. 1cd78ec8e6dd clearly does not trigger switching to View, and also the docbook says "enter Fullscreen Mode" when talking about the hover button and later "Switches to Full Screen Mode" when talking about the toolbar button (both do not switch to View automatically when going fullscreen). I don't think "enter" vs. "switch" is enough to conclude this was about "view" vs. "browse".

I think it's pretty clear that clicking on a full screen button for an image should enter full screen mode for that image. The other three hover buttons act on the image itself, so if this wasn't the intention all along, not only would the hover Full Screen button be inconsistent with the other three, but it would also be completely superfluous since there's already a Full Screen button on the default toolbar that accomplishes the same thing.

But regardless...

The difference between 17.12.3 and 18.04 is only a single month anyway…

True enough. However, the upcoming Kubuntu 18.04 LTS is going to ship with KDE Apps 17.12.x, not 18.04, and a lot of people are going to be using that for 5 years.

On the other hand, tiny, icon-less, contextual buttons have low usability for average user, and tooltips may help people actually use the buttons more.

What about adding more pictures/examples and a better explanation to the docbook?

Not really; most people don't actually read the documentation. IMHO interfaces should be as self-documenting as possible, within the bounds of aesthetic and design acceptability

rkflx added a comment.Feb 19 2018, 4:31 PM

I knew you would bring this up. That's a very slippery slope to cram massive changes into stable releases only because a distro does not manage to align in such a way to ship Applications/18.04. Kubuntu does not even have 17.12 at this point, I tried yesterday and it was stuck at 17.08 even for the very latest testing version.

KDE's releases make promises regarding stability in the form of major and minor releases, and I think everybody should be able to depend on that. If distros have special wishes, they should come to KDE beforehand, like openSUSE did for making certain Plasma releases an LTS and releasing them at a certain point in time. That's different to coming after the fact and trying do "make it unstable" for everybody (I'm exaggerating, of course). Updating to a minor release means "no surprises", and the button changing what it did would be a surprise. We just cannot rule out that some users might use the old behaviour, even if you'd find this strange. I know it's hard to see nice features and sensible changes landing in Git after the feature freeze, but such is life of a non-rolling distro. And BTW, my comment is not only about Gwenview.

Other than that, I support the change, obviously.

I knew it would be a long-shot argument. :)

I see your point, but I still find it hard to believe that anyone could depend on the current behavior, especially when even with this patch, the current behavior is still available via a visible-by-default toolbar item.

Still, I'm not sure it's worth arguing about. A fix is a fix. @muhlenpfordt, go ahead and land it on master.

rkflx added a comment.Feb 19 2018, 4:49 PM

IMO it's important to have a common understanding what constitutes a stable release. Your's is probably a bit different from what's tradition. However, don't come to every single project for exceptions from the guidelines. Discuss it on a bigger scale if you must, but keep in mind that not every software is as young and agile as Discover (SCNR).

Anyway, thanks for caring, otherwise you wouldn't have tried ;)

Do we have a guideline written down somewhere for what's acceptable for a stable branch in the KDE world? My understanding of this is only slowly evolving over time, mostly from negative examples ("Don't put that in a stable branch!")

rkflx added a comment.Feb 19 2018, 5:06 PM

Not sure, you know the wiki better than I do. If it is missing, bring it up on T7116. Other than that, don't underestimate what's common practice in software development in general and tradition in KDE in particular. My understanding (I'm not saying it is the only correct way) evolved over time when watching the project, nevertheless it's good to question the status quo from time to time. Indeed we are releasing applications more often than before, and Frameworks even every month. I'm not saying this cannot change, but currently it's the way it is.

It's a huge topic, my take on it is that we should make it easy for everybody to get to new feature releases if they want, and allow the rest to use our software with as few disruptions as possible. KDE's software is only a small part of the overall picture.

Landing on master now to be on the safe side.

This revision was automatically updated to reflect the committed changes.