Merge branch 'issue/GH-22' into develop

mopidy/backends/base/current_playlist.py

@@ -12,13 +12,10 @@ class BaseCurrentPlaylistController(object):
     :type backend: :class:`BaseBackend`
     """
 
-    #: The current playlist version. Integer which is increased every time the
-    #: current playlist is changed. Is not reset before Mopidy is restarted.
-    version = 0
-
     def __init__(self, backend):
         self.backend = backend
         self._cp_tracks = []
+        self._version = 0
 
     def destroy(self):
         """Cleanup after component."""
@@ -42,6 +39,19 @@ class BaseCurrentPlaylistController(object):
         """
         return [ct[1] for ct in self._cp_tracks]
 
+    @property
+    def version(self):
+        """
+        The current playlist version. Integer which is increased every time the
+        current playlist is changed. Is not reset before Mopidy is restarted.
+        """
+        return self._version
+
+    @version.setter
+    def version(self, version):
+        self._version = version
+        self.backend.playback.on_current_playlist_change()
+
     def add(self, track, at_position=None):
         """
         Add the track to the end of, or at the given position in the current
@@ -71,16 +81,13 @@ class BaseCurrentPlaylistController(object):
         :param tracks: tracks to append
         :type tracks: list of :class:`mopidy.models.Track`
         """
-        self.version += 1
         for track in tracks:
             self.add(track)
-        self.backend.playback.on_current_playlist_change()
 
     def clear(self):
         """Clear the current playlist."""
         self._cp_tracks = []
         self.version += 1
-        self.backend.playback.on_current_playlist_change()
 
     def get(self, **criteria):
         """
@@ -146,7 +153,6 @@ class BaseCurrentPlaylistController(object):
             to_position += 1
         self._cp_tracks = new_cp_tracks
         self.version += 1
-        self.backend.playback.on_current_playlist_change()
 
     def remove(self, **criteria):
         """
@@ -191,7 +197,6 @@ class BaseCurrentPlaylistController(object):
         random.shuffle(shuffled)
         self._cp_tracks = before + shuffled + after
         self.version += 1
-        self.backend.playback.on_current_playlist_change()
 
     def mpd_format(self, *args, **kwargs):
         """Not a part of the generic backend API."""

mopidy/backends/base/playback.py

@@ -316,8 +316,7 @@ class BasePlaybackController(object):
             self._trigger_stopped_playing_event()
             self.play(self.cp_track_at_eot)
         else:
-            self.stop()
+            self.stop(clear_current_track=True)
-            self.current_cp_track = None
 
         if self.consume:
             self.backend.current_playlist.remove(cpid=original_cp_track[0])
@@ -331,13 +330,10 @@ class BasePlaybackController(object):
         self._first_shuffle = True
         self._shuffled = []
 
