Runner: make recentdocument use KActivityStats data
Needs ReviewPublic

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 20820
Build 20838: 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.Fri, Jan 24, 7:14 PM
meven edited the summary of this revision. (Show Details)
meven added a comment.Fri, Jan 24, 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.Sun, Feb 16, 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.Sun, Feb 16, 5:19 PM

Works for me with documents on my samba share.