<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/127618/">https://git.reviewboard.kde.org/r/127618/</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;">The device combobox for streams was there (in my older review and also in much older version of sound applet), but it was decided to not show it in applet to keep it simple.
For the icons I have review (https://git.reviewboard.kde.org/r/127467/), but it needs to be reworked as it adds the icon property to Client, but it instead should be probably in PulseObject (as not every stream has Client).
I also think we should not show icon for devices in applet because almost all devices will have same icon (generic "audio-card"). Showing icon for Applicaton streams makes sense because hopefully all icons are different.
So -1 from me.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, not related to this review, but what do you think about moving sinkIndex and sourceIndex to a deviceIndex property in the Stream class ?
It would avoid to send an hardcoded deviceType to delegates.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, that would indeed be much better.</p></pre>
<br />
<p>- David Rosca</p>
<br />
<p>On April 9th, 2016, 1:04 p.m. UTC, Yoann Laissus 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.</div>
<div>By Yoann Laissus.</div>
<p style="color: grey;"><i>Updated April 9, 2016, 1:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-pa
</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;">The first commit fixes stream icons by using ClientIcon (like in the KCM).
It also adds an icon for playback and capture devices to improve consistency with the KCM.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The second commit adds, for each streams, the combobox used in the KCM to select the device.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Branch here : https://quickgit.kde.org/?p=clones%2Fplasma-pa%2Flaissus%2Fplasma-pa.git&a=shortlog&h=9eadc541cd55862b85399719904cbe85b4af1147</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, not related to this review, but what do you think about moving sinkIndex and sourceIndex to a deviceIndex property in the Stream class ?
It would avoid to send an hardcoded deviceType to delegates.
You can take a look at my first implementation (done before the introduction of that in the KCM, dont' take care of the QML code) : https://quickgit.kde.org/?p=clones%2Fplasma-pa%2Flaissus%2Fplasma-pa.git&a=commit&h=9d06be259f9438d2f489159b0b05b676fe26aacd</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>applet/contents/ui/ListItemBase.qml <span style="color: grey">(d77d25a51b09149a1cf479eec0053184ae8625e6)</span></li>
<li>applet/contents/ui/StreamListItem.qml <span style="color: grey">(7e84505902ca116b466bcd408f00ecaa9a4ce8da)</span></li>
<li>applet/contents/ui/main.qml <span style="color: grey">(086716d739e5c67ccabfceaca4fea86fd40cc8a0)</span></li>
<li>src/kcm/package/contents/ui/DeviceComboBox.qml <span style="color: grey">()</span></li>
<li>src/qml/CMakeLists.txt <span style="color: grey">(9d021fcf61e4820efd0623060381dea0d7c30506)</span></li>
<li>src/qml/ClientIcon.qml <span style="color: grey">(8256758048907fd8bf62f5d6e7a7176e1855c1e8)</span></li>
<li>src/qml/qmldir <span style="color: grey">(de44fca76deb79017be8c3033d1eb462242145e8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127618/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/04/09/f52f75ba-a800-4a82-8d75-08051edf7100__Screenshot_20160409_145911.png">Streams</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/04/09/8adff934-e24d-4933-870e-7b23803ba5e3__Screenshot_20160409_145925.png">Devices</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>