Merge lp:~ahayzen/music-app/add-url-dispatcher-album into lp:music-app/trusty

[Andrew Hayzen]

* Add album:///artist/album and music:///path/to/file url dispatcher support
* Simplification of file:// handling

* Add album:///artist/album and music:///path/to/file url dispatcher support
* Simplification of file:// handling

Installation:
- Desktop
cd /tmp
bzr branch lp:~andrew-hayzen/music-app/add-url-dispatcher-album music-app
cd music-app
click-buddy --dir . --provision
rm -rf /tmp/music-app

- Mobile
cp -r /usr/share/click/preinstalled/com.ubuntu.music/current/lib /opt/click.ubuntu.com/com.ubuntu.music/current/
pkill unity8

Testing:
On device use the commands below to, for example, start playing the album 'Day & Age' or the track 'Flesh and Bone'.
The protocols file:// and album:// should be tested when the app is in both an open and closed state.
sudo -u phablet -i
url-dispatcher "album:///The Killers/Day & Age"
url-dispatcher "file:///home/phablet/Music/Battle Born/01 Flesh and Bone.m4a"

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:425
http://91.189.93.70:8080/job/music-app-ci/688/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1999
    SUCCESS: http://91.189.93.70:8080/job/music-app-raring-amd64-ci/688
    SUCCESS: http://91.189.93.70:8080/job/music-app-saucy-amd64-ci/690
    SUCCESS: http://91.189.93.70:8080/job/music-app-trusty-amd64-ci/409

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/music-app-ci/688/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

9 + set(EXEC "qmlscene -qt5 ${MAIN_QML} --file=%u -I ./plugins")

Would be probably better to rename the "file" param to "url".

[Andrew Hayzen (ahayzen)]

Good idea when I'm back at my pc I'll update it, thanks :-)

[Victor Thompson (vthompson)]

On Trusty r278 url-dispatcher is not in $PATH and when executing the following directly:

/usr/lib/arm-linux-gnueabihf/url-dispatcher/url-dispatcher

I get the following error:

** (process:4424): ERROR **: Unable to connect to D-Bus: Cannot autolaunch D-Bus without X11 $DISPLAY

Is there something else I need to do to utilize this program?

[Andrew Hayzen (ahayzen)]

Victor, are you logged in as the phablet user rather than root. If root run the following command to switch then try again.
sudo -u phablet -i

[Victor Thompson (vthompson)]

I am:

root@ubuntu-phablet:~# sudo -u phablet -i
phablet@ubuntu-phablet:~$ url-dispatcher "file:///home/phablet/Music/Islands/Return to the Sea/09 If.mp3"
-bash: url-dispatcher: command not found
phablet@ubuntu-phablet:~$ /usr/lib/arm-linux-gnueabihf/url-dispatcher/url-dispatcher "file:///home/phablet/Music/Islands/Return to the Sea/09 If.mp3"

** (process:3060): WARNING **: Unable to get name 'com.canonical.URLDispatcher'
phablet@ubuntu-phablet:~$

I just upgraded to r279 and the error message is different than it was for my attempt with r278.

[Andrew Hayzen (ahayzen)]

Hmmm strange IIRC I was on 227, I'm currently traveling home for easter I'll have a look when I get back.

[Michal Hruby (mhr3)]

> On Trusty r278 url-dispatcher is not in $PATH and when executing the following
> directly:
>
> /usr/lib/arm-linux-gnueabihf/url-dispatcher/url-dispatcher

That's the url-dispatcher daemon, not the binary to launch urls, that one is in pkg url-dispatcher-tools.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:427
http://91.189.93.70:8080/job/music-app-ci/689/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2000
    SUCCESS: http://91.189.93.70:8080/job/music-app-raring-amd64-ci/689
    SUCCESS: http://91.189.93.70:8080/job/music-app-saucy-amd64-ci/691
    SUCCESS: http://91.189.93.70:8080/job/music-app-trusty-amd64-ci/410

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/music-app-ci/689/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

Michal, thank you.

Andrew, would it make sense to enforce the album uri scheme to be "album://Artist/Album" rather than "album:///Artist/Album". The 3rd "/" in most URI handlers seem to indicate the root directory--which doesn't exist in the URI scheme we are introducing.

I'll try to review the rest shortly.

review: Needs Information

[Victor Thompson (vthompson)]

One thing to think about is that albums and artists can have the slash "/" character in them [1]. Perhaps we should pick another delimiter? I think we should talk with the Music scope folks to see if we can come up with a better solution.

[1] http://www.last.fm/search?q=/&type=album and http://www.last.fm/search?q=/&type=artist

[Michal Hruby (mhr3)]

> Andrew, would it make sense to enforce the album uri scheme to be
> "album://Artist/Album" rather than "album:///Artist/Album". The 3rd "/" in
> most URI handlers seem to indicate the root directory--which doesn't exist in
> the URI scheme we are introducing.

album://Artist/Album would make the Artist host part of the uri, and the host part has encoding limitations, that's why we're forcing everything to be path. Also, if there's a '/' in the name, it'd be encoded with '%2F', as in http uris.

[Victor Thompson (vthompson)]

Thanks again. Andrew, in that case most of this looks great. I think we should decode the album:// URI in the split array after we split the string, however.

review: Needs Fixing

[lp:~ahayzen/music-app/add-url-dispatcher-album]

428. By Andrew Hayzen on 2014-04-07

* Add decodeURIComponent to split album:// parameters

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:428
http://91.189.93.70:8080/job/music-app-ci/690/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2007
    SUCCESS: http://91.189.93.70:8080/job/music-app-raring-amd64-ci/690
    SUCCESS: http://91.189.93.70:8080/job/music-app-saucy-amd64-ci/692
    SUCCESS: http://91.189.93.70:8080/job/music-app-trusty-amd64-ci/411

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/music-app-ci/690/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

This modification breaks the fix for lp:1292921. It appears as though the decided file parameter won't no longer matches the encoded (?) file in the music library?

review: Needs Fixing

[Victor Thompson (vthompson)]

Sorry, decoded file parameter

[lp:~ahayzen/music-app/add-url-dispatcher-album]

429. By Andrew Hayzen on 2014-04-08

* Add music:// protocol
* Modify URL handling

430. By Andrew Hayzen on 2014-04-08

* Fix for encoding

[Andrew Hayzen (ahayzen)]

After investigations I found that the url stored in the database is neither encoded nor decoded, it is somewhat in between.

If you have a track with the file path ending
~!@#$%^&*(){}[]:,;?+\'"\\ .mp3
It is stored in the database as
~!@%23$%25%5E&*()%7B%7D%5B%5D:,%3B%3F+%5C'%22%5C%5C .mp3
It appears from the url-dispatcher as
~!@%23$%25%5E&*()%7B%7D[]:,;%3F+%5C'%22%5C%5C%20.mp3

Therefore ; [ ] need to be switched to %3B %5B %5D and a %20 needs to be switched to a space.

Note that a / and = cannot be used in the filename as a / is used to declare a folder and a = breaks the command line arguments.

[Andrew Hayzen (ahayzen)]

Ah international characters now cause issues such as é. Anyone got any better ideas to make the dispatcher url the same as the database url?

review: Needs Fixing

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:430
http://91.189.93.70:8080/job/music-app-ci/691/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2012
    SUCCESS: http://91.189.93.70:8080/job/music-app-raring-amd64-ci/691
    SUCCESS: http://91.189.93.70:8080/job/music-app-saucy-amd64-ci/693
    SUCCESS: http://91.189.93.70:8080/job/music-app-trusty-amd64-ci/412

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/music-app-ci/691/rebuild

review: Approve (continuous-integration)

[lp:~ahayzen/music-app/add-url-dispatcher-album]

431. By Andrew Hayzen on 2014-04-08

* decode file before it goes in the database

432. By Andrew Hayzen on 2014-04-09

* Revert to using linear search for processFile()

[Andrew Hayzen (ahayzen)]

I've reverting to using a linear search like before. I believe everything is fixed now, please retest :)

review: Approve

[lp:~ahayzen/music-app/add-url-dispatcher-album]

433. By Andrew Hayzen on 2014-04-09

* Remove tracing

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:433
http://91.189.93.70:8080/job/music-app-ci/692/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2023
    SUCCESS: http://91.189.93.70:8080/job/music-app-raring-amd64-ci/692
    SUCCESS: http://91.189.93.70:8080/job/music-app-saucy-amd64-ci/694
    SUCCESS: http://91.189.93.70:8080/job/music-app-trusty-amd64-ci/413

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/music-app-ci/692/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

Awesome, thanks. We'll look at a way not to do the 'needle in a haystack' linear search in the future. Nice work!

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-03-10 10:17:36 +0000
+++ CMakeLists.txt	2014-04-09 20:30:41 +0000
@@ -25,7 +25,7 @@
     set(CMAKE_INSTALL_BINDIR /)
     set(DATA_DIR /)
     set(ICON ${ICON_FILE})
