[Places panel] Revamp the Recently Saved section
ClosedPublic

Authored by meven on Aug 20 2017, 11:06 PM.

Details

Summary

FEATURE: 110016
FEATURE: 159299
FEATURE: 311218
FIXED-IN: 5.63

Revamp the Recently Saved section, including the following changes:

  • Change the section name to "Recent"
  • Change the name of the existing entries to "Modified Today" and "Modified Yesterday", for clarity.
  • Add "Recent Files" and "Recent Locations" item corresponding to recentlyused:/files and recentlyused:/locations listing the last 30 files or directories accessed in the current activity, and adjust the code to support this.

Depends on D22144
Depends on D23742
Depends on D23737
Depends on D23741
Depends on D23760

Test Plan

LANG="EN" ctest

Adds "Recent Files" "Recent Locations" entries to user accounts places recent group.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven added a comment.Sep 1 2019, 5:49 AM

Should we add this to existing users places settings ?
Like we do with withBaloo for instance, adding it if it was not added before.

src/filewidgets/kfileplacesmodel.cpp
328

Should we add this to existing places settings ?
Like we do with withBaloo for instance, adding it if it was not added before.

I think so. It's a very big upgrade, and we're going to be mentioning it in a lot of promo materials. People may get confused and annoyed if they don't see it after reading about it in the release announcement or a U&P blog post.

meven updated this revision to Diff 65191.Sep 2 2019, 5:14 AM

Rebase

meven updated this revision to Diff 65197.Sep 2 2019, 6:32 AM

Add recentlyused:/ entry to Recent group to existing users

meven updated this revision to Diff 65198.Sep 2 2019, 6:35 AM

Better variable naming

meven edited the test plan for this revision. (Show Details)Sep 2 2019, 6:39 AM
meven updated this revision to Diff 65199.Sep 2 2019, 6:39 AM

arc amend

meven added a comment.Sep 5 2019, 1:53 PM

ping @ngraham any luck testing this ?

ngraham accepted this revision.Sep 5 2019, 2:08 PM

This works great.

one minor concern I have with the new IOSlave in general is how it shows both files and folders. For me, the folders are a distraction from the file listing. But I was thinking: how about adding modes that allow it to show only files or only folders? Like the IOSlave could support recentlyused:/files and recentlyused:/folders, each of which would limit the display to just those things. Then under the "Recent" section, we could have "Recent Files" and "Recent Locations", which would allow us to remove the useless old "Today" entry without any trouble.

This would provide a better fix for https://bugs.kde.org/show_bug.cgi?id=159299.

ngraham edited the summary of this revision. (Show Details)Sep 5 2019, 2:09 PM
meven added a comment.Sep 5 2019, 2:18 PM

This works great.

one minor concern I have with the new IOSlave in general is how it shows both files and folders. For me, the folders are a distraction from the file listing. But I was thinking: how about adding modes that allow it to show only files or only folders? Like the IOSlave could support recentlyused:/files and recentlyused:/folders, each of which would limit the display to just those things. Then under the "Recent" section, we could have "Recent Files" and "Recent Locations", which would allow us to remove the useless old "Today" entry without any trouble.

This would provide a better fix for https://bugs.kde.org/show_bug.cgi?id=159299.

Great suggestions, I will add those to the kio slave.

meven added a comment.Sep 5 2019, 3:53 PM
In D7446#526039, @meven wrote:

This works great.

one minor concern I have with the new IOSlave in general is how it shows both files and folders. For me, the folders are a distraction from the file listing. But I was thinking: how about adding modes that allow it to show only files or only folders? Like the IOSlave could support recentlyused:/files and recentlyused:/folders, each of which would limit the display to just those things. Then under the "Recent" section, we could have "Recent Files" and "Recent Locations", which would allow us to remove the useless old "Today" entry without any trouble.

This would provide a better fix for https://bugs.kde.org/show_bug.cgi?id=159299.

Great suggestions, I will add those to the kio slave.

D23742 is here, depends on D23736, also two fix to make sure we have mime type of files in the database : fix to kactivitymanagerd D23737 when getting history from gtk apps and D23741 from gwenview

Fantastic work. That's exactly what I had in mind and it works perfectly!

With those, we can have Recent Files and Recent Locations (or maybe Recent Folders) in the Recent section, and we can get rid of the existing Today entry. That will make this patch perfect.

meven added a comment.Sep 5 2019, 5:57 PM

Fantastic work. That's exactly what I had in mind and it works perfectly!

With those, we can have Recent Files and Recent Locations (or maybe Recent Folders) in the Recent section, and we can get rid of the existing Today entry. That will make this patch perfect.

+1 for Recent Locations

Will replace the entry recentlyused:/ by recentlyused:/files aka "Recent files" and recentlyused:/folders aka "Recent Locations".

In D7446#526304, @meven wrote:

Fantastic work. That's exactly what I had in mind and it works perfectly!

With those, we can have Recent Files and Recent Locations (or maybe Recent Folders) in the Recent section, and we can get rid of the existing Today entry. That will make this patch perfect.