-        if not self.backend.current_playlist.cp_tracks:
+        if (not self.backend.current_playlist.cp_tracks or
-            self.stop()
+                self.current_cp_track not in
-            self.current_cp_track = None
-        elif (self.current_cp_track not in
                 self.backend.current_playlist.cp_tracks):
-            self.current_cp_track = None
+            self.stop(clear_current_track=True)
-            self.stop()
 
     def next(self):
         """Play the next track."""
@@ -348,8 +344,7 @@ class BasePlaybackController(object):
             self._trigger_stopped_playing_event()
             self.play(self.cp_track_at_next)
         else:
-            self.stop()
+            self.stop(clear_current_track=True)
-            self.current_cp_track = None
 
     def pause(self):
         """Pause playback."""
@@ -475,13 +470,21 @@ class BasePlaybackController(object):
         """
         raise NotImplementedError
 
-    def stop(self):
+    def stop(self, clear_current_track=False):
-        """Stop playing."""
+        """
+        Stop playing.
+
+        :param clear_current_track: whether to clear the current track _after_
+            stopping
+        :type clear_current_track: boolean
+        """
         if self.state == self.STOPPED:
             return
         self._trigger_stopped_playing_event()
         if self._stop():
             self.state = self.STOPPED
+        if clear_current_track:
+            self.current_cp_track = None
 
     def _stop(self):
         """

tests/backends/base/current_playlist.py

@@ -128,7 +128,7 @@ class BaseCurrentPlaylistControllerTest(object):
     def test_append_does_not_reset_version(self):
         version = self.controller.version
         self.controller.append([])
-        self.assertEqual(self.controller.version, version + 1)
+        self.assertEqual(self.controller.version, version)
 
     @populate_playlist
     def test_append_preserves_playing_state(self):
@@ -249,7 +249,12 @@ class BaseCurrentPlaylistControllerTest(object):
         self.assertEqual(self.tracks[0], shuffled_tracks[0])
         self.assertEqual(set(self.tracks), set(shuffled_tracks))
 
-    def test_version(self):
+    def test_version_does_not_change_when_appending_nothing(self):
         version = self.controller.version
         self.controller.append([])
+        self.assertEquals(version, self.controller.version)
+
+    def test_version_increases_when_appending_something(self):
+        version = self.controller.version
+        self.controller.append([Track()])
         self.assert_(version < self.controller.version)

tests/backends/base/playback.py

@@ -524,7 +524,7 @@ class BasePlaybackControllerTest(object):
         wrapper.called = False
 
         self.playback.on_current_playlist_change = wrapper
-        self.backend.current_playlist.append([])
+        self.backend.current_playlist.append([Track()])
 
         self.assert_(wrapper.called)
 

tests/frontends/mpd/current_playlist_test.py

@@ -135,7 +135,7 @@ class CurrentPlaylistHandlerTest(unittest.TestCase):
     def test_deleteid(self):
         self.b.current_playlist.append([Track(), Track()])
         self.assertEqual(len(self.b.current_playlist.tracks), 2)
-        result = self.h.handle_request(u'deleteid "2"')
+        result = self.h.handle_request(u'deleteid "1"')
         self.assertEqual(len(self.b.current_playlist.tracks), 1)
         self.assert_(u'OK' in result)
 
@@ -193,7 +193,7 @@ class CurrentPlaylistHandlerTest(unittest.TestCase):
             Track(name='a'), Track(name='b'), Track(name='c'),
             Track(name='d'), Track(name='e'), Track(name='f'),
         ])
-        result = self.h.handle_request(u'moveid "5" "2"')
+        result = self.h.handle_request(u'moveid "4" "2"')
         self.assertEqual(self.b.current_playlist.tracks[0].name, 'a')
         self.assertEqual(self.b.current_playlist.tracks[1].name, 'b')
         self.assertEqual(self.b.current_playlist.tracks[2].name, 'e')
@@ -229,7 +229,7 @@ class CurrentPlaylistHandlerTest(unittest.TestCase):
         result = self.h.handle_request(
             u'playlistfind filename "file:///exists"')
         self.assert_(u'file: file:///exists' in result)
-        self.assert_(u'Id: 1' in result)
+        self.assert_(u'Id: 0' in result)
         self.assert_(u'Pos: 0' in result)
         self.assert_(u'OK' in result)
 
@@ -242,11 +242,11 @@ class CurrentPlaylistHandlerTest(unittest.TestCase):
 
     def test_playlistid_with_songid(self):
         self.b.current_playlist.append([Track(name='a'), Track(name='b')])
-        result = self.h.handle_request(u'playlistid "2"')
+        result = self.h.handle_request(u'playlistid "1"')
         self.assert_(u'Title: a' not in result)
-        self.assert_(u'Id: 1' not in result)
+        self.assert_(u'Id: 0' not in result)
         self.assert_(u'Title: b' in result)
-        self.assert_(u'Id: 2' in result)
+        self.assert_(u'Id: 1' in result)
         self.assert_(u'OK' in result)
 
     def test_playlistid_with_not_existing_songid_fails(self):
@@ -429,7 +429,7 @@ class CurrentPlaylistHandlerTest(unittest.TestCase):
             Track(name='a'), Track(name='b'), Track(name='c'),
             Track(name='d'), Track(name='e'), Track(name='f'),
         ])
-        result = self.h.handle_request(u'swapid "2" "5"')
+        result = self.h.handle_request(u'swapid "1" "4"')
         self.assertEqual(self.b.current_playlist.tracks[0].name, 'a')
         self.assertEqual(self.b.current_playlist.tracks[1].name, 'e')
         self.assertEqual(self.b.current_playlist.tracks[2].name, 'c')

