Repository: mopidy/mopidy
Issue: Fix unplayable track behavior in consume mode

We need to reintroduce the tests and fixes done by @jcass77 in v1.1.2 for the combination of consume mode and unplayable tracks (tracks with a backend, but where the backend URI translation returns `None`). I dropped these fixes in the release-1.1 into develop merge (#1400). I have a branch I need to finish to reintroduce/redo the tests and fixes.


Changed files:
tests/core/test_playback.py, mopidy/core/tracklist.py, mopidy/core/playback.py

diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py
index 63259f7d..e0eab403 100644
--- a/mopidy/core/playback.py
+++ b/mopidy/core/playback.py
@@ -346,7 +346,6 @@ class PlaybackController(object):
         pending = tl_track or current or self.core.tracklist.next_track(None)
 
         while pending:
-            # TODO: should we consume unplayable tracks in this loop?
             if self._change(pending, PlaybackState.PLAYING):
                 break
             else:
diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py
index 2a4ec8b6..6d7ceeb7 100644
--- a/mopidy/core/tracklist.py
+++ b/mopidy/core/tracklist.py
@@ -621,6 +621,8 @@ class TracklistController(object):
     def _mark_unplayable(self, tl_track):
         """Internal method for :class:`mopidy.core.PlaybackController`."""
         logger.warning('Track is not playable: %s', tl_track.track.uri)
+        if self.get_consume() and tl_track is not None:
+            self.remove({'tlid': [tl_track.tlid]})
         if self.get_random() and tl_track in self._shuffled:
             self._shuffled.remove(tl_track)
 
diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py
index 4f20830e..a43724f0 100644
--- a/tests/core/test_playback.py
+++ b/tests/core/test_playback.py
@@ -185,6 +185,17 @@ class TestNextHandling(BaseTest):
 
         self.assertIn(tl_track, self.core.tracklist.tl_tracks)
 
+    def test_next_skips_over_unplayable_track(self):
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+        self.audio.trigger_fake_playback_failure(tl_tracks[1].track.uri)
+        self.core.playback.play(tl_tracks[0])
+        self.replay_events()
+
+        self.core.playback.next()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[2]
+
 
 class TestPreviousHandling(BaseTest):
     # TODO Test previous() more
@@ -230,6 +241,17 @@ class TestPreviousHandling(BaseTest):
 
         self.assertIn(tl_tracks[1], self.core.tracklist.tl_tracks)
 
+    def test_previous_skips_over_unplayable_track(self):
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+        self.audio.trigger_fake_playback_failure(tl_tracks[1].track.uri)
+        self.core.playback.play(tl_tracks[2])
+        self.replay_events()
+
+        self.core.playback.previous()
+        self.replay_events()
+
+        assert self.core.playback.get_current_tl_track() == tl_tracks[0]
+
 
 class OnAboutToFinishTest(BaseTest):
 
@@ -256,6 +278,20 @@ class TestConsumeHandling(BaseTest):
 
         self.assertNotIn(tl_track, self.core.tracklist.get_tl_tracks())
 
+    def test_next_in_consume_mode_removes_unplayable_track(self):
+        last_playable_tl_track = self.core.tracklist.get_tl_tracks()[-2]
+        unplayable_tl_track = self.core.tracklist.get_tl_tracks()[-1]
+        self.audio.trigger_fake_playback_failure(unplayable_tl_track.track.uri)
+
+        self.core.playback.play(last_playable_tl_track)
+        self.core.tracklist.set_consume(True)
+
+        self.core.playback.next()
+        self.replay_events()
+
+        self.assertNotIn(
+            unplayable_tl_track, self.core.tracklist.get_tl_tracks())
+
     def test_on_about_to_finish_in_consume_mode_removes_finished_track(self):
         tl_track = self.core.tracklist.get_tl_tracks()[0]
 
@@ -964,3 +1000,30 @@ class TestBug1177Regression(unittest.TestCase):
         c.playback.pause()
         c.playback.next()
         b.playback.change_track.assert_called_once_with(track2)
+
+
+class TestBug1352Regression(BaseTest):
+    tracks = [
+        Track(uri='dummy:a', length=40000),
+        Track(uri='dummy:b', length=40000),
+    ]
+
+    def test_next_when_paused_updates_history(self):
+        self.core.history._add_track = mock.Mock()
+        self.core.tracklist._mark_playing = mock.Mock()
+        tl_tracks = self.core.tracklist.get_tl_tracks()
+
+        self.playback.play()
+        self.replay_events()
+
+        self.core.history._add_track.assert_called_once_with(self.tracks[0])
+        self.core.tracklist._mark_playing.assert_called_once_with(tl_tracks[0])
+        self.core.history._add_track.reset_mock()
+        self.core.tracklist._mark_playing.reset_mock()
+
+        self.playback.pause()
+        self.playback.next()
+        self.replay_events()
+
+        self.core.history._add_track.assert_called_once_with(self.tracks[1])
+        self.core.tracklist._mark_playing.assert_called_once_with(tl_tracks[1])