From ca02dbb676989023dddafc2383b2a8bf92234b85 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 00:11:59 +0100 Subject: [PATCH 1/6] core: Make change_track internal as it going away in 1.x --- mopidy/core/playback.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 453a07d7..96c5e7da 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -183,7 +183,7 @@ class PlaybackController(object): # Methods # TODO: remove this. - def change_track(self, tl_track, on_error_step=1): + def _change_track(self, tl_track, on_error_step=1): """ Change to the given track, keeping the current playback state. @@ -215,7 +215,7 @@ class PlaybackController(object): next_tl_track = self.core.tracklist.eot_track(original_tl_track) if next_tl_track: - self.change_track(next_tl_track) + self._change_track(next_tl_track) else: self.stop() self.set_current_tl_track(None) @@ -250,7 +250,7 @@ class PlaybackController(object): # TODO: switch to: # backend.play(track) # wait for state change? - self.change_track(next_tl_track) + self._change_track(next_tl_track) else: self.stop() self.set_current_tl_track(None) @@ -344,7 +344,7 @@ class PlaybackController(object): # TODO: switch to: # self.play(....) # wait for state change? - self.change_track( + self._change_track( self.core.tracklist.previous_track(tl_track), on_error_step=-1) def resume(self): From fd04cd918fd5cd6c17ae1cde9d0f77e672117c9d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 00:15:56 +0100 Subject: [PATCH 2/6] core: Remove on_error_step from play arguments --- mopidy/core/playback.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 96c5e7da..aeb5edbf 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -197,7 +197,7 @@ class PlaybackController(object): self.stop() self.set_current_tl_track(tl_track) if old_state == PlaybackState.PLAYING: - self.play(on_error_step=on_error_step) + self._play(on_error_step=on_error_step) elif old_state == PlaybackState.PAUSED: self.pause() @@ -267,20 +267,17 @@ class PlaybackController(object): self.set_state(PlaybackState.PAUSED) self._trigger_track_playback_paused() - def play(self, tl_track=None, on_error_step=1): + def play(self, tl_track=None): """ Play the given track, or if the given track is :class:`None`, play the currently active track. :param tl_track: track to play :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` - :param on_error_step: direction to step at play error, 1 for next - track (default), -1 for previous track. **INTERNAL** - :type on_error_step: int, -1 or 1 """ + self._play(tl_track, 1) - assert on_error_step in (-1, 1) - + def _play(self, tl_track=None, on_error_step=1): if tl_track is None: if self.get_state() == PlaybackState.PAUSED: return self.resume() From 07f0453c6ee1d8865b366f500f91143ae6b86c08 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 00:37:50 +0100 Subject: [PATCH 3/6] core: Make event triggers internal --- mopidy/core/actor.py | 4 +-- mopidy/core/playback.py | 6 ++-- mopidy/core/tracklist.py | 2 +- tests/core/test_playback.py | 9 ++++-- tests/local/test_playback.py | 53 +++++++++++++++++++----------------- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index 32070684..b21e9e20 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -84,10 +84,10 @@ class Core( """ def reached_end_of_stream(self): - self.playback.on_end_of_track() + self.playback._on_end_of_track() def stream_changed(self, uri): - self.playback.on_stream_changed(uri) + self.playback._on_stream_changed(uri) def state_changed(self, old_state, new_state, target_state): # XXX: This is a temporary fix for issue #232 while we wait for a more diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index aeb5edbf..e97d5c5e 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -202,7 +202,7 @@ class PlaybackController(object): self.pause() # TODO: this is not really end of track, this is on_need_next_track - def on_end_of_track(self): + def _on_end_of_track(self): """ Tell the playback controller that end of track is reached. @@ -222,7 +222,7 @@ class PlaybackController(object): self.core.tracklist._mark_played(original_tl_track) - def on_tracklist_change(self): + def _on_tracklist_change(self): """ Tell the playback controller that the current playlist has changed. @@ -233,7 +233,7 @@ class PlaybackController(object): self.stop() self.set_current_tl_track(None) - def on_stream_changed(self, uri): + def _on_stream_changed(self, uri): self._stream_title = None def next(self): diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 963dcadf..9186ae42 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -67,7 +67,7 @@ class TracklistController(object): def _increase_version(self): self._version += 1 - self.core.playback.on_tracklist_change() + self.core.playback._on_tracklist_change() self._trigger_tracklist_changed() version = deprecated_property(get_version) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 6f3c3274..972b6dea 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -50,6 +50,9 @@ class CorePlaybackTest(unittest.TestCase): self.unplayable_tl_track = self.tl_tracks[2] self.duration_less_tl_track = self.tl_tracks[4] + def trigger_end_of_track(self): + self.core.playback._on_end_of_track() + def test_get_current_tl_track_none(self): self.core.playback.set_current_tl_track(None) @@ -419,7 +422,7 @@ class CorePlaybackTest(unittest.TestCase): tl_track = self.tl_tracks[0] self.core.playback.play(tl_track) - self.core.playback.on_end_of_track() + self.trigger_end_of_track() self.assertIn(tl_track, self.core.tracklist.tl_tracks) @@ -428,7 +431,7 @@ class CorePlaybackTest(unittest.TestCase): self.core.playback.play(tl_track) self.core.tracklist.consume = True - self.core.playback.on_end_of_track() + self.trigger_end_of_track() self.assertNotIn(tl_track, self.core.tracklist.tl_tracks) @@ -438,7 +441,7 @@ class CorePlaybackTest(unittest.TestCase): self.core.playback.play(self.tl_tracks[0]) listener_mock.reset_mock() - self.core.playback.on_end_of_track() + self.trigger_end_of_track() self.assertListEqual( listener_mock.send.mock_calls, diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index 4c4ded24..6ea82f2d 100644 --- a/tests/local/test_playback.py +++ b/tests/local/test_playback.py @@ -39,6 +39,9 @@ class LocalPlaybackProviderTest(unittest.TestCase): track = Track(uri=uri, length=4464) self.tracklist.add([track]) + def trigger_end_of_track(self): + self.playback._on_end_of_track() + def setUp(self): # noqa: N802 self.audio = dummy_audio.create_proxy() self.backend = actor.LocalBackend.start( @@ -163,7 +166,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_current_track_after_completed_playlist(self): self.playback.play(self.tracklist.tl_tracks[-1]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) self.assertEqual(self.playback.current_track, None) @@ -406,7 +409,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): old_position = self.tracklist.index(tl_track) old_uri = tl_track.track.uri - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertEqual( @@ -416,11 +419,11 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_end_of_track_return_value(self): self.playback.play() - self.assertEqual(self.playback.on_end_of_track(), None) + self.assertEqual(self.trigger_end_of_track(), None) @populate_tracklist def test_end_of_track_does_not_trigger_playback(self): - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) @populate_tracklist @@ -433,7 +436,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): tl_track = self.playback.current_tl_track self.assertEqual(self.tracklist.index(tl_track), i) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) @@ -442,7 +445,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.play() for _ in self.tracks: - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, None) self.assertEqual(self.playback.state, PlaybackState.STOPPED) @@ -452,7 +455,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.assertEqual(self.playback.current_track, self.tracks[0]) def test_end_of_track_for_empty_playlist(self): - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) @populate_tracklist @@ -462,7 +465,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.backend.playback.play = lambda: return_values.pop() self.playback.play() self.assertEqual(self.playback.current_track, self.tracks[0]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertNotEqual(self.playback.current_track, self.tracks[1]) self.assertEqual(self.playback.current_track, self.tracks[2]) @@ -482,7 +485,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_end_of_track_track_after_previous(self): self.playback.play() - self.playback.on_end_of_track() + self.trigger_end_of_track() self.playback.previous() tl_track = self.playback.current_tl_track self.assertEqual( @@ -496,7 +499,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): def test_end_of_track_track_at_end_of_playlist(self): self.playback.play() for _ in self.tracklist.tl_tracks[1:]: - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertEqual(self.tracklist.next_track(tl_track), None) @@ -505,7 +508,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.repeat = True self.playback.play() for _ in self.tracks[1:]: - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertEqual( self.tracklist.next_track(tl_track), self.tl_tracks[0]) @@ -524,7 +527,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): def test_end_of_track_with_consume(self): self.tracklist.consume = True self.playback.play() - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertNotIn(self.tracks[0], self.tracklist.tracks) @populate_tracklist @@ -535,7 +538,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.random = True self.playback.play() self.assertEqual(self.playback.current_track, self.tracks[-1]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, self.tracks[-2]) @populate_tracklist @@ -654,19 +657,19 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_tracklist_position_at_end_of_playlist(self): self.playback.play(self.tracklist.tl_tracks[-1]) - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertEqual(self.tracklist.index(tl_track), None) def test_on_tracklist_change_gets_called(self): - callback = self.playback.on_tracklist_change + callback = self.playback._on_tracklist_change def wrapper(): wrapper.called = True return callback() wrapper.called = False - self.playback.on_tracklist_change = wrapper + self.playback._on_tracklist_change = wrapper self.tracklist.add([Track()]) self.assert_(wrapper.called) @@ -917,7 +920,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.consume = True self.playback.play() for _ in range(len(self.tracklist.tracks)): - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(len(self.tracklist.tracks), 0) @populate_tracklist @@ -944,7 +947,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_end_of_song_starts_next_track(self): self.playback.play() - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, self.tracks[1]) @populate_tracklist @@ -953,7 +956,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.repeat = True self.playback.play() self.assertEqual(self.playback.current_track, self.tracks[0]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, self.tracks[0]) @populate_tracklist @@ -963,7 +966,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.random = True self.playback.play() current_track = self.playback.current_track - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, current_track) @populate_tracklist @@ -971,7 +974,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.single = True self.playback.play() self.assertEqual(self.playback.current_track, self.tracks[0]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, None) self.assertEqual(self.playback.state, PlaybackState.STOPPED) @@ -980,14 +983,14 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.single = True self.tracklist.random = True self.playback.play() - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.current_track, None) self.assertEqual(self.playback.state, PlaybackState.STOPPED) @populate_tracklist def test_end_of_playlist_stops(self): self.playback.play(self.tracklist.tl_tracks[-1]) - self.playback.on_end_of_track() + self.trigger_end_of_track() self.assertEqual(self.playback.state, PlaybackState.STOPPED) def test_repeat_off_by_default(self): @@ -1013,7 +1016,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.random = True self.playback.play() for _ in self.tracks[1:]: - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertEqual(self.tracklist.eot_track(tl_track), None) @@ -1034,7 +1037,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.random = True self.playback.play() for _ in self.tracks: - self.playback.on_end_of_track() + self.trigger_end_of_track() tl_track = self.playback.current_tl_track self.assertNotEqual(self.tracklist.eot_track(tl_track), None) self.assertEqual(self.playback.state, PlaybackState.STOPPED) From 6d22c4fd5970430f96e937432862b0cf23d18004 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 00:48:48 +0100 Subject: [PATCH 4/6] core: Remove set_current_tl_track --- mopidy/core/playback.py | 15 +++++++-------- tests/core/test_playback.py | 17 ++++++++++------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index e97d5c5e..8d9b7777 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -41,15 +41,14 @@ class PlaybackController(object): """ return self._current_tl_track - def set_current_tl_track(self, value): + def _set_current_tl_track(self, value): """Set the currently playing or selected track. *Internal:* This is only for use by Mopidy's test suite. """ self._current_tl_track = value - current_tl_track = deprecated_property( - get_current_tl_track, set_current_tl_track) + current_tl_track = deprecated_property(get_current_tl_track) """ .. deprecated:: 1.0 Use :meth:`get_current_tl_track` instead. @@ -195,7 +194,7 @@ class PlaybackController(object): """ old_state = self.get_state() self.stop() - self.set_current_tl_track(tl_track) + self._set_current_tl_track(tl_track) if old_state == PlaybackState.PLAYING: self._play(on_error_step=on_error_step) elif old_state == PlaybackState.PAUSED: @@ -218,7 +217,7 @@ class PlaybackController(object): self._change_track(next_tl_track) else: self.stop() - self.set_current_tl_track(None) + self._set_current_tl_track(None) self.core.tracklist._mark_played(original_tl_track) @@ -231,7 +230,7 @@ class PlaybackController(object): tracklist = self.core.tracklist.get_tl_tracks() if self.get_current_tl_track() not in tracklist: self.stop() - self.set_current_tl_track(None) + self._set_current_tl_track(None) def _on_stream_changed(self, uri): self._stream_title = None @@ -253,7 +252,7 @@ class PlaybackController(object): self._change_track(next_tl_track) else: self.stop() - self.set_current_tl_track(None) + self._set_current_tl_track(None) self.core.tracklist._mark_played(original_tl_track) @@ -302,7 +301,7 @@ class PlaybackController(object): if self.get_state() == PlaybackState.PLAYING: self.stop() - self.set_current_tl_track(tl_track) + self._set_current_tl_track(tl_track) self.set_state(PlaybackState.PLAYING) backend = self._get_backend() success = False diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 972b6dea..7c4db0d6 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -53,8 +53,11 @@ class CorePlaybackTest(unittest.TestCase): def trigger_end_of_track(self): self.core.playback._on_end_of_track() + def set_current_tl_track(self, tl_track): + self.core.playback._set_current_tl_track(tl_track) + def test_get_current_tl_track_none(self): - self.core.playback.set_current_tl_track(None) + self.set_current_tl_track(None) self.assertEqual( self.core.playback.get_current_tl_track(), None) @@ -217,7 +220,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.pause.assert_called_once_with() def test_pause_changes_state_even_if_track_is_unplayable(self): - self.core.playback.current_tl_track = self.unplayable_tl_track + self.set_current_tl_track(self.unplayable_tl_track) self.core.playback.pause() self.assertEqual(self.core.playback.state, core.PlaybackState.PAUSED) @@ -260,7 +263,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.resume.assert_called_once_with() def test_resume_does_nothing_if_track_is_unplayable(self): - self.core.playback.current_tl_track = self.unplayable_tl_track + self.set_current_tl_track(self.unplayable_tl_track) self.core.playback.state = core.PlaybackState.PAUSED self.core.playback.resume() @@ -303,7 +306,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.stop.assert_called_once_with() def test_stop_changes_state_even_if_track_is_unplayable(self): - self.core.playback.current_tl_track = self.unplayable_tl_track + self.set_current_tl_track(self.unplayable_tl_track) self.core.playback.state = core.PlaybackState.PAUSED self.core.playback.stop() @@ -498,7 +501,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.seek.assert_called_once_with(10000) def test_seek_fails_for_unplayable_track(self): - self.core.playback.current_tl_track = self.unplayable_tl_track + self.set_current_tl_track(self.unplayable_tl_track) self.core.playback.state = core.PlaybackState.PLAYING success = self.core.playback.seek(1000) @@ -507,7 +510,7 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback2.seek.called) def test_seek_fails_for_track_without_duration(self): - self.core.playback.current_tl_track = self.duration_less_tl_track + self.set_current_tl_track(self.duration_less_tl_track) self.core.playback.state = core.PlaybackState.PLAYING success = self.core.playback.seek(1000) @@ -557,7 +560,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.get_time_position.assert_called_once_with() def test_time_position_returns_0_if_track_is_unplayable(self): - self.core.playback.current_tl_track = self.unplayable_tl_track + self.set_current_tl_track(self.unplayable_tl_track) result = self.core.playback.time_position From 97fd102fa2520d2e992e6e4be3fcb19f363db0fe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 15:02:25 +0100 Subject: [PATCH 5/6] docs: Add core API cleanup to changelog --- docs/changelog.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 5155fc79..40d707a9 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -77,6 +77,18 @@ v1.0.0 (UNRELEASED) :meth:`mopidy.core.LibraryController.find_exact` to normalize and warn about bad queries from clients. (Fixes: :issue:`1067`, PR: :issue:`1073`) +- Reduced API surface of core. (Fixes: :issue:`1070`, PR: :issue:`1076`) + + - Made ``mopidy.core.PlaybackController.change_track`` internal. + - Removed ``on_error_step`` from :meth:`mopidy.core.PlaybackController.play` + - Made the following event triggers internal: + + - ``mopidy.core.PlaybackController.on_end_of_track`` + - ``mopidy.core.PlaybackController.on_stream_changed`` + - ``mopidy.core.PlaybackController.on_tracklist_changed`` + + - Made ``mopidy.core.PlaybackController.set_current_tl_track`` internal. + **Backend API** - Remove default implementation of From f4452b22db064b7843fd8111d94103521cf9a2e6 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 23 Mar 2015 15:02:37 +0100 Subject: [PATCH 6/6] core: Minor readability improvement --- mopidy/core/playback.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 8d9b7777..61bbc60c 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -274,7 +274,7 @@ class PlaybackController(object): :param tl_track: track to play :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` """ - self._play(tl_track, 1) + self._play(tl_track, on_error_step=1) def _play(self, tl_track=None, on_error_step=1): if tl_track is None: