D22144: Add kio recentlyused:/ to access KActivityStats data

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

D22144: Add kio recentlyused:/ to access KActivityStats data

Nathaniel Graham
View Revision
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.

You asked for it :-)


INLINE COMMENTS
View InlineCMakeLists.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?


View InlineCMakeLists.txt:11
set(
kio_recentlyused_SRCS

join with previous line, it looks very strange this way


View InlineCMakeLists.txt:29
set_target_properties(recentlyused PROPERTIES OUTPUT_NAME "recentlyused")

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


View Inlinerecentlyused.cpp:72
bool isRootUrl(const QUrl &url)
{

prepend "static"


View Inlinerecentlyused.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.


View Inlinerecentlyused.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)


View Inlinerecentlyused.cpp:245
KIO::UDSEntry uds;
uds.reserve(5);
uds.fastInsert(KIO::UDSEntry::UDS_NAME, dirName);

I count 6...


View Inlinerecentlyused.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....


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

s/there/they/


View Inlinerecentlyused.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?


View Inlinerecentlyused.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


View Inlinerecentlyused.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"


View Inlinerecentlyused.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


View Inlinerecentlyused.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.


View Inlinerecentlyused.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.


View Inlinerecentlyused.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


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