<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126688/">https://git.reviewboard.kde.org/r/126688/</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Nice idea!
I'd set the dely until the popup closes again shorter, though. This is just a feedback message that things happened as expected, so throwing the popup in the user's face for five seconds is too much distraction in that case (especially as nothing bad happens if the user misses the feedback). I'd just use the standard notification delay (or is that five seconds?). When unmounting failed, however, it should stay for five seconds because it's important that the user knows that the should not unplug the device.</p></pre>
<br />
<p>- Thomas Pfeiffer</p>
<br />
<p>On January 9th, 2016, 8:15 p.m. UTC, Kai Uwe Broulik wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma and KDE Usability.</div>
<div>By Kai Uwe Broulik.</div>
<p style="color: grey;"><i>Updated Jan. 9, 2016, 8:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Instead of showing a detached status bar with the device name, show the message below the device.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When removing a device, device notifier will pop up and a "You can now safely remove this device" message will show up; after 5 seconds the message and the device will disappear. When trying to unmount a device and it fails, device notifier will show up with an error message that will stay there until device notifier is closed or another message appears.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This review consists of two patches: one for fixing the device notifications engine's wording (I'm open to better verbalizations) and one for the device notifier</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There a still a couple of glitches:
- the layout doesn't properly reset at times (eg. the delegate height doesn't update) or fails to show certain items (also looks like a Qt bug)
- sometimes the delegates suddenly overlap each other beacuse the section thing gets confused when the one item is already gone in the model (looks like a Qt bug)
- the "no devices" heading doesn't know that there's still a pseudo-device there
- it cannot actually highlight the device that was safely removed (it's no longer part of the model and thus has no index), we could do a hack for this though
- the message doesn't disappear reliably or spontaneously re-appears
- depending on your screen dpi you sometimes get a black and white Info icon but the error thing is always red, there's a smaller variant for the former but not the latter apparently, also it seems we lack a proper "task done" icon, Oxygen had one</p></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>applets/devicenotifier/package/contents/ui/DeviceItem.qml <span style="color: grey">(1b3bb1b)</span></li>
<li>applets/devicenotifier/package/contents/ui/FullRepresentation.qml <span style="color: grey">(9e9aa25)</span></li>
<li>applets/devicenotifier/package/contents/ui/StatusBar.qml <span style="color: grey">(ba827c3)</span></li>
<li>dataengines/devicenotifications/ksolidnotify.h <span style="color: grey">(fac8cc1)</span></li>
<li>dataengines/devicenotifications/ksolidnotify.cpp <span style="color: grey">(3935ba3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126688/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/01/09/8e7142b2-38a9-4867-a067-0e6cdbcaa076__devicenotifierinlinefeedback1.png">Failed to mount</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/01/09/a43199cf-36a3-4337-9843-cb7375462c57__devicenotifierinlinefeedback2.png">Safely remove</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>