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

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in device.h:99
> 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 :/

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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven added a comment.


  The patch currently does not work.

INLINE COMMENTS

> ervin wrote in device.h:99
> 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).

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 :/

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:cdbfb3e799c7: Add a QString Solid::Device::displayName, 
used in Fstab Device for network… (authored by meven).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=83128&id=83146

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

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.cpp
  src/solid/devices/ifaces/device.h

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

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

[Stefan Brüns]

bruns accepted this revision.

REPOSITORY
  R245 Solid

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

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

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  LGTM. @bruns?

REPOSITORY
  R245 Solid

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

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

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

[Kevin Ottens]

ervin added a comment.


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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven added a comment.


  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

REPOSITORY
  R245 Solid

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

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

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

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven added a comment.


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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven updated this revision to Diff 83128.
meven added a comment.


  Remove unneeded change

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=83127&id=83128

BRANCH
  arcpatch-D28590_1

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

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.cpp
  src/solid/devices/ifaces/device.h

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

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

[Méven Car]

meven updated this revision to Diff 83127.
meven marked 5 inline comments as done.
meven added a comment.


  Use a default implementation for Device::displayName()

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=83112&id=83127

BRANCH
  arcpatch-D28590_1

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

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/udisks2/udisksdevicebackend.cpp
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.cpp
  src/solid/devices/ifaces/device.h

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

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

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> device.h:99
> +  */
> +virtual QString displayName() const = 0;
> +

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).

REPOSITORY
  R245 Solid

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

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

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

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fstabdevice.cpp:73
> +if (m_displayName.isEmpty()) {
> +QStringList currentMountPoints = 
> FstabHandling::currentMountPoints(m_device);
> +if (currentMountPoints.isEmpty()) {

`const`

> fstabdevice.cpp:75
> +if (currentMountPoints.isEmpty()) {
> +QStringList mountPoints = FstabHandling::mountPoints(m_device);
> +m_displayName = mountPoints.isEmpty() ? m_description : 
> mountPoints.first();

`const` so that first() doesn't detach.

> device.h:93
> +/**
> +  * Retrieves the display Name to use for this device.
> +  * Same as description when not defined.

Why "Name" with an uppercase letter?

> device.h:96
> +  *
> +  * @return the display Name
> +  * @since 5.71

(same)

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven added subscribers: broulik, dfaure.
meven added a comment.


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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven updated this revision to Diff 83112.
meven added a comment.


  Update @since, improve doc

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=81061&id=83112

BRANCH
  arcpatch-D28590_1

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: ervin, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

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

[Méven Car]

meven added a subscriber: ervin.
meven added a comment.


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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven marked 2 inline comments as done.

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven marked 3 inline comments as done.
meven added a comment.


  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).

REPOSITORY
  R245 Solid

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

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

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

[Stefan Brüns]

bruns added a comment.


  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.

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven updated this revision to Diff 81061.
meven added a comment.


  avoid instanciating m_storageAccess in FstabDevice::FstabDevice

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28590?vs=79922&id=81061

BRANCH
  arcpatch-D28590

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, michaelh, ngraham, bruns

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

[Méven Car]

meven added a comment.


  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.

REPOSITORY
  R245 Solid

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

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

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

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  Do not create m_storageAccess in the constructor

REPOSITORY
  R245 Solid

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

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

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

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> meven wrote in fstabdevice.cpp:145
> 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).

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

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

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

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:145
> use `FstabHandling::mountPoints(m_device).first()` instead of m_storageAccess.

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).

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

meven updated this revision to Diff 79922.
meven added a comment.


  Fix m_displayName variable naming, move  block to constructor

REPOSITORY
  R245 Solid

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

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, michaelh, ngraham, bruns

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

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fstabdevice.cpp:63
>  }
>  
> +if (m_storageType == StorageType::NetworkShare) {

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

> fstabdevice.cpp:145
> +}
> +const auto filePath = m_storageAccess->filePath();
> +if (!filePath.isEmpty()) {

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

> fstabdevice.h:64
>  QString m_vendor;
> +QString m_label;
>  QString m_description;

Wrong variable name

REPOSITORY
  R245 Solid

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

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

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

[Méven Car]

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".

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::displayName, used in Fstab Device for network mounts

[Méven Car]

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".

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