-    set(EXEC "qmlscene -qt5 ${MAIN_QML} --file=%f -I ./plugins")
+    set(EXEC "qmlscene -qt5 ${MAIN_QML} --url=%u -I ./plugins")
     set(DESKTOP_DIR ${DATA_DIR})
     set(URLS_DIR ${DATA_DIR})
 else(CLICK_MODE)

=== modified file 'com.ubuntu.music_music.url-dispatcher'
--- com.ubuntu.music_music.url-dispatcher	2014-01-31 20:43:59 +0000
+++ com.ubuntu.music_music.url-dispatcher	2014-04-09 20:30:41 +0000
@@ -0,0 +1,8 @@
+[
+    {
+        "protocol": "album"
+    },
+    {
+        "protocol": "music"
+    }
+]

=== modified file 'meta-database.js'
--- meta-database.js	2014-03-06 01:57:57 +0000
+++ meta-database.js	2014-04-09 20:30:41 +0000
@@ -123,17 +123,16 @@
 }
 
 // This function is used to retrieve meta data from the database
-function getMetadata(file,type) {
+function getMetadata(file) {
    var db = getDatabase();
    var res="";
 
    try {
        db.transaction(function(tx) {
-         //var rs = tx.executeSql('SELECT type=?;',[type],' FROM metadata WHERE file=?;', [file]); // tries to get the title of track
-         var rs = tx.executeSql('SELECT ? FROM metadata WHERE file=?;', [type,file]); // tries to get the title of track
+         var rs = tx.executeSql('SELECT * FROM metadata WHERE file=?;', [file]); // tries to get the title of track
 
          if (rs.rows.length > 0) {
-              res = rs.rows.item(0).value;
+              res = rs.rows.item(0);
          } else {
              res = "Unknown";
          }
@@ -335,6 +334,21 @@
     return res;
 }
 
+function getArtistAlbumTracks(artist, album) {
+    var res = [];
+    var db = getDatabase();
+    //console.log("Album: " + album);
+    db.transaction( function(tx) {
+        var rs = tx.executeSql("SELECT * FROM metadata WHERE artist=? AND album=? ORDER BY artist COLLATE NOCASE ASC, album COLLATE NOCASE ASC, CAST(number AS int) ASC", [artist, album]);
+        for(var i = 0; i < rs.rows.length; i++) {
+            var dbItem = rs.rows.item(i);
+            //console.log("Artist:"+ dbItem.artist + ", Album:"+dbItem.album + ", Title:"+dbItem.title + ", File:"+dbItem.file + ", Art:"+dbItem.cover + ", Genre:"+dbItem.genre);
+            res.push({artist:dbItem.artist, album:dbItem.album, title:dbItem.title, file:dbItem.file, cover:dbItem.cover, length:dbItem.length, year:dbItem.year, genre:dbItem.genre});
+        }
+    });
+    return res;
+}
+
 function getGenres() {
     var res = [];
     var db = getDatabase();

=== modified file 'music-app.qml'
--- music-app.qml	2014-03-28 18:59:23 +0000
+++ music-app.qml	2014-04-09 20:30:41 +0000
@@ -117,7 +117,7 @@
         //defaultArgument.valueNames: ["URI"] // should be used when bug is resolved
         // grab a file
         Argument {
-            name: "file"
+            name: "url"
             help: "URI for track to run at start."
             required: false
             valueNames: ["track"]
@@ -198,42 +198,85 @@
     // (already in the database), not e.g. http:// URIs or files in directories
     // not picked up by Grilo
     Connections {
+        id: uriHandler
         target: UriHandler
+
+        function processAlbum(uri) {
+            var split = uri.split("/");
+
+            if (split.length < 2) {
+                console.debug("Unknown artist-album " + uri + ", skipping")
+                return;
+            }
+
+            // Get tracks
+            var tracks = Library.getArtistAlbumTracks(decodeURIComponent(split[0]), decodeURIComponent(split[1]));
+
+            if (tracks.length === 0) {
+                console.debug("Unknown artist-album " + uri + ", skipping")
+                return;
+            }
+
+            // Enqueue
+            for (var track in tracks) {
+                trackQueue.append(tracks[track]);
+            }
+
+            // Play first track
+            trackClicked(trackQueue, 0, true);
+        }
+
+        function processFile(uri, play) {
+            uri = decodeURIComponent(uri);
+
+            // search for path in library
+            var library = Library.getAll();
+            var track = false;
+
+            for (var item in library) {
+                if (decodeURIComponent(library[item].file) === uri) {
+                    track = library[item];
+                    break;
+                }
+            }
+
+            if (!track) {
+                console.debug("Unknown file " + uri + ", skipping")
+                return;
+            }
+
+            // enqueue
+            trackQueue.append(track);
+
+            // play first URI
+            if (play) {
+                trackClicked(trackQueue, 0, true)
+            }
+        }
+
+        function process(uri, play) {
+            if (uri.indexOf("album:///") === 0) {
+                uriHandler.processAlbum(uri.substring(9));
+            }
+            else if (uri.indexOf("file://") === 0) {
+                uriHandler.processFile(uri.substring(7), play);
+            }
+            else if (uri.indexOf("music://") === 0) {
+                uriHandler.processFile(uri.substring(8), play);
+            }
+
+            else {
+                console.debug("Unsupported URI " + uri + ", skipping")
+            }
+        }
+
         onOpened: {
             // clear play queue
             trackQueue.model.clear()
             for (var i=0; i < uris.length; i++) {
                 console.debug("URI=" + uris[i])
-                // skip non-file:// URIs
-                if (uris[i].substring(0, 7) !== "file://") {
-                    console.debug("Unsupported URI " + uris[i] + ", skipping")
-                    continue
-                }
-
-                // search pathname in library
-                var file = decodeURIComponent(uris[i])
-                var index = -1;
-
-                for (var j=0; j < griloModel.count; j++)
-                {
-                    if (decodeURIComponent(griloModel.get(j).url.toString()) == file)
-                    {
-                        index = j;
-                    }
-                }
-
-                if (index <= -1) {
-                    console.debug("Unknown file " + file + ", skipping")
-                    continue
-                }
-
-                // enqueue
-                trackQueue.append(griloModel.get(index));
-
-                // play first URI
-                if (i == 0) {
-                    trackClicked(trackQueue, 0, true)
-                }
+
+                uriHandler.process(uris[i], i === 0);
             }
         }
     }
@@ -280,19 +323,6 @@
         customdebug("Arguments on startup: Debug: "+args.values.debug)
 
         customdebug("Arguments on startup: Debug: "+args.values.debug+ " and file: ")
-        if (args.values.file) {
-            argFile = args.values.file
-            if (argFile.indexOf("file://") != -1) {
-                //customdebug("arg contained file://")
-                // strip that!
-                argFile = argFile.substring(7)
-            }
-            else {
-                // do nothing
-                customdebug("arg did not contain file://")
-            }
-            customdebug(argFile)
-        }
 
         Settings.initialize()
         console.debug("INITIALIZED in tracks")
@@ -333,7 +363,6 @@
     property string lastfmusername
     property string lastfmpassword
     property string timestamp // used to scrobble
-    property string argFile // used for argumented track
     property var chosenElement: null
     property LibraryListModel currentModel: null  // Current model being used
     property var currentQuery: null
@@ -638,8 +667,6 @@
             }
 
             onFinished: {
-                var read_arg = false;
-
                 // FIXME: remove when grilo is fixed
                 var files = [];
                 var duplicates = 0;
@@ -673,18 +700,6 @@
                         genre: media.genre || i18n.tr("Unknown Genre")
                     };
 
-                    if (read_arg === false && decodeURIComponent(argFile) === decodeURIComponent(file))
-                    {
-                        trackQueue.model.clear();
-                        trackQueue.model.append(record)
-                        trackClicked(trackQueue, 0, true);
-
-                        // grilo model sometimes has duplicates
-                        // causing the track to be paused the second time
-                        // this ignores the second time
-                        read_arg = true;
-                    }
-
                     //console.log("Artist:"+ media.artist + ", Album:"+media.album + ", Title:"+media.title + ", File:"+file + ", Cover:"+media.thumbnail + ", Number:"+media.trackNumber + ", Genre:"+media.genre);
                     Library.setMetadata(record)
                 }
@@ -694,6 +709,10 @@
                 console.debug("Grilo duplicates:", duplicates);  // FIXME: remove when grilo is fixed
                 griloModel.loaded = true
                 tabs.ensurePopulated(tabs.selectedTab);
+
+                if (args.values.url) {
+                    uriHandler.process(args.values.url, true);
+                }
             }
         }
     }