Differential D26121

Kicker/RecentDocument display file path as decoration
ClosedPublic
Actions

Authored by meven on Dec 20 2019, 1:44 PM.

Details

Reviewers
ngraham
ervin
hein
Group Reviewers
VDG
Plasma
Commits
R120:9099652598fa: Kicker/RecentDocument display file path as decoration
Summary

Also add an open containing folder action

Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20084
Build 20102: arc lint + arc unit
meven created this revision.Dec 20 2019, 1:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 20 2019, 1:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Dec 20 2019, 1:44 PM
meven edited the test plan for this revision. (Show Details)Dec 20 2019, 1:44 PM
meven updated this revision to Diff 71892.Dec 20 2019, 1:45 PM

Correct case

broulik added a subscriber: broulik.Dec 20 2019, 1:48 PM
broulik added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
269

Can you use the "~" for HOME like the runners do please

meven updated this revision to Diff 71893.Dec 20 2019, 2:09 PM

Replace homePath by '~'

meven marked an inline comment as done.Dec 20 2019, 2:09 PM
meven added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
269

I am thinking about adding this utility feature to KCoreAddons and avoid more duplication.

meven marked 2 inline comments as done.Dec 20 2019, 3:31 PM
meven added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
269

https://phabricator.kde.org/D26122

meven marked 2 inline comments as done.Dec 20 2019, 3:31 PM
ngraham added a comment.Dec 20 2019, 5:06 PM

Hmm, as your screenshot indicates, the subtitle is not very useful because it usually gets elided:

Not a -1, but I wonder how useful this will be in practice.

However big +1 on adding an "Open containing folder" menu item. Maybe that should be in a separate patch so it can go in quickly and we can discuss the subtitle separately>

davidre added a subscriber: davidre.Dec 20 2019, 5:42 PM
In D26121#580666, @ngraham wrote:

Hmm, as your screenshot indicates, the subtitle is not very useful because it usually gets elided:

Not a -1, but I wonder how useful this will be in practice.

However big +1 on adding an "Open containing folder" menu item. Maybe that should be in a separate patch so it can go in quickly and we can discuss the subtitle separately>

I think it could be useful if there are multiple files with the same name

ngraham accepted this revision.Dec 20 2019, 7:34 PM

Actually I see that other list items on other tabs already have path captions like this, so it's not a newly-introduced issue.

applets/kicker/plugin/recentusagemodel.cpp
360

Probably worth just doing this in a single line rather than defining a variable used only once

This revision is now accepted and ready to land.Dec 20 2019, 7:34 PM
meven marked an inline comment as done.Dec 20 2019, 9:14 PM
meven added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
360

I did this for readability, I think we should keep this.
The alternative is a little convoluted :

KIO::highlightInFileManager({QUrl::fromUserInput(resourceAt(row))});
meven marked an inline comment as done.EditedDec 20 2019, 9:18 PM
In D26121#580666, @ngraham wrote:

Hmm, as your screenshot indicates, the subtitle is not very useful because it usually gets elided:

Not a -1, but I wonder how useful this will be in practice.

However big +1 on adding an "Open containing folder" menu item. Maybe that should be in a separate patch so it can go in quickly and we can discuss the subtitle separately>

Your screenshot made me think, we might want to replace for instance "/var/run/100/kio..." but the user friendly name that dolphin would display in places when the path correspond to a mounted place or a place, i.e "Pictures" instead of "~/Pictures".
Like dolphin breadcrumbs does basically.

ngraham added a comment.Dec 20 2019, 10:26 PM

Yeah, I think that makes a lot of sense.

meven updated this revision to Diff 71950.EditedDec 21 2019, 2:45 PM

Use KFilePlacesModel to improve display of history

Before:

After:

meven edited the test plan for this revision. (Show Details)Dec 21 2019, 2:46 PM
ngraham added a comment.Dec 21 2019, 4:17 PM

Nice, I think this makes sense. Now Kickoff's Computer tab needs the same treatment or else the paths are oddly inconsistent.

meven added a comment.Dec 21 2019, 5:38 PM
In D26121#581070, @ngraham wrote:

Nice, I think this makes sense. Now Kickoff's Computer tab needs the same treatment or else the paths are oddly inconsistent.

In another diff, the patch is very simple : D26147

ngraham accepted this revision.Dec 21 2019, 7:27 PM
hein accepted this revision.Dec 22 2019, 4:20 PM
meven added a comment.Dec 24 2019, 8:14 AM

This was done to match my proposed recentlyused krunner : D26111
And the computer should be good now : D26147

ervin accepted this revision.Dec 26 2019, 2:19 PM

LGTM, might need an update if D26122 lands though.

meven added a comment.Dec 29 2019, 5:46 PM
In D26121#583212, @ervin wrote:

LGTM, might need an update if D26122 lands though.

No need to update here after D26122, the code uses now KFilePlacesModel to replace all places'path by their name rather than just ~.

Revision Contents
Changeset List

Diff 71892

View Options

applets/kicker/plugin/recentusagemodel.cpp

Loading...