Review Request 112208: KMix qml applet

Thu Aug 22 14:34:40 UTC 2013

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

Overall, it looks pretty good. There's a number of UI, code and coding style issues there, which will need sorting.

As to coding style, please have a look at http://community.kde.org/Plasma/QMLStyle
I've pointed some of the issues out, but overall, this needs going over the whole code and fixing.

Nice to see this coming together, btw.

One thing: kde-workspace is frozen, and even if this is in the kmix repo, it might be too late to replace it in Plasma Desktop. What are the concrete plans here?

File Attachment: Default look
<http://git.reviewboard.kde.org//r/112208/#fcomment65>
    this tooltip seems redundant, it's covering the exact same information it's showing, no?

File Attachment: Menu Actions
<http://git.reviewboard.kde.org//r/112208/#fcomment66>
    KMix and Phonon are jargon and have to go.

    Proposal:

    "Mixer Setup"
    "Audio Setup"

    Seems more in line with kmix

File Attachment: Menu Actions
<http://git.reviewboard.kde.org//r/112208/#fcomment67>
    jargon, see other comment

File Attachment: Menu Actions
<http://git.reviewboard.kde.org//r/112208/#fcomment68>
    Maybe we could align this in the same way as the batter applet does?

File Attachment: Applet Config Options
<http://git.reviewboard.kde.org//r/112208/#fcomment69>
    No groupboxes please for these rather single things.

    Use titles instead and get rid of the frames.

    The first title seems unnecessary, the options itself are way clear.

File Attachment: Vertical Control
<http://git.reviewboard.kde.org//r/112208/#fcomment70>
    Clipping

plasma/CMakeLists.txt
<http://git.reviewboard.kde.org/r/112208/#comment28392>

    use the installPackage macro for this and the following line

plasma/kmix-applet-qml/contents/config/main.xml
<http://git.reviewboard.kde.org/r/112208/#comment28360>

    Why two values here? One bool seems enough?

plasma/kmix-applet-qml/contents/ui/ButtonBar.qml
<http://git.reviewboard.kde.org/r/112208/#comment28395>

    Any reason to not leave this out, defaults to UTF-8?

plasma/kmix-applet-qml/contents/ui/ButtonBar.qml
<http://git.reviewboard.kde.org/r/112208/#comment28361>

    Please add the year in the copyright notice

plasma/kmix-applet-qml/contents/ui/ButtonBar.qml
<http://git.reviewboard.kde.org/r/112208/#comment28362>

    no spaces inside (), also elsewhere

plasma/kmix-applet-qml/contents/ui/ButtonBar.qml
<http://git.reviewboard.kde.org/r/112208/#comment28396>

    Bad naming, I got confused while reading this, as it reads like "run this program".

    Maybe rename to something more descriptive?

plasma/kmix-applet-qml/contents/ui/ButtonBar.qml
<http://git.reviewboard.kde.org/r/112208/#comment28363>

    spaces inside parentheses should go

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28397>

    For all JS code in the applet, please don't forget the line-ending ;

    (will need to be fixed throughout)

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28365>

    Qt.Horizontal?

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28398>

    here, ; usage is even inconsistent

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28399>

    add context for translators

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28366>

    PlasmaCore.IconItem does this for you

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28400>

    not translatable this way:

    use condition ? i18n(...) : i18(...) instead

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28401>

    I wonder if this can't be done without the roundtrip, i.e. directly use the icon name?

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28368>

    if (_control.muted)

plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28369>

    You're not actually doing anything with the return value here. Oversight?

plasma/kmix-applet-qml/contents/ui/HorizontalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28402>

    not needed here

plasma/kmix-applet-qml/contents/ui/HorizontalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28370>

    namespacing: ... as QtExtras

plasma/kmix-applet-qml/contents/ui/MixersList.qml
<http://git.reviewboard.kde.org/r/112208/#comment28403>

    namespace it as QtExtras

plasma/kmix-applet-qml/contents/ui/MixersList.qml
<http://git.reviewboard.kde.org/r/112208/#comment28404>

    Qt.Vertical

plasma/kmix-applet-qml/contents/ui/MixersList.qml
<http://git.reviewboard.kde.org/r/112208/#comment28371>

    Qt.Vertical

plasma/kmix-applet-qml/contents/ui/MixersList.qml
<http://git.reviewboard.kde.org/r/112208/#comment28372>

    whitespace

plasma/kmix-applet-qml/contents/ui/MixersList.qml
<http://git.reviewboard.kde.org/r/112208/#comment28405>

    ws

plasma/kmix-applet-qml/contents/ui/VerticalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28373>

    this will drive our translator nuts, unless there's a comment :)

plasma/kmix-applet-qml/contents/ui/VerticalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28374>

    Qt.Vertical

plasma/kmix-applet-qml/contents/ui/VerticalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28375>

    use PlasmaCore.IconItem

plasma/kmix-applet-qml/contents/ui/VerticalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28376>

    not translatable, try:

    condition ? i18n("...", bla) : i18n("...", bla2)

plasma/kmix-applet-qml/contents/ui/VerticalControl.qml
<http://git.reviewboard.kde.org/r/112208/#comment28377>

    again, return value not used

plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28378>

    Seems unnecessary, could just be UTF-8? (also elsewhere)

plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28379>

    unused

plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28380>

    unused

plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml
<http://git.reviewboard.kde.org/r/112208/#comment28381>

    namespace QtExtras

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28382>

    UTF-8

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28383>

    add year

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28384>

    not used

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28385>

    should be in line with sizing in metadata.desktop?

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28387>

    Qt.Horizontal, Qt.Vertical

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28389>

    for (var i =...

    (whitespace)

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28390>

    no jargon please

plasma/kmix-applet-qml/contents/ui/kmixapplet.qml
<http://git.reviewboard.kde.org/r/112208/#comment28391>

    no jargon please

plasma/kmix-applet-qml/metadata.desktop
<http://git.reviewboard.kde.org/r/112208/#comment28358>

    Explanation?

    Keywords would also be nice.

plasma/kmix-applet-qml/metadata.desktop
<http://git.reviewboard.kde.org/r/112208/#comment28357>

    --jargon

plasma/kmix-applet-qml/metadata.desktop
<http://git.reviewboard.kde.org/r/112208/#comment28359>

    Use "Multimedia" as category

plasma/kmix-applet-qml/metadata.desktop
<http://git.reviewboard.kde.org/r/112208/#comment28393>

    sync size with toplevel Item

plasma/kmix-applet-qml/metadata.desktop
<http://git.reviewboard.kde.org/r/112208/#comment28394>

    Just remove this one

- Sebastian Kügler

On Aug. 22, 2013, 1:31 p.m., Diego Casella wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112208/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2013, 1:31 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/CMakeLists.txt 5e1dc90 
>   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
> 
> 
> Thanks,
> 
> Diego Casella
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130822/a792eaa8/attachment-0001.html>