MPD: Fix 'play[id] -1' behaviour when current track is set

docs/changes.rst

@@ -40,7 +40,8 @@ greatly improved MPD client support.
   - Relocate from :mod:`mopidy.mpd` to :mod:`mopidy.frontends.mpd`.
   - Split gigantic protocol implementation into eleven modules.
   - Search improvements, including support for multi-word search.
-  - Fixed ``play "-1"`` and ``playid "-1"`` behaviour when playlist is empty.
+  - Fixed ``play "-1"`` and ``playid "-1"`` behaviour when playlist is empty
+    or when a current track is set.
   - Support ``plchanges "-1"`` to work better with MPDroid.
   - Support ``pause`` without arguments to work better with MPDroid.
   - Support ``plchanges``, ``play``, ``consume``, ``random``, ``repeat``, and

mopidy/frontends/mpd/protocol/playback.py

@@ -139,9 +139,7 @@ def playid(frontend, cpid):
     cpid = int(cpid)
     try:
         if cpid == -1:
-            if not frontend.backend.current_playlist.cp_tracks:
-                return # Fail silently
-            cp_track = frontend.backend.current_playlist.cp_tracks[0]
+            cp_track = _get_cp_track_for_play_minus_one(frontend)
         else:
             cp_track = frontend.backend.current_playlist.get(cpid=cpid)
         return frontend.backend.playback.play(cp_track)
@@ -158,10 +156,11 @@ def playpos(frontend, songpos):
 
         Begins playing the playlist at song number ``SONGPOS``.
 
-    *MPoD:*
+    *Many clients:*
 
-    - issues ``play "-1"`` after playlist replacement to start playback at
-      the first track.
+    - issue ``play "-1"`` after playlist replacement to start the current
+      track. If the current track is not set, start playback at the first
+      track.
 
     *BitMPC:*
 
@@ -170,15 +169,21 @@ def playpos(frontend, songpos):
     songpos = int(songpos)
     try:
         if songpos == -1:
-            if not frontend.backend.current_playlist.cp_tracks:
-                return # Fail silently
-            cp_track = frontend.backend.current_playlist.cp_tracks[0]
+            cp_track = _get_cp_track_for_play_minus_one(frontend)
         else:
             cp_track = frontend.backend.current_playlist.cp_tracks[songpos]
         return frontend.backend.playback.play(cp_track)
     except IndexError:
         raise MpdArgError(u'Bad song index', command=u'play')
 
+def _get_cp_track_for_play_minus_one(frontend):
+    if not frontend.backend.current_playlist.cp_tracks:
+        return # Fail silently
+    cp_track = frontend.backend.playback.current_cp_track
+    if cp_track is None:
+        cp_track = frontend.backend.current_playlist.cp_tracks[0]
+    return cp_track
+
 @handle_pattern(r'^previous$')
 def previous(frontend):
     """

tests/frontends/mpd/playback_test.py

@@ -225,13 +225,25 @@ class PlaybackControlHandlerTest(unittest.TestCase):
         self.assertEqual(result[0], u'ACK [2@0] {play} Bad song index')
         self.assertEqual(self.b.playback.STOPPED, self.b.playback.state)
 
-    def test_play_minus_one_plays_first_in_playlist(self):
-        track = Track()
-        self.b.current_playlist.load([track])
+    def test_play_minus_one_plays_first_in_playlist_if_no_current_track(self):
+        self.assertEqual(self.b.playback.current_track, None)
+        self.b.current_playlist.load([Track(uri='a'), Track(uri='b')])
         result = self.h.handle_request(u'play "-1"')
         self.assert_(u'OK' in result)
         self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
-        self.assertEqual(self.b.playback.current_track, track)
+        self.assertEqual(self.b.playback.current_track.uri, 'a')
+
+    def test_play_minus_one_plays_current_track_if_current_track_is_set(self):
+        self.b.current_playlist.load([Track(uri='a'), Track(uri='b')])
+        self.assertEqual(self.b.playback.current_track, None)
+        self.b.playback.play()
+        self.b.playback.next()
+        self.b.playback.stop()
+        self.assertNotEqual(self.b.playback.current_track, None)
+        result = self.h.handle_request(u'play "-1"')
+        self.assert_(u'OK' in result)
+        self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
+        self.assertEqual(self.b.playback.current_track.uri, 'b')
 
     def test_play_minus_one_on_empty_playlist_does_not_ack(self):
         self.b.current_playlist.clear()
@@ -246,13 +258,25 @@ class PlaybackControlHandlerTest(unittest.TestCase):
         self.assert_(u'OK' in result)
         self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
 
-    def test_playid_minus_one_plays_first_in_playlist(self):
-        track = Track()
-        self.b.current_playlist.load([track])
+    def test_playid_minus_one_plays_first_in_playlist_if_no_current_track(self):
+        self.assertEqual(self.b.playback.current_track, None)
+        self.b.current_playlist.load([Track(uri='a'), Track(uri='b')])
         result = self.h.handle_request(u'playid "-1"')
         self.assert_(u'OK' in result)
         self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
-        self.assertEqual(self.b.playback.current_track, track)
+        self.assertEqual(self.b.playback.current_track.uri, 'a')
+
+    def test_play_minus_one_plays_current_track_if_current_track_is_set(self):
+        self.b.current_playlist.load([Track(uri='a'), Track(uri='b')])
+        self.assertEqual(self.b.playback.current_track, None)
+        self.b.playback.play()
+        self.b.playback.next()
+        self.b.playback.stop()
+        self.assertNotEqual(self.b.playback.current_track, None)
+        result = self.h.handle_request(u'playid "-1"')
+        self.assert_(u'OK' in result)
+        self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
+        self.assertEqual(self.b.playback.current_track.uri, 'b')
 
     def test_playid_minus_one_on_empty_playlist_does_not_ack(self):
         self.b.current_playlist.clear()