Consider all thumbnailers for the information panel and tooltips
Needs RevisionPublic

Authored by fvogt on Aug 29 2018, 8:01 AM.

Details

Summary

The text thumbnail is really useful in tooltips, but disabled by default in
KIO. Having just a larger version of the placeholder icon from the theme
doesn't make a lot of sense, so try the best to get a proper preview.
Same applies to the information panel, which would otherwise just
show the same icon.

Test Plan

Got textfile previews in tooltips and in the information panel even if
disabled.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Aug 29 2018, 8:01 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptAug 29 2018, 8:01 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
fvogt requested review of this revision.Aug 29 2018, 8:01 AM
broulik accepted this revision.Aug 29 2018, 8:03 AM
This revision is now accepted and ready to land.Aug 29 2018, 8:03 AM
fvogt added a comment.Aug 29 2018, 8:08 AM

@broulik doesn't use tooltips himself - so I'll wait a day whether someone who does objects to this

markg requested changes to this revision.Aug 29 2018, 9:22 AM
markg added a subscriber: markg.

This will give an inconsistency with the thumbnailes that are enabled in Dolphin's settings and the preview tooltip.
The preview tooltip would now always show a generated thumbnail regardless of the dolphin settings.

I think it would be best to use the thumbnailers that are enabled in Dolphin for the preview as well.

This revision now requires changes to proceed.Aug 29 2018, 9:22 AM
fvogt added a comment.Aug 29 2018, 9:26 AM

Of course, that's the main and only point of this patch.

I think it would be best to use the thumbnailers that are enabled in Dolphin for the preview as well.

No, because (quoting the description):

Having just a larger version of the placeholder icon from the theme
doesn't make a lot of sense, so try the best to get a proper preview.

markg added a comment.Aug 29 2018, 9:30 AM

Of course, that's the main and only point of this patch.

I think it would be best to use the thumbnailers that are enabled in Dolphin for the preview as well.

No, because (quoting the description):

Having just a larger version of the placeholder icon from the theme
doesn't make a lot of sense, so try the best to get a proper preview.

I don't think we should want that.
Unless it would be a setting to let the tooltip preview use all available thumbnailers.

I don't think we should want that.

Why not? The tooltip is shown on-demand when you hover a file for a while. There you get more information when the thumbnailer is available. Just showing a bigger file icon makes no sense for the tooltip.

Unless it would be a setting to let the tooltip preview use all available thumbnailers.

Please no stupid setting

markg added a comment.Aug 29 2018, 9:40 AM

I don't think we should want that.

Why not? The tooltip is shown on-demand when you hover a file for a while. There you get more information when the thumbnailer is available. Just showing a bigger file icon makes no sense for the tooltip.

I would "expect" that the thumbnailers I explicitly enable in the settings are used for, well, thumbnails. Wherever they are used. Previews are thumbnails so the expectation is the same.

Unless it would be a setting to let the tooltip preview use all available thumbnailers.

Please no stupid setting

It could be a simple checkbox where you select the thumbnailers:
[ ] Use enabled thumbnailers fot tooltip previews

Or something alike. But i agree, a setting would not be the preferred way.

@ngraham could you chime in from a usability point of view?

ngraham accepted this revision.Aug 29 2018, 1:43 PM

IMHO, not everything always needs a setting; we should consider the user's intentions based on their actions, too. Since tooltips are off by default, what is the profile of the user who turns them on? This is a person who likes to see as much information about their files as possible, but contextually. I think it makes a lot of sense that this person would prefer a preview of their text files' contents over a huge icon.

For that matter, the same logic applies to the Information Panel, so I would support also always showing all previews there too

If we get complaints (which I doubt), we can always later add a checkbox under the Previews list saying something like "Always show all previews in Tooltips and Information Panel" that defaults to on.

abetts added a subscriber: abetts.EditedAug 29 2018, 2:28 PM

+ 1. I agree with the proposed change. I feel that there are users who love seeing previews. Previewing a text file in a thumbnail might also not be as demanding on the system as something like a video or graphic work. Therefore, I feel this would be a sensible addition to Dolphin. I personally love seeing previews, mostly because they work as reminders and when I see them, I can quickly identify what they are about.

fvogt updated this revision to Diff 40650.Aug 29 2018, 2:47 PM

Do the same for the information panel

fvogt retitled this revision from Consider all thumbnailers for tooltip previews to Consider all thumbnailers for the information panel and tooltips.Aug 29 2018, 2:48 PM
fvogt edited the summary of this revision. (Show Details)
fvogt edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Aug 29 2018, 3:10 PM
fvogt added a comment.Aug 29 2018, 3:29 PM

Landed as https://commits.kde.org/dolphin/1d943518ad2b7f6f26bbe05588134758ec424055.
Phab didn't close this revision, presumably because it's in "Needs Review" state.

Probably need to close it manually now.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 29 2018, 3:51 PM
This revision was automatically updated to reflect the committed changes.

Oke, right. Go for it :)

(which is done already...)

elvisangelaccio reopened this revision.Sep 2 2018, 10:01 AM
elvisangelaccio requested changes to this revision.

I really don't like this change. At the very least, this patch should have changed the label in the Confirmations tab from "Show previews for:" to "Show previews in the view for:", to fix the inconsistency Mark pointed out.

But I think we should *not* ignore disabled thumbnailers, at all. If I want to disable JPG previews for whatever reason, dolphin should not think it knows what I want better than I do. (btw, dolphin already ignores disabled thumbnailers for folder previews, which is also equally bad).

That said, I agree that showing a big placeholder icon in the tooltip is not useful. But we need to find a better solution.

This revision now requires changes to proceed.Sep 2 2018, 10:01 AM
fvogt added a comment.Sep 2 2018, 10:12 AM

At the very least, this patch should have changed the label in the Confirmations tab from "Show previews for:" to "Show previews in the view for:", to fix the inconsistency Mark pointed out.

Sure, I can do that: https://phabricator.kde.org/D15216

But I think we should *not* ignore disabled thumbnailers, at all. If I want to disable JPG previews for whatever reason, dolphin should not think it knows what I want better than I do.
That said, I agree that showing a big placeholder icon in the tooltip is not useful. But we need to find a better solution.

Please come up with a better idea - something which doesn't involve more user configuration...
Maybe a new property for thumbnailers which describes whether they are useful for smaller previews?

(btw, dolphin already ignores disabled thumbnailers for folder previews, which is also equally bad).

Not anymore: https://phabricator.kde.org/D15096 and https://phabricator.kde.org/D15097

I'm not a big fan of adding another option for this. What's the use case for someone who deliberately turns on the tooltips or Information panel but then doesn't want to see a preview of the file's contents instead of a huge thumbnail? Does this user exist?