Places: Use Solid::Device::DisplayName for DisplayRole
ClosedPublic

Authored by meven on Dec 20 2019, 7:08 AM.

Details

Summary

KIO part of D26114

BUG: 415281
FIXED-IN: 5.71

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D26113
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27079
Build 27097: arc lint + arc unit
meven created this revision.Dec 20 2019, 7:08 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 20 2019, 7:08 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Dec 20 2019, 7:08 AM
meven updated this revision to Diff 71861.Dec 20 2019, 7:10 AM

Remove unrelated change

Not a fan of this special handling in Places. Also I think it should stay with the remote path as the fact that it's mounted locally somewhere is only an implementation detail.

Short name (mount point) should be used or this should at least be configurable.

It is configurable. You can set a custom name by placing x-gvfs-name in fstab

src/filewidgets/kfileplacesitem.cpp
265

This variable is superfluous

266

What if it isn't mounted?

bruns added a subscriber: bruns.Dec 21 2019, 2:02 PM

I would consider an item changing its name based on the state a bug.

As said, the name *is*configurable.

meven added a comment.Dec 21 2019, 2:51 PM

I would consider an item changing its name based on the state a bug.

As said, the name *is*configurable.

I am leaning towards abandoning this and D26114.

Instead we might want to improve discoverability and documentation of x-gvfs-name configuration path.

We can make the name override more discoverable for sure. But IMO we should also improve the default presentation, because this is just yucky:

None of my other mounted devices insist on showing their full path or the format of the mount/device.

Maybe we should discuss that in a phab task before submitting patches.

bruns added a comment.Dec 21 2019, 7:54 PM

The mount path is the only useful identifier. There is no filesystem label or similar.

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

The mount path is the only useful identifier. There is no filesystem label or similar.

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

meven added a comment.Dec 22 2019, 8:45 AM

We can make the name override more discoverable for sure. But IMO we should also improve the default presentation, because this is just yucky:

None of my other mounted devices insist on showing their full path or the format of the mount/device.

Maybe we should discuss that in a phab task before submitting patches.

So your concern is plasma vault / fuse specific ?

meven edited the summary of this revision. (Show Details)Apr 4 2020, 10:47 AM
meven updated this revision to Diff 79277.Apr 4 2020, 10:50 AM
meven marked 2 inline comments as done.

Clean superfluous variable

meven added inline comments.Apr 4 2020, 10:51 AM
src/filewidgets/kfileplacesitem.cpp
266

if (m_access) checks this

bruns added a comment.Apr 4 2020, 11:02 AM

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

What I'm saying is that Vaults should set a meaningful value via x-gvfs-name so Solid has not to invent something. Then Solid can use that, and it would be consistent everywhere.

meven added a comment.Apr 5 2020, 10:15 AM

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

What I'm saying is that Vaults should set a meaningful value via x-gvfs-name so Solid has not to invent something. Then Solid can use that, and it would be consistent everywhere.

