Add kio recentlyused:/ to access KActivityStats data
ClosedPublic

Authored by meven on Jun 28 2019, 2:50 PM.

Details

Summary

Prior to finish D7446, create a better recently used documents feature
See https://phabricator.kde.org/D7446#485917 for motivation.

Test Plan

compile & install
kdeinit5
kioclient5 ls recentlyused:/?limit=100
Or in dolphin

Current filter options :
agent, activity, type(mimetype), url(path fitering)
order and limit

Examples (to use with kioclient5 or see in dolphin) :
recentlyused:/?type=inode/directory
recentlyused:/?type=video/*,audio/*
recentlyused:/?path=/home/meven/kde/src/*&type=text/plain
recentlyused:/?order=HighScoredFirst
recentlyused:/?agent=gwenview

See recentlyused.h for more details.

Diff Detail

Repository
R320 KIO Extras
Branch
arcpatch-D22144
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14336
Build 14354: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven updated this revision to Diff 63829.Aug 15 2019, 5:50 PM

run uncrustify-kf5

ivan added a comment.Aug 19 2019, 6:26 PM

Looks OK to me, I guess some of our resident KIO experts should review it. What do you think?

meven added a comment.Aug 20 2019, 6:00 AM
In D22144#514633, @ivan wrote:

Looks OK to me, I guess some of our resident KIO experts should review it. What do you think?

Definitely. Do you have an idea who ?

BTW, I have one change planned for this waiting for D22775.

I'v added @dfaure as reviewer.

David feel free to not review this if you think somebody else would be better suited.

meven updated this revision to Diff 64347.Aug 22 2019, 6:30 PM

Add date and date range filtering after D22775

In D22144#514633, @ivan wrote:

Looks OK to me, I guess some of our resident KIO experts should review it. What do you think?

Could you change your status to Accepted or Resigned??

dfaure requested changes to this revision.Aug 22 2019, 9:10 PM

You asked for it :-)

recentlyused/CMakeLists.txt
4

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

12

join with previous line, it looks very strange this way

30

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

recentlyused/recentlyused.cpp
73

prepend "static"

92

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.

110

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

246

I count 6...

257

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/recentlyused.h
37

s/there/they/

39

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

45

typo: yesterday

46

you mean "YYYY is year"

48

typo: first

53

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

58

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.

83

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?

This revision now requires changes to proceed.Aug 22 2019, 9:10 PM
meven updated this revision to Diff 64403.Aug 23 2019, 9:27 AM
meven marked 15 inline comments as done.

Fix documentation, CMakeLists.txt, a couple of code fix

meven added inline comments.Aug 23 2019, 9:31 AM
recentlyused/CMakeLists.txt
30

This will be the first ioslave in kio-extras using it...
Some of the ioslave even use the old protocol files...
Might be a followup maintenance Diff...

recentlyused/recentlyused.cpp
92

I started my work upon the recentdocument ioslave that used it and ForwardingSlaveBase documentation states "It has been designed to support only local filesystem like ioslaves.. If the resulting ioslave should be a simple proxy, you only need to implement the ForwardingSlaveBase::rewriteUrl() method." makes it sound like that is what I needed.
Using SlaveBase would need a lot more code added also that I don't particularly need to implement here.
But please correct me.

110

Tried implementing this in D23372 but @ivan was not found of it because of induced complexity it entails to work properly.

257

I implemented the kioslave to allow only stating and listing "/" path on purpose : I could not find a use case for subdirs.
I chose url parameters to pass pararmeters for simplicity albeit it makes it less appealing in UI as you cannot have subpath breaking down your parameters.
Any files or folders appearing in the ioslave keep their true urls.

recentlyused/recentlyused.h
58

It is path currently due to limitation in RecentlyUsed::udsEntryFromResource being able to create valid UDS::Entry only for files.
But it could evolve later to support any url that the sqlite database ~/.local/share/kactivitymanagerd/resources/database stores in its targettedResource column, including ioslave urls, kcm urls, applications desktop files...

meven updated this revision to Diff 64406.Aug 23 2019, 9:59 AM

Fix date parameter passing

meven marked 5 inline comments as done.Aug 23 2019, 11:03 AM
dhaumann added inline comments.
recentlyused/recentlyused.h
47

NN should be MM.

I would suggest to remove the additional explanation in brackes: YYYY-MM-DD is already clear enough.

meven updated this revision to Diff 64454.Aug 24 2019, 6:52 AM

Fix ISDATE description

meven marked an inline comment as done.Aug 24 2019, 6:52 AM
ivan added inline comments.Aug 24 2019, 8:07 AM
recentlyused/recentlyused.cpp
110

Also, there are set* and add* member functions which can be used for non-chained changes.

meven added inline comments.Aug 24 2019, 8:52 AM
recentlyused/recentlyused.cpp
110

We don't have yet set function for Activity, Type, Agent, and Url only add.
Which means we would need to use query.addActivity(QStringList() << ":any")) sometimes for instance, that is not ideal since it makes the user class use internal details such as ":any".
I would be in favor to add setActivity(Terms::Activity) in KactivitiesStats same for Type, Agent and Url, to get arount that and maybe also addActivity(Terms::Activity) as well for consistency.

ivan added inline comments.Aug 24 2019, 10:49 AM
recentlyused/recentlyused.cpp
110

You have Activity::any() - no need for the client to use the special values. You can do a addActivities({Activity::any()}).

I would not mind to add set* variants of add* functions - setActivities, setTypes, etc. to avoid the need of calling clear* and then add*.

I'm also thinking whether it would be useful to have a variadic template versions of these so that you can skip the QList<T> construction. That might be better to wait for Qt 6 and raised C++ version requirement. Not sure.

dfaure added inline comments.Aug 24 2019, 12:12 PM
recentlyused/recentlyused.cpp
92

I'm confused. Which code from ForwardingSlaveBase is this class benefiting from?
With internalRewriteUrl returning false all SlaveBase reimplementations in that class do nothing,

If you decide not to support listing/stat'ing subdirs then indeed you don't need ForwardingSlaveBase as base class.

meven planned changes to this revision.Aug 24 2019, 5:25 PM

inherit from SlaveBase instead of ForwardingSlaveBase

meven updated this revision to Diff 64504.Aug 24 2019, 5:39 PM

Use set*(Term)

meven planned changes to this revision.Aug 25 2019, 7:23 AM

inherit from SlaveBase instead of ForwardingSlaveBase

dfaure added inline comments.Aug 25 2019, 10:23 AM
recentlyused/recentlyused.cpp
187

You don't need spaces between items, when using q[C]Debug

recentlyused/recentlyused.h
58

If you want this to support non-local URLs one day then it should probably take URLs already now.

Alternatively call the argument "path" and leave "url" for later.

meven updated this revision to Diff 64736.Aug 27 2019, 1:10 PM

Inherit from SlaveBase instead of ForwardingSlaveBase, rename url parameter to path parameter

meven edited the test plan for this revision. (Show Details)Aug 27 2019, 1:10 PM
meven marked 7 inline comments as done.Aug 28 2019, 5:22 AM
meven added inline comments.
recentlyused/recentlyused.cpp
92

After more testing, inheriting from ForwardSlaveBase was not needed.

meven marked 2 inline comments as done.Aug 28 2019, 5:22 AM
meven added a comment.Aug 28 2019, 5:43 AM

@dfaure A missing piece for this kio slave is to be able to filter to the current app, meaning the app using the slave.
I am not sure there is a way to achieve this yet, please correct me if I am wrong, I've looked around but I could have missed something.

dfaure accepted this revision.Aug 28 2019, 7:38 AM

No idea about kactivitystats, that's a question for Ivan.

ivan accepted this revision.Aug 28 2019, 7:42 AM

Filtering can be done as far as the KAStats is concerned. The question is how to know inside the KIO slave which application requested its services.

This revision is now accepted and ready to land.Aug 28 2019, 7:42 AM

. The question is how to know inside the KIO slave which application requested its services.

If all else fails we could pass a query parameter recentdocuments://?application=... since this is mainly meant for the file dialog I presume, there could be some magic in KFilePlacesModel similiar to how we generate the baloo tags:/ URLs.

Yep. Either query argument, or KIO metaData(). Both require explicit action from the application, there's no magic currently to know which app is making the request. We could add that, but there are so many way to identify an application that I'm not sure it would work for all cases.

meven added a comment.Aug 28 2019, 9:05 AM

Yep. Either query argument, or KIO metaData(). Both require explicit action from the application, there's no magic currently to know which app is making the request. We could add that, but there are so many way to identify an application that I'm not sure it would work for all cases.

This "magic" precisely is what I would have wished to find, it seemed to me something other ioslaves might want, to have contextual log or error message at least.
The pid of the calling application could be a minimum.
The application parameter is already here, it is called agent here.
It will be on KFileWidget to fill this parameter, which should be fine.
Anyway that's another day issue.

Should I wait a few days more days for review, or merge to broaden the testing population ?
D7446 is the next step

meven updated this revision to Diff 64845.Aug 28 2019, 1:23 PM

Fix path example

I think this can go in now. It's been reviewed by a bunch of the big cheeses. :) Very nice work!

This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.
CMakeLists.txt
68

Please add a min version, as the code seems to rely on new API only added recently:

find_package(KF5ActivitiesStats 5.62.0 QUIET)
meven marked an inline comment as done.Aug 29 2019, 7:01 AM
meven added inline comments.
CMakeLists.txt
68

See D23556

meven marked 2 inline comments as done.Aug 29 2019, 7:01 AM
kossebau added inline comments.Aug 29 2019, 1:51 PM
CMakeLists.txt
68

Thanks for the quick fixes :)

dfaure added a comment.Sep 2 2019, 1:15 PM

This "magic" precisely is what I would have wished to find, it seemed to me something other ioslaves might want, to have contextual log or error message at least.
The pid of the calling application could be a minimum.

This can be done: http://www.davidfaure.fr/2019/app_pid.diff works for me, feel free to use it.