Ubuntu Music App

Merge lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework into lp://qastaging/music-app

media-hub-bg-playlists-rework
Merge into trunk

Proposed by Andrew Hayzen on 2015-10-27

Status:

Merged

Approved by:

Victor Thompson on 2016-01-26

Approved revision:

943

Merged at revision:

964

Proposed branch:

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework

Merge into:

lp://qastaging/music-app

Diff against target:

2709 lines (+461/-752)

30 files modified

app/components/BlurredBackground.qml (+2/-2)
app/components/Flickables/MultiSelectListView.qml (+9/-2)
app/components/HeadState/MultiSelectHeadState.qml (+4/-4)
app/components/Helpers/ContentHubHelper.qml (+11/-8)
app/components/Helpers/UriHandlerHelper.qml (+17/-32)
app/components/Helpers/UserMetricsHelper.qml (+10/-7)
app/components/ListItemActions/AddToPlaylist.qml (+6/-2)
app/components/ListItemActions/AddToQueue.qml (+2/-2)
app/components/MusicToolbar.qml (+37/-23)
app/components/NowPlayingFullView.qml (+57/-17)
app/components/NowPlayingToolbar.qml (+18/-30)
app/components/Player.qml (+0/-240)
app/components/Queue.qml (+18/-20)
app/components/ViewButton/PlayAllButton.qml (+6/-2)
app/components/ViewButton/QueueAllButton.qml (+6/-2)
app/components/ViewButton/ShuffleButton.qml (+6/-2)
app/logic/meta-database.js (+14/-4)
app/logic/playlists.js (+9/-2)
app/music-app.qml (+93/-220)
app/ui/AddToPlaylist.qml (+0/-1)
app/ui/ContentHubExport.qml (+8/-17)
app/ui/NowPlaying.qml (+29/-16)
app/ui/Playlists.qml (+0/-1)
app/ui/Recent.qml (+0/-1)
app/ui/Songs.qml (+2/-3)
debian/changelog (+4/-1)
debian/control (+1/-0)
manifest.json.in (+1/-1)
tests/autopilot/music_app/__init__.py (+30/-11)
tests/autopilot/music_app/tests/test_music.py (+61/-79)

To merge this branch:

bzr merge lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework

Related bugs:

Bug #1251624: back button should not be random when in shuffle mode	Medium	Fix Released
Bug #1480280: Songs repeated when listening in shuffle mode	Medium	Fix Released
Bug #1500457: Pressing previous button sets duration to 0	Low	Fix Released
Bug #1534172: With bgplaylists if an alert sounds, the playback stops and the queue is lost	High	Fix Released
Bug #1534840: Second import from webbrowser/download-manager doesn't auto-start although kicked out of play mode	High	Fix Released
Bug #1535530: Music app: Next button doesn't become disabled when coming to the very last track of the album	Low	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jenkins Bot	continuous-integration		Approve on 2016-01-26
Victor Thompson		2015-10-27	Approve on 2016-01-26
Review via email: mp+275912@code.qastaging.launchpad.net

Commit message

* Add support for media-hub background playlists
* Bump framework to 15.04.3-qml
* Bump QtMultimedia import to 5.6

Description of the change

* Add support for media-hub background playlists
* Bump framework to 15.04.3-qml
* Bump QtMultimedia import to 5.6

Revision history for this message

Victor Thompson (vthompson) wrote on 2015-11-25:

I added a few inline comments. There are many TODOs and FIXMEs that should still be investigated/fixed. If we end up leaving any in the merge into trunk, we should maybe clearly tag them with something like "bgplaylists" so we can easily find them in the near future.

review: Needs Fixing

Revision history for this message

Victor Thompson (vthompson) wrote on 2015-11-26:

Another small notable thing to mention is that the applicable debian/control packages should be updated such that the prereq media-hub packages are installed.

Revision history for this message

Andrew Hayzen (ahayzen) wrote on 2016-01-02:

I got autopilot passing \o/

autopilot PASS

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-12:

This is looking pretty good. I have some inline comments. The only other minor thing is to update the copyright header where appropriate with 2016.

Also, I mentioned this before: I think we need to put something in the control file to require the new media-hub and other packages be installed.

review: Needs Fixing

Revision history for this message

Andrew Hayzen (ahayzen) wrote on 2016-01-12:

1) Is there a reason this length of time was chosen?

48ms is the maximum possible time we can wait without being noticeable to the user (less than 50ms) that is still divisible by 16ms. (note that this worked when set to 16ms on mako to me, I just wanted to give extra time to slower devices)

2) Hasn't this bug been fixed and released?

Nope, using print_tree I could not see the MediaPlayer or the property var :-/ (Would be nice if it was though :-) )

3) Consider changing to a switch statement. Since there are only 3 conditions it isn't too bad though.

I'm not sure that can be a switch statement? What would be the control ? if you did switch(settings.shuffle) you couldn't have a case for repeat? I think it reads fine as an if, else if, else

4) Could you change "bgplaylists" to something more specific? MediaPlayer's playlist/queue or something?

Fixed.

I also noticed that the positionAt currentIndex was broken when switching to the queue view, currently investigating could be due to the Layouts change as then the height isn't 'fixed'.

