Review Request 112208: KMix qml applet

Mon Sep 2 21:59:16 UTC 2013

> On Aug. 28, 2013, 9:10 p.m., Xuetian Weng wrote:
> > plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml, line 55
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183953#file183953line55>
> >
> >     if volume is changed from kmix, not from the applet, this would issue a duplicate service call to dataengine.
> >     
> >     my solution is to add a bool protector to disable this signal if the data change is not from user.
> 
> Xuetian Weng wrote:
>     sorry for noise, maybe you should try onSliderMoved signal instead.
> 
> Diego Casella wrote:
>     cannot bind to a non-existent property, says plasma ... are you sure this method exists?
> 
> Xuetian Weng wrote:
>     ah sorry... I initially thought it's Plasma/Slider in cpp but now it seems it's another implementation in qml.
>     the cpp version of Plasma/Slider is actually in plasma.graphicswidget but since we're moving to qt5 in the future graphicswidget implementation is not a preferable solution...
>     
>     So you might want to use the solution in my first comment.
> 
> Diego Casella wrote:
>     imho it is too hackish and the applet works good even in this state. Do we really need to introduce something like that when you change the volume levels for, say, a total of 5 minutes every day (overstimated)?
> 
> Xuetian Weng wrote:
>     There is some possible data race when last time I looked at the brightness slider in battery plasmoid. Especially when user drag the slider which generates more than one onValueChanged (Click on slider is usually ok).
>     
>     And the slider looks very jumpy in that case.
>     
>     user drags the slider 
>     onValueChanged caused by user
>     call setVolume to A to backend
>     user drags slider further to B
>     onValueChanged caused by user
>     call setVolume B to backend
>     
>     backend notifies the change of A
>     onValueChanged caused by backend notificatoin
>     slider change back to A but user mouse is actually already on B.
>     and now it will send setVolume to A again.
>     and it will generate another onValueChanged.
>     
>     Eventually it will have volume at B but if the difference of value A and B is huge, the visual reflection would be really bad.
>     And in some case it might generate really quite a lot onValueChanged (since theoretically it can generate infinite ping-pong loop if all events arrives just-in-time.). 
>     
>     I didn't test your plasmoid but you can try to drag it aggressively from on side to the other side to see if this is really a issue.
> 
> Diego Casella wrote:
>     Believe me, I know exactly the issue you are talking about. I tried various fixes it since 2010, when I did my first (ugly) proof of concept applet[1] in C++. Back at that time, i simply disconnected the slider, updated it, and then restored the connection.
>     Then, moving to javascript and after that, into qml world, I tried the boolean solution too, but I realized that: I was adding kind of hackish code to address an issue which is in fact just a corner case, really. No one (mentally healthy enough :) would ever drag aggressively the audio slider back and forth and blow his/her auditive apparatus just because it's funny doing so.
>     I'm not saying your observation is wrong but, imho, we are adding more code to cover a really improper usage of the applet which is unlikely to happen.
>     Let's hear from other devs what they think about it, so we will better decide whether to deal with that issue or not :)
>     
>     
>     [1] http://quickgit.kde.org/?p=scratch%2Fcasella%2Fplasma-applet-kmix.git&a=blob&h=551c9eab7827c4d1d4a162bb772d9ec30091a1ad&hb=e5d4ddd21943cbf93166ff8fbd61755ad00d495e&f=mixercontrol.cpp#L61

This can work out differently per machine. I would say that it makes sense to disable reacting to the changed signal while the user is interacting, so maybe catch external changes, block them for a short amount when the user has just changed the value. It's a common case, and can make using the slider unnaturally jerky, so I think it does warrant some extra complexity in the code here, hopefully a pretty simple change.

Another solution would be to introduce a sender in the whole volume setting chain, and make sure to not react to one's own changes after it has done a round-trip. That would probably be a bit of overkill, though. :)

- Sebastian

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112208/#review38813
-----------------------------------------------------------

On Aug. 27, 2013, 8:40 a.m., Diego Casella wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112208/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2013, 8:40 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo, Christian Esken, Marco Martin, and Igor Poboiko.
> 
> 
> Description
> -------
> 
> KMix qml applet.
> As you can see from the screenshot, the applet is pretty much functional: you can display all the controls available, change its orientation, and decide to whether show all of them or just the Master Control, and refresh its status when new controls are added/removed/updated (such as Amarok current playing track). See screenshots below :)
> Differences from the old kmix tray:
> * no media player controls ( I never investigated how to get them, but honestly opening the audio applet to change/skip/pause audio track makes little sense to me ... if anyone wants this feature back, don't be shy and step in);
> * the button used to select which Mixers are visible has been changed to open Phonon kcm page: since visible mixers are already configurable from KMix app, having a button to show KMix *and* a button to modify Mixers visibilty made little sense here too, so I preferred to give more visibility to Phonon kcm;
> 
> Known issues:
> * there is still no way to get notified of mouse wheel events over the popupIcon, so it is not possible to scroll over to increase/decrease the master control volume;
> * no scroll events over the sliders too;
> * if you want to use the applet you most likely will disable KMix tray icon but, if you do so, KMix will show its GUI at every login and you have to close it manually. This requires KMix to be patched. Furthermore, if you click "KMix Setup" button, KMix window will not restored anymore: this needs to be pathed as well.
> * resize doesn't work properly.
> 
> 
> Diffs
> -----
> 
>   plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/HorizontalMixerListDelegate.qml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/VerticalControl.qml PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112208/diff/
> 
> 
> Testing
> -------
> 
> Tested against master and works fine.
> 
> 
> File Attachments
> ----------------
> 
> Default look
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet.png
> Menu Actions
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet1.png
> Applet Config Options
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet2.png
> Vertical Control
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet3.png
> ToolButton label and Config page after updates
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/24/kmix_applet5.png
> Control Icon and Label left aligned
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/27/kmix_applet6.png
> 
> 
> Thanks,
> 
> Diego Casella
> 
>

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