> 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
