From 3cfc282acca31d781781007db6c7650be8ec48b0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:11:36 +0200 Subject: [PATCH 1/6] Add regression test for GH-22 --- tests/frontends/mpd/regression_test.py | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/frontends/mpd/regression_test.py b/tests/frontends/mpd/regression_test.py index 29656ac4..3d0dca12 100644 --- a/tests/frontends/mpd/regression_test.py +++ b/tests/frontends/mpd/regression_test.py @@ -75,3 +75,35 @@ 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') From 8e74b946061c6087af5c909fccc0ecbe1d90a6b7 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:12:21 +0200 Subject: [PATCH 2/6] Fix for GH-22 playback.on_current_playlist_change() was not called for all changes to the current playlist. Thus, the playback controllers internal shuffled version of the current playlist (used for random mode), was not always updated when the current playlist was updated. --- mopidy/backends/base/current_playlist.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index c8c83a62..7802adc5 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/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 From 1d25a2ddea52d36c5dbd45ea29364ffcdfc7291d Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:25:52 +0200 Subject: [PATCH 3/6] Remove redundant calls to playback.on_current_playlist_change() --- mopidy/backends/base/current_playlist.py | 5 ----- tests/backends/base/current_playlist.py | 9 +++++++-- tests/backends/base/playback.py | 2 +- tests/frontends/mpd/current_playlist_test.py | 14 +++++++------- tests/frontends/mpd/playback_test.py | 8 ++++---- tests/frontends/mpd/status_test.py | 4 ++-- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 7802adc5..34a16369 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -81,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): """ @@ -156,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): """ @@ -201,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.""" diff --git a/tests/backends/base/current_playlist.py b/tests/backends/base/current_playlist.py index 59c7b39f..05f08e18 100644 --- a/tests/backends/base/current_playlist.py +++ b/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) diff --git a/tests/backends/base/playback.py b/tests/backends/base/playback.py index ca4d9941..4caaf44b 100644 --- a/tests/backends/base/playback.py +++ b/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) diff --git a/tests/frontends/mpd/current_playlist_test.py b/tests/frontends/mpd/current_playlist_test.py index 8e4b62f9..8a4b9ab5 100644 --- a/tests/frontends/mpd/current_playlist_test.py +++ b/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') diff --git a/tests/frontends/mpd/playback_test.py b/tests/frontends/mpd/playback_test.py index 3ba48a54..801be6d8 100644 --- a/tests/frontends/mpd/playback_test.py +++ b/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"') - self.assertEqual(self.b.playback.current_cpid, 2) + result = self.h.handle_request(u'seekid "1" "30"') + self.assertEqual(self.b.playback.current_cpid, 1) self.assertEqual(self.b.playback.current_track, seek_track) def test_stop(self): diff --git a/tests/frontends/mpd/status_test.py b/tests/frontends/mpd/status_test.py index fbd0ff9e..1afe6ccd 100644 --- a/tests/frontends/mpd/status_test.py +++ b/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)]) From 6e3f4a0fbbb62ab7ac020adff32fa373a13633af Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:29:26 +0200 Subject: [PATCH 4/6] Simplify logic in on_current_playlist_change --- mopidy/backends/base/playback.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 6d888d7d..dd350b20 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -331,13 +331,11 @@ class BasePlaybackController(object): self._first_shuffle = True self._shuffled = [] - if not self.backend.current_playlist.cp_tracks: - self.stop() - self.current_cp_track = None - elif (self.current_cp_track not in + if (not self.backend.current_playlist.cp_tracks or + self.current_cp_track not in self.backend.current_playlist.cp_tracks): - self.current_cp_track = None self.stop() + self.current_cp_track = None def next(self): """Play the next track.""" From 37426c6b54245a65e08cb8e8ec565635eee60e3d Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:32:36 +0200 Subject: [PATCH 5/6] Formatting --- tests/frontends/mpd/regression_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/frontends/mpd/regression_test.py b/tests/frontends/mpd/regression_test.py index 3d0dca12..3cfdb855 100644 --- a/tests/frontends/mpd/regression_test.py +++ b/tests/frontends/mpd/regression_test.py @@ -76,6 +76,7 @@ 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 From 24a1c61d49cb278decd3fa10b9804a497dd9d635 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:40:08 +0200 Subject: [PATCH 6/6] Move clearing of current track into stop() to ensure that it is done _after_ the stopped playing event is called --- mopidy/backends/base/playback.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index dd350b20..779903a7 100644 --- a/mopidy/backends/base/playback.py +++ b/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.current_cp_track = None + self.stop(clear_current_track=True) if self.consume: self.backend.current_playlist.remove(cpid=original_cp_track[0]) @@ -334,8 +333,7 @@ class BasePlaybackController(object): if (not self.backend.current_playlist.cp_tracks or self.current_cp_track not in self.backend.current_playlist.cp_tracks): - self.stop() - self.current_cp_track = None + self.stop(clear_current_track=True) def next(self): """Play the next track.""" @@ -346,8 +344,7 @@ class BasePlaybackController(object): self._trigger_stopped_playing_event() self.play(self.cp_track_at_next) else: - self.stop() - self.current_cp_track = None + self.stop(clear_current_track=True) def pause(self): """Pause playback.""" @@ -473,13 +470,21 @@ class BasePlaybackController(object): """ raise NotImplementedError - def stop(self): - """Stop playing.""" + def stop(self, clear_current_track=False): + """ + 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): """