Prior to finish D7446, create a better recently used documents feature
See https://phabricator.kde.org/D7446#485917 for motivation.
Details
- Reviewers
ivan ngraham dfaure - Group Reviewers
Frameworks - Commits
- R320:ae44042e541c: Add kio recentlyused:/ to access KActivityStats data
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 14056 Build 14074: arc lint + arc unit
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.
You asked for it :-)
recentlyused/CMakeLists.txt | ||
---|---|---|
4 ↗ | (On Diff #61860) | this was already done by the parent CMakeLists.txt, wasn't it? Why do it again? |
12 ↗ | (On Diff #61860) | join with previous line, it looks very strange this way |
30 ↗ | (On Diff #61860) | You should also set LIBRARY_OUTPUT_DIRECTORY, see https://community.kde.org/Guidelines_and_HOWTOs/Making_apps_run_uninstalled |
recentlyused/recentlyused.cpp | ||
73 ↗ | (On Diff #61860) | prepend "static" |
92 ↗ | (On Diff #61860) | 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 ↗ | (On Diff #61860) | (hmm, someone should implement operator |= in kactivities-stat, to make such code simpler and possibly faster) |
246 ↗ | (On Diff #61860) | I count 6... |
257 ↗ | (On Diff #61860) | 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 ↗ | (On Diff #61860) | s/there/they/ |
39 ↗ | (On Diff #61860) | You mean: the "any" value means... |
45 ↗ | (On Diff #61860) | typo: yesterday |
46 ↗ | (On Diff #61860) | you mean "YYYY is year" |
48 ↗ | (On Diff #61860) | typo: first |
53 ↗ | (On Diff #61860) | The help on this option doesn't mention an "any" value. |
58 ↗ | (On Diff #61860) | That's ambiguous. Is it a path or a URL? A "path that can contain schemes" is an invalid mixture of two different things. |
83 ↗ | (On Diff #61860) | 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? |
recentlyused/CMakeLists.txt | ||
---|---|---|
30 ↗ | (On Diff #61860) | This will be the first ioslave in kio-extras using it... |
recentlyused/recentlyused.cpp | ||
92 ↗ | (On Diff #61860) | 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. |
110 ↗ | (On Diff #61860) | |
257 ↗ | (On Diff #61860) | I implemented the kioslave to allow only stating and listing "/" path on purpose : I could not find a use case for subdirs. |
recentlyused/recentlyused.h | ||
58 ↗ | (On Diff #61860) | It is path currently due to limitation in RecentlyUsed::udsEntryFromResource being able to create valid UDS::Entry only for files. |
recentlyused/recentlyused.h | ||
---|---|---|
47 ↗ | (On Diff #61860) | NN should be MM. I would suggest to remove the additional explanation in brackes: YYYY-MM-DD is already clear enough. |
recentlyused/recentlyused.cpp | ||
---|---|---|
110 ↗ | (On Diff #61860) | Also, there are set* and add* member functions which can be used for non-chained changes. |
recentlyused/recentlyused.cpp | ||
---|---|---|
110 ↗ | (On Diff #61860) | We don't have yet set function for Activity, Type, Agent, and Url only add. |
recentlyused/recentlyused.cpp | ||
---|---|---|
110 ↗ | (On Diff #61860) | 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. |
recentlyused/recentlyused.cpp | ||
---|---|---|
92 ↗ | (On Diff #61860) | I'm confused. Which code from ForwardingSlaveBase is this class benefiting from? If you decide not to support listing/stat'ing subdirs then indeed you don't need ForwardingSlaveBase as base class. |
recentlyused/recentlyused.cpp | ||
---|---|---|
187 ↗ | (On Diff #61860) | You don't need spaces between items, when using q[C]Debug |
recentlyused/recentlyused.h | ||
58 ↗ | (On Diff #61860) | 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. |
Inherit from SlaveBase instead of ForwardingSlaveBase, rename url parameter to path parameter
recentlyused/recentlyused.cpp | ||
---|---|---|
92 ↗ | (On Diff #61860) | After more testing, inheriting from ForwardSlaveBase was not needed. |
@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.
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.
. 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.
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
I think this can go in now. It's been reviewed by a bunch of the big cheeses. :) Very nice work!
CMakeLists.txt | ||
---|---|---|
68 ↗ | (On Diff #61860) | Please add a min version, as the code seems to rely on new API only added recently: find_package(KF5ActivitiesStats 5.62.0 QUIET) |
CMakeLists.txt | ||
---|---|---|
68 ↗ | (On Diff #61860) | Thanks for the quick fixes :) |
This can be done: http://www.davidfaure.fr/2019/app_pid.diff works for me, feel free to use it.