It is much more versatile over the current recentdocument runner :
- It is activity aware
- It has a longer history
- It includes data from gtk apps
- We will be able to add some quick filtering in krunner
No Linters Available |
No Unit Test Coverage |
Buildable 20819 | |
Build 20837: arc lint + arc unit |
Why not change the recentdocuments runner? We have various places where we whitelist recentdocuments as a runner and if a user disabled it, this would not be carried over without a confi update script.
Also, you might want to look at the old runner code for some of its behavior (e.g. the subtext and mimetype handling) to keep it consistent.
Also, is kactivitiesstats thread-safe?
runners/recentlyused/recentlyused.cpp | ||
---|---|---|
93 ↗ | (On Diff #71857) | I think plain mime type icon would be sufficient? |
95 ↗ | (On Diff #71857) | setData takes QVariant, so you can just store the QUrl in here, saves you the string to URL dances below |
97 ↗ | (On Diff #71857) | We typically just show the folder path and use ~ for home, cf. existing runner |
109 ↗ | (On Diff #71857) | the first part isn't needed, the comparison is enough |
125 ↗ | (On Diff #71857) | I think it's perfectly fine trying to show where a folder is inside another folder |
135 ↗ | (On Diff #71857) | setUrls |
runners/recentlyused/recentlyused.h | ||
25 ↗ | (On Diff #71857) | Unused in the header |
Two points raised by @broulik but not resolved :
I am in favor of keeping those two behaviors, but I would welcome other constructive opinions.
runners/recentlyused/recentlyused.cpp | ||
---|---|---|
93 ↗ | (On Diff #71857) | I don't think so, having icons to distinguish between folders, pdfs and images is very user useful I think. |
Why not change the recentdocuments runner?
Looks fine to me, I just don't see a response to this comment.
I'm not the original author. I guess I appear because of the git repository split or something.
Gentle ping dear reviewers.
runners/recentdocuments/recentdocuments.cpp | ||
---|---|---|
69 | Request for now is limited, it will need some work in KActivityStats to be more clever |
Please if someone could review/test, it makes the recent document krunner so much more useful, with longer history and gtk apps history.
It seems to me like a worthwhile feature.
runners/recentdocuments/recentdocuments.cpp | ||
---|---|---|
98 | You could use KShell:: tildeCollapse here :-) |
runners/recentdocuments/recentdocuments.cpp | ||
---|---|---|
98 | Indeed, the irony is that I created KShell::titdeCollapes, but this diff is older than KF that introduced it... |
Clean CMakeLists, use KShell::tildeCollapse, avoid stating in RecentDocuments::actionsForMatch
runners/recentdocuments/CMakeLists.txt | ||
---|---|---|
13 | Where's this being used? I only see stats | |
runners/recentdocuments/recentdocuments.cpp | ||
77–78 | Do we really want QUrl::fromUserInput here? This thing typically assumes web addresses. Maybe pass AssumeLocalFile? But I recal that doing a file system access every time checking for existance.. probably not a big deal in this instance since it's in a thread | |
87 | Coding style |
I need to change dev machine, now that plasma-workspace needs Qt5.14, that explains the delay.
runners/recentdocuments/recentdocuments.cpp | ||
---|---|---|
77–78 | It is needed for QString coming from KActivitiesStats, they can be "/home/meven/doc.txt" or "file://" or "application://" or "http://". |
runners/recentdocuments/recentdocuments.cpp | ||
---|---|---|
77–78 | Well workingdirectory can be empty in fact. |