Merge lp:~ahayzen/music-app/basic-repeat-implementation into lp:music-app/trusty

Proposed by Andrew Hayzen
Status: Merged
Approved by: Victor Thompson
Approved revision: 172
Merged at revision: 174
Proposed branch: lp:~ahayzen/music-app/basic-repeat-implementation
Merge into: lp:music-app/trusty
Diff against target: 59 lines (+12/-2)
3 files modified
MusicToolbar.qml (+3/-0)
manifest.json (+1/-0)
music-app.qml (+8/-2)
To merge this branch: bzr merge lp:~ahayzen/music-app/basic-repeat-implementation
Reviewer Review Type Date Requested Status
David Planella Approve
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Victor Thompson Approve
Review via email: mp+190519@code.launchpad.net

Commit message

* Implementation of a basic optional repeat system

Description of the change

* Implementation of a basic optional repeat system

To post a comment you must log in.
Revision history for this message
Victor Thompson (vthompson) wrote :

There's some odd stuff happening when I turn on or off repeat and/or shuffle. I'll look into it, but I apologize for allowing this to break. It needs to be fixed before we release.

Revision history for this message
Victor Thompson (vthompson) :
review: Needs Fixing
Revision history for this message
Andrew Hayzen (ahayzen) wrote :

Victor, could you expand on 'odd stuff happening' cause this was working under testing yesterday.

Revision history for this message
Andrew Hayzen (ahayzen) wrote :

Note that shuffle requires a restart to be taken into affect in this branch, however this had been fixed in lp:~music-app-dev/music-app/expandable-and-more

All this branch does is make getSong() actually take the repeat into account.

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Victor Thompson (vthompson) wrote :

Ok, this looks good. I misunderstood the intent. I thought repeat was the portion of the design for "repeat current item" not repeat the list. This is fine.

Revision history for this message
Victor Thompson (vthompson) :
review: Approve
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Victor Thompson (vthompson) wrote :

Andrew, could we also either a) comment out the "Shuf" and "Rep" UbuntuShapes from the MusicToolbar or b) find icons for them? It's still valuable to have the implementation for repeat ready... but if this is to land I think we also need to do a) or b) above.

Revision history for this message
Victor Thompson (vthompson) wrote :

I did the icon work in the other branch. I think both of these branches need to land prior to our 1.0 release. Once that happens we are in a good place.

We do have one bug that I believe is latent and needs to be fixed. Occasionally when the now playing view is launched, the tab header re-appears and covers the back button at the top of the view. This makes it impossible to go back to the other views. The user can only select songs within the queue until they restart the app.

Revision history for this message
David Planella (dpm) wrote :

Now that lp:~music-app-dev/music-app/expandable-and-more has landed, this one needs to be merged with trunk.

review: Needs Fixing
171. By Andrew Hayzen

* Merge of trunk

172. By Andrew Hayzen

* Fix for manifest.json

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Andrew Hayzen (ahayzen) wrote :

Ok think this is ready to go.

Victor, regarding that bug, Daniel spotted it the other day and added a Back option in the HUD as a fallback. But I agree it would be nice to fix.

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) :
review: Approve (continuous-integration)
Revision history for this message
David Planella (dpm) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'MusicToolbar.qml'
2--- MusicToolbar.qml 2013-10-12 01:07:17 +0000
3+++ MusicToolbar.qml 2013-10-12 15:29:50 +0000
4@@ -526,6 +526,8 @@
5 onClicked: {
6 // Invert shuffle settings
7 Settings.setSetting("shuffle", !(Settings.getSetting("shuffle") === "1"))
8+ console.debug("Shuffle:", Settings.getSetting("shuffle") === "1")
9+
10 mainView.random = Settings.getSetting("shuffle") === "1"
11 shuffleIcon.opacity = Settings.getSetting("shuffle") === "1" ? 1 : .25
12 }
13@@ -681,6 +683,7 @@
14 onClicked: {
15 // Invert repeat settings
16 Settings.setSetting("repeat", !(Settings.getSetting("repeat") === "1"))
17+ console.debug("Repeat:", Settings.getSetting("repeat") === "1")
18 repeatIcon.opacity = Settings.getSetting("repeat") === "1" ? 1 : .25
19 }
20 }
21
22=== modified file 'manifest.json'
23--- manifest.json 2013-10-12 01:39:53 +0000
24+++ manifest.json 2013-10-12 15:29:50 +0000
25@@ -14,3 +14,4 @@
26 "title": "music",
27 "version": "1.0"
28 }
29+
30
31=== modified file 'music-app.qml'
32--- music-app.qml 2013-10-12 01:50:13 +0000
33+++ music-app.qml 2013-10-12 15:29:50 +0000
34@@ -295,17 +295,23 @@
35 console.log("trackQueue.count: " + trackQueue.model.count)
36 currentIndex += direction
37 player.source = Qt.resolvedUrl(trackQueue.model.get(currentIndex).file)
38- } else if(direction === 1) {
39+ } else if(direction === 1 && Settings.getSetting("repeat") === "1") {
40 console.log("currentIndex: " + currentIndex)
41 console.log("trackQueue.count: " + trackQueue.model.count)
42 currentIndex = 0
43 player.source = Qt.resolvedUrl(trackQueue.model.get(currentIndex).file)
44- } else if(direction === -1) {
45+ } else if(direction === -1 && Settings.getSetting("repeat") === "1") {
46 console.log("currentIndex: " + currentIndex)
47 console.log("trackQueue.count: " + trackQueue.model.count)
48 currentIndex = trackQueue.model.count - 1
49 player.source = Qt.resolvedUrl(trackQueue.model.get(currentIndex).file)
50 }
51+ else
52+ {
53+ player.stop()
54+ return;
55+ }
56+
57 console.log("MediaPlayer statusChanged, currentIndex: " + currentIndex)
58 }
59 player.stop() // Add stop so that if same song is selected it restarts

Subscribers

People subscribed via source and target branches

to status/vote changes: