Merge pull request #919 from jodal/fix/902-consume-track-on-next

playback: Remove skipped track on next in consume mode

docs/changelog.rst

@@ -32,6 +32,10 @@ Bug fix release.
   :attr:`mopidy.models.Track.track_no`, and
   :attr:`mopidy.models.Track.last_modified` from ``0`` to :class:`None`.
 
+- Core: When skipping to the next track in consume mode, remove the skipped
+  track from the tracklist. This is consistent with the original MPD server's
+  behavior. (Fixes: :issue:`902`)
+
 - Local: Fix scanning of modified files. (PR: :issue:`904`)
 
 - MPD: Re-enable browsing of empty directories. (PR: :issue:`906`)

mopidy/core/playback.py

@@ -179,12 +179,16 @@ class PlaybackController(object):
         The current playback state will be kept. If it was playing, playing
         will continue. If it was paused, it will still be paused, etc.
         """
-        tl_track = self.core.tracklist.next_track(self.current_tl_track)
-        if tl_track:
-            self.change_track(tl_track)
+        original_tl_track = self.current_tl_track
+        next_tl_track = self.core.tracklist.next_track(original_tl_track)
+
+        if next_tl_track:
+            self.change_track(next_tl_track)
         else:
             self.stop(clear_current_track=True)
 
+        self.core.tracklist.mark_played(original_tl_track)
+
     def pause(self):
         """Pause playback."""
         backend = self._get_backend()

mopidy/core/tracklist.py

@@ -448,10 +448,10 @@ class TracklistController(object):
 
     def mark_played(self, tl_track):
         """Private method used by :class:`mopidy.core.PlaybackController`."""
-        if not self.consume:
-            return False
-        self.remove(tlid=[tl_track.tlid])
-        return True
+        if self.consume and tl_track is not None:
+            self.remove(tlid=[tl_track.tlid])
+            return True
+        return False
 
     def _trigger_tracklist_changed(self):
         if self.random:

tests/core/test_playback.py

@@ -239,6 +239,23 @@ class CorePlaybackTest(unittest.TestCase):
 
     # TODO Test next() more
 
+    def test_next_keeps_finished_track_in_tracklist(self):
+        tl_track = self.tl_tracks[0]
+        self.core.playback.play(tl_track)
+
+        self.core.playback.next()
+
+        self.assertIn(tl_track, self.core.tracklist.tl_tracks)
+
+    def test_next_in_consume_mode_removes_finished_track(self):
+        tl_track = self.tl_tracks[0]
+        self.core.playback.play(tl_track)
+        self.core.tracklist.consume = True
+
+        self.core.playback.next()
+
+        self.assertNotIn(tl_track, self.core.tracklist.tl_tracks)
+
     @mock.patch(
         'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener)
     def test_next_emits_events(self, listener_mock):
@@ -265,6 +282,23 @@ class CorePlaybackTest(unittest.TestCase):
 
     # TODO Test previous() more
 
+    def test_previous_keeps_finished_track_in_tracklist(self):
+        tl_track = self.tl_tracks[1]
+        self.core.playback.play(tl_track)
+
+        self.core.playback.previous()
+
+        self.assertIn(tl_track, self.core.tracklist.tl_tracks)
+
+    def test_previous_keeps_finished_track_even_in_consume_mode(self):
+        tl_track = self.tl_tracks[1]
+        self.core.playback.play(tl_track)
+        self.core.tracklist.consume = True
+
+        self.core.playback.previous()
+
+        self.assertIn(tl_track, self.core.tracklist.tl_tracks)
+
     @mock.patch(
         'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener)
     def test_previous_emits_events(self, listener_mock):
@@ -291,6 +325,23 @@ class CorePlaybackTest(unittest.TestCase):
 
     # TODO Test on_end_of_track() more
 
+    def test_on_end_of_track_keeps_finished_track_in_tracklist(self):
+        tl_track = self.tl_tracks[0]
+        self.core.playback.play(tl_track)
+
+        self.core.playback.on_end_of_track()
+
+        self.assertIn(tl_track, self.core.tracklist.tl_tracks)
+
+    def test_on_end_of_track_in_consume_mode_removes_finished_track(self):
+        tl_track = self.tl_tracks[0]
+        self.core.playback.play(tl_track)
+        self.core.tracklist.consume = True
+
+        self.core.playback.on_end_of_track()
+
+        self.assertNotIn(tl_track, self.core.tracklist.tl_tracks)
+
     @mock.patch(
         'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener)
     def test_on_end_of_track_emits_events(self, listener_mock):

tests/local/test_playback.py

@@ -347,7 +347,7 @@ class LocalPlaybackProviderTest(unittest.TestCase):
         self.tracklist.consume = True
         self.playback.play()
         self.playback.next()
-        self.assertIn(self.tracks[0], self.tracklist.tracks)
+        self.assertNotIn(self.tracks[0], self.tracklist.tracks)
 
     @populate_tracklist
     def test_next_with_single_and_repeat(self):