diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index d995029c..57da7901 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -21,7 +21,6 @@ class TracklistController(object): self._version = 0 self._shuffled = [] - self._first_shuffle = True ### Properties @@ -89,6 +88,9 @@ class TracklistController(object): def set_random(self, value): if self.get_random() != value: self._trigger_options_changed() + if value: + self._shuffled = self.tl_tracks + random.shuffle(self._shuffled) return setattr(self, '_random', value) random = property(get_random, set_random) @@ -187,14 +189,16 @@ class TracklistController(object): return None if self.random and not self._shuffled: - if self.repeat or self._first_shuffle: + if self.repeat or not tl_track: logger.debug('Shuffling tracks') self._shuffled = self.tl_tracks random.shuffle(self._shuffled) - self._first_shuffle = False - if self.random and self._shuffled: - return self._shuffled[0] + if self.random: + try: + return self._shuffled[0] + except IndexError: + return None if tl_track is None: return self.tl_tracks[0] @@ -435,8 +439,11 @@ class TracklistController(object): return True def _trigger_tracklist_changed(self): - self._first_shuffle = True - self._shuffled = [] + if self.random: + self._shuffled = self.tl_tracks + random.shuffle(self._shuffled) + else: + self._shuffled = [] logger.debug('Triggering event: tracklist_changed()') listener.CoreListener.send('tracklist_changed') diff --git a/tests/backends/local/playback_test.py b/tests/backends/local/playback_test.py index 5543b3ab..ab135766 100644 --- a/tests/backends/local/playback_test.py +++ b/tests/backends/local/playback_test.py @@ -1,7 +1,6 @@ from __future__ import unicode_literals import mock -import random import time import unittest @@ -27,8 +26,12 @@ class LocalPlaybackProviderTest(unittest.TestCase): 'tag_cache_file': path_to_data_dir('empty_tag_cache'), } } + + # We need four tracks so that our shuffled track tests behave nicely with + # reversed as a fake shuffle. Ensuring that shuffled order is [4,3,2,1] and + # normal order [1,2,3,4] which means next_track != next_track_with_random tracks = [ - Track(uri=generate_song(i), length=4464) for i in range(1, 4)] + Track(uri=generate_song(i), length=4464) for i in (1, 2, 3, 4)] def add_track(self, uri): track = Track(uri=uri, length=4464) @@ -320,12 +323,14 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.next_track(tl_track), self.tl_tracks[0]) @populate_tracklist - def test_next_track_with_random(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_next_track_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True - tl_track = self.playback.current_tl_track - self.assertEqual( - self.tracklist.next_track(tl_track), self.tl_tracks[2]) + current_tl_track = self.playback.current_tl_track + next_tl_track = self.tracklist.next_track(current_tl_track) + self.assertEqual(next_tl_track, self.tl_tracks[-1]) @populate_tracklist def test_next_with_consume(self): @@ -344,25 +349,41 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.assertEqual(self.playback.current_track, self.tracks[1]) @populate_tracklist - def test_next_with_random(self): - # FIXME feels very fragile - random.seed(1) + @mock.patch('random.shuffle') + def test_next_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True self.playback.play() + self.assertEqual(self.playback.current_track, self.tracks[-1]) self.playback.next() - self.assertEqual(self.playback.current_track, self.tracks[1]) + self.assertEqual(self.playback.current_track, self.tracks[-2]) @populate_tracklist - def test_next_track_with_random_after_append_playlist(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_next_track_with_random_after_append_playlist(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True - tl_track = self.playback.current_tl_track - self.assertEqual(self.tracklist.next_track(tl_track), - self.tl_tracks[2]) + current_tl_track = self.playback.current_tl_track + + expected_tl_track = self.tracklist.tl_tracks[-1] + next_tl_track = self.tracklist.next_track(current_tl_track) + + # Baseline checking that first next_track is last tl track per our fake + # shuffle. + self.assertEqual(next_tl_track, expected_tl_track) + self.tracklist.add(self.tracks[:1]) - tl_track = self.playback.current_tl_track - self.assertEqual( - self.tracklist.next_track(tl_track), self.tl_tracks[1]) + + old_next_tl_track = next_tl_track + expected_tl_track = self.tracklist.tl_tracks[-1] + next_tl_track = self.tracklist.next_track(current_tl_track) + + # Verify that first next track has changed since we added to the + # playlist. + self.assertEqual(next_tl_track, expected_tl_track) + self.assertNotEqual(next_tl_track, old_next_tl_track) @populate_tracklist def test_end_of_track(self): @@ -476,12 +497,14 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.tracklist.next_track(tl_track), self.tl_tracks[0]) @populate_tracklist - def test_end_of_track_track_with_random(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_end_of_track_track_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True tl_track = self.playback.current_tl_track self.assertEqual( - self.tracklist.next_track(tl_track), self.tl_tracks[2]) + self.tracklist.next_track(tl_track), self.tl_tracks[-1]) @populate_tracklist def test_end_of_track_with_consume(self): @@ -491,25 +514,42 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.assertNotIn(self.tracks[0], self.tracklist.tracks) @populate_tracklist - def test_end_of_track_with_random(self): - # FIXME feels very fragile - random.seed(1) + @mock.patch('random.shuffle') + def test_end_of_track_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True self.playback.play() + self.assertEqual(self.playback.current_track, self.tracks[-1]) self.playback.on_end_of_track() - self.assertEqual(self.playback.current_track, self.tracks[1]) + self.assertEqual(self.playback.current_track, self.tracks[-2]) @populate_tracklist - def test_end_of_track_track_with_random_after_append_playlist(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_end_of_track_track_with_random_after_append_playlist( + self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True - tl_track = self.playback.current_tl_track - self.assertEqual( - self.tracklist.next_track(tl_track), self.tl_tracks[2]) + current_tl_track = self.playback.current_tl_track + + expected_tl_track = self.tracklist.tl_tracks[-1] + eot_tl_track = self.tracklist.eot_track(current_tl_track) + + # Baseline checking that first eot_track is last tl track per our fake + # shuffle. + self.assertEqual(eot_tl_track, expected_tl_track) + self.tracklist.add(self.tracks[:1]) - tl_track = self.playback.current_tl_track - self.assertEqual( - self.tracklist.next_track(tl_track), self.tl_tracks[1]) + + old_eot_tl_track = eot_tl_track + expected_tl_track = self.tracklist.tl_tracks[-1] + eot_tl_track = self.tracklist.eot_track(current_tl_track) + + # Verify that first next track has changed since we added to the + # playlist. + self.assertEqual(eot_tl_track, expected_tl_track) + self.assertNotEqual(eot_tl_track, old_eot_tl_track) @populate_tracklist def test_previous_track_before_play(self): @@ -873,15 +913,19 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.assertEqual(len(self.tracklist.tracks), 0) @populate_tracklist - def test_play_with_random(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_play_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True self.playback.play() - self.assertEqual(self.playback.current_track, self.tracks[2]) + self.assertEqual(self.playback.current_track, self.tracks[-1]) @populate_tracklist - def test_previous_with_random(self): - random.seed(1) + @mock.patch('random.shuffle') + def test_previous_with_random(self, shuffle_mock): + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + self.tracklist.random = True self.playback.play() self.playback.next() @@ -956,9 +1000,19 @@ class LocalPlaybackProviderTest(unittest.TestCase): tl_track = self.playback.current_tl_track self.assertEqual(self.tracklist.next_track(tl_track), None) + @populate_tracklist + def test_random_with_eot_until_end_of_playlist(self): + self.tracklist.random = True + self.playback.play() + for _ in self.tracks[1:]: + self.playback.on_end_of_track() + tl_track = self.playback.current_tl_track + self.assertEqual(self.tracklist.eot_track(tl_track), None) + @populate_tracklist def test_random_until_end_of_playlist_and_play_from_start(self): - self.tracklist.repeat = True + self.tracklist.random = True + self.playback.play() for _ in self.tracks: self.playback.next() tl_track = self.playback.current_tl_track @@ -967,12 +1021,24 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.play() self.assertEqual(self.playback.state, PlaybackState.PLAYING) + @populate_tracklist + def test_random_with_eot_until_end_of_playlist_and_play_from_start(self): + self.tracklist.random = True + self.playback.play() + for _ in self.tracks: + self.playback.on_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) + self.playback.play() + self.assertEqual(self.playback.state, PlaybackState.PLAYING) + @populate_tracklist def test_random_until_end_of_playlist_with_repeat(self): self.tracklist.repeat = True self.tracklist.random = True self.playback.play() - for _ in self.tracks: + for _ in self.tracks[1:]: self.playback.next() tl_track = self.playback.current_tl_track self.assertNotEqual(self.tracklist.next_track(tl_track), None) @@ -987,6 +1053,22 @@ class LocalPlaybackProviderTest(unittest.TestCase): played.append(self.playback.current_track) self.playback.next() + @populate_tracklist + @mock.patch('random.shuffle') + def test_play_track_then_enable_random(self, shuffle_mock): + # Covers underlying issue IssueGH17RegressionTest tests for. + shuffle_mock.side_effect = lambda tracks: tracks.reverse() + + expected = self.tl_tracks[::-1] + [None] + actual = [] + + self.playback.play() + self.tracklist.random = True + while self.playback.state != PlaybackState.STOPPED: + self.playback.next() + actual.append(self.playback.current_tl_track) + self.assertEqual(actual, expected) + @populate_tracklist def test_playing_track_that_isnt_in_playlist(self): test = lambda: self.playback.play((17, Track())) diff --git a/tests/data/song4.wav b/tests/data/song4.wav new file mode 100644 index 00000000..0041c7ba Binary files /dev/null and b/tests/data/song4.wav differ