On Sat, May 5, 2012 at 2:29 PM, Alex Merry <k...@randomguy3.me.uk> wrote:
> 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 don't think we're in much disagreement. I'm saying others bold the
value part of the label, you've un-bolded the key part of the label.
Effectively the same idea. Until there's a desktop HIG neither is
right nor wrong.
> 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.
>
Glad I commented on it then!
Ship it!
Has anyone reviewed the Mpris-dataengine? If not I'll do that this evening.
> Alex
>
> _______________________________________________
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
