nuvola-player-devel team mailing list archive

[mpdeimos <mpdeimos@xxxxxxxxx>]

Thanks for the review, here are few comments/questions before I start to rework.

> > Hence I've completely rewritten message handling for JS API 2.3 onwards.
> 
> So significant change is not acceptable for minor update of JavaScript API.

As I've said, the modification should be fully backwards compatible and not break existing services. Just in case a service wants to use the button integration, it has to register a handler instead of overriding it.
However, I can understand your concerns. Depending on your plans for 3.0 I would opt for the following:

* Wait with the whole implementation till 3.0 (I'm also ok with that)
* Move implementation back to gMusic (assuming that there will no other services to follow till 3.0 release?)
* Doing a mixture with keeping the implementation in main.js and passing the callbacks from the service integration side. However, I am not sure if it is worth if 3.0 contains JS API changes (see next question).

> However, I will take it into account during designing of JS API 3.0.

Is 3.0 planned to be JS-backwards compatible?
I haven't found code for 3.0 yet, when do you plan to push the branch to LP?

> > Nuvola.Action and Nuvola.NavigationMuttonIntegration are wrapped in
> additional curly brackets to indicate that they are separate objects and
> support readability.
> 
> Nice :-)
> 
> > I'm not sure if my integration code somehow messes with hide googleplus
> controls
> 
> Yes, your changes broke the fix bar dimensions code. However, the feature has
> been just removed from trunk.

I'll merge trunk when starting to rework, so will see what happened to this.

> 278     + * Copyright 2014 Martin Pöhlmann <http://mpdeimos.com>
> 
> Please use your contact e-mail instead of a website.

I was a bit picky about adding my gmail addy, because of spam :)

> 
> 381     + // Otherwise, if not yet integrated and disabled, do nothing.
> 382     + if (Nuvola.config.navigationButtons !== true)
> 383     + {
> 384     + return;
> 385     + }
> 
> The button should be enabled by default, so condition
> Nuvola.config.navigationButtons === false has to be used to allow
> Nuvola.config.navigationButtons to be undefined (the default value).

True, good catch. Dealing with undefined (besides null) is a bit strange for a Java/C# guy like me :)
-- 
https://code.launchpad.net/~mpdeimos/nuvola-player/bug-1212167/+merge/200249
Your team Nuvola Player Development is subscribed to branch lp:nuvola-player.