Differential D28590

Add a QString Solid::Device::displayName, used in Fstab Device for network mounts
ClosedPublic
Actions

Authored by meven on Apr 5 2020, 11:59 AM.

Details

Reviewers
bruns
sitter
dfaure
ngraham
Group Reviewers
Frameworks
Commits
R245:cdbfb3e799c7: Add a QString Solid::Device::displayName, used in Fstab Device for network…
Summary

This adds a label field, so that devices can provide a label and a more detailled description.
This label should be used as the device label and description used for subText or tooltips for instance.
The description is used as label by default.

CCBUG: 415281

For use in D26113 and D26114

Diff Detail

Repository
R245 Solid
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25104
Build 25122: arc lint + arc unit
meven created this revision.Apr 5 2020, 11:59 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 5 2020, 11:59 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Apr 5 2020, 11:59 AM
bruns added a comment.Apr 5 2020, 1:08 PM

I think it would be better to not change behaviour for any backend, but just default label() to description() everywhere.

Then in the next step, adjust description() and label() for each backend, shorten label where possible, and extend description() so it becomes more informative.

src/solid/devices/backends/fstab/fstabdevice.cpp
159

filePath in most cases does not depend on isAccessible ... it would be annoying if the label changed everytime the device is mounted.

src/solid/devices/backends/udisks2/udisksdevice.cpp
234

This would be the correct value only for label() now, no longer for the description (which should be more verbose).

bruns added a comment.Apr 5 2020, 1:12 PM

Btw, label() is a bad name, it can be confused with the filesystem label to easily. Maybe shortName().

bruns requested changes to this revision.Apr 5 2020, 1:12 PM
This revision now requires changes to proceed.Apr 5 2020, 1:12 PM
meven marked an inline comment as done.Apr 5 2020, 3:17 PM
In D28590#642133, @bruns wrote:

Btw, label() is a bad name, it can be confused with the filesystem label to easily. Maybe shortName().

I meant to reuse this name on purpose as it serves the same use.
I would favor displayName() as it is reminiscent somewhat of Qt::DisplayRole and clearer than name.
shortName implies there is another longer name somewhere, describing what it contains rather than what use case it fulfills.

In D28590#642130, @bruns wrote:

I think it would be better to not change behaviour for any backend, but just default label() to description() everywhere.

So you mean I should split commit the label/displayName() addition and the fstabdevice change ?
I was keeping label() as an alias to description() everywhere expect in RootDevice, and FstabDevice.

I didn't know how to avoid defining label() everywhere because of the way the Solid interface is made.

Then in the next step, adjust description() and label() for each backend, shorten label where possible, and extend description() so it becomes more informative.

Agreed but so in a subsequent diff(s), as it will change the meaning of description, which essentially will be a behavior change, potentially breaking UIs,
We will need to look for the usage of all description and decide whether it should use the new function instead of description.

src/solid/devices/backends/fstab/fstabdevice.cpp
159

Indeed thanks for pointing it out.

src/solid/devices/backends/udisks2/udisksdevice.cpp
234

Can it be missing ? And what should be it then ?
I don't have an answer to this.
In the meantime I meant to keep label() as description() as a first step.

meven updated this revision to Diff 79807.Apr 11 2020, 6:57 AM
meven marked 2 inline comments as done.

Rename label to displayName

