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
