Review Request 112208: KMix qml applet

Sat Aug 24 16:12:06 UTC 2013

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > File Attachment: Menu Actions
> > <http://git.reviewboard.kde.org/r/112208/#fcomment75>
> >
> >     Maybe we could align this in the same way as the batter applet does?

The menu comes from a right-click from within the widget -not the applet icon in the panel-, but i forgot to check the "include mouse pointer" option when I took that screenshot :)

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > File Attachment: Vertical Control
> > <http://git.reviewboard.kde.org/r/112208/#fcomment76>
> >
> >     Clipping

This falls in the "known issues -> resizing" stuff: it is kinda hard to get the sizes right but the applet itself is resizeable so, until we get this right, the user can expand it as much as needed to fix it. That operation needs to be done only once because then plasma keeps track of it, so it would be a single-time hassle.. what do you think?

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > plasma/CMakeLists.txt, line 2
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183950#file183950line2>
> >
> >     use the installPackage macro for this and the following line

First it needs to be exported, since it lives inside kde-workspace/plasma/CMakeLists.txt ;)

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > plasma/kmix-applet-qml/contents/ui/ButtonBar.qml, line 77
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183952#file183952line77>
> >
> >     Bad naming, I got confused while reading this, as it reads like "run this program".
> >     
> >     Maybe rename to something more descriptive?

It does run an external program indeed. Changed the name, I hope this one is more descriptive.

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml, line 80
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183953#file183953line80>
> >
> >     PlasmaCore.IconItem does this for you

Hmm are you sure? PlasmaCore.Svg is a non graphical item which is used to get svg elements and then to forward them to the applet popup icon and tooltip, while PlasmaCore.IconItem creates a graphical item, which is not needed here.

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml, line 105
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183953#file183953line105>
> >
> >     I wonder if this can't be done without the roundtrip, i.e. directly use the icon name?

Tooltips removed, anyway that code is used few lines below to set the popup applet icon and its tooltip.
Now the interesting part: I've managed to remove the temporary QIcon() creation when setting the popup icon tooltip data, but I couldn't do the same with the popup icon because otherwise the icon wont work ... Something broken in libplasma perhaps?

> On Aug. 22, 2013, 2:34 p.m., Sebastian Kügler wrote:
> > plasma/kmix-applet-qml/contents/ui/VerticalControl.qml, line 79
> > <http://git.reviewboard.kde.org/r/112208/diff/1/?file=183956#file183956line79>
> >
> >     use PlasmaCore.IconItem

See the comment above.

- Diego

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

On Aug. 24, 2013, 3:11 p.m., Diego Casella wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112208/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2013, 3:11 p.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/config/main.xml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/ButtonBar.qml PRE-CREATION 
>   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/MixersList.qml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/VerticalControl.qml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/config.ui PRE-CREATION 
>   plasma/kmix-applet-qml/contents/ui/kmixapplet.qml PRE-CREATION 
>   plasma/kmix-applet-qml/metadata.desktop 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
> 
> 
> Thanks,
> 
> Diego Casella
> 
>

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