Kicker/RecentDocument display file path as decoration
ClosedPublic

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

Details

Summary

Also add an open containing folder action

Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Fri, Dec 20, 1:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Dec 20, 1:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Fri, Dec 20, 1:44 PM
meven edited the test plan for this revision. (Show Details)Fri, Dec 20, 1:44 PM
meven updated this revision to Diff 71892.Fri, Dec 20, 1:45 PM

Correct case

broulik added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
278

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

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

Replace homePath by '~'

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

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

meven marked 2 inline comments as done.Fri, Dec 20, 3:31 PM
meven added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
278
meven marked 2 inline comments as done.Fri, Dec 20, 3:31 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>

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.Fri, Dec 20, 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
384

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.Fri, Dec 20, 7:34 PM
meven marked an inline comment as done.Fri, Dec 20, 9:14 PM
meven added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
384

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.EditedFri, Dec 20, 9:18 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>

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.

Yeah, I think that makes a lot of sense.

meven updated this revision to Diff 71950.EditedSat, Dec 21, 2:45 PM

Use KFilePlacesModel to improve display of history

Before:

After:

meven edited the test plan for this revision. (Show Details)Sat, Dec 21, 2:46 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.Sat, Dec 21, 5:38 PM

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.Sat, Dec 21, 7:27 PM
hein accepted this revision.Sun, Dec 22, 4:20 PM
meven added a comment.Tue, Dec 24, 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.Thu, Dec 26, 2:19 PM

LGTM, might need an update if D26122 lands though.

meven added a comment.Sun, Dec 29, 5:46 PM

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 ~.