Review Request 112208: KMix qml applet

Wed Aug 28 07:31:28 UTC 2013

> On Aug. 22, 2013, 10:46 p.m., Aaron J. Seigo wrote:
> > File Attachment: Applet Config Options
> > <http://git.reviewboard.kde.org/r/112208/#fcomment87>
> >
> >     Do we even really need horizontal/vertical orientation for the sliders as a configuration option? Other than "because we can" what is the actual end user value of that option?
> 
> Igor Poboiko wrote:
>     AFAIK MacOS and Windows use vertical slider orientation, while Gnome&Ubuntu use horizontal sliders. Some people may be used to one of the variants and find another one inconvenient. So why not let them choose? There is not that many options in that applet anyways.
> 
> Marco Martin wrote:
>     i would prefer not having that as well, plasma appletswere designed to make easy for the user to change a plasmoid with another if they don't like it, rther than making the developer maintain many different code paths of which some combination will always go untested...
>     
>     one thing without a config dialog that may be tried is adjusting horizontal or vertical how better suits the size of the config dialog at the moment...
> 
> Sebastian Kügler wrote:
>     Case in point: The use of QtHorizontal and QtVertical throughout the code suggest to me that the horizontal case wasn't really tested in the submitted version. Kinda supports Marco's fear of untested code pathes. ;-)
> 
> Diego Casella wrote:
>     I've tried to keep compatibility with the "old" KMix interface, which lets you choose whether you want horizontal or vertical sliders.
>     
>     @Marco You are completely right, but we don't want to end with two applets like "KMix with horizontal sliders" and "KMix with vertical sliders" right? We should try to get a reliable procedure to retrieve the size of the elements in the listview, which is the root of my issues with the applet resize.
>     
>     @Sebas Care to explain? Both the cases have been tested.
> 
> Sebastian Kügler wrote:
>     sure, QtVertical and QtHorizontal do not exist, that's a syntax error. I don't see how that could work, other than by complete accident.
> 
> Diego Casella wrote:
>     Maybe hose enums were kept for backwards compatibilty, because they still do exists. Right here:
>     https://projects.kde.org/projects/kde/kde-runtime/repository/revisions/master/entry/plasma/scriptengines/javascript/plasmoid/appletinterface.h#L156
>     
>     Since my very first draft of the kmix applet is litteraly *years* old, even though I heavily refactored/modified/dropped the code, the QtVertical enums is something iI never checked if they were still valid or not, since they always worked fine :)

Out of curiosity: can you now confirm rev1 is working even if it isn't supposed to, because somehow QtVertical/QtHorizontal are still defined in qml when they shouldn't?

Do you want to keep the possibility to change the orientation of the mixer controls?

- Diego

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

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/20130828/1494f87b/attachment.html>