mpris2 engine and nowplaying QML applet

[David Edmundson]

On Mon, Apr 30, 2012 at 2:24 PM, Alex Merry <kde at randomguy3.me.uk> wrote:
> On 27/04/12 23:52, Alex Merry wrote:
>>
>> This is following up my previous email about the mpris2 engine, and
>> asking for a formal review.
>>
>> I've lost track of how the review procedure has changed since the move
>> to git, and I can't find documentation of it anywhere, so I apologise if
>> I screw this up.
>
>
> Since my previous email got a generally favourable response, I'll wait two
> weeks (11th May) and, if there are no objections by then, I'll put mpris2 in
> kde-workspace, replace nowplaying in kdeplasma-addons and put a kWarning()
> in the nowplaying engine (in kde-workspace) saying that it is deprecated and
> mpris2 should be used instead.
>

Review of the now-playing QML applet (note I've not looked at mpris2-dataengine)

Now Playing QML Review:
    all code should be in -> contents/ui, not at the top level dir.

    Do you really want it set to keep-aspect ratio? It's not a great
default aspect ratio anyway.

    Controls can overlap with the bottom row of text when resized. It
may be better to set it so that the gap for the controls is always
present.

MetadataPanel.qml
    albumLabel:
    Don't set the opacity on a font to change the colour. It works for
black on white, but easily has the potential to be completely
unreadable on some colour palettes/plasma themes. You're also
implicitly using a new colour that the user hasn't specified, if
everyone did this we would end up with so many inconsistent colours as
every author chooses their own opacity. Specify it directly using
something from the user's theme. ("inactive" would be a similar colour
on a standard oxygen setup)

    Though if we were following other plasma applets such as the Power
Applet (so a convention that is setting in, though we really need to
write some HIG) the convention would be
        "by: <b>The Beatles</b>" all in the regular colour.

 (note also the additional colon in the label)

Controls.qml / PlayPauseButton.qml
    why do you have a singleshot timer to associate controls? I
imagine it's a work around for something not being ready, but it needs
documenting for when the next guy comes along and simplifies(breaks)
it.


Overall:
 I'd like these comments addressed or discussed if you disagree,
though overall it's a massive "Ship it!" from me.

Dave