Revision history for this message

Andrew Hayzen (ahayzen) wrote on 2016-01-12:

OK, fixed the positionAt issue, if we land trunk into the store before bgplaylists we should include this patch [0]

0 - http://bazaar.launchpad.net/~music-app-dev/music-app/media-hub-bg-playlists-rework/revision/931

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-15:

FAILED: Continuous integration, rev:932
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/121/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/396/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/121/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-15

933. By Andrew Hayzen on 2016-01-15: * Fix for ContentHubExport.qml missing migration to listitem layout
* Fix for ContentHubHelper.qml referencing queue worker and not pushing now playing page

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-15:

FAILED: Continuous integration, rev:933
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/122/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/397/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/122/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-15:

Recent changes look good and fix the Export song issue.

Do we need to add anything to the debian/control file for the in interface? Also, please update the copyright years where applicable.

review: Needs Fixing

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-16

934. By Andrew Hayzen on 2016-01-16: * Add qml-module-qtmultimedia (>=5.5.1-1ubuntu2) to control

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-16:

PASSED: Continuous integration, rev:934
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/123/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/398/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/123/rebuild

review: Approve (continuous-integration)

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-16

935. By Andrew Hayzen on 2016-01-16: * Update copyright dates

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-16:

Clicking "shuffle" or "play all" on an empty playlist brings the user to the Now Playing page. It shouldn't do so and didn't do so before.

review: Needs Fixing

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-16

936. By Andrew Hayzen on 2016-01-16: * Fix Play All/Shuffle All/Queue All so that they only perform their action when the model.count > 0

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-16:

FAILED: Continuous integration, rev:935
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/124/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/399/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/124/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-16:

Both the "Shuffle" and "Queue all" buttons now do not work for a playlist.

review: Needs Fixing

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-16

937. By Andrew Hayzen on 2016-01-16: * Fix for Shuffle All and Queue All not working with playlists due to them using LibraryListModel

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-16:

FAILED: Continuous integration, rev:936
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/125/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/400/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/125/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-16:

URL handler actions worked as they did previously.

One thing I noticed is that we do not go to the Now Playing page when just a track is played, whereas when an album is played we do go to the Now Playing page. The current app does this too--did we consciously do this?

review: Needs Information

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-16:

PASSED: Continuous integration, rev:937
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/126/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/401/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/126/rebuild

review: Approve (continuous-integration)

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-16

938. By Andrew Hayzen on 2016-01-16: * Fix for toolbar progress hint breaking
939. By Andrew Hayzen on 2016-01-16: * Ensure that now playing page is shown when a single track is played via uri-handler

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-16:

FAILED: Continuous integration, rev:939
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/131/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/406/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/131/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-17:

This looks good now. We should figure out the Jenkins failure before landing, however.

review: Needs Fixing

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-18

940. By Andrew Hayzen on 2016-01-18: * Wait for dialog to be visible in autopilot

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-18:

FAILED: Continuous integration, rev:940
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/139/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/416/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/139/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andrew Hayzen (ahayzen) wrote on 2016-01-20:

I have reported bug 1536361 for the keyboard issue.

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-21

941. By Andrew Hayzen on 2016-01-21: * Fix for usermetrics

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-21:

FAILED: Continuous integration, rev:941
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/158/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/437/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/158/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-22:

Did something change in rc-proposed for this to break? Currently, loading the app shows a blank header and no data. I see the following in the log:

file:///opt/click.ubuntu.com/com.ubuntu.music/2.3.939/app/logic/meta-database.js:162:
TypeError: Cannot call method 'indexOf' of null^M
file:///opt/click.ubuntu.com/com.ubuntu.music/2.3.939/app/music-app.qml:579: TypeErro
r: Cannot read property 'showToolbar' of null^M

Screenshot: http://i.imgur.com/VyHbi1E.png

review: Needs Information

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-25:

I resolved my issue by clearing some of the stored data for the app. I'm not sure if this was due to other MP's being tested or not. I did some testing to make sure the issue isn't something caused by the upgrade path. Before we merge this, I suggest we do some additional upgrade testing.

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-25

942. By Andrew Hayzen on 2016-01-25: * Protect against null filenames when retrieving the queue

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-25:

FAILED: Continuous integration, rev:942
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/179/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/463/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/179/rebuild

review: Needs Fixing (continuous-integration)

lp://qastaging/~music-app-dev/music-app/media-hub-bg-playlists-rework updated on 2016-01-26

943. By Andrew Hayzen on 2016-01-26: * Remove play + pause in test_next_previous that are now redundant due to how bgplaylists works

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote on 2016-01-26:

FAILED: Continuous integration, rev:943
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/185/
Executed test runs:
None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/469/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/185/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Victor Thompson (vthompson) wrote on 2016-01-26:

Thanks for your patience with this MP. The code looks good, hands-on testing looks good, and with the only remaining issue being the keyboard issues with AP, I say let's land this. Thanks!

review: Approve

Revision history for this message

Jenkins Bot (ubuntu-core-apps-jenkins-bot) on 2016-01-26:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Music App Developers