From 6a7005be1e2570352d7643045f0ef7ff9fa72bd0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Apr 2015 20:42:52 +0200 Subject: [PATCH 1/7] core: Add `tlid` argument to index calls. Should save clients from having to pass tl_track models around. --- mopidy/core/tracklist.py | 23 +++++++++++---- tests/core/test_tracklist.py | 54 ++++++++++++++++++++++++++++++++++- tests/local/test_tracklist.py | 12 +------- 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index e0497a9a..de875081 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -200,19 +200,32 @@ class TracklistController(object): # Methods - def index(self, tl_track): + def index(self, tl_track=None, tlid=None): """ The position of the given track in the tracklist. :param tl_track: the track to find the index of :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` + :param tlid: of the track to find the index of + :type tlid: TLID number or :class:`None` :rtype: :class:`int` or :class:`None` + + .. versionchanged:: 1.1 + Added the *tlid* parameter """ tl_track is None or validation.check_instance(tl_track, TlTrack) - try: - return self._tl_tracks.index(tl_track) - except ValueError: - return None + tlid is None or validation.check_integer(tlid, min=0) + + if tl_track is not None: + try: + return self._tl_tracks.index(tl_track) + except ValueError: + pass + elif tlid is not None: + for i, tl_track in enumerate(self._tl_tracks): + if tl_track.tlid == tlid: + return i + return None def eot_track(self, tl_track): """ diff --git a/tests/core/test_tracklist.py b/tests/core/test_tracklist.py index 1ff089cb..d9f918a8 100644 --- a/tests/core/test_tracklist.py +++ b/tests/core/test_tracklist.py @@ -5,7 +5,7 @@ import unittest import mock from mopidy import backend, core -from mopidy.models import Track +from mopidy.models import TlTrack, Track from mopidy.utils import deprecation @@ -102,3 +102,55 @@ class TracklistTest(unittest.TestCase): self.core.tracklist.filter({'uri': 'a'}) # TODO Extract tracklist tests from the local backend tests + + +class TracklistIndexTest(unittest.TestCase): + + def setUp(self): # noqa: N802 + self.tracks = [ + Track(uri='dummy1:a', name='foo'), + Track(uri='dummy1:b', name='foo'), + Track(uri='dummy1:c', name='bar'), + ] + + def lookup(uris): + return {u: [t for t in self.tracks if t.uri == u] for u in uris} + + self.core = core.Core(mixer=None, backends=[]) + self.core.library.lookup = mock.Mock() + self.core.library.lookup.side_effect = lookup + + self.tl_tracks = self.core.tracklist.add(uris=[ + t.uri for t in self.tracks]) + + def test_index_returns_index_of_track(self): + self.assertEqual(0, self.core.tracklist.index(self.tl_tracks[0])) + self.assertEqual(1, self.core.tracklist.index(self.tl_tracks[1])) + self.assertEqual(2, self.core.tracklist.index(self.tl_tracks[2])) + + def test_index_returns_none_if_item_not_found(self): + tl_track = TlTrack(0, Track()) + self.assertEqual(self.core.tracklist.index(tl_track), None) + + def test_index_returns_none_if_called_with_none(self): + self.assertEqual(self.core.tracklist.index(None), None) + + def test_index_errors_out_for_invalid_tltrack(self): + with self.assertRaises(ValueError): + self.core.tracklist.index('abc') + + def test_index_return_index_when_called_with_tlids(self): + tl_tracks = self.tl_tracks + self.assertEqual(0, self.core.tracklist.index(tlid=tl_tracks[0].tlid)) + self.assertEqual(1, self.core.tracklist.index(tlid=tl_tracks[1].tlid)) + self.assertEqual(2, self.core.tracklist.index(tlid=tl_tracks[2].tlid)) + + def test_index_returns_none_if_tlid_not_found(self): + self.assertEqual(self.core.tracklist.index(tlid=123), None) + + def test_index_returns_none_if_called_with_tlid_none(self): + self.assertEqual(self.core.tracklist.index(tlid=None), None) + + def test_index_errors_out_for_invalid_tlid(self): + with self.assertRaises(ValueError): + self.core.tracklist.index(tlid=-1) diff --git a/tests/local/test_tracklist.py b/tests/local/test_tracklist.py index f405f218..a0add637 100644 --- a/tests/local/test_tracklist.py +++ b/tests/local/test_tracklist.py @@ -8,7 +8,7 @@ import pykka from mopidy import core from mopidy.core import PlaybackState from mopidy.local import actor -from mopidy.models import Playlist, TlTrack, Track +from mopidy.models import Playlist, Track from mopidy.utils import deprecation from tests import dummy_audio, path_to_data_dir @@ -176,16 +176,6 @@ class LocalTracklistProviderTest(unittest.TestCase): tl_tracks = self.controller.add(self.controller.tracks[1:2]) self.assertEqual(tl_tracks[0].track, self.controller.tracks[1]) - def test_index_returns_index_of_track(self): - tl_tracks = self.controller.add(self.tracks) - self.assertEqual(0, self.controller.index(tl_tracks[0])) - self.assertEqual(1, self.controller.index(tl_tracks[1])) - self.assertEqual(2, self.controller.index(tl_tracks[2])) - - def test_index_returns_none_if_item_not_found(self): - tl_track = TlTrack(0, Track()) - self.assertEqual(self.controller.index(tl_track), None) - @populate_tracklist def test_move_single(self): self.controller.move(0, 0, 2) From 691abb2431547f811aa303a1c5684e341332b98d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Apr 2015 20:54:15 +0200 Subject: [PATCH 2/7] core: Stop making tl track copies all over the place --- mopidy/core/tracklist.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index de875081..b0396d53 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -264,13 +264,13 @@ class TracklistController(object): """ tl_track is None or validation.check_instance(tl_track, TlTrack) - if not self.get_tl_tracks(): + if not self._tl_tracks: return None if self.get_random() and not self._shuffled: if self.get_repeat() or not tl_track: logger.debug('Shuffling tracks') - self._shuffled = self.get_tl_tracks() + self._shuffled = self._tl_tracks[:] random.shuffle(self._shuffled) if self.get_random(): @@ -280,14 +280,14 @@ class TracklistController(object): return None if tl_track is None: - return self.get_tl_tracks()[0] + return self._tl_tracks[0] next_index = self.index(tl_track) + 1 if self.get_repeat(): - next_index %= len(self.get_tl_tracks()) + next_index %= len(self._tl_tracks) try: - return self.get_tl_tracks()[next_index] + return self._tl_tracks[next_index] except IndexError: return None @@ -314,7 +314,9 @@ class TracklistController(object): if position in (None, 0): return None - return self.get_tl_tracks()[position - 1] + # Note that since we know we are at position 1-n we know this will + # never be out bounds for the tl_tracks list. + return self._tl_tracks[position - 1] def add(self, tracks=None, at_position=None, uri=None, uris=None): """ @@ -567,7 +569,7 @@ class TracklistController(object): def _trigger_tracklist_changed(self): if self.get_random(): - self._shuffled = self.get_tl_tracks() + self._shuffled = self._tl_tracks[:] random.shuffle(self._shuffled) else: self._shuffled = [] From aab143aeec79f63f5df1c3433ede655bb1bb71b2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Apr 2015 20:59:13 +0200 Subject: [PATCH 3/7] core: Cleanup internals of next_track a bit --- mopidy/core/tracklist.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index b0396d53..6ffb0f60 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -274,23 +274,22 @@ class TracklistController(object): random.shuffle(self._shuffled) if self.get_random(): - try: + if self._shuffled: return self._shuffled[0] - except IndexError: - return None + return None if tl_track is None: - return self._tl_tracks[0] + next_index = 0 + else: + next_index = self.index(tl_track) + 1 - next_index = self.index(tl_track) + 1 if self.get_repeat(): next_index %= len(self._tl_tracks) - - try: - return self._tl_tracks[next_index] - except IndexError: + elif next_index >= len(self._tl_tracks): return None + return self._tl_tracks[next_index] + def previous_track(self, tl_track): """ Returns the track that will be played if calling From a88d3cf61369c9c71fd87f62dc50c5704ea2a0b9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 18 Apr 2015 15:47:36 +0200 Subject: [PATCH 4/7] core: Add get_*_tlid helpers --- mopidy/core/tracklist.py | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 6ffb0f60..13877795 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -227,6 +227,20 @@ class TracklistController(object): return i return None + def get_eot_tlid(self): + """ + The TLID of the track that will be played after the given track. + + Not necessarily the same track as :meth:`get_next_tlid`. + + :rtype: TLID or :class:`None` + + .. versionadded:: 1.1 + """ + + current_tl_track = self.core.playback.get_current_tl_track() + return getattr(self.eot_track(current_tl_track), 'tlid', None) + def eot_track(self, tl_track): """ The track that will be played after the given track. @@ -248,6 +262,23 @@ class TracklistController(object): # shared. return self.next_track(tl_track) + def get_next_tlid(self): + """ + The tlid of the track that will be played if calling + :meth:`mopidy.core.PlaybackController.next()`. + + For normal playback this is the next track in the tracklist. If repeat + is enabled the next track can loop around the tracklist. When random is + enabled this should be a random track, all tracks should be played once + before the tracklist repeats. + + :rtype: TLID or :class:`None` + + .. versionadded:: 1.1 + """ + current_tl_track = self.core.playback.get_current_tl_track() + return getattr(self.next_track(current_tl_track), 'tlid', None) + def next_track(self, tl_track): """ The track that will be played if calling @@ -290,6 +321,22 @@ class TracklistController(object): return self._tl_tracks[next_index] + def get_previous_tlid(self): + """ + Returns the TLID of the track that will be played if calling + :meth:`mopidy.core.PlaybackController.previous()`. + + For normal playback this is the previous track in the tracklist. If + random and/or consume is enabled it should return the current track + instead. + + :rtype: TLID or :class:`None` + + .. versionadded:: 1.1 + """ + current_tl_track = self.core.playback.get_current_tl_track() + return getattr(self.previous_track(current_tl_track), 'tlid', None) + def previous_track(self, tl_track): """ Returns the track that will be played if calling From 2e705cf8d4cb61fb06f25a30dcb1b47540255e2d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 18 Apr 2015 16:52:02 +0200 Subject: [PATCH 5/7] core: Add pending depraction for *_track methods --- mopidy/core/tracklist.py | 3 +++ mopidy/utils/deprecation.py | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 13877795..7850acc1 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -251,6 +251,7 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + deprecation.warn('core.tracklist.eot_track', pending=True) tl_track is None or validation.check_instance(tl_track, TlTrack) if self.get_single() and self.get_repeat(): return tl_track @@ -293,6 +294,7 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + deprecation.warn('core.tracklist.next_track', pending=True) tl_track is None or validation.check_instance(tl_track, TlTrack) if not self._tl_tracks: @@ -350,6 +352,7 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + deprecation.warn('core.tracklist.previous_track', pending=True) tl_track is None or validation.check_instance(tl_track, TlTrack) if self.get_repeat() or self.get_consume() or self.get_random(): diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index db263e6d..be3cc650 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -45,13 +45,25 @@ _MESSAGES = { 'core.tracklist.remove:kwargs_criteria': 'tracklist.remove() with "kwargs" as criteria is deprecated', + 'core.tracklist.eot_track': + 'tracklist.eot_track() is deprecated, use tracklist.get_eot_tlid()', + 'core.tracklist.next_track': + 'tracklist.next_track() is deprecated, use tracklist.get_next_tlid()', + 'core.tracklist.previous_track': + 'tracklist.previous_track() is deprecated, use ' + 'tracklist.get_previous_tlid()', + 'models.immutable.copy': 'ImmutableObject.copy() is deprecated, use ImmutableObject.replace()', } -def warn(msg_id): - warnings.warn(_MESSAGES.get(msg_id, msg_id), DeprecationWarning) +def warn(msg_id, pending=False): + if pending: + category = PendingDeprecationWarning + else: + category = DeprecationWarning + warnings.warn(_MESSAGES.get(msg_id, msg_id), category) @contextlib.contextmanager From fba4069cfd8b77b93718a5e5d101382ec6e1d095 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 18 Apr 2015 21:01:20 +0200 Subject: [PATCH 6/7] core: Make index return current index when missing args --- mopidy/core/tracklist.py | 6 ++++++ tests/core/test_tracklist.py | 13 ++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 7850acc1..8b630403 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -204,6 +204,9 @@ class TracklistController(object): """ The position of the given track in the tracklist. + If neither *tl_track* or *tlid* is given we return the index of + the currently playing track. + :param tl_track: the track to find the index of :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :param tlid: of the track to find the index of @@ -216,6 +219,9 @@ class TracklistController(object): tl_track is None or validation.check_instance(tl_track, TlTrack) tlid is None or validation.check_integer(tlid, min=0) + if tl_track is None and tlid is None: + tl_track = self.core.playback.get_current_tl_track() + if tl_track is not None: try: return self._tl_tracks.index(tl_track) diff --git a/tests/core/test_tracklist.py b/tests/core/test_tracklist.py index d9f918a8..6339a18c 100644 --- a/tests/core/test_tracklist.py +++ b/tests/core/test_tracklist.py @@ -117,9 +117,11 @@ class TracklistIndexTest(unittest.TestCase): return {u: [t for t in self.tracks if t.uri == u] for u in uris} self.core = core.Core(mixer=None, backends=[]) - self.core.library.lookup = mock.Mock() + self.core.library = mock.Mock(spec=core.LibraryController) self.core.library.lookup.side_effect = lookup + self.core.playback = mock.Mock(spec=core.PlaybackController) + self.tl_tracks = self.core.tracklist.add(uris=[ t.uri for t in self.tracks]) @@ -154,3 +156,12 @@ class TracklistIndexTest(unittest.TestCase): def test_index_errors_out_for_invalid_tlid(self): with self.assertRaises(ValueError): self.core.tracklist.index(tlid=-1) + + def test_index_without_args_returns_current_tl_track_index(self): + self.core.playback.get_current_tl_track.side_effect = [ + None, self.tl_tracks[0], self.tl_tracks[1], self.tl_tracks[2]] + + self.assertEqual(None, self.core.tracklist.index()) + self.assertEqual(0, self.core.tracklist.index()) + self.assertEqual(1, self.core.tracklist.index()) + self.assertEqual(2, self.core.tracklist.index()) From 9cec66696f618e03732a630a8b7fd7f700a89539 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 21 Apr 2015 00:55:24 +0200 Subject: [PATCH 7/7] core: Fix comments and docstrings per review comments --- mopidy/core/tracklist.py | 16 ++++++++-------- mopidy/utils/deprecation.py | 8 +++++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 8b630403..01da6019 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -209,8 +209,8 @@ class TracklistController(object): :param tl_track: the track to find the index of :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` - :param tlid: of the track to find the index of - :type tlid: TLID number or :class:`None` + :param tlid: TLID of the track to find the index of + :type tlid: :class:`int` or :class:`None` :rtype: :class:`int` or :class:`None` .. versionchanged:: 1.1 @@ -237,9 +237,9 @@ class TracklistController(object): """ The TLID of the track that will be played after the given track. - Not necessarily the same track as :meth:`get_next_tlid`. + Not necessarily the same TLID as returned by :meth:`get_next_tlid`. - :rtype: TLID or :class:`None` + :rtype: :class:`int` or :class:`None` .. versionadded:: 1.1 """ @@ -279,7 +279,7 @@ class TracklistController(object): enabled this should be a random track, all tracks should be played once before the tracklist repeats. - :rtype: TLID or :class:`None` + :rtype: :class:`int` or :class:`None` .. versionadded:: 1.1 """ @@ -338,7 +338,7 @@ class TracklistController(object): random and/or consume is enabled it should return the current track instead. - :rtype: TLID or :class:`None` + :rtype: :class:`int` or :class:`None` .. versionadded:: 1.1 """ @@ -369,8 +369,8 @@ class TracklistController(object): if position in (None, 0): return None - # Note that since we know we are at position 1-n we know this will - # never be out bounds for the tl_tracks list. + # Since we know we are not at zero we have to be somewhere in the range + # 1 - len(tracks) Thus 'position - 1' will always be within the list. return self._tl_tracks[position - 1] def add(self, tracks=None, at_position=None, uri=None, uris=None): diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index be3cc650..a650d79e 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -46,11 +46,13 @@ _MESSAGES = { 'tracklist.remove() with "kwargs" as criteria is deprecated', 'core.tracklist.eot_track': - 'tracklist.eot_track() is deprecated, use tracklist.get_eot_tlid()', + 'tracklist.eot_track() is pending deprecation, use ' + 'tracklist.get_eot_tlid()', 'core.tracklist.next_track': - 'tracklist.next_track() is deprecated, use tracklist.get_next_tlid()', + 'tracklist.next_track() is pending deprecation, use ' + 'tracklist.get_next_tlid()', 'core.tracklist.previous_track': - 'tracklist.previous_track() is deprecated, use ' + 'tracklist.previous_track() is pending deprecation, use ' 'tracklist.get_previous_tlid()', 'models.immutable.copy':