ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #06207
Re: [Merge] lp:~music-app-dev/music-app/media-hub-bg-playlists-rework into lp:music-app
Review: Needs Fixing
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.
Diff comments:
>
> === added file 'app/components/NewPlayer.qml'
> --- app/components/NewPlayer.qml 1970-01-01 00:00:00 +0000
> +++ app/components/NewPlayer.qml 2015-11-23 02:12:24 +0000
> @@ -0,0 +1,292 @@
> +/*
> + * Copyright (C) 2015
> + * Andrew Hayzen <ahayzen@xxxxxxxxx>
> + * Victor Thompson <victor.thompson@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +import QtMultimedia 5.4
> +import QtQuick 2.4
> +import Qt.labs.settings 1.0
> +
> +import QtQuick.LocalStorage 2.0
> +import "../logic/meta-database.js" as Library
> +
> +Item {
> + objectName: "player"
> +
> + // For autopilot
> + readonly property bool isPlaying: mediaPlayer.playbackState === MediaPlayer.PlayingState
> + readonly property alias count: mediaPlayerPlaylist.itemCount
> + readonly property alias currentIndex: mediaPlayerPlaylist.currentIndex
> +
> + property var currentMeta: ({})
> + property alias mediaPlayer: mediaPlayer
> + property alias repeat: settings.repeat
> + property alias shuffle: settings.shuffle
> +
> + function metaForSource(source) {
> + var blankMeta = {
> + album: "",
> + art: "",
> + author: "",
> + filename: "",
> + title: ""
> + };
> +
> + source = source.toString();
> +
> + if (source.indexOf("file://") === 0) {
> + source = source.substring(7);
> + }
> +
> + return musicStore.lookup(decodeFileURI(source)) || blankMeta;
> + }
> +
> + Settings {
> + id: settings
> + category: "PlayerSettings"
> +
> + property bool repeat: true
> + property bool shuffle: false
> + }
> +
> + // https://code.launchpad.net/~phablet-team/kubuntu-packaging/qtmultimedia-opensource-src-playlist-support/+merge/262229
> + MediaPlayer {
> + id: mediaPlayer
> + playlist: Playlist {
> + id: mediaPlayerPlaylist
> + playbackMode: { // FIXME: doesn't see to work
> + if (settings.shuffle) {
> + Playlist.Random
> + } else if (settings.repeat) {
> + Playlist.Loop
> + } else {
> + Playlist.Sequential
> + }
> + }
> +
> + // FIXME: Bind to settings.repeat/shuffle instead of playbackMode
> + // as that doesn't emit changes
> + readonly property bool canGoPrevious: { // FIXME: pad.lv/1517580 use previousIndex() > -1 after mh implements it
> + currentIndex !== 0 ||
> + settings.repeat ||
> + settings.shuffle || // FIXME: pad.lv/1517580 not way to know when we are at the end of a shuffle yet
> + mediaPlayer.position > 5000
> + }
> + readonly property bool canGoNext: { // FIXME: pad.lv/1517580 use nextIndex() > -1 after mh implements it
> + currentIndex !== (itemCount - 1) ||
> + settings.repeat ||
> + settings.shuffle // FIXME: pad.lv/1517580 not way to know when we are at the end of a shuffle yet
> + }
> + readonly property int count: itemCount // header actions etc depend on the model having 'count'
> + readonly property bool empty: itemCount === 0
> + property int pendingCurrentIndex: -1
> + property var pendingCurrentState: null
> + property int pendingShuffle: -1
> +
> + onCurrentItemSourceChanged: currentMeta = metaForSource(currentItemSource)
> + onItemChanged: {
> + console.debug("*** Saving play queue in onItemChanged");
> + saveQueue()
> +
> + // FIXME: shouldn't be needed? seems to be a bug where when appending currentItemChanged is not emitted
> + //if (start === currentIndex) {
> + // currentMeta = metaForSource(currentSource)
> + //}
> + }
> + onItemInserted: {
> + // When add to queue is done on an empty list currentIndex needs to be set
> + if (start === 0 && currentIndex === -1 && pendingCurrentIndex < 1 && pendingShuffle === -1) {
> + currentIndex = 0;
> +
> + pendingCurrentIndex = -1;
> + processPendingCurrentState();
> + }
> +
> + // Check if the pendingCurrentIndex is now valid
> + if (pendingCurrentIndex !== -1 && pendingCurrentIndex < itemCount) {
> + currentIndex = pendingCurrentIndex;
> +
> + pendingCurrentIndex = -1;
> + processPendingCurrentState();
> + }
> +
> + // Check if there is pending shuffle
> + // pendingShuffle holds the expected size of the model
> + if (pendingShuffle > -1 && pendingShuffle <= itemCount) {
> + pendingShuffle = -1;
> +
> + if (canGoNext) {
> + next(); // find a random track
> + }
> + mediaPlayer.play(); // next does not enforce play
> + }
> +
> + console.debug("*** Saving play queue in onItemInserted");
> + saveQueue()
> +
> + // FIXME: shouldn't be needed? seems to be a bug where when appending currentItemChanged is not emitted
> + if (start === currentIndex) {
> + currentMeta = metaForSource(currentItemSource)
> + }
> + }
> + onItemRemoved: {
> + console.debug("*** Saving play queue in onItemRemoved");
> + saveQueue()
> +
> + // FIXME: shouldn't be needed? seems to be a bug where when appending currentItemChanged is not emitted
> + if (start === currentIndex) {
> + currentMeta = metaForSource(currentItemSource)
> + }
> + }
> +
> + function addSourcesFromModel(model) {
> + var sources = []
> +
> + for (var i=0; i < model.rowCount; i++) {
> + sources.push(Qt.resolvedUrl(model.get(i, model.RoleModelData).filename));
> + }
> +
> + addItems(sources);
> + }
> +
> + // Wrap the clear() method because we need to call stop first
> + function clear_wrapper() {
> + // Stop the current playback (this ensures that play is run later)
> + if (mediaPlayer.playbackState === MediaPlayer.PlayingState) {
> + mediaPlayer.stop();
> + }
> +
> + clear();
> + }
> +
> + function processPendingCurrentState() {
> + // Process the pending current PlaybackState
> +
Errant newline?
> + if (pendingCurrentState === MediaPlayer.PlayingState) {
> + console.debug("Loading pending state play()");
> + mediaPlayer.play();
> + } else if (pendingCurrentState === MediaPlayer.PausedState) {
> + console.debug("Loading pending state pause()");
> + mediaPlayer.pause();
> + } else if (pendingCurrentState === MediaPlayer.StoppedState) {
> + console.debug("Loading pending state stop()");
> + mediaPlayer.stop();
> + }
> +
> + pendingCurrentState = null;
> + }
> +
> + function removeItems(items) {
> + items.sort();
> +
> + for (var i=0; i < items.length; i++) {
> + removeItem(items[i] - i);
> + }
> + }
> +
> + function saveQueue(start, end) {
> + // TODO: should not be hardcoded
> + // FIXME: doesn't work
> + // FIXME: disabled for now to not cause errors/slow down
> + // save("/home/phablet/.local/share/com.ubuntu.music/queue.m3u");
> +
> + // FIXME: using old queueList for now, move to load()/save() long term
> + if (mainView.loadedUI) {
> + Library.clearQueue();
> +
> + var sources = [];
> +
> + for (var i=0; i < mediaPlayerPlaylist.itemCount; i++) {
> + sources.push(mediaPlayerPlaylist.itemSource(i));
> + }
> +
> + if (sources.length > 0) {
> + Library.addQueueList(sources);
> + }
> + }
> + }
> +
> + function setCurrentIndex(index) {
> + // Set the currentIndex but if the itemCount is too low then wait
> + if (index < mediaPlayerPlaylist.itemCount) {
> + mediaPlayerPlaylist.currentIndex = index;
> + } else {
> + pendingCurrentIndex = index;
> + }
> + }
> +
> + function setPendingCurrentState(pendingState) {
> + // Set the PlaybackState to set once pendingCurrentIndex is set
> + pendingCurrentState = pendingState;
> +
> + if (pendingCurrentIndex === -1) {
> + processPendingCurrentState();
> + }
> + }
> +
> + function setPendingShuffle(modelSize) {
> + // Run next() and play() when the modelSize is reached
> + if (modelSize <= itemCount) {
> + if (canGoNext) {
> + mediaPlayerPlaylist.next(); // find a random track
> + }
> +
> + mediaPlayer.play(); // next does not enforce play
> + } else {
> + pendingShuffle = modelSize;
> + }
> + }
> + }
> +
> + property double progress: 0
> +
> + onDurationChanged: _calcProgress()
> + onPositionChanged: _calcProgress()
> +
> + onStatusChanged: {
> + if (status == MediaPlayer.EndOfMedia) {
> + console.debug("End of media, stopping.")
> + playlist.currentIndex = 0;
> + stop();
> +
> + _calcProgress(); // ensures progress bar has reset
> + }
> + }
> +
> + onStopped: { // hit when pressing next() on last track with repeat off
> + console.debug("onStopped.")
> + _calcProgress(); // ensures progress bar has reset
> + }
> +
> + function _calcProgress() {
> + if (duration > 0) {
> + progress = position / duration;
> + } else if (position >= duration) {
> + progress = 0;
> + } else {
> + progress = 0;
> + }
> + }
> +
> + function toggle() {
> + if (playbackState === MediaPlayer.PlayingState) {
> + pause();
> + } else {
> + play();
> + }
> + }
> + }
> +}
>
> === modified file 'app/logic/meta-database.js'
> --- app/logic/meta-database.js 2015-06-20 18:01:59 +0000
> +++ app/logic/meta-database.js 2015-11-23 02:12:24 +0000
> @@ -156,8 +156,15 @@
> db.transaction( function(tx) {
> var rs = tx.executeSql("SELECT * FROM queue ORDER BY ind ASC");
> for(var i = 0; i < rs.rows.length; i++) {
> - if (musicStore.lookup(decodeFileURI(rs.rows.item(i).filename)) != null) {
> - res.push(makeDict(musicStore.lookup(decodeFileURI(rs.rows.item(i).filename))));
> + var filename = rs.rows.item(i).filename;
> +
> + // Strip file:// from path as ms2 only accepts /home
Let's update this commentary to say "ms2 doesn't expect the URI scheme"
> + if (filename.indexOf("file://") == 0) {
> + filename = filename.substr(7);
> + }
> +
> + if (musicStore.lookup(decodeFileURI(filename)) != null) {
> + res.push(Qt.resolvedUrl(filename));
> }
> }
> });
>
> === modified file 'app/music-app.qml'
> --- app/music-app.qml 2015-11-17 01:51:14 +0000
> +++ app/music-app.qml 2015-11-23 02:12:24 +0000
> @@ -73,33 +78,41 @@
>
> switch (event.key) {
> case Qt.Key_Right: // Alt+Right Seek forward +10secs
> - position = player.position + 10000 < player.duration
> - ? player.position + 10000 : player.duration;
> - player.seek(position);
> + position = newPlayer.mediaPlayer.position + 10000 < newPlayer.mediaPlayer.duration
> + ? newPlayer.mediaPlayer.position + 10000 : newPlayer.mediaPlayer.duration;
> + newPlayer.mediaPlayer.seek(position);
> break;
> case Qt.Key_Left: // Alt+Left Seek backwards -10secs
> - position = player.position - 10000 > 0
> - ? player.position - 10000 : 0;
> - player.seek(position);
> + position = newPlayer.mediaPlayer.position - 10000 > 0
> + ? newPlayer.mediaPlayer.position - 10000 : 0;
> + newPlayer.mediaPlayer.seek(position);
> break;
> }
> }
> else if(event.modifiers === Qt.ControlModifier) {
> switch (event.key) {
> case Qt.Key_Left: // Ctrl+Left Previous Song
> - player.previousSong(true);
> + if (newPlayer.mediaPlayer.playlist.canGoPrevious) {
> + newPlayer.mediaPlayer.playlist.previous();
> + }
> +
> break;
> case Qt.Key_Right: // Ctrl+Right Next Song
> - player.nextSong(true, true);
> + if (newPlayer.mediaPlayer.playlist.canGoNext) {
We make these same checks many times. I'll put aside the fact that I think it is incorrect to do for for the time being, but if we ever decide to allow this to loop around, it'd be nice if this condition was in some utility function in NewPlayer. Perhaps newPlayer.next().
> + newPlayer.mediaPlayer.playlist.next();
> + }
> +
> break;
> + /*
Why are these commented out? Are they not still useful on the desktop?
> case Qt.Key_Up: // Ctrl+Up Volume up
> - player.volume = player.volume + .1 > 1 ? 1 : player.volume + .1
> + newPlayer.mediaPlayer.volume = newPlayer.mediaPlayer.volume + .1 > 1 ? 1 : newPlayer.mediaPlayer.volume + .1
> break;
> case Qt.Key_Down: // Ctrl+Down Volume down
> - player.volume = player.volume - .1 < 0 ? 0 : player.volume - .1
> + newPlayer.mediaPlayer.volume = newPlayer.mediaPlayer.volume - .1 < 0 ? 0 : newPlayer.mediaPlayer.volume - .1
> break;
> + */
> case Qt.Key_R: // Ctrl+R Repeat toggle
> - player.repeat = !player.repeat
> + newPlayer.mediaPlayer.repeat = !newPlayer.mediaPlayer.repeat
> break;
> case Qt.Key_F: // Ctrl+F Show Search popup
> if (mainPageStack.currentMusicPage.searchable && mainPageStack.currentMusicPage.state === "default") {
> @@ -175,16 +192,22 @@
> id: prevAction
> text: i18n.tr("Previous")
> keywords: i18n.tr("Previous Track")
> - onTriggered: player.previousSong()
> + onTriggered: {
> + if (newPlayer.mediaPlayer.playlist.canGoPrevious) {
> + newPlayer.mediaPlayer.playlist.previous()
> + }
> + }
> }
> + /*
> Action {
> id: stopAction
> text: i18n.tr("Stop")
> keywords: i18n.tr("Stop Playback")
> - onTriggered: player.stop()
> + onTriggered: newPlayer.mediaPlayer.stop()
> }
> + */
Is there a reason this is commented out? Do we still need all these actions?
>
> - actions: [nextAction, playsAction, prevAction, stopAction, backAction]
> + actions: [nextAction, playsAction, prevAction, backAction]
>
> UriHandlerHelper {
> id: uriHandler
> @@ -229,14 +235,31 @@
> customdebug("Version "+appVersion) // print the curren version
>
> Library.createRecent() // initialize recent
> + Library.createQueue() // create queue if it doesn't exist
>
> // initialize playlists
> Playlists.initializePlaylist()
>
> if (!args.values.url) {
> - // allow the queue loader to start
> - queueLoaderWorker.canLoad = !Library.isQueueEmpty()
> - queueLoaderWorker.list = Library.getQueue()
> + // load the previous queue as there are no args
> + // TODO: should not be hardcoded
> + // FIXME: doesn't work
> + //newPlayer.mediaPlayer.playlist.load("/home/phablet/.local/share/com.ubuntu.music/queue.m3u")
Will we having this working as part of the next round of media-hub updates before the next OTA?
> +
> + // use onloaded() and onLoadFailed() to confirm it is complete
> + // TODO: test if sync/async as future events may change the queue
> +
> + // FIXME: using old queueList for now, move to load()/save() long term
> + if (!Library.isQueueEmpty()) {
> + console.debug("*** Restoring library queue");
> + newPlayer.mediaPlayer.playlist.addItems(Library.getQueue());
> +
> + newPlayer.mediaPlayer.playlist.setCurrentIndex(queueIndex);
> + newPlayer.mediaPlayer.playlist.setPendingCurrentState(MediaPlayer.PausedState); // TODO: confirm this works if index changes after
> + }
> + else {
> + console.debug("Queue is empty, not loading any recent tracks");
> + }
> }
>
> // everything else
> @@ -548,8 +535,8 @@
> }
>
> // WHERE THE MAGIC HAPPENS
> - Player {
I assume it won't be possible to support both the old and new players? If it is possible, I assume it will be painful. Given that, why are we calling this NewPlayer rather than just updating the old one?
> - id: player
> + NewPlayer {
> + id: newPlayer
> }
>
> // TODO: Used by playlisttracks move to U1DB
--
https://code.launchpad.net/~music-app-dev/music-app/media-hub-bg-playlists-rework/+merge/275912
Your team Music App Developers is subscribed to branch lp:music-app.
References
