Details
- Reviewers
bruns sitter dfaure ngraham - Group Reviewers
Frameworks - Commits
- R245:cdbfb3e799c7: Add a QString Solid::Device::displayName, used in Fstab Device for network…
Diff Detail
- Repository
- R245 Solid
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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 | ||
|---|---|---|
| 155 | 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 ↗ | (On Diff #79397) | This would be the correct value only for label() now, no longer for the description (which should be more verbose). |
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.
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 | ||
|---|---|---|
| 155 | Indeed thanks for pointing it out. | |
| src/solid/devices/backends/udisks2/udisksdevice.cpp | ||
| 234 ↗ | (On Diff #79397) | Can it be missing ? And what should be it then ? |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 154 | 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). | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 154 | I think it is. You always create a FstabStorageAccess instance now. | |
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.
- m_storageAccess is comparatively expensive
- I showed you how to get the mount state without StorageAccess
- 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).
The patch currently does not work.
| src/solid/devices/ifaces/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()); | |
| src/solid/devices/ifaces/device.h | ||
|---|---|---|
| 99 | Woops... my bad, indeed didn't notice it wasn't marked virtual anymore. It has to be indeed. | |