Runner: make recentdocument use KActivityStats data
AbandonedPublic

Authored by meven on Dec 19 2019, 9:18 PM.

Details

Reviewers
ivan
ngraham
broulik
Group Reviewers
Plasma
Summary

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
Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D26111
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20819
Build 20837: arc lint + arc unit
meven created this revision.Dec 19 2019, 9:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 19 2019, 9:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Dec 19 2019, 9:18 PM
meven edited the test plan for this revision. (Show Details)Dec 19 2019, 9:19 PM

I am planning to remove recentdocument later.

broulik requested changes to this revision.EditedDec 19 2019, 9:30 PM
broulik added a subscriber: broulik.

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

This revision now requires changes to proceed.Dec 19 2019, 9:30 PM
meven updated this revision to Diff 71858.Dec 19 2019, 10:15 PM
meven marked 5 inline comments as done.

address review

Two points raised by @broulik but not resolved :

  • should display a specific icon based on file mimetype ?
  • should we allow to open a folder's parent folder through an action

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.

meven updated this revision to Diff 71865.Dec 20 2019, 7:31 AM

Better Name in json

meven marked 3 inline comments as done.Jan 1 2020, 6:40 PM

Two points raised by @broulik but not resolved :

  • should display a specific icon based on file mimetype ?
  • should we allow to open a folder's parent folder through an action

    I am in favor of keeping those two behaviors, but I would welcome other constructive opinions.

ping

meven added a comment.Jan 6 2020, 8:37 PM

Anyone to review this ?

ivan added a comment.Jan 6 2020, 9:41 PM

Why not change the recentdocuments runner?

Looks fine to me, I just don't see a response to this comment.

meven planned changes to this revision.Jan 8 2020, 10:16 AM
In D26111#589209, @ivan wrote:

Why not change the recentdocuments runner?

Will do

meven retitled this revision from KRunner add a recentlyused runner accessing KActivityStats data to Runner: make recentdocument use KActivityStats data.Jan 8 2020, 6:37 PM
meven updated this revision to Diff 73086.Jan 8 2020, 6:40 PM

Instead of adding a runner update the documentrecent one

meven updated this revision to Diff 73087.Jan 8 2020, 6:42 PM

Clean bad referrence

meven edited the test plan for this revision. (Show Details)Jan 8 2020, 7:24 PM
meven added a reviewer: apol.Jan 16 2020, 9:04 PM

Adding @apol as reviewers as he was the original author of this runner.

apol added a comment.Jan 17 2020, 1:15 AM

I'm not the original author. I guess I appear because of the git repository split or something.

meven removed a reviewer: apol.Jan 24 2020, 7:14 PM
meven edited the summary of this revision. (Show Details)
meven added a comment.Jan 24 2020, 7:18 PM

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

meven marked an inline comment as done.Feb 16 2020, 9:45 AM

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.

ngraham accepted this revision.Feb 16 2020, 5:19 PM

Works for me with documents on my samba share.

I will this for a few days and merge if no more feedback is sent.

alex added a subscriber: alex.Apr 11 2020, 12:22 PM
meven updated this revision to Diff 80516.Apr 19 2020, 5:56 AM

Rebasing after D28369

meven added a comment.Apr 27 2020, 1:34 PM

ping whoever...

alex added inline comments.Apr 27 2020, 1:47 PM
runners/recentdocuments/recentdocuments.cpp
98

You could use KShell:: tildeCollapse here :-)

meven added inline comments.Apr 27 2020, 1:50 PM
runners/recentdocuments/recentdocuments.cpp
98

Indeed, the irony is that I created KShell::titdeCollapes, but this diff is older than KF that introduced it...
Thanks

+1

CMakeLists.txt
85

Do we not have this dep in p-w these days already through kicker backend?

runners/recentdocuments/recentdocuments.cpp
129

Why not for dirs? If you have a recent docs on a defunct network share it would block now

meven updated this revision to Diff 81413.Apr 28 2020, 7:35 AM
meven marked 3 inline comments as done.

Clean CMakeLists, use KShell::tildeCollapse, avoid stating in RecentDocuments::actionsForMatch

meven marked an inline comment as done.Apr 28 2020, 7:36 AM
broulik accepted this revision.Apr 28 2020, 3:30 PM
broulik added inline comments.
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

This revision is now accepted and ready to land.Apr 28 2020, 3:30 PM
meven marked 2 inline comments as done.May 9 2020, 5:54 AM

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://".
So QUrl::fromEncoded or QUrl::fromLocalFile can't be used.
AssumeLocalFile would be nice to use, but it requires to provide a working directory. https://doc.qt.io/qt-5/qurl.html#fromUserInput-1

meven abandoned this revision.May 29 2020, 6:52 AM
meven marked an inline comment as done.
runners/recentdocuments/recentdocuments.cpp
77–78

Well workingdirectory can be empty in fact.