D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Méven Car]

meven updated this revision to Diff 79807.
meven marked 2 inline comments as done.
meven added a comment.


  Rename label to displayName

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=79397=79807

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28590

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakedevice.cpp
  src/solid/devices/backends/fakehw/fakedevice.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabmanager.cpp
  src/solid/devices/backends/shared/rootdevice.cpp
  src/solid/devices/backends/shared/rootdevice.h
  src/solid/devices/backends/udev/udevdevice.cpp
  src/solid/devices/backends/udev/udevdevice.h
  src/solid/devices/backends/udev/udevmanager.cpp
  src/solid/devices/backends/udisks2/udisksdevice.cpp
  src/solid/devices/backends/udisks2/udisksdevice.h
  src/solid/devices/backends/udisks2/udisksmanager.cpp
  src/solid/devices/backends/upower/upowerdevice.cpp
  src/solid/devices/backends/upower/upowerdevice.h
  src/solid/devices/backends/upower/upowermanager.cpp
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.h

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Méven Car]

meven marked an inline comment as done.
meven added a comment.


  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.

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:146
> filePath in most cases does not depend on isAccessible ... it would be 
> annoying if the label changed everytime the device is mounted.

Indeed thanks for pointing it out.

> bruns wrote in udisksdevice.cpp:234
> This would be the correct value only for label() now, no longer for the 
> description (which should be more verbose).

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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

bruns added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

bruns added a comment.


  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.

INLINE COMMENTS

> fstabdevice.cpp:146
> +if (m_storageAccess->isAccessible()) {
> +return m_storageAccess->filePath();
> +} else {

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

> udisksdevice.cpp:234
>  {
>  const QString hintName = property("HintName").toString(); // non-cached
>  if (!hintName.isEmpty()) {

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D28590

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Méven Car]

meven created this revision.
meven added reviewers: Frameworks, bruns, sitter.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION 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 


REPOSITORY
  R245 Solid

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28590

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakedevice.cpp
  src/solid/devices/backends/fakehw/fakedevice.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabmanager.cpp
  src/solid/devices/backends/shared/rootdevice.cpp
  src/solid/devices/backends/shared/rootdevice.h
  src/solid/devices/backends/udev/udevdevice.cpp
  src/solid/devices/backends/udev/udevdevice.h
  src/solid/devices/backends/udev/udevmanager.cpp
  src/solid/devices/backends/udisks2/udisksdevice.cpp
  src/solid/devices/backends/udisks2/udisksdevice.h
  src/solid/devices/backends/udisks2/udisksmanager.cpp
  src/solid/devices/backends/upower/upowerdevice.cpp
  src/solid/devices/backends/upower/upowerdevice.h
  src/solid/devices/backends/upower/upowermanager.cpp
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.h

To: meven, #frameworks, bruns, sitter
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns