<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106378/">http://git.reviewboard.kde.org/r/106378/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 12th, 2012, 2:20 p.m., <b>Alex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From where I stand, I see HAL as the official backend for freeBSD, no modern linux distro (or any distro that will update their libsolid version) is shipping with HAL.
So, if with this patch freeBSD support is better, please go ahead! </pre>
</blockquote>
<p>On September 12th, 2012, 5 p.m., <b>Alberto Villa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Andriy Gapon submitted an equivalent review request time ago: https://git.reviewboard.kde.org/r/105432
It solves the same issue (I was not aware of it), introducing a function which was proposed for the Solid API. Apart from the specific code, for which you seem to trust our judgement, what's your preferred approach? I admit that at this very moment I am not aware of other places which require that function, but I guess there could be some (I may have seen one where CD drives need to address their parent storage drive).
By the way, would you approve a change to set both individual Removable and Hotpluggable properties? This would make the dataengine more consistent, while it's the plasmoid which has to check for both the properties to identify removable devices (I've read the HAL spec - which I think can be a reliable reference for those properties - and yes, one should not be set when the other one is, so we're handling them in the wrong way).</pre>
</blockquote>
<p>On September 12th, 2012, 6:05 p.m., <b>Alberto Villa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> I admit that at this very moment I am not aware of other places which require that function
OK, now I know of at least another place.</pre>
</blockquote>
<p>On October 2nd, 2012, 7:44 p.m., <b>Alex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">About the properties I think you are correct and this patch should go on (btw we don't maintain that datanegine, that's plasma work).
About the method, It totally makes sense but we will have to wait until libsolid2 at least if we want it public (I may accept a version of it private).
Good job btw :)
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't find a private version useful at the moment, so I committed the patch with the local function. About the method, I hope to see it in libsolid2. :)</pre>
<br />
<p>- Alberto</p>
<br />
<p>On September 12th, 2012, 5:07 p.m., Alberto Villa wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Solid.</div>
<div>By Alberto Villa.</div>
<p style="color: grey;"><i>Updated Sept. 12, 2012, 5:07 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Current hack to check for Removable property in StorageAccess devices goes up only one level to search for the StorageDrive device. This works fine with UDev, but not with HAL, which can have (at least on FreeBSD, where I'm testing it) a StorageVolume device in the middle. Going up the whole tree of the Block device ensures that we eventually get to the StorageDrive one to fetch the correct Removable property.
While here, I'd like to set both Removable and Hotpluggable properties (as done few lines above), as they are exclusive and have a very different meaning (Removable being stuff that can be removed while its device node survives, like CDs and floppies, and Hotpluggable being stuff like USB and eSATA devices, which can be removed while the system is running without leaving behind a stale device node). Plasmoids using the dataengine should check for both the properties, as they shouldn't be defined at the same time (we're using them in the wrong way, according to the HAL spec which, I guess, inspired the Solid interface; it's not a big problem, but the code should handle the correct usage too, as a future devd backend might follow it).
If approved, I'd like to commit this also to 4.9.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested successfully on FreeBSD 10-CURRENT r239665 with KDE SC 4.9.0: my USB flash drives now appear in the device notifier plasmoid (and a console.log() in the plasmoid itself confirms that the device now has the Removable property).
I also double-checked with a UDev-backed `solid-hardware list details` log that the logic correctly applies to UDev.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/generic/dataengines/soliddevice/soliddeviceengine.cpp <span style="color: grey">(86f123c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106378/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>