nuvola-player-devel team mailing list archive

[Jiří Janoušek <janousek.jiri@xxxxxxxxx>]

Review: Needs Fixing

Thanks for your work. There is one major issue: Songza.com redirects to daily.songza.com, probably everywhere except for US and Canada. Since daily.songza.com doesn't match the sandbox pattern, the Songza Daily website is opened in an user's default web browsers, which is very confusing. It's better to allow navigation to the Songza Daily website and to inform the user what's going on. See inline comments with proposed modifications.

Please attach a screenshot of Nuvola Player with a visible developer's bar with loaded Songza home page as it would be loaded for the first time.

https://bugs.launchpad.net/nuvola-player/+bug/1048347

Diff comments:

> === added directory 'data/nuvolaplayer/services/songza'
> === added file 'data/nuvolaplayer/services/songza/description.html'
> --- data/nuvolaplayer/services/songza/description.html	1970-01-01 00:00:00 +0000
> +++ data/nuvolaplayer/services/songza/description.html	2014-08-11 15:13:26 +0000
> @@ -0,0 +1,10 @@
> +<p><strong>Songza</strong> is a free music streaming and recommendation
> +service.</p>
> +<p>
> +<em>Source:
> +<a href="http://songza.com";>Official website</a></em>
> +<a href="http://en.wikipedia.org/wiki/Songza";>Songza on Wikipedia</a>,
> +</p>
> +<h2>Country Availability</h2>
> +<p><strong>Songza</strong> is currently only available in the United States and
> +Canada.</p>
> 
> === added file 'data/nuvolaplayer/services/songza/integration.js'
> --- data/nuvolaplayer/services/songza/integration.js	1970-01-01 00:00:00 +0000
> +++ data/nuvolaplayer/services/songza/integration.js	2014-08-11 15:13:26 +0000
> @@ -0,0 +1,185 @@
> +/*
> + * Copyright 2014 Google Inc. All Rights Reserved.
> + * Author: dharan@xxxxxxxxxx (Ramesh Dharan)
> + *
> + * Copyright 2011-2013 Jiří Janoušek <janousek.jiri@xxxxxxxxx>
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright notice, this
> + *    list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright notice,
> + *    this list of conditions and the following disclaimer in the documentation
> + *    and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +/* Anonymous function is used not to pollute environment */
> +(function(Nuvola)
> +{
> +  /**
> +   * Creates integration binded to Nuvola JS API
> +   */
> +  var Integration = function()
> +  {
> +    /* For debug output */
> +    this.name = "Songza";
> +
> +    /* Overwrite default command function */
> +    Nuvola.onMessageReceived = Nuvola.bind(this, this.messageHandler);
> +
> +    require(["songza/app", ], Nuvola.bind(this, function(App) {

Please try-catch ReferenceError: "integration.js:42: ReferenceError: Can't find variable: require in frame '__main__'."

> +      App.postInit(Nuvola.bind(this, function() {
> +        this.songza = App.getInstance();
> +      }));
> +    }));
> +
> +    /* Let's run */
> +    this.state = Nuvola.STATE_NONE;
> +    this.can_prev = null;
> +    this.can_next = null;
> +    this.can_thumbs_up = null;
> +    this.can_thumbs_down = null;
> +
> +    this.update();

-    this.update();
+    
+    var state = document.readyState;
+    if (state === "interactive" || state === "complete")
+      this._onPageReady();
+    else
+      document.addEventListener("DOMContentLoaded", Nuvola.bind(this, this._onPageReady));

> +  };
> +

Songza.com redirects to daily.songza.com, probably everywhere except fo US and Canada. It's better to inform user what's going on. Feel free to improve wording.

+  
+  Integration.prototype._onPageReady = function()
+  {
+    if (location.hostname == "daily.songza.com")
+    {
+      if (Nuvola.warn)
+      {
+        var gst = Nuvola.libVersion("gstreamer");
+        if (Nuvola.config.warnNotAvailable !== false)
+        {
+          var show_again = Nuvola.warn("You are on Songza Daily website",
+          "You are currently browsing the Songza Daily website. You might have been redirected here " +
+          "because the Songza music streaming and recommendation service is not available in your " +
+          "country, but only in the United States and Canada.");
+          if (show_again === false)
+          {
+            Nuvola.config.warnNotAvailable = show_again;
+            Nuvola.saveConfig();
+          }
+        }
+      }
+    }
+    else
+    {
+      this.update();
+    }
+  }

> +  /**
> +   * Updates current playback state
> +   */
> +  Integration.prototype.update = function()
> +  {
> +    // Default values
> +    var state = Nuvola.STATE_NONE;
> +    var can_prev = false;
> +    var can_next = false;
> +    var can_thumbs_up = false;
> +    var can_thumbs_down = false;
> +
> +    var song = null;
> +    var artist = null;
> +    var album = null;
> +    var art = null;
> +
> +    try {
> +      var player = this.songza.getPlayer();
> +      var manager = player.getManager();
> +      var songContext = player.songContext(player.findSongOrCurrent());
> +
> +      state = player.isPlaying() ? Nuvola.STATE_PLAYING :
> +              player.isPaused() ? Nuvola.STATE_PAUSED : state;
> +
> +      can_next = manager.canSkip();
> +
> +      song = songContext.song.title;
> +      artist = songContext.song.artist;
> +      album = songContext.album;
> +      art = songContext.song.image_url;
> +
> +      can_thumbs_up = true;
> +      can_thumbs_down = true;
> +
> +    } catch(e) {
> +      console.debug(this.name + ": could not update song state");
> +    }
> +
> +    // Save state
> +    this.state = state;
> +
> +    // Submit data to Nuvola backend
> +    Nuvola.updateSong(song, artist, album, art, state);
> +
> +    if (this.can_next !== can_next) {
> +      this.can_next = can_next;
> +      Nuvola.updateAction(Nuvola.ACTION_NEXT_SONG, can_next);
> +    }
> +
> +    if (this.can_thumbs_up !== can_thumbs_up) {
> +      this.can_thumbs_up = can_thumbs_up;
> +      Nuvola.updateAction(Nuvola.ACTION_THUMBS_UP, can_thumbs_up);
> +    }
> +
> +    if (this.can_thumbs_down !== can_thumbs_down) {
> +      this.can_thumbs_down = can_thumbs_down;
> +      Nuvola.updateAction(Nuvola.ACTION_THUMBS_DOWN, can_thumbs_down);
> +    }
> +
> +    // Schedule update
> +    setTimeout(Nuvola.bind(this, this.update), 500);
> +  };
> +
> +  /**
> +   * Message handler
> +   * @param cmd command to execute
> +   */
> +  Integration.prototype.messageHandler = function(cmd)
> +  {
> +    /* Respond to user actions */
> +    try
> +    {
> +      var player = this.songza.getPlayer();
> +      var song = player.findSongOrCurrent();
> +      var stream = song.get("streamSong");
> +
> +      switch (cmd)
> +      {
> +        case Nuvola.ACTION_PLAY:
> +          player.play();
> +          break;
> +        case Nuvola.ACTION_PAUSE:
> +          player.pause();
> +          break;
> +        case Nuvola.ACTION_TOGGLE_PLAY:
> +          player.play_or_pause();
> +          break;
> +        case Nuvola.ACTION_NEXT_SONG:
> +          player.skip();
> +          break;
> +
> +        /*
> +         * XXX player.voteUp() method relies on a jQuery selector that doesn't
> +         *     seem to work in this context so we can't use it. This code mimics
> +         *     what the player method actually does, but it feels fragile since
> +         *     the upstream implementation could change.
> +         */
> +        case Nuvola.ACTION_THUMBS_UP:
> +          song.set({vote : "up"});
> +          stream.voteUp();
> +          player.songTrigger("vote-up");
> +          break;
> +        case Nuvola.ACTION_THUMBS_DOWN:
> +          song.set({vote : "down"});
> +          stream.voteDown();
> +          player.songTrigger("vote-down");
> +          break;
> +
> +        default:
> +          // Other commands are not supported
> +          throw {"message": "Not supported."};
> +      }
> +      console.log(this.name + ": comand '" + cmd + "' executed.");
> +    }
> +    catch (e)
> +    {
> +      // Older API expected exception to be a string.
> +      throw (this.name + ": " + e.message);
> +    }
> +  };
> +
> +  /* Store reference */
> +  Nuvola.integration = new Integration(); // Singleton
> +
> +  // Immediately call the anonymous function with Nuvola JS API main object as an argument.
> +  // Note that "this" is set to the Nuvola JS API main object.
> +})(this);
> 
> === added file 'data/nuvolaplayer/services/songza/metadata.conf'
> --- data/nuvolaplayer/services/songza/metadata.conf	1970-01-01 00:00:00 +0000
> +++ data/nuvolaplayer/services/songza/metadata.conf	2014-08-11 15:13:26 +0000
> @@ -0,0 +1,10 @@
> +name = Songza
> +home_page = https://songza.com/

Set home_page = https://daily.songza.com/ during testing of the user warning and select "Go → Home" to navigate to the home page.

> +sandbox_pattern = https?://songza.com/

Songza.com redirects to daily.songza.com, probably everywhere except fo US and Canada.

-sandbox_pattern = https?://songza.com/
+sandbox_pattern = https?://(daily\.)?songza.com/

> +maintainer_name = Ramesh Dharan
> +maintainer_link = https://launchpad.net/~rameshdharan
> +version = 1
> +version_minor = 0
> +flash_plugin = no
> +requirements_specified = yes
> +api_major = 2
> 


-- 
https://code.launchpad.net/~rameshdharan/nuvola-player/songza/+merge/230330
Your team Nuvola Player Development is subscribed to branch lp:nuvola-player.