Turn on Dolphin icon previews by default
ClosedPublic

Authored by ngraham on Aug 20 2017, 6:58 PM.

Details

Summary

BUG: 338492
BUG: 350212

By default, turn on all preview plugins and turn on previews themselves.

Depends on D8347

Test Plan

Tested this change in an up-to-date KDE Neon: removed the existing dolphinrc file, deployed Dolphin with the change, and observed that previews are now turned on for all file types for which a plugin exists except for text files, which are in the blacklist because they're mostly useless at nearly all icon sizes (still available in case people want them, though).

Here's how Dolphin's main window looks by default now in KDE Neon:


(You may notice that there are no previews for the video files; that's because Neon doesn't ship with any plugins for them, so that would be expected at this point)

And here is how the Settings > General > Preview window looks like now by default:

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.
ngraham created this revision.Aug 20 2017, 6:58 PM
Restricted Application added a subscriber: Konqueror. · View Herald TranscriptAug 20 2017, 6:58 PM
ngraham edited the test plan for this revision. (Show Details)Aug 20 2017, 7:02 PM
ngraham updated this revision to Diff 18464.Aug 20 2017, 8:21 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Aug 20 2017, 9:36 PM

Make default config list static in a function and return const reference.

This is yet another config that is duplicated across dolphin and folder view...

Anyway +1, I think it's reasonable to flip the default. Please also add the VDG as reviewer and screenshots before/after of some folder with many file types.

-1 Urgh. Can we perhaps have a static QStringList KIO::PreviewJob::defaultPlugins()? It's not just Dolphin that has a copy of this (file dialog et al). And this way we could have flip it around to whitelist all plugins (find them through KService) and then have a user blacklist and perhaps by default blacklist stupid thumbnailers like "plain text".

markg added a subscriber: markg.Aug 21 2017, 2:34 PM

With all the thumbnailers you turned on, i'd be against it.
It's probably not going to slow down scrolling speed because the thumbnail generation is done in another process. Thankfully! Otherwise you would be horrible slow :)
But on large folders it will have an impact on the CPU i think.

But i personally don't like how dolphin handles thumbnailing.
What it does is:

  • Thumbnail the current viewport (so that all you "currently" see is thumbnailed
  • Then stop

If you scroll a bit it does it again. Files that don't have a thumbnail yet are thumbnailed then.

This leads to a very annoying scroll behavior (could just be me...). I tend to scroll a bit, let it generate the thumbnails and scroll a bit further till everything is done.
Granted, this is only the first time when there are no thumbnails.
That scrolling i do is only needed because dolphin doesn't do it otherwise.

What i think needs to happen is this:

  1. Dolphin should thumbnail the current viewport (it does, no change needed here)
  2. Then continue on with rows above and below it till it's done. OR
  3. If i pump to another part of the view, it should abort the task it had and restart from the new current viewport
  4. In any case, thumbnail everything in a given folder on the background (obviously)

This probably does give another concern.. Like when to stop thumbnailing and the size.
I don't think size is an issue. When to stop might be, i'd say on a folder change or app exit.

Lastly, there is a performance issue in thumbnailing. I had downloaded a ~4GB file with 1000 QHD wallpapers, just to see how fast it would generate those thumbnails.
They were all JPG files and roughly 4MB per file. The speed was... not stellar, which is really surprising! My SSD can read about 120 of those files a second (it really can, i tested that!)! Yet the thumbnails only seems to be appearing at a rate of ~10/second (just by looking at it, no specific benchmarks have been done). That tells me that there is lots of room to speed this up! I think that needs to happen first before enabling this, to give the best possible experience to the user.

I haven't forgotten about this, but life has intervened and I'm not going to have time to work on it for a few days, maybe even a couple of weeks. If anyone else wants to run with it and take over, I won't be in the least bit offended. If not, I'll resume work at some point in the near to medium term future.

-1 Urgh. Can we perhaps have a static QStringList KIO::PreviewJob::defaultPlugins()? It's not just Dolphin that has a copy of this (file dialog et al). And this way we could have flip it around to whitelist all plugins (find them through KService) and then have a user blacklist and perhaps by default blacklist stupid thumbnailers like "plain text".

Blacklisting is a good idea!
More logical from a users POV e.g. when installing a new thumbnailer.

I have already talked with @andreask about this topic some time ago.

Agreed. I haven't forgotten about this FYI.

update the previewer (speed) is always a good idea. I always didn't understand that there is an preview icon in the main toolbar but by default all previews are turned off via the settings so after a fresh installation open dolphin click on preview you didn't get a preview. that's strange, because when the user want to see the file preview the user should get it. In addition some modules aren't in the default installation (e.g. video and pdf) so you get first not what you want (no preview when click on preview) and second when you are in the settings you see that there isn't a preview for videos and pdf available.

so it would be nice to

  1. make a better performance for the previews
  2. enable them by default (exe, ... are not that importend so if there are some safety issues for specific previews it's ok to turn them off, but office, pictures, video, pdf, text, should be on by default)
  3. add a link to discover for available other previewers in the dolphin settings.

thanks for link me to this task and for the dev work.

ngraham planned changes to this revision.Oct 6 2017, 10:17 PM
ngraham updated this revision to Diff 20917.Oct 17 2017, 7:03 PM
  • Use KIO::PreviewJob::defaultPlugins() to get the list of which plugins should be enabled by default
ngraham edited the summary of this revision. (Show Details)Oct 17 2017, 7:05 PM
ngraham edited the test plan for this revision. (Show Details)

OK, I think this is reviewable now. The change requires D8347, which implements a new function in KIO that returns a QStringList of default plugins: all of them, minus the plugins specified in an internal blacklist (right now just the text plugin, per @broulik's suggestion).

If performance is problematic with huge directory sizes, as @markg indicated, then the button to turn previews off is still available in the default toolbar (and that's why I didn't remove it as part of this patch). But IMHO for 99% of users and 99% of folders, there shouldn't be any appreciable performance issues that should block this.

In D7440#156711, @cfeck wrote:

Good point. I'll make another patch to change that.

https://phabricator.kde.org/D8352 addresses that, and depends on D8347 just like this does.

ngraham edited the test plan for this revision. (Show Details)Oct 21 2017, 3:12 AM
ngraham edited the summary of this revision. (Show Details)Oct 21 2017, 3:15 AM

With dependencies now merged, I think this is ready once more for the firing squad errr I mean review process. :)

With dependencies now merged, I think this is ready once more for the firing squad errr I mean review process. :)

My earlier concern is still very much an issue.
See my lengthy reply about scrolling and previews turned on.

To put it bluntly, i don't think having previews turned on with the described scrolling behavior is a good user experience.
That should be improved first. Don't look at me for that though, i'm already busy enough with a places panel re-organization :)

