Merge lp:~vthompson/music-app/lookup-file into lp:music-app/trusty

Proposed by Victor Thompson on 2014-08-05
Status: Merged
Approved by: Victor Thompson on 2014-08-07
Approved revision: 551
Merged at revision: 557
Proposed branch: lp:~vthompson/music-app/lookup-file
Merge into: lp:music-app/trusty
Diff against target: 73 lines (+18/-17)
1 file modified
music-app.qml (+18/-17)
To merge this branch: bzr merge lp:~vthompson/music-app/lookup-file
Reviewer Review Type Date Requested Status
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2014-08-07
Andrew Hayzen 2014-08-05 Approve on 2014-08-07
Review via email: mp+229612@code.launchpad.net

Commit message

* Use mediascanner SongsModel.lookup() function to play song from Music Scope
* Catch the filled signal for the SongsModel used for playing an album from the Music Scope

Description of the change

* Use mediascanner SongsModel.lookup() function to play song from Music Scope
* Catch the filled signal for the SongsModel used for playing an album from the Music Scope

To post a comment you must log in.
Andrew Hayzen (ahayzen) wrote :

Looks good so far, see inline diff comments. Also I wonder if selectedAlbum should be set back to false at the end of onFilled?

review: Needs Fixing
Victor Thompson (vthompson) wrote :

I'll reset selectedAlbum back to false... I don't think we'd catch the filled signal again. BUT if it's called when a new song is added by ms2 we wouldn't want the album to reload.

I think we should still have the track var be set to false by default. The code that handles lookup [1] throws an exception when found which should exit the function... leaving the track variable false as we are assuming.

[1] http://bazaar.launchpad.net/~mediascanner-team/mediascanner2/trunk/view/head:/src/mediascanner/MediaStore.cc#L394

lp:~vthompson/music-app/lookup-file updated on 2014-08-06
550. By Victor Thompson on 2014-08-06

Reset selectedAlbum... just in case.

Andrew Hayzen (ahayzen) wrote :

What would happen if album A was selected in the scope, the user changed the queue, and then album A was selected again. I think nothing would happen as the model would not Fill again?

Suggest blanking the filter at the end of onFilled and then inside onFilled wrap all the code in a check to ensure the filter is not blank.

review: Needs Fixing
lp:~vthompson/music-app/lookup-file updated on 2014-08-07
551. By Victor Thompson on 2014-08-07

Clear filter

Andrew Hayzen (ahayzen) wrote :

Looks good :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'music-app.qml'
2--- music-app.qml 2014-08-06 23:41:30 +0000
3+++ music-app.qml 2014-08-07 00:46:20 +0000
4@@ -205,6 +205,7 @@
5 target: UriHandler
6
7 function processAlbum(uri) {
8+ selectedAlbum = true;
9 var split = uri.split("/");
10
11 if (split.length < 2) {
12@@ -215,16 +216,6 @@
13 // Filter by artist and album
14 songsAlbumArtistModel.artist = decodeURIComponent(split[0]);
15 songsAlbumArtistModel.album = decodeURIComponent(split[1]);
16-
17- // Play album it tracks exist
18- if (songsAlbumArtistModel.rowCount > 0) {
19- // trackClicked(model, index, play, clear=true) will clear the model
20- trackClicked(songsAlbumArtistModel, 0, true, true);
21- }
22- else {
23- console.debug("Unknown artist-album " + uri + ", skipping")
24- return;
25- }
26 }
27
28 function processFile(uri, play) {
29@@ -232,13 +223,8 @@
30
31 var track = false;
32
33- // Search for track in songs model
34- for (var i=0; i < allSongsModel.rowCount; i++) {
35- if (decodeURIComponent(allSongsModel.get(i, allSongsModel.RoleModelData).filename) === uri) {
36- track = allSongsModel.get(i, allSongsModel.RoleModelData);
37- break;
38- }
39- }
40+ // Lookup track in songs model
41+ track = musicStore.lookup(decodeURIComponent(uri));
42
43 if (!track) {
44 console.debug("Unknown file " + uri + ", skipping")
45@@ -383,6 +369,7 @@
46 property string timestamp // used to scrobble
47 property var chosenElement: null
48 property bool toolbarShown: musicToolbar.shown
49+ property bool selectedAlbum: false
50 signal collapseExpand();
51 signal collapseSwipeDelete(int index);
52 signal onToolbarShownChanged(bool shown, var currentPage, var currentTab)
53@@ -518,6 +505,20 @@
54 SongsModel {
55 id: songsAlbumArtistModel
56 store: musicStore
57+ onFilled: {
58+ // Play album it tracks exist
59+ if (rowCount > 0 && selectedAlbum) {
60+ trackClicked(songsAlbumArtistModel, 0, true, true);
61+ } else if (selectedAlbum) {
62+ console.debug("Unknown artist-album " + artist + "/" + album + ", skipping")
63+ }
64+
65+ selectedAlbum = false;
66+
67+ // Clear filter for artist and album
68+ songsAlbumArtistModel.artist = ""
69+ songsAlbumArtistModel.album = ""
70+ }
71 }
72
73 // WHERE THE MAGIC HAPPENS

Subscribers

People subscribed via source and target branches

to status/vote changes: