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

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

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

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

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

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

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

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

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,

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

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,

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

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

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

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

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,

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

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=83128 BRANCH arcpatch-D28590_1 REVISION DETAIL https://phabricator.kde.org/D28590 AFFECTED FILES

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

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=83127 BRANCH arcpatch-D28590_1

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

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

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

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); > +

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

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,

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

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=83112 BRANCH arcpatch-D28590_1 REVISION DETAIL https://phabricator.kde.org/D28590 AFFECTED FILES

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

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,

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

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

meven marked 3 inline comments as done. meven added a comment. In D28590#656297 , @bruns wrote: > In D28590#656133 , @meven wrote: > > > In D28590#654139 ,

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

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

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

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=81061 BRANCH arcpatch-D28590 REVISION DETAIL

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

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

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

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:

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

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 >

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

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

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

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=79922 BRANCH master REVISION DETAIL

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

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

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

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,

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

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,