Review Request: Broadcast setup/teardown/eject related signals across object referring to the same device

[Kevin Ottens]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3550/#review4971
-----------------------------------------------------------


Found a few issues in there that I commented. Also I wonder if the patch could be improved by sharing some of the facilities between the two backend classes, namely:
 - broadcastDBusSignal()
 - dbusObjectPath()
They seem to express the same thing in slightly different ways, so I'd be surprised if they couldn't be shared.


trunk/KDE/kdelibs/solid/solid/backends/hal/halcdrom.cpp
<http://reviewboard.kde.org/r/3550/#comment4412>

    Maybe add a sendEjectRequested() like you added a sendEjectDone().



trunk/KDE/kdelibs/solid/solid/backends/hal/halstorageaccess.h
<http://reviewboard.kde.org/r/3550/#comment4413>

    Undeeded space.



trunk/KDE/kdelibs/solid/solid/backends/hal/halstorageaccess.cpp
<http://reviewboard.kde.org/r/3550/#comment4415>

    QDBusVariant(QString("")) should likely be QDBusVariant().



trunk/KDE/kdelibs/solid/solid/backends/hal/halstorageaccess.cpp
<http://reviewboard.kde.org/r/3550/#comment4414>

    Why isn't it going through broadcastDBusSignal()?


- Kevin


On 2010-04-10 17:12:57, Jacopo De Simoi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3550/
> -----------------------------------------------------------
> 
> (Updated 2010-04-10 17:12:57)
> 
> 
> Review request for kdelibs and Kevin Ottens.
> 
> 
> Summary
> -------
> 
> At the moment a processes which owns a Solid::Device on some device does not know if other processes are performing important operations such as calling setup/teardown/eject on the same device.
> 
> This patch solve this situation in the following way:
> 1) new signals {setup,teardown,eject}Requested(const QString& udi) have been added to the relevant classes; they are emitted whenever any process acting on the device wrapped by the class request the corresponding action.
> 2) the signals {setup,teardown,eject}Done(...) are now emitted even if the corresponding action has been performed by another process.
> 
> The broadcasting is done via emission of a QDBusMessage; a slot is then connected to the DBus signal which resets the device state and sends the corresponding Qt signal. This is quite straightforward, but there is one important drawback. It appears that an invalid QVariant cannot be marshalled across DBus; this is work-arounded by sending an empty string instead of an invalid QVariant and converting it back to an invalid QVariant in the slot. I'm open to change this provided a better working solution can be found.
> 
> Usecase: 
> This allows interested clients of solid (e.g. the plasma device notifier) to monitor the status of the devices and react accordingly even if they are not directly involved in the process. For instance, during a (long) unmounting process, the value for "free space" is bogus; knowing in advance that a teardown is in progress could tell the client to hide any "free space" visualization.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/solid/solid/backends/hal/halcdrom.h 1112487 
>   trunk/KDE/kdelibs/solid/solid/backends/hal/halcdrom.cpp 1112487 
>   trunk/KDE/kdelibs/solid/solid/backends/hal/halstorageaccess.h 1112487 
>   trunk/KDE/kdelibs/solid/solid/backends/hal/halstorageaccess.cpp 1112487 
>   trunk/KDE/kdelibs/solid/solid/opticaldrive.h 1112487 
>   trunk/KDE/kdelibs/solid/solid/opticaldrive.cpp 1112487 
>   trunk/KDE/kdelibs/solid/solid/storageaccess.h 1112487 
>   trunk/KDE/kdelibs/solid/solid/storageaccess.cpp 1112487 
> 
> Diff: http://reviewboard.kde.org/r/3550/diff
> 
> 
> Testing
> -------
> 
> Works well; a patched notifier is needed to take full advantage of this improved behaviour; dolphin doesn't seem to notice any change. 
> 
> 
> Thanks,
> 
> Jacopo
> 
>