tests/frontends/mpd/playback_test.py

@@ -254,7 +254,7 @@ class PlaybackControlHandlerTest(unittest.TestCase):
 
     def test_playid(self):
         self.b.current_playlist.append([Track()])
-        result = self.h.handle_request(u'playid "1"')
+        result = self.h.handle_request(u'playid "0"')
         self.assert_(u'OK' in result)
         self.assertEqual(self.b.playback.PLAYING, self.b.playback.state)
 
@@ -310,7 +310,7 @@ class PlaybackControlHandlerTest(unittest.TestCase):
 
     def test_seekid(self):
         self.b.current_playlist.append([Track(length=40000)])
-        result = self.h.handle_request(u'seekid "1" "30"')
+        result = self.h.handle_request(u'seekid "0" "30"')
         self.assert_(u'OK' in result)
         self.assert_(self.b.playback.time_position >= 30000)
 
@@ -318,8 +318,8 @@ class PlaybackControlHandlerTest(unittest.TestCase):
         seek_track = Track(uri='2', length=40000)
         self.b.current_playlist.append(
             [Track(length=40000), seek_track])
-        result = self.h.handle_request(u'seekid "2" "30"')
+        result = self.h.handle_request(u'seekid "1" "30"')
-        self.assertEqual(self.b.playback.current_cpid, 2)
+        self.assertEqual(self.b.playback.current_cpid, 1)
         self.assertEqual(self.b.playback.current_track, seek_track)
 
     def test_stop(self):

tests/frontends/mpd/regression_test.py

@@ -75,3 +75,36 @@ class IssueGH18RegressionTest(unittest.TestCase):
 
         self.assertNotEqual(cp_track_1, cp_track_2)
         self.assertNotEqual(cp_track_2, cp_track_3)
+
+
+class IssueGH22RegressionTest(unittest.TestCase):
+    """
+    The issue: http://github.com/jodal/mopidy/issues/#issue/22
+
+    How to reproduce:
+
+        Play, random on, remove all tracks from the current playlist (as in
+        "delete" each one, not "clear").
+
+        Alternatively: Play, random on, remove a random track from the current
+        playlist, press next until it crashes.
+    """
+
+    def setUp(self):
+        self.backend = DummyBackend(mixer_class=DummyMixer)
+        self.backend.current_playlist.append([
+            Track(uri='a'), Track(uri='b'), Track(uri='c'),
+            Track(uri='d'), Track(uri='e'), Track(uri='f')])
+        self.mpd = dispatcher.MpdDispatcher(backend=self.backend)
+
+    def test(self):
+        random.seed(1)
+        self.mpd.handle_request(u'play')
+        self.mpd.handle_request(u'random "1"')
+        self.mpd.handle_request(u'deleteid "1"')
+        self.mpd.handle_request(u'deleteid "2"')
+        self.mpd.handle_request(u'deleteid "3"')
+        self.mpd.handle_request(u'deleteid "4"')
+        self.mpd.handle_request(u'deleteid "5"')
+        self.mpd.handle_request(u'deleteid "6"')
+        self.mpd.handle_request(u'status')

tests/frontends/mpd/status_test.py

@@ -27,7 +27,7 @@ class StatusHandlerTest(unittest.TestCase):
         self.assert_(u'Track: 0' in result)
         self.assert_(u'Date: ' in result)
         self.assert_(u'Pos: 0' in result)
-        self.assert_(u'Id: 1' in result)
+        self.assert_(u'Id: 0' in result)
         self.assert_(u'OK' in result)
 
     def test_currentsong_without_song(self):
@@ -166,7 +166,7 @@ class StatusHandlerTest(unittest.TestCase):
         self.b.playback.play()
         result = dict(dispatcher.status.status(self.h))
         self.assert_('songid' in result)
-        self.assertEqual(int(result['songid']), 1)
+        self.assertEqual(int(result['songid']), 0)
 
     def test_status_method_when_playing_contains_time_with_no_length(self):
         self.b.current_playlist.append([Track(length=None)])