+1 for Recent Locations

Will replace the entry recentlyused:/ by recentlyused:/files aka "Recent files" and recentlyused:/folders aka "Recent Locations".

Yay!

meven added a comment.Sep 6 2019, 3:25 AM

@ngraham you added some dependencies but D23737 and D23741 are not direct dependencies, but rather related fixes.
Still, I don't mind leaving them as dependencies.

meven added a comment.Sep 6 2019, 4:17 AM

It would be great to have two different icons for recentlyused:/locations and recentlyused:/files.
Also I am not sure recentlyuserd:/ should use "document-open-recent-symbolic" since it is monochrone, the kio declares currently "document-open-recent".
Maybe "folder-temp" for /locations.
Any suggestion @ngraham ?

In D7446#526494, @meven wrote:

It would be great to have two different icons for recentlyused:/locations and recentlyused:/files.
Also I am not sure recentlyuserd:/ should use "document-open-recent-symbolic" since it is monochrone, the kio declares currently "document-open-recent".
Maybe "folder-temp" for /locations.
Any suggestion @ngraham ?

Good point. We probably need a new icon. document-open-recent is fine for recentlyused:/files, however for recentlyused:/locations we'll need a folder-open-recent that uses the same style, but with a folder as the background instead of a file. Ideally both would also have appropriate colorful versions for the >22px sizes but that's not a hard requirement.

There's plenty of time before the 19.12 release. Please file a bug to Breeze | Icons requesting the new icon and mention it in the VDG chatroom. Thanks!

meven added a comment.Sep 6 2019, 6:15 AM
In D7446#526494, @meven wrote:

It would be great to have two different icons for recentlyused:/locations and recentlyused:/files.
Also I am not sure recentlyuserd:/ should use "document-open-recent-symbolic" since it is monochrone, the kio declares currently "document-open-recent".
Maybe "folder-temp" for /locations.
Any suggestion @ngraham ?

Good point. We probably need a new icon. document-open-recent is fine for recentlyused:/files, however for recentlyused:/locations we'll need a folder-open-recent that uses the same style, but with a folder as the background instead of a file. Ideally both would also have appropriate colorful versions for the >22px sizes but that's not a hard requirement.

There's plenty of time before the 19.12 release. Please file a bug to Breeze | Icons requesting the new icon and mention it in the VDG chatroom. Thanks!

Thanks
Done: https://bugs.kde.org/show_bug.cgi?id=411635

meven edited the summary of this revision. (Show Details)Sep 6 2019, 3:49 PM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 65519.Sep 6 2019, 3:49 PM

Add two entries to Recent 'Recent Files' and 'Recent Locations'

So we now have a bit of a problem here. The changes to the IOSlave that support files-only and folders-only modes will land in KDE Applications 19.12.0. But this code is in KIO and will land in Frameworks 5.63, which will be released first.

We have a few options:

  • Land the IOSlave changes in KDE Applications 19.08.2
  • Conditionalize the code here with version ifdefs so that people only get both new entries when using kio-extras from 19.12.0
  • Wait several months to land this

And maybe we should also discuss moving kio-extras to the frameworks release cycle. It's not an app so I'm not sure it makes sense to have it distributed with KDE Applications right now.

And maybe we should also discuss moving kio-extras to the frameworks release cycle. It's not an app so I'm not sure it makes sense to have it distributed with KDE Applications right now.

It's probably a matter of dependency and guarantee of stability for all the librraries included, which may not be trivial
(I don't want to reopen old discussions right now, but the bundle also ships several libraries, despite the name; so the name itself shouldn't be a blocker for keeping kio-extras as part of KDE Applications.)

dfaure added a comment.Sep 6 2019, 6:22 PM

Sounds to me like KIO depends at runtime on this particular ioslave, so THAT ioslave should move to KIO. Just like I did with KIO trash some time ago.

+1, no objection to that.

meven added a comment.Sep 6 2019, 9:16 PM

Conditionalize the code here with version ifdefs so that people only get both new entries when using kio-extras from 19.12.0

It is already the case : users won't get the new entries without recentlyused:/ installed, KProtocolInfo::isKnownProtocol(QStringLiteral("recentlyused") takes care of that without ifdefs (still that is not a perfect solution).
The code in KIO will stay dormant as a sort of runtime dependency, where as long as something is missing it stays almost non-present.

A nice consequence of moving recentlyused:/ to KIO is that it will hit users sooner than Applications 19.12 is released consequently.

But the feature in kactivitymanagerd D23112, its improvement D23737 and gwenview D23741 will only be part of 19.12, making look recentlyused:/files somewhat unfinished until 19.12 is released...
We could backport the gwenview improvement to 19.08.x though.

I don't mind moving recentlyused:/ to KIO if we feel it necessary, it is not without consequences though.

ngraham edited the summary of this revision. (Show Details)Sep 6 2019, 10:01 PM
In D7446#526837, @meven wrote:

