From c6ff9eee86404aae2dc8afd28e7b9481eaf98228 Mon Sep 17 00:00:00 2001
From: Stein Magnus Jodal <stein.magnus@jodal.no>
Date: Tue, 23 Dec 2014 23:58:20 +0100
Subject: [PATCH] playback: Remove skipped track on next in consume mode

Also adds core level tests of consume behavior on next/prev/eot.

Fixes #902
---
 docs/changelog.rst           |  4 +++
 mopidy/core/playback.py      | 10 ++++---
 mopidy/core/tracklist.py     |  8 +++---
 tests/core/test_playback.py  | 51 ++++++++++++++++++++++++++++++++++++
 tests/local/test_playback.py |  2 +-
 5 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/docs/changelog.rst b/docs/changelog.rst
index 19257e46..4e3662f1 100644
--- a/docs/changelog.rst
+++ b/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`)
diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py
index 097a9401..d6307e2f 100644
--- a/mopidy/core/playback.py
+++ b/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()
diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py
index 4c567086..80ed5aea 100644
--- a/mopidy/core/tracklist.py
+++ b/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:
diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py
index ce6c8571..909ca4fa 100644
--- a/tests/core/test_playback.py
+++ b/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):
diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py
index 7b64e495..89015739 100644
--- a/tests/local/test_playback.py
+++ b/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):