I see, so I think we should add a "displayName" (or label() similarly to StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this or the mount point when not defined and keeping description as is : a verbose description of the Device that could be used in tooltips for instance.
This displayName would default on description() so that it would be an opt-in change by Device implementation and then users can choose between description() and displayName() depending on the context and role they need. So here in kfileplacesitem we would replace description() by displayName().
Does this idea make sense to you @bruns ?

bruns added a comment.Apr 5 2020, 10:47 AM

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

What I'm saying is that Vaults should set a meaningful value via x-gvfs-name so Solid has not to invent something. Then Solid can use that, and it would be consistent everywhere.

I see, so I think we should add a "displayName" (or label() similarly to StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this or the mount point when not defined and keeping description as is : a verbose description of the Device that could be used in tooltips for instance.
This displayName would default on description() so that it would be an opt-in change by Device implementation and then users can choose between description() and displayName() depending on the context and role they need. So here in kfileplacesitem we would replace description() by displayName().
Does this idea make sense to you @bruns ?

Its already there:

grep x-gvfs-name  /etc/fstab 
//pebbles/share /mnt cifs defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
solid-hardware5  details /org/kde/fstab///pebbles/share
udi = '/org/kde/fstab///pebbles/share'
  parent = '/org/kde/fstab'  (string)
  vendor = 'pebbles'  (string)
  product = 'share'  (string)
  description = 'SambaShare'  (string)
  icon = 'yast-samba-server'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = '/mnt'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://pebbles/share'  (string)

meven added a comment.Apr 5 2020, 10:57 AM

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

What I'm saying is that Vaults should set a meaningful value via x-gvfs-name so Solid has not to invent something. Then Solid can use that, and it would be consistent everywhere.

I see, so I think we should add a "displayName" (or label() similarly to StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this or the mount point when not defined and keeping description as is : a verbose description of the Device that could be used in tooltips for instance.
This displayName would default on description() so that it would be an opt-in change by Device implementation and then users can choose between description() and displayName() depending on the context and role they need. So here in kfileplacesitem we would replace description() by displayName().
Does this idea make sense to you @bruns ?

Its already there:

grep x-gvfs-name  /etc/fstab 
//pebbles/share /mnt cifs defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
solid-hardware5  details /org/kde/fstab///pebbles/share
udi = '/org/kde/fstab///pebbles/share'
  parent = '/org/kde/fstab'  (string)
  vendor = 'pebbles'  (string)
  product = 'share'  (string)
  description = 'SambaShare'  (string)
  icon = 'yast-samba-server'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = '/mnt'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://pebbles/share'  (string)

A lot of users won't have a x-gvfs-name for whatever reason and we can't expect them to be take savy with root access to set up a x-gvfs-name mount option.
I see your point but my last one was how to make the display by default better (not using description()), but keeping displaying x-gvfs-name value when defined by the user.

So we need a more general solution, which is what I suggest.

Moreover setting description to x-gvfs-name makes the original description content inaccessible to application despite it could be valuable, for instance for tooltip.
So I think putting x-gvfs-name in description was abusing the API for it was lacking a proper displayName/label field to begin with.
I hope this clarifies my suggestion in https://phabricator.kde.org/D26113#641998

bruns added a comment.Apr 5 2020, 11:08 AM

As Vaults are not backed by an fstab entry, it would be the responsibility of Vaults to set a useful name (via x-gvfs-name).

What I'm saying is that I think it makes sense for Solid to provide a better default display string so each app doesn't have to do this.

What I'm saying is that Vaults should set a meaningful value via x-gvfs-name so Solid has not to invent something. Then Solid can use that, and it would be consistent everywhere.

I see, so I think we should add a "displayName" (or label() similarly to StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this or the mount point when not defined and keeping description as is : a verbose description of the Device that could be used in tooltips for instance.
This displayName would default on description() so that it would be an opt-in change by Device implementation and then users can choose between description() and displayName() depending on the context and role they need. So here in kfileplacesitem we would replace description() by displayName().
Does this idea make sense to you @bruns ?

Its already there:

grep x-gvfs-name  /etc/fstab 
//pebbles/share /mnt cifs defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
solid-hardware5  details /org/kde/fstab///pebbles/share
udi = '/org/kde/fstab///pebbles/share'
  parent = '/org/kde/fstab'  (string)
  vendor = 'pebbles'  (string)
  product = 'share'  (string)
  description = 'SambaShare'  (string)
  icon = 'yast-samba-server'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = '/mnt'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://pebbles/share'  (string)

A lot of users won't have a x-gvfs-name for whatever reason and we can't expect them to be take savy with root access to set up a x-gvfs-name mount option.
I see your point but my last one was how to make the display by default better (not using description()), but keeping displaying x-gvfs-name value when defined by the user.

I say its the responsibility of Vaults to set the option. For Vaults, there is no fstab anyway, just an options value in the mtab at runtime.

If you want to display more info in the tooltip, probably start with StorageAccess.filePath.

meven added a comment.Apr 5 2020, 11:20 AM

I say its the responsibility of Vaults to set the option. For Vaults, there is no fstab anyway, just an options value in the mtab at runtime.

My primary concern is for network shares.
The vault issue is a distinct bug than the one I am meaning to fix here.

If you want to display more info in the tooltip, probably start with StorageAccess.filePath.

The point of this diff is to use the filePath by default as label, which is way more explicit than the description "/export on 192.168.0.10" for network shares for instance, while not breaking the x-gvfs-name case (which the state of this patch currently does not respect)
All you are saying is not contradicting my suggestion in https://phabricator.kde.org/D26113#641998
So I feel this conversation is sterile, and I should go ahead with it in solid and loop back here once it is done.

I should go ahead with it in solid and loop back here once it is done.

Yeah, that makes sense to me.

meven planned changes to this revision.Apr 10 2020, 6:20 AM

Working on solid : D28590

meven edited the summary of this revision. (Show Details)May 25 2020, 6:08 AM
meven retitled this revision from Places: For mounted volume display mount points instead of description to Places: Use Solid::Device::DisplayName for DisplayRole.May 25 2020, 6:10 AM
meven updated this revision to Diff 83162.May 27 2020, 5:44 AM

Update to new Solid::Device::DisplayName

meven added a subscriber: dfaure.May 28 2020, 6:50 AM

This should be ready to land @dfaure @ngraham

(D26114 will land after KF 5.71 release)

meven edited the summary of this revision. (Show Details)May 29 2020, 4:40 AM

Please check that kfileplacesmodeltest and kfileplacesviewtest still pass.

(they fail here with baloosearch: stuff for some reason, I didn't investigate; but it passes on CI)

meven added a comment.May 30 2020, 5:04 AM

Please check that kfileplacesmodeltest and kfileplacesviewtest still pass.

Just did, nothing changed, it is expected : only fstab declared filesystem or manually mounted fs display text is changing and this is not tested here.

(they fail here with baloosearch: stuff for some reason, I didn't investigate; but it passes on CI)

I am fixing this in https://invent.kde.org/frameworks/kio/-/merge_requests/25

dfaure accepted this revision.May 30 2020, 9:56 AM

Thanks!

This revision is now accepted and ready to land.May 30 2020, 9:56 AM
meven closed this revision.May 30 2020, 10:15 AM