<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On avril 9th, 2016, 1:18 après-midi UTC, <b>David Rosca</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;"><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>
</blockquote>
<p>On avril 11th, 2016, 11:19 matin UTC, <b>Yoann Laissus</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;"><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;">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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, you're right, after rethink that, most users will probably never realize that stream entries can be expanded. It can be useful but not that intuitive.
In this case, maybe should we totally remove the sub component support in ListItems ?</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;">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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, it's a good idea to have the icon in PulseObject.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I discard this review.</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;">
<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>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll do another review for that.</p></pre>
</blockquote>
<p>On avril 11th, 2016, 11:35 matin UTC, <b>David Rosca</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;"><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;">In this case, maybe should we totally remove the sub component support in ListItems ?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, should be removed also with cleanup of all applet code. I plan to do it eventually, but if you want, you can take it instead :)</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, I'll take care of that too ;)</p></pre>
<br />
<p>- Yoann</p>
<br />
<p>On avril 11th, 2016, 11:19 matin 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 avr. 11, 2016, 11:19 matin</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>