In other words. +1 if you commit this without turning previews on by default.

markg requested changes to this revision.Oct 21 2017, 11:34 AM

At the very least - regardless of my previous comment - you'd also have to raise the minimum required frameworks version in Dolphin' CMakeLists.txt.
It's the line: set(KF5_MIN_VERSION "5.37.0") which has to be changed to set(KF5_MIN_VERSION "5.40.0").

And that very action will cause the build to break as 5.40.0 isn't released yet :)
A push of this change should only happen after Frameworks 5.40 is released, to not break compiling Dolphin.

@others, isn't there some trick in CMake to say "i require a minimum version of 5.40 or current GIT master?"

This revision now requires changes to proceed.Oct 21 2017, 11:34 AM

Then the fix should wait.

Frameworks 5.40 is scheduled to be released on November 11th, while the feature freeze for Applications 17.12 is on November 9th.

While it is possible to ask for an exception, given the strict timing, I would say that it's better to postpone this to Applications 18.04.

@markg
I'm not sure I agree that the current preview generation behavior is bad enough to hold this up.

  1. Thumbnailing only the files in view is I believe how macOS finder handles this, too. Maybe it's just me, but I don't find this behavior distracting at all, and after 20+ years of supporting Mac users professionally and in my personal life, I don't think I can recall a single person complaining about the thumbnailing behavior.
  2. Use cases where this could actually be annoying (folder with thousands of huge images) are very rare corner cases; let's not let those overwhelm the benefits for the general case! And you can always turn off previews on a per-directory basis anyway using the button in the toolbar.
  3. If performance could use improvement, I'm all for it, but can we do that in another patch?

@ltoscano
As an alternative, I could change this to use KIO::PreviewJob::availablePlugins(), which differs from the new API I've added only in that it will also enable the text previewer plugin (but maybe poor Kai could live with that for a few days. :) ). Then I could switch to KIO::PreviewJob::defaultPlugins() once KF5.40 is released. Does that sound reasonable?

If the entire point was to have a new API to not enable that plugin, then maybe it's better to wait. And itt's not going to be a matter of few days: the entire Applications 17.12 lifecycle will use the API, so 4 months in the best case, or more for distributions which won't upgrade that version.

Personally I'm agnostic about it; I just added it to Please Kai who doesn't like the text previewer plugin. :)

ngraham updated this revision to Diff 21060.Oct 21 2017, 3:00 PM

Use KIO::PreviewJob::defaultPlugins() only if we've got KIO 5.40 or later; otherwise, emulate the API ourselves

I've tested this approach and it works without needing KF5.40 yet, and still produces the desired behavior (i.e. doesn't turn on the text plugin by default).

markg resigned from this revision.Oct 21 2017, 6:13 PM

I have my doubts for this one. The same block of code is now duplicated 4 times.. 3x in Dolphin, 1x in KIO.
Why not wait? Getting this in is not *that* important.. On the other hand i do get that waiting half a year (for applications 18.04) also seems excessively long which is why i understand the workaround.

Anyhow, code wise the change is fine.
I don't want to be the one holding this change back so i'm resigning as reviewer to not block it.

Thanks Mark.

FWIW there was already code duplicated 4 times, which is why I wrote KIO::PreviewJob::defaultPlugins() to reduce that duplication. It's a shame we can't use it for everything right now, but I'll definitely be cleaning this up some more once Dolphin and kio-extras can link against 5.40.

In D7440#157913, @markg wrote:

I have my doubts for this one. The same block of code is now duplicated 4 times.. 3x in Dolphin, 1x in KIO.

You can remove duplicate code and introduce a static function in Settings

ngraham planned changes to this revision.Oct 22 2017, 11:04 PM
ngraham updated this revision to Diff 21875.EditedNov 4 2017, 6:39 PM

Move the conditional call to KIO::PreviewJob::defaultPlugins() (as well as the part that opens the config file) into a globally-available function to eliminate code duplication

ngraham updated this revision to Diff 21876.Nov 4 2017, 6:51 PM

Remove unnecessary changes

abetts accepted this revision.Nov 5 2017, 9:09 PM
abetts added a subscriber: abetts.

I have no objections to this change. One thing I am concerned about is the speed that these thumbnails will show up. Imagine a movie a few gigs big versus a 10Kb icon. If there are ways to optimize the loading of these previews, it can help tremendously.

This revision is now accepted and ready to land.Nov 5 2017, 9:09 PM

Yes I definitely intend to look into performance optimizations in the near future.

Does anyone in Dolphin want to offer a thumbs up or thumbs down? I'd like to either land this, or implement changes to make it land-able (assuming they're in scope).

hmm, I'd rather duplicate the #if KIO_VERSION check than adding a defaultPlugins() method in global.cpp.
Once we can depend on KIO >= 5.40 that method would become pretty useless.

That was my thought too, but @markg and @anthonyfieroni weren't too keen on all the duplicated code, and it does allow us to centralize the kconfig part rather than duplicating it three times.

Well, the duplicated code (by which I mean the kio version check with fallback) will be removed once we can depend on KIO >= 5.40, which is next month.
We could add a TODO comment just in case we forget, but I don't think it's necessary.

About KConfig, it is already used everywhere in Dolphin, I don't see a reason to use a wrapper function only for this specific usage.

markg added a comment.Nov 6 2017, 4:57 PM

That was my thought too, but @markg and @anthonyfieroni weren't too keen on all the duplicated code, and it does allow us to centralize the kconfig part rather than duplicating it three times.

I like the global one much more then your earlier attempt.
Also, the global one can go entirely once you can depend on the appropriate framework so it's temp anyhow.

As said before, I still have my doubts about this whole feature but don't want to be the one blocking it. Thus no +1 or -1 from me, it's just up to others to decide.
Code wise, it's a +1 though.

markg added a comment.Nov 6 2017, 5:00 PM

Note: i do think that you need to get an opinion from the Dolphin maintainer before pushing regardless of who else gives a +1 as this change could potentially be quite invasive and is going to change the default look (thumbnails be default).

I marked myself as a member of the Dolphin project in Pbabricator but I'd really like to get a final yea-or-nay from some of the people listed as reviewers here so I can land this or make changes.

The Applications/17.12 branch has been created, so on master we could already depend on KIO >= 5.40. How about doing that?

ngraham updated this revision to Diff 22376.Wed, Nov 15, 5:43 AM

Rely on KF 5.40 and simplify the code a ton

dfaure accepted this revision.Wed, Nov 15, 8:34 AM
ngraham updated this revision to Diff 22421.Wed, Nov 15, 11:45 PM

Actually use the value of Plugins specified in the config, if it's present (this makes saving user plugin selections work again)

This revision was automatically updated to reflect the committed changes.