> 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. > > Yoann Laissus 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. > > 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. > > David Rosca wrote: > > In this case, maybe should we totally remove the sub component support > in ListItems ? > > 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 :)
Ok, I'll take care of that too ;) - Yoann ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127618/#review94457 ----------------------------------------------------------- On avr. 11, 2016, 11:19 matin, Yoann Laissus wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127618/ > ----------------------------------------------------------- > > (Updated avr. 11, 2016, 11:19 matin) > > > 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
