D8347: KIO::PreviewJob::defaultPlugins() function
This revision was automatically updated to reflect the committed changes. Closed by commit R241:3042e12296db: KIO::PreviewJob::defaultPlugins() function (authored by ngraham). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8347?vs=21004=21005#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=21004=21005 REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham updated this revision to Diff 21004. ngraham added a comment. Correcting the @since version number REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=20975=21004 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
broulik added a comment. 5.39 was just released so it will be 5.40 REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked 2 inline comments as done. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked an inline comment as done. ngraham added inline comments. INLINE COMMENTS > markg wrote in previewjob.h:198 > You miss an @since line. 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. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham updated this revision to Diff 20975. ngraham added a comment. Updating header documentation with @returns and @since REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=20927=20975 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added a comment. 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. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
markg accepted this revision. markg added a comment. This revision is now accepted and ready to land. In https://phabricator.kde.org/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! REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added a comment. I still need to add a @since in the header documentation, after which point hopefully @markg will approve. :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
broulik added a comment. Agreed. I'd say if nobody objects this week this is good to go REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked 2 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked an inline comment as done.
ngraham added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in previewjob.cpp:740
> const QStringList blackList = { QStringLiteral("textthumbnail") };
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.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8347
To: ngraham, #frameworks, broulik, #dolphin, markg
Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
broulik added a comment.
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.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8347
To: ngraham, #frameworks, broulik, #dolphin, markg
Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
markg requested changes to this revision. markg added a comment. This revision now requires changes to proceed. 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. INLINE COMMENTS > previewjob.h:198 > + * Returns a list of plugins that should be enabled by default, which is > all plugins > + * Minus the plugins specified in an internal blacklist > + */ You miss an @since line. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin, markg Cc: markg, anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
anthonyfieroni added inline comments.
INLINE COMMENTS
> previewjob.cpp:740
> +{
> +QStringList blacklist = QStringList()
> +<< QStringLiteral("textthumbnail");
const QStringList blackList = { QStringLiteral("textthumbnail") };
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8347
To: ngraham, #frameworks, broulik, #dolphin
Cc: anthonyfieroni, elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added a dependent revision: D8352: Use KIO::PreviewJob::defaultPlugins(). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham updated this revision to Diff 20927. ngraham added a comment. Whitespace fix REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=20926=20927 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham updated this revision to Diff 20926. ngraham marked an inline comment as done. ngraham added a comment. Style improvements/corrections REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=20923=20926 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham marked 3 inline comments as done. ngraham added inline comments. INLINE COMMENTS > elvisangelaccio wrote in previewjob.cpp:740-741 > Shouldn't the blacklist be user-configurable? 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 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
elvisangelaccio added inline comments.
INLINE COMMENTS
> previewjob.cpp:740-741
> +{
> +QStringList blacklist = QStringList()
> +<< QStringLiteral("textthumbnail");
> +
Shouldn't the blacklist be user-configurable?
> previewjob.cpp:745
> +foreach (const QString plugin, blacklist)
> +{
> +defaultPlugins.removeAll(plugin);
code style, brace should be at the end of the previous line
> previewjob.h:203-206
> +/**
> + * Returns a list of plugins that should be enabled by default, which is
> all plugins
> + * Minus the plugins specified in an internal blacklist
> + */
Documentation should be above the method, not below.
> previewjob.h:216
> */
> +
> #ifndef KIOWIDGETS_NO_DEPRECATED
Unrelated whitespace change
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8347
To: ngraham, #frameworks, broulik, #dolphin
Cc: elvisangelaccio, #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham updated this revision to Diff 20923. ngraham added a comment. Updating comments/documentation REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8347?vs=20916=20923 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham, #frameworks, broulik, #dolphin Cc: #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added a dependent revision: D7440: Turn on Dolphin icon previews by default. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham added reviewers: Frameworks, broulik, Dolphin. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8347 To: ngraham, #frameworks, broulik, #dolphin Cc: #frameworks
D8347: KIO::PreviewJob::defaultPlugins() function
ngraham created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Adds a new KIO::PreviewJob::defaultPlugins() function that returns all available plugins minus ones specified in an internal blacklist. This supports https://phabricator.kde.org/D7440. TEST PLAN Tested in KDE Neon. Function is available and works as expected. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D8347 AFFECTED FILES src/widgets/previewjob.cpp src/widgets/previewjob.h To: ngraham Cc: #frameworks
