Don't store view properties on network shares
ClosedPublic

Authored by broulik on Aug 26 2019, 7:39 AM.

Details

Summary

Entering a folder will have Dolphin determine whether it can store the view properties in a .directory file within the folder or in a generic config location.
It does so by checking for permissions on the file and parent dir causing various stat calls that potentially block on a slow mount. Also, the config reading thereafter can be very slow.
Moreover, network shares are typically shared between users, so one user's view properties shouldn't affect or be overwritten by this Dolphin instance.
It doesn't resolve symlinks but is surely an improvement over the status quo.

Test Plan

Mounted a slow mount, entered the folder, did no longer block in ViewProperties at all.
Settings were written to ~/.local/share/dolphin/view_properties/local/home/foo/slowmount/.directory instead of ~/myslowmount/.directory
Since Solid has already been set up by KFilePlacesModel, the overhead of this is negligible.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Aug 26 2019, 7:39 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptAug 26 2019, 7:39 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
broulik requested review of this revision.Aug 26 2019, 7:39 AM
meven added a comment.EditedAug 26 2019, 8:42 AM

I'd really like this code to be in a lib rather that repeating all over the place.
Something like bool Solid::IsOnNetworkShare(path)

+1 on the idea though

src/views/viewproperties.cpp
79

You can add a const here.

broulik updated this revision to Diff 64666.Aug 26 2019, 2:14 PM
broulik edited the summary of this revision. (Show Details)
  • Properly check parent of url

I don't like the duplication either.

I think you could use KFileItem::isSlow here.
Otherwise, this means moving the code to Solid indeed.

There's no KFileItem here

Sure, but all you have to do is KFileItem{url}, and you have one :)

That constructor doesn't do much, just two string manipulations.

That constructor doesn't do much, just two string manipulations.

Then I call isSlow which will call localPath which will call init and then stat the location

localPath() has a shortcut when m_bIsLocalUrl is true, which the Private ctor sets. So no, it shouldn't end up in init().

broulik updated this revision to Diff 64793.Aug 28 2019, 7:39 AM
broulik edited the summary of this revision. (Show Details)
  • Remove duplicated code by using KFileItem
dfaure accepted this revision.Aug 28 2019, 7:46 AM
This revision is now accepted and ready to land.Aug 28 2019, 7:46 AM
This revision was automatically updated to reflect the committed changes.