But the feature in kactivitymanagerd D23112, its improvement D23737 and gwenview D23741 will only be part of 19.12, making look recentlyused:/files somewhat unfinished until 19.12 is released...

Actually kactivitymanagerd is on yet another release cycle: the Plasma one. Happily, your improvements to kactivitymanagerd will land in Plasma 5.17 which is just around the corner.

We could backport the gwenview improvement to 19.08.x though.

Done.

meven updated this revision to Diff 65602.Sep 7 2019, 9:04 PM

Use new folder-open-recent for /locations bookmark

A plan of action would be :

  1. Check add recenly_used only on plasma KDE_FULL_SESSION (it can't be used outside )

To improve dolphin and kde apps outside of plasma, make KRecentDocuments a recentlyused file user :

  1. Outside of plasma, make recentlyused:/ use recentlyused.xbel as backend
  2. Make KRecentDocumensts use recentlyused.xbel
  1. Remove the KDE_FULL_SESSION check added in 1. (Alternatively, not do 1. and 4. as 2. and 3. make them not necessary)
  1. Kactivities mode without kactivitymanagerd could write data to recentlyused.xbel ? Seems not essential as with 3, at least File dialogs will mark files opened, only ResourceInstance calls will be added.

The last comment from Méven are the notes from a discussion we had together.
In that discussion I realized that the kioslave depends on kactivities-stats, so I'm withdrawing my suggestion to move it to kio, where this dependency isn't wanted.
It actually makes sense in kio-extras, especially if it gains support for non-plasma use cases.

meven updated this revision to Diff 65980.Sep 13 2019, 12:36 PM

add recentlyused:/ bookmarks only in plasma, update following D23742 changes

meven edited the summary of this revision. (Show Details)Sep 13 2019, 12:37 PM
meven updated this revision to Diff 65981.Sep 13 2019, 12:39 PM

Update since version

LGTM besides the inline nitpicks.

autotests/kfileplacesmodeltest.cpp
686–687

const, and maybe a more descriptive name (count or similar?)

967–968

I'd avoid cryptic names. If we can't use index, maybe just i?

Ooh, I'm so excited that this will be landing soon!

@meven do you think you could address the final comments and land this?

meven updated this revision to Diff 67203.Oct 2 2019, 4:09 PM

Small changes

ngraham accepted this revision.Oct 2 2019, 4:10 PM

Shipit!

meven added a comment.Oct 2 2019, 4:55 PM

Code is ready for review, tests pass @elvisangelaccio @dfaure
Following https://phabricator.kde.org/D7446#528536 recentlyused:/ bookmarks are only added when KDE_FULL_SESSION is set.

D23557 will get merged after this one.

If no-one objects I will merge this in a couple of days.
Thanks @ngraham for the first accept ;)

elvisangelaccio accepted this revision.Oct 2 2019, 8:03 PM
This revision is now accepted and ready to land.Oct 2 2019, 8:03 PM
pino added a subscriber: pino.Oct 2 2019, 8:23 PM
pino added inline comments.
src/filewidgets/kfileplacesitem.cpp
113

this string needs a context, as "recent" basically leaves translators clueless on what it refers to and/or where it is used

src/filewidgets/kfileplacesmodel.h
139

this method must be const

pino removed a subscriber: pino.Oct 2 2019, 8:23 PM
meven updated this revision to Diff 67241.Oct 3 2019, 7:03 AM

Make bookmarkForUrl const, add context to translate Recent

meven updated this revision to Diff 67242.Oct 3 2019, 7:05 AM
meven marked 4 inline comments as done.

Better variable naming

meven marked an inline comment as done.Oct 3 2019, 7:05 AM
meven added a comment.Oct 4 2019, 5:47 PM

Any new feedback ?

I think you can go for it. :) Tagging is tomorrow, so please do!

This revision was automatically updated to reflect the committed changes.
meven added a comment.Oct 5 2019, 5:44 AM

I think you can go for it. :) Tagging is tomorrow, so please do!

Just landed ;)

Woohoooooooo!

meven added a comment.Oct 6 2019, 11:25 AM

See fix in D24439

The second failure seems to me sporadic, or at least not related :

QDEBUG : KFilePlacesModelTest::testPlaceGroupHidden() Expected: ("/tmp/kfileplacesmodeltest-alwhrs", "trash:/", "remote:/", "/media/nfs", "/foreign", "/media/floppy0", "/media/XO-Y4", "/media/cdrom")
QDEBUG : KFilePlacesModelTest::testPlaceGroupHidden() Got: ("/tmp/kfileplacesmodeltest-alwhrs", "trash:/", "remote:/", "/media/nfs", "/foreign", "/media/cdrom", "/media/floppy0", "/media/XO-Y4")
FAIL!  : KFilePlacesModelTest::testPlaceGroupHidden() Compared lists differ at index 5.
   Actual   (placesUrls()): "/media/cdrom"
   Expected (urls): "/media/floppy0"
   Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.12/autotests/kfileplacesmodeltest.cpp(1091)]