D22144: Add kio recentlyused:/ to access KActivityStats data

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  You asked for it :-)

INLINE COMMENTS

> CMakeLists.txt:3
> +
> +find_package(KF5 ${KF5_MIN_VERSION} REQUIRED COMPONENTS
> +    KIO

this was already done by the parent CMakeLists.txt, wasn't it? Why do it again?

> CMakeLists.txt:11
> +set(
> +    kio_recentlyused_SRCS
> +

join with previous line, it looks very strange this way

> CMakeLists.txt:29
> +set_target_properties(recentlyused PROPERTIES OUTPUT_NAME "recentlyused")
> +install(TARGETS recentlyused DESTINATION ${KDE_INSTALL_PLUGINDIR}/kf5/kio)

You should also set LIBRARY_OUTPUT_DIRECTORY, see https://community.kde.org/Guidelines_and_HOWTOs/Making_apps_run_uninstalled

> recentlyused.cpp:72
> +
> +bool isRootUrl(const QUrl &url)
> +{

prepend "static"

> recentlyused.cpp:91
> +    Q_UNUSED(newUrl);
> +    return false;
> +}

I may be missing something, but if you don't really reimplement rewriteUrl, then what's the point of using ForwardingSlaveBase in the first place, instead of just SlaveBase?

IIRC *all* of ForwardingSlaveBase's code is based on the rewriteUrl idea.

> recentlyused.cpp:109
> +        if (types.count() == 1) {
> +            query = query | Type(typeValue);
> +        } else {

(hmm, someone should implement operator |= in kactivities-stat, to make such code simpler and possibly faster)

> recentlyused.cpp:245
> +        KIO::UDSEntry uds;
> +        uds.reserve(5);
> +        uds.fastInsert(KIO::UDSEntry::UDS_NAME, dirName);

I count 6...

> recentlyused.cpp:256
> +    } else {
> +        // only the root path is supported
> +        error(KIO::ERR_DOES_NOT_EXIST, url.toDisplayString());

Isn't that a problem? Any user of this ioslave might stat() a URL representing a file or directory, coming from this kioslave.

E.g. if you paste a URL of a subdir from one dolphin window to another, I suspect it will happen then. At least in konqueror it does, maybe dolphin assumes everything is a directory since it can't do much with a file URL....

> recentlyused.h:36
> + *
> + *   Allows to filter the resources based on the activity there were used in.
> + *   Defaults to the current user activity.

s/there/they/

> recentlyused.h:38
> + *   Defaults to the current user activity.
> + *   Any option means include resources from any activity.
> + *   Example: recentlyused:/?activity=428fa590-1920-4b3c-a7e1-1842e6164707

You mean: the "any" value means...
Right?

> recentlyused.h:44
> + *   today and yesterday returns resource that had an event today or
> + *   yseterday respectively.
> + *   ISODATE must be of the form YYYY-MM-DD (YYYY is date, NN month, DD day of month)

typo: yesterday

> recentlyused.h:45
> + *   yseterday respectively.
> + *   ISODATE must be of the form YYYY-MM-DD (YYYY is date, NN month, DD day of month)
> + *   If a secondary date is passed, the filtering occurs on the date range starting

you mean "YYYY is year"

> recentlyused.h:47
> + *   If a secondary date is passed, the filtering occurs on the date range starting
> + *   at the fist date passed and ending at the second date passed inclusively
> + *   Example: recentlyused:/?date=2019-07-30

typo: first

> recentlyused.h:52
> + *   Filter on the name or names of the application that used the resource.
> + *   Default to any, meaning no agent filtering
> + *   Example: recentlyused:/?agent=kate,dolphin

The help on this option doesn't mention an "any" value.

> recentlyused.h:57
> + *
> + *   Filters resources based on the url of the resource, can contain schemes.
> + *   Path can contain '*', defaults to no filtering.

That's ambiguous. Is it a path or a URL?

A "path that can contain schemes" is an invalid mixture of two different things.
If it's a path, a '#' will mean an actual '#' in the filename.
If it's a URL, a '#' in the filename will require encoding as %23, since a '#' in the URL would actually mean a fragment.

> recentlyused.h:82
> + *
> + * @brief The RecentlyUsed implements an ioslave to access recently used resources
> + */

resources is such a generic word. We're talking about files here, and only files, aren't we? Or maybe files and directories. But not emails, contacts, databases, servers and whatever else, right?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22144

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190822/39cd0871/attachment.htm>