<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/105615/">http://git.reviewboard.kde.org/r/105615/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">One issue and one possible improvement, in my opinion.
Currently, when parentUdi is set and type is Unknown, all the children are returned. In your patch, none of the children are returned, unless the last routine in queryDeviceInterface() returns true with Unknown type, but I doubt it as it's optimised for types like Generic, etc., but not for Unknown. Anyway, it's a case we should optimise here.
Should we think that the current behaviour is wrong, then why is allDevices() returned when both parentUdi and type aren't specified?
About the improvement, I'm speaking of removing duplication. The first two checks are mostly equal, they could be condensed into one (tweaking the if() inside the foreach()), with "return allDevices()" being moved on top if (parentUdi.isEmpty() and type == Unknown).</pre>
<br />
<p>- Alberto</p>
<br />
<p>On July 19th, 2012, 2:17 a.m., Raphael Kubo da Costa 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 and Alberto Villa.</div>
<div>By Raphael Kubo da Costa.</div>
<p style="color: grey;"><i>Updated July 19, 2012, 2:17 a.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;">HAL: Make devicesFromQuery more similar to the UDisks implementation.
The devicesFromQuery implementation in the UDisks backend is saner, as
it simply relays the required checks for validity to each Device we
are interested in.
So far, HalDevice::queryDeviceInterface() performed some checks
depending on the DeviceInterface type being passed to it, while
HalManager::devicesFromQuery() did not filter the results it got in
the same way. What's more, some checks such as the video4linux ones
were being made in both places, leading to some needless duplication
in functionality.
As a side-effect, this fixes a bug made visibile by a recent commit to
libktorrent: retrieving StorageAccess devices with listFromType()
would simply query HAL for devices with the "volume" capability (which
includes swap partitions and other non-mountable things), so using
Device::asDeviceInterface(Solid::DeviceInterface::StorageAccess) would
sometimes return 0 on a few of those entries retrieved earlier.</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;">libktorrent stopped crashing on bt::MountPoint()</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>solid/solid/backends/hal/halmanager.h <span style="color: grey">(ec42fac1d2b5dc306e9b8e00432bcbe5854a6fb9)</span></li>
<li>solid/solid/backends/hal/halmanager.cpp <span style="color: grey">(2c9c6c0c0a8385bee37ce77d488e0395f2f90a06)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105615/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>