mpris2 engine and nowplaying QML applet

[Alex Merry]

On 03/05/12 22:18, David Edmundson wrote:
> Now Playing QML Review:
>     all code should be in -> contents/ui, not at the top level dir.

Done

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

Ah, I'd forgotten that it's necessary to explicitly turn off keeping the
aspect ratio.  Fixed.

>     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.

This was a deliberate decision; most of the time the controls are not
needed, so reserving a space for them just prevents the user from saving
space for no real reason.

> 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)

I went for setting font.weight to light, instead.  The reason I didn't
go for increasing the font weight of the fields themselves is that most
of the time those labels are unnecessary, as the user will know which
field is which from knowledge of their own music collection.  It's
really just a hint for when it's ambiguous.

I also think that including the colon looks significantly worse than
omitting it.  In English, at least, it makes the whole text read like a
sentence, but the arrangement allows for it to be read a line-at-a-time
as well (which may be the only sensible way to read it in other
languages).  From a translating POV, Amarok's Current Track applet (on
its internal contextual view) follows the same pattern (in fact, I
copied the idea from there).

> 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.

Ah, that was entirely unnecessary, in fact, and was a relic from before
I figured out that it is possible to keep around a Plasma::Service in a
separate .js file.  Removed now.

Alex