[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Broadcast setup/teardown/eject related signals
From:       "Jacopo De Simoi" <wilderkde () gmail ! com>
Date:       2010-04-12 22:27:48
Message-ID: 20100412222748.3445.24636 () localhost
[Download RAW message or body]


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

(Updated 2010-04-12 22:27:48.727487)


Review request for kdelibs and Kevin Ottens.


Changes
-------

Thanks ervin for the quick review and for the hints in irc.

Factored most of the code in HalDevice, now it is much cleaner. The manual \
marshalling of the QDBusVariant is indeed needed and requires the two different \
methods broadcastAction{Requested,Done}. The word "Action"  can be changed if \
believed to generate confusion

I found another usecase: this patch allows errors triggered by calling actions via \
soliduiserver to be shown e.g. in the notifier.


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 (updated)
-----

  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/haldevice.h 1112487 
  trunk/KDE/kdelibs/solid/solid/backends/hal/haldevice.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


[prev in list] [next in list] [prev in thread] [next in thread]