meven retitled this revision from Add a QString Solid::Device::label, used in Fstab Device for network mounts to Add a QString Solid::Device::DisplayName, used in Fstab Device for network mounts.Apr 11 2020, 10:08 AM
meven retitled this revision from Add a QString Solid::Device::DisplayName, used in Fstab Device for network mounts to Add a QString Solid::Device::displayName, used in Fstab Device for network mounts.
bruns added inline comments.Apr 12 2020, 10:39 AM
src/solid/devices/backends/fstab/fstabdevice.cpp
63 ↗(On Diff #79807)

the if (m_displayName.isEmpty()) block belongs here

145 ↗(On Diff #79807)

use FstabHandling::mountPoints(m_device).first() instead of m_storageAccess.

src/solid/devices/backends/fstab/fstabdevice.h
64 ↗(On Diff #79807)

Wrong variable name

meven updated this revision to Diff 79922.Apr 12 2020, 11:58 AM

Fix m_displayName variable naming, move block to constructor

meven marked an inline comment as done.Apr 12 2020, 11:59 AM
meven added inline comments.
src/solid/devices/backends/fstab/fstabdevice.cpp
145 ↗(On Diff #79807)

I don't think that it is necessary : m_storageAccess already equals to FstabHandling::mountPoints(m_device).first(), looking at FstabStorageAccess::FstabStorageAccess and FstabStorageAccess::onMtabChanged. (That's why I wrote it like this in the first place).

bruns added inline comments.Apr 12 2020, 1:59 PM
src/solid/devices/backends/fstab/fstabdevice.cpp
145 ↗(On Diff #79807)

I think it is. You always create a FstabStorageAccess instance now.

bruns requested changes to this revision.Apr 21 2020, 10:19 PM

Do not create m_storageAccess in the constructor

This revision now requires changes to proceed.Apr 21 2020, 10:19 PM
meven added a comment.Apr 24 2020, 5:31 AM
In D28590#654139, @bruns wrote:

Do not create m_storageAccess in the constructor

Hmm, you told me the f (m_displayName.isEmpty()) block belongs here, I don't see why instantiating m_storageAccess here is bad but mehh.

meven updated this revision to Diff 81061.Apr 24 2020, 5:34 AM

avoid instanciating m_storageAccess in FstabDevice::FstabDevice

bruns added a comment.Apr 24 2020, 11:01 AM
In D28590#656133, @meven wrote:
In D28590#654139, @bruns wrote:

Do not create m_storageAccess in the constructor

Hmm, you told me the f (m_displayName.isEmpty()) block belongs here, I don't see why instantiating m_storageAccess here is bad but mehh.

Please read again.

  1. m_storageAccess is comparatively expensive
  2. I showed you how to get the mount state without StorageAccess
  3. I told you where to initialize it, yes. Doing it correctly of course still applies.
meven marked 3 inline comments as done.Apr 24 2020, 1:32 PM
In D28590#656297, @bruns wrote:
In D28590#656133, @meven wrote:
In D28590#654139, @bruns wrote:

Do not create m_storageAccess in the constructor

Hmm, you told me the f (m_displayName.isEmpty()) block belongs here, I don't see why instantiating m_storageAccess here is bad but mehh.

Please read again.

  1. m_storageAccess is comparatively expensive
  2. I showed you how to get the mount state without StorageAccess
  3. I told you where to initialize it, yes. Doing it correctly of course still applies.

Btw I updated already the code to use FstabHandling::currentMountPoints and FstabHandling::currentMountPoints(m_device); as you suggested (as StorageAccess does).

meven marked 2 inline comments as done.May 9 2020, 5:52 PM
meven added a subscriber: ervin.May 22 2020, 1:57 PM

@ervin perhaps you might review this as @bruns seems too busy.

meven updated this revision to Diff 83112.May 22 2020, 2:00 PM

Update @since, improve doc

meven added subscribers: broulik, dfaure.May 23 2020, 10:15 AM

Maybe @dfaure or @broulik would be nice to help the review effort.

dfaure requested changes to this revision.May 23 2020, 10:42 AM
dfaure added inline comments.
src/solid/devices/backends/fstab/fstabdevice.cpp
72 ↗(On Diff #79807)

const

74 ↗(On Diff #79807)

const so that first() doesn't detach.

src/solid/devices/ifaces/device.h
93 ↗(On Diff #79807)

Why "Name" with an uppercase letter?

96 ↗(On Diff #79807)

(same)

This revision now requires changes to proceed.May 23 2020, 10:42 AM
ervin added inline comments.May 23 2020, 10:56 AM
src/solid/devices/ifaces/device.h
99 ↗(On Diff #79807)

Why not have a default implementation which returns descriptions()? This would make for a less intrusive patch (I think it's in part what @bruns suggested earlier).

meven updated this revision to Diff 83127.May 23 2020, 11:10 AM
meven marked 5 inline comments as done.

Use a default implementation for Device::displayName()

meven updated this revision to Diff 83128.May 23 2020, 11:12 AM

Remove unneeded change

meven added a comment.May 23 2020, 11:12 AM

This makes the patch way less intrusive in the process, thanks @dfaure

dfaure added a comment.May 23 2020, 11:14 AM

@meven you're confusing me with my clone @ervin.

meven added a comment.May 23 2020, 11:21 AM
In D28590#673609, @dfaure wrote:

@meven you're confusing me with my clone @ervin.

lol
I thought this was still you when I read the comment.
Thanks @ervin

ervin added a comment.May 23 2020, 1:16 PM

LGTM now, I'll let the other reviewers weight in though.

ngraham accepted this revision.May 24 2020, 7:11 PM
ngraham added a subscriber: ngraham.

LGTM. @bruns?

bruns accepted this revision.May 24 2020, 7:41 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 25 2020, 6:06 AM
Closed by commit R245:cdbfb3e799c7: Add a QString Solid::Device::displayName, used in Fstab Device for network… (authored by meven). · Explain Why
This revision was automatically updated to reflect the committed changes.
meven added a comment.May 25 2020, 6:47 AM

The patch currently does not work.

src/solid/devices/ifaces/device.h
99 ↗(On Diff #79807)

Well I have to make this virtual it seems so this call is dynamically dispatched by return_SOLID_CALL(Ifaces::Device *, d->backendObject(), QString(), displayName());
I assumed this would work based on my review comments rather than on tests :/

ervin added inline comments.May 25 2020, 8:17 AM
src/solid/devices/ifaces/device.h
99 ↗(On Diff #79807)

Woops... my bad, indeed didn't notice it wasn't marked virtual anymore. It has to be indeed.

Showing Only Differences

This revision modifies 15 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

Diff 79922

View Options

src/solid/devices/backends/fstab/fstabdevice.h

Loading...
View Options

src/solid/devices/backends/fstab/fstabdevice.cpp

Loading...
View Options

src/solid/devices/backends/udisks2/udisksdevice.cpp

Loading...
View Options

src/solid/devices/ifaces/device.h

Loading...