Differential D8347

KIO::PreviewJob::defaultPlugins() function
ClosedPublic
Actions

Authored by ngraham on Oct 17 2017, 6:55 PM.

Details

Reviewers
broulik
markg
Group Reviewers
Frameworks
Dolphin
Commits
R241:3042e12296db: KIO::PreviewJob::defaultPlugins() function
Summary

Adds a new KIO::PreviewJob::defaultPlugins() function that can lets clients get a list of thumbnail plugins that should be enabled by default (disabled plugins can still be enabled via the GUI in Dolphin).

The function returns a QStringList of all available plugins minus ones specified in an internal blacklist, because some plugins may not be useful enough to enable by default, but also are not useless enough to remove. Currently the only blacklisted plugin is the .txt file previewer, because at most (but not all) icon sizes, the preview is unreadably small and not very useful.

This supports D7440 and D8352.

Test Plan

Tested in KDE Neon. Function is available and works as expected.

Diff Detail

Repository
R241 KIO
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.Oct 17 2017, 6:55 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 17 2017, 6:55 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
ngraham updated this revision to Diff 20923.Oct 17 2017, 7:49 PM

Updating comments/documentation

elvisangelaccio added a subscriber: elvisangelaccio.Oct 17 2017, 10:01 PM
elvisangelaccio added inline comments.
src/widgets/previewjob.cpp
740–741

Shouldn't the blacklist be user-configurable?

745

code style, brace should be at the end of the previous line

src/widgets/previewjob.h
212–215

Documentation should be above the method, not below.

218

Unrelated whitespace change

ngraham updated this revision to Diff 20926.Oct 17 2017, 10:08 PM
ngraham marked an inline comment as done.

Style improvements/corrections

ngraham marked 3 inline comments as done.Oct 17 2017, 10:08 PM
ngraham added inline comments.
src/widgets/previewjob.cpp
740–741

In fact, the whole list of plugins to enable is user-configurable in Dolphin. The blacklist just defines which plugins come turned off by default; the user can always turn them on. This is how it is today, in fact; I just changed the internal whitelist to an internal blacklist

ngraham updated this revision to Diff 20927.Oct 17 2017, 10:09 PM

Whitespace fix

ngraham marked an inline comment as done.Oct 17 2017, 10:10 PM
ngraham edited the summary of this revision. (Show Details)Oct 17 2017, 11:04 PM
ngraham edited the summary of this revision. (Show Details)Oct 17 2017, 11:08 PM
anthonyfieroni added a subscriber: anthonyfieroni.Oct 18 2017, 4:38 AM
anthonyfieroni added inline comments.
src/widgets/previewjob.cpp
740
const QStringList blackList = { QStringLiteral("textthumbnail") };
markg requested changes to this revision.Oct 18 2017, 7:29 AM
markg added a subscriber: markg.

I'm a bit skeptical about this function...
It in fact is all plugins minus (in this case) the text thumbnail with no way to configure it afterwards.
That means a updates blacklist can only be distributed in a new kio release.

That's not ideal but i don't know which way would be acceptable either.

It could be a config file,
It could be a environment variable as a comma separated list.

Anyhow, -1 for the missing @since, but others might have other suggestions on how to get that blacklist filled.

src/widgets/previewjob.h
206

You miss an @since line.

This revision now requires changes to proceed.Oct 18 2017, 7:29 AM
broulik added a comment.Oct 18 2017, 9:46 AM

Unfortunately this will not automatically enable newly installed thumbnailers once the user has changed the setting, right? Dolphin does readEntry("foo", defaultPlugins), as soon as it's there new ones aren't taken into account. Perhaps we need another function that takes care of reading the setting/blacklist for use in all apps? Overall a good change imho.

ngraham added a comment.Oct 18 2017, 3:39 PM

Right, I deliberately made this a change of the default values. I didn't want to override existing user preferences.

I do agree that there should probably be a mechanism to immediately enable new plugins as they're installed, but IMHO that's a different change that's out of scope for this patch.

ngraham marked an inline comment as done.Oct 18 2017, 3:40 PM
ngraham added inline comments.
src/widgets/previewjob.cpp
740

I wrote it the way I did on purpose, to facilitate people adding new entries if and when the blacklist needs to be updated without having the diff show the whole line as having been changed.

ngraham marked 2 inline comments as done.Oct 18 2017, 3:40 PM
broulik added a comment.Oct 18 2017, 3:43 PM

Agreed. I'd say if nobody objects this week this is good to go

ngraham added a comment.Oct 18 2017, 4:10 PM

I still need to add a @since in the header documentation, after which point hopefully @markg will approve. :)

markg accepted this revision.Oct 18 2017, 5:06 PM
In D8347#156930, @ngraham wrote:

I still need to add a @since in the header documentation, after which point hopefully @markg will approve. :)

+1 then. I trust you do that.
Also note that a proper container for what you're doing is two sets (default plugins and the blacklisted ones) and calling the std::set_difference algorithm on them; see: https://www.fluentcpp.com/2017/01/09/know-your-algorithms-algos-on-sets/
That's just minor details though, feel free to ignore that :)

I remain skeptical of your entire goal here though. It's good and well intended, that's for sure. But ultimately this is for Dolphin and the previews which in turn means that by default all but one plugin is going to be enabled. I'm skeptical about that because there is always an issue with doing that. Either bad plugins causing weird behavior, bad data (just plain and simple broken data) causing weird behavior. And probably a gazillion other reasons once it is enabled and used in the next dolphin version...

I hope you're up for a lot of bug reports :D

On the other hand, it would be very cool if this starts working properly!

This revision is now accepted and ready to land.Oct 18 2017, 5:06 PM
ngraham added a comment.Oct 18 2017, 5:10 PM

I am indeed up for lots of bug reports. :) We can't find the bugs if many (most?) users don't use the feature because it's off by default!

FWIW, I've always turned all plugins on as one of the first things I do on a new install, and I have yet to see a significant preview bug. I'm sure there are plenty that people will quickly find, of course, but I don't think the previews are some kind of bug-o-matic right now.

ngraham updated this revision to Diff 20975.Oct 19 2017, 1:09 PM

Updating header documentation with @returns and @since

ngraham marked an inline comment as done.Oct 19 2017, 1:11 PM
ngraham added inline comments.
src/widgets/previewjob.h
206

I've updated the header documentation with @returns and @since, as well as updated the documentation for the above availablePlugins() to help distinguish it from defaultPlugins().

Is 5.39 the right version? I'm a bit new at this.

ngraham marked 2 inline comments as done.Oct 19 2017, 1:11 PM
broulik added a comment.Oct 19 2017, 1:21 PM

5.39 was just released so it will be 5.40

ngraham updated this revision to Diff 21004.Oct 20 2017, 12:53 PM

Correcting the @since version number

Closed by commit R241:3042e12296db: KIO::PreviewJob::defaultPlugins() function (authored by ngraham). · Explain WhyOct 20 2017, 12:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/widgets/previewjob.h (10 lines)
Msrc/widgets/previewjob.cpp (13 lines)

Diff 21005

View Options

src/widgets/previewjob.h

Loading...
View Options

src/widgets/previewjob.cpp

Loading...