Review Request 127618: Applet : Display icons and device selection for streams

[Yoann Laissus]

> On avr. 9, 2016, 1:18 après-midi, David Rosca wrote:
> > 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.
> > 
> > > 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.
> > 
> > Yes, that would indeed be much better.

> 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.

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 ?

> 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.

Yes, it's a good idea to have the icon in PulseObject.


I discard this review.

>> 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.

> Yes, that would indeed be much better.

I'll do another review for that.


- Yoann


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127618/#review94457
-----------------------------------------------------------


On avr. 9, 2016, 1:04 après-midi, Yoann Laissus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127618/
> -----------------------------------------------------------
> 
> (Updated avr. 9, 2016, 1:04 après-midi)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-pa
> 
> 
> Description
> -------
> 
> 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.
> 
> The second commit adds, for each streams, the combobox used in the KCM to select the device.
> 
> Branch here : https://quickgit.kde.org/?p=clones%2Fplasma-pa%2Flaissus%2Fplasma-pa.git&a=shortlog&h=9eadc541cd55862b85399719904cbe85b4af1147
> 
> 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
> 
> 
> Diffs
> -----
> 
>   applet/contents/ui/ListItemBase.qml d77d25a51b09149a1cf479eec0053184ae8625e6 
>   applet/contents/ui/StreamListItem.qml 7e84505902ca116b466bcd408f00ecaa9a4ce8da 
>   applet/contents/ui/main.qml 086716d739e5c67ccabfceaca4fea86fd40cc8a0 
>   src/kcm/package/contents/ui/DeviceComboBox.qml  
>   src/qml/CMakeLists.txt 9d021fcf61e4820efd0623060381dea0d7c30506 
>   src/qml/ClientIcon.qml 8256758048907fd8bf62f5d6e7a7176e1855c1e8 
>   src/qml/qmldir de44fca76deb79017be8c3033d1eb462242145e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127618/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Streams
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/04/09/f52f75ba-a800-4a82-8d75-08051edf7100__Screenshot_20160409_145911.png
> Devices
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/04/09/8adff934-e24d-4933-870e-7b23803ba5e3__Screenshot_20160409_145925.png
> 
> 
> Thanks,
> 
> Yoann Laissus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160411/b795bcd5/attachment.html>