D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  In D26111#589209 , @ivan wrote:
  
  > > Why not change the recentdocuments runner?
  >
  >
  
  
  Will do

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Ivan Čukić]

ivan added a comment.


  > Why not change the recentdocuments runner?
  
  Looks fine to me, I just don't see a response to this comment.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven added a comment.


  Anyone to review this ?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven marked 3 inline comments as done.
meven added a comment.


  In D26111#580344 , @meven wrote:
  
  > 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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven updated this revision to Diff 71865.
meven added a comment.


  Better Name in json

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26111?vs=71858=71865

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26111

AFFECTED FILES
  CMakeLists.txt
  runners/CMakeLists.txt
  runners/recentlyused/CMakeLists.txt
  runners/recentlyused/Messages.sh
  runners/recentlyused/plasma-runner-recentlyused.desktop
  runners/recentlyused/recentlyused.cpp
  runners/recentlyused/recentlyused.h

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven added a comment.


  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.

INLINE COMMENTS

> broulik wrote in recentlyused.cpp:93
> I think plain mime type icon would be sufficient?

I don't think so, having icons to distinguish between folders, pdfs and images 
is very user useful I think.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven updated this revision to Diff 71858.
meven marked 5 inline comments as done.
meven added a comment.


  address review

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26111?vs=71857=71858

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26111

AFFECTED FILES
  CMakeLists.txt
  runners/CMakeLists.txt
  runners/recentlyused/CMakeLists.txt
  runners/recentlyused/Messages.sh
  runners/recentlyused/plasma-runner-recentlyused.desktop
  runners/recentlyused/recentlyused.cpp
  runners/recentlyused/recentlyused.h

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Kai Uwe Broulik]

broulik requested changes to this revision.
broulik added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> recentlyused.cpp:93
> +}
> +match.setIconName(KIO::iconNameForUrl(url));
> +match.setRelevance(relevance);

I think plain mime type icon would be sufficient?

> recentlyused.cpp:95
> +match.setRelevance(relevance);
> +match.setData(url);
> +match.setText(name);

`setData` takes `QVariant`, so you can just store the `QUrl` in here, saves you 
the string to URL dances below

> recentlyused.cpp:97
> +match.setText(name);
> +match.setSubtext(i18n("In %1", 
> url.adjusted(QUrl::RemoveFilename).path()));
> +

We typically just show the folder path and use `~` for home, cf. existing runner

> recentlyused.cpp:109
> +
> +if (match.selectedAction() && match.selectedAction() == 
> action(s_openParentDirId)) {
> +KIO::highlightInFileManager({QUrl(url)});

the first part isn't needed, the comparison is enough

> recentlyused.cpp:125
> +
> +if (url.isLocalFile() && !QFileInfo(url.toLocalFile()).isDir()) {
> +actions << action(s_openParentDirId);

I think it's perfectly fine trying to show where a folder is inside another 
folder

> recentlyused.cpp:135
> +QMimeData *result = new QMimeData();
> +result->setText(match.data().toString());
> +return result;

`setUrls`

> recentlyused.h:25
> +
> +#include 
> +

Unused in the header

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham, broulik
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven added a comment.


  I am planning to remove recentdocument later.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26111

To: meven, #plasma, ivan, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26111: KRunner add a recentlyused runner accessing KActivityStats data

[Méven Car]

meven created this revision.
meven added reviewers: Plasma, ivan, ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
meven requested review of this revision.

REVISION 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

TEST PLAN
  Manually tested

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26111

AFFECTED FILES
  CMakeLists.txt
  runners/CMakeLists.txt
  runners/recentlyused/CMakeLists.txt
  runners/recentlyused/Messages.sh
  runners/recentlyused/plasma-runner-recentlyused.desktop
  runners/recentlyused/recentlyused.cpp
  runners/recentlyused/recentlyused.h

To: meven, #plasma, ivan, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart