From d6798ac870562094916a588f962396efa330f612 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 26 Aug 2010 19:04:08 +0200 Subject: [PATCH 01/40] Fix GH-16: 'addid ""' crashes with SpotifyError --- docs/changes.rst | 12 ++++++++++++ mopidy/backends/libspotify/library.py | 15 ++++++++++----- .../frontends/mpd/protocol/current_playlist.py | 16 ++++++++++++++-- tests/frontends/mpd/current_playlist_test.py | 10 ++++++++++ 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index e84d7aa9..d061f892 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -5,6 +5,18 @@ Changes This change log is used to track all major changes to Mopidy. +0.1.1 (in development) +====================== + +No description yet. + +**Changes** + +- MPD frontend: + + - ``add ""`` and ``addid ""`` now behaves as expected. + + 0.1.0 (2010-08-23) ================== diff --git a/mopidy/backends/libspotify/library.py b/mopidy/backends/libspotify/library.py index ffb9ee57..eb1c24d9 100644 --- a/mopidy/backends/libspotify/library.py +++ b/mopidy/backends/libspotify/library.py @@ -1,7 +1,7 @@ import logging import multiprocessing -from spotify import Link +from spotify import Link, SpotifyError from mopidy.backends.base import BaseLibraryController from mopidy.backends.libspotify import ENCODING @@ -14,10 +14,15 @@ class LibspotifyLibraryController(BaseLibraryController): return self.search(**query) def lookup(self, uri): - spotify_track = Link.from_string(uri).as_track() - # TODO Block until metadata_updated callback is called. Before that the - # track will be unloaded, unless it's already in the stored playlists. - return LibspotifyTranslator.to_mopidy_track(spotify_track) + try: + spotify_track = Link.from_string(uri).as_track() + # TODO Block until metadata_updated callback is called. Before that + # the track will be unloaded, unless it's already in the stored + # playlists. + return LibspotifyTranslator.to_mopidy_track(spotify_track) + except SpotifyError as e: + logger.warning(u'Failed to lookup: %s', uri, e) + return None def refresh(self, uri=None): pass # TODO diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 90a53f5f..2f0a9f8f 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -11,14 +11,19 @@ def add(frontend, uri): Adds the file ``URI`` to the playlist (directories add recursively). ``URI`` can also be a single file. + + *Clarifications:* + + - ``add ""`` should add all tracks in the library to the current playlist. """ + if not uri: + return for handler_prefix in frontend.backend.uri_handlers: if uri.startswith(handler_prefix): track = frontend.backend.library.lookup(uri) if track is not None: frontend.backend.current_playlist.add(track) return - raise MpdNoExistError( u'directory or file not found', command=u'add') @@ -36,7 +41,13 @@ def addid(frontend, uri, songpos=None): addid "foo.mp3" Id: 999 OK + + *Clarifications:* + + - ``addid ""`` should return an error. """ + if not uri: + raise MpdNoExistError(u'No such song', command=u'addid') if songpos is not None: songpos = int(songpos) track = frontend.backend.library.lookup(uri) @@ -44,7 +55,8 @@ def addid(frontend, uri, songpos=None): raise MpdNoExistError(u'No such song', command=u'addid') if songpos and songpos > len(frontend.backend.current_playlist.tracks): raise MpdArgError(u'Bad song index', command=u'addid') - cp_track = frontend.backend.current_playlist.add(track, at_position=songpos) + cp_track = frontend.backend.current_playlist.add(track, + at_position=songpos) return ('Id', cp_track[0]) @handle_pattern(r'^delete "(?P\d+):(?P\d+)*"$') diff --git a/tests/frontends/mpd/current_playlist_test.py b/tests/frontends/mpd/current_playlist_test.py index c53e2b8d..8e4b62f9 100644 --- a/tests/frontends/mpd/current_playlist_test.py +++ b/tests/frontends/mpd/current_playlist_test.py @@ -33,6 +33,11 @@ class CurrentPlaylistHandlerTest(unittest.TestCase): self.assertEqual(result[0], u'ACK [50@0] {add} directory or file not found') + def test_add_with_empty_uri_should_add_all_known_tracks_and_ok(self): + result = self.h.handle_request(u'add ""') + # TODO check that we add all tracks (we currently don't) + self.assert_(u'OK' in result) + def test_addid_without_songpos(self): needle = Track(uri='dummy://foo') self.b.library._library = [Track(), Track(), needle, Track()] @@ -46,6 +51,11 @@ class CurrentPlaylistHandlerTest(unittest.TestCase): in result) self.assert_(u'OK' in result) + def test_addid_with_empty_uri_does_not_lookup_and_acks(self): + self.b.library.lookup = lambda uri: self.fail("Shouldn't run") + result = self.h.handle_request(u'addid ""') + self.assertEqual(result[0], u'ACK [50@0] {addid} No such song') + def test_addid_with_songpos(self): needle = Track(uri='dummy://foo') self.b.library._library = [Track(), Track(), needle, Track()] From f428546b72ebdfa0861ab9de945b218ff7609c72 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 14:02:20 +0200 Subject: [PATCH 02/40] Update 'list' docs with a bunch of valid examples --- mopidy/frontends/mpd/protocol/music_db.py | 74 +++++++++++++++++++---- 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/music_db.py b/mopidy/frontends/mpd/protocol/music_db.py index d4dcf50d..dfdec727 100644 --- a/mopidy/frontends/mpd/protocol/music_db.py +++ b/mopidy/frontends/mpd/protocol/music_db.py @@ -101,22 +101,70 @@ def list_(frontend, field, artist=None): This filters the result list by an artist. + *Clarifications:* + + The musicpd.org documentation for ``list`` is far from complete. The + command also supports the following variant: + + ``list {TYPE} {QUERY}`` + + Where ``QUERY`` applies to all ``TYPE``. ``QUERY`` is one or more pairs + of a field name and a value. If the ``QUERY`` consists of more than one + pair, the pairs are AND-ed together to find the result. Examples of + valid queries and what they should return: + + ``list "artist" "artist" "ABBA"`` + List artists where the artist name is "ABBA". Response:: + + Artist: ABBA + OK + + ``list "album" "artist" "ABBA"`` + Lists albums where the artist name is "ABBA". Response:: + + Album: More ABBA Gold: More ABBA Hits + Album: Absolute More Christmas + Album: Gold: Greatest Hits + OK + + ``list "artist" "album" "Gold: Greatest Hits"`` + Lists artists where the album name is "Gold: Greatest Hits". + Response:: + + Artist: ABBA + OK + + ``list "artist" "artist" "ABBA" "artist" "TLC"`` + Lists artists where the artist name is "ABBA" *and* "TLC". Should + never match anything. Response:: + + OK + + ``list "date" "artist" "ABBA"`` + Lists dates where artist name is "ABBA". Response:: + + Date: + Date: 1992 + Date: 1993 + OK + + ``list "date" "artist" "ABBA" "album" "Gold: Greatest Hits"`` + Lists dates where artist name is "ABBA" and album name is "Gold: + Greatest Hits". Response:: + + Date: 1992 + OK + + ``list "genre" "artist" "The Rolling Stones"`` + Lists genres where artist name is "The Rolling Stones". Response:: + + Genre: + Genre: Rock + OK + *GMPC:* - does not add quotes around the field argument. - - asks for "list artist" to get available artists and will not query - for artist/album information if this is not retrived - - asks for multiple fields, i.e.:: - - list album artist "an artist name" - - returns the albums available for the asked artist:: - - list album artist "Tiesto" - Album: Radio Trance Vol 4-Promo-CD - Album: Ur A Tear in the Open CDR - Album: Simple Trance 2004 Step One - Album: In Concert 05-10-2003 *ncmpc:* From 802811e43524a93d3adb138a8159fc47775dc2bb Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 15:56:34 +0200 Subject: [PATCH 03/40] libspotify: Return all tracks in stored playlists upon empty search query --- mopidy/backends/libspotify/library.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mopidy/backends/libspotify/library.py b/mopidy/backends/libspotify/library.py index ffb9ee57..68512ffa 100644 --- a/mopidy/backends/libspotify/library.py +++ b/mopidy/backends/libspotify/library.py @@ -6,6 +6,7 @@ from spotify import Link from mopidy.backends.base import BaseLibraryController from mopidy.backends.libspotify import ENCODING from mopidy.backends.libspotify.translator import LibspotifyTranslator +from mopidy.models import Playlist logger = logging.getLogger('mopidy.backends.libspotify.library') @@ -23,6 +24,13 @@ class LibspotifyLibraryController(BaseLibraryController): pass # TODO def search(self, **query): + if not query: + # Since we can't search for the entire Spotify library, we return + # all tracks in the stored playlists when the query is empty. + tracks = [] + for playlist in self.backend.stored_playlists.playlists: + tracks += playlist.tracks + return Playlist(tracks=tracks) spotify_query = [] for (field, values) in query.iteritems(): if not hasattr(values, '__iter__'): From 4b4c4b709e937d1ffda0c05d559343206f1b06c9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 16:07:44 +0200 Subject: [PATCH 04/40] Rewrite list command to support more advanced queries --- mopidy/frontends/mpd/protocol/music_db.py | 75 ++++++--- tests/frontends/mpd/music_db_test.py | 177 ++++++++++++++++------ 2 files changed, 182 insertions(+), 70 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/music_db.py b/mopidy/frontends/mpd/protocol/music_db.py index dfdec727..2bccab3d 100644 --- a/mopidy/frontends/mpd/protocol/music_db.py +++ b/mopidy/frontends/mpd/protocol/music_db.py @@ -1,7 +1,8 @@ import re +import shlex from mopidy.frontends.mpd.protocol import handle_pattern, stored_playlists -from mopidy.frontends.mpd.exceptions import MpdNotImplemented +from mopidy.frontends.mpd.exceptions import MpdArgError, MpdNotImplemented def _build_query(mpd_query): """ @@ -81,13 +82,9 @@ def findadd(frontend, query): # TODO Add result to current playlist #result = frontend.find(query) -@handle_pattern(r'^list (?P[Aa]rtist)$') -@handle_pattern(r'^list "(?P[Aa]rtist)"$') -@handle_pattern(r'^list (?Palbum( artist)?)' - '( "(?P[^"]+)")*$') -@handle_pattern(r'^list "(?Palbum(" "artist)?)"' - '( "(?P[^"]+)")*$') -def list_(frontend, field, artist=None): +@handle_pattern(r'^list "?(?P([Aa]rtist|[Aa]lbum|[Dd]ate|[Gg]enre))"?' + '( (?P.*))?$') +def list_(frontend, field, mpd_query=None): """ *musicpd.org, music database section:* @@ -172,29 +169,59 @@ def list_(frontend, field, artist=None): - capitalizes the field argument. """ field = field.lower() + query = _list_build_query(field, mpd_query) if field == u'artist': - return _list_artist(frontend) - elif field == u'album artist': - return _list_album_artist(frontend, artist) - # TODO More to implement + return _list_artist(frontend, query) + elif field == u'album': + return _list_album(frontend, query) + elif field == u'date': + pass # TODO + elif field == u'genre': + pass # TODO -def _list_artist(frontend): - """ - Since we don't know exactly all available artists, we respond with - the artists we know for sure, which is all artists in our stored playlists. - """ +def _list_build_query(field, mpd_query): + """Converts a ``list`` query to a Mopidy query.""" + if mpd_query is None: + return {} + # shlex does not seem to be friends with unicode objects + tokens = shlex.split(mpd_query.encode('utf-8')) + tokens = [t.decode('utf-8') for t in tokens] + if len(tokens) == 1: + if field == u'album': + return {'artist': [tokens[0]]} + else: + raise MpdArgError( + u'should be "Album" for 3 arguments', command=u'list') + elif len(tokens) % 2 == 0: + query = {} + while tokens: + key = tokens[0].lower() + value = tokens[1] + tokens = tokens[2:] + if key not in (u'artist', u'album', u'date', u'genre'): + raise MpdArgError(u'not able to parse args', command=u'list') + if key in query: + query[key].append(value) + else: + query[key] = [value] + return query + else: + raise MpdArgError(u'not able to parse args', command=u'list') + +def _list_artist(frontend, query): artists = set() - for playlist in frontend.backend.stored_playlists.playlists: - for track in playlist.tracks: - for artist in track.artists: - artists.add((u'Artist', artist.name)) + playlist = frontend.backend.library.find_exact(**query) + for track in playlist.tracks: + for artist in track.artists: + artists.add((u'Artist', artist.name)) return artists -def _list_album_artist(frontend, artist): - playlist = frontend.backend.library.find_exact(artist=[artist]) +def _list_album(frontend, query): albums = set() + playlist = frontend.backend.library.find_exact(**query) for track in playlist.tracks: - albums.add((u'Album', track.album.name)) + if track.album is not None: + albums.add((u'Album', track.album.name)) return albums @handle_pattern(r'^listall "(?P[^"]+)"') diff --git a/tests/frontends/mpd/music_db_test.py b/tests/frontends/mpd/music_db_test.py index 5fcc393c..408961b2 100644 --- a/tests/frontends/mpd/music_db_test.py +++ b/tests/frontends/mpd/music_db_test.py @@ -15,6 +15,59 @@ class MusicDatabaseHandlerTest(unittest.TestCase): self.assert_(u'playtime: 0' in result) self.assert_(u'OK' in result) + def test_findadd(self): + result = self.h.handle_request(u'findadd "album" "what"') + self.assert_(u'OK' in result) + + def test_listall(self): + result = self.h.handle_request(u'listall "file:///dev/urandom"') + self.assert_(u'ACK [0@0] {} Not implemented' in result) + + def test_listallinfo(self): + result = self.h.handle_request(u'listallinfo "file:///dev/urandom"') + self.assert_(u'ACK [0@0] {} Not implemented' in result) + + def test_lsinfo_without_path_returns_same_as_listplaylists(self): + lsinfo_result = self.h.handle_request(u'lsinfo') + listplaylists_result = self.h.handle_request(u'listplaylists') + self.assertEqual(lsinfo_result, listplaylists_result) + + def test_lsinfo_with_empty_path_returns_same_as_listplaylists(self): + lsinfo_result = self.h.handle_request(u'lsinfo ""') + listplaylists_result = self.h.handle_request(u'listplaylists') + self.assertEqual(lsinfo_result, listplaylists_result) + + def test_lsinfo_for_root_returns_same_as_listplaylists(self): + lsinfo_result = self.h.handle_request(u'lsinfo "/"') + listplaylists_result = self.h.handle_request(u'listplaylists') + self.assertEqual(lsinfo_result, listplaylists_result) + + def test_update_without_uri(self): + result = self.h.handle_request(u'update') + self.assert_(u'OK' in result) + self.assert_(u'updating_db: 0' in result) + + def test_update_with_uri(self): + result = self.h.handle_request(u'update "file:///dev/urandom"') + self.assert_(u'OK' in result) + self.assert_(u'updating_db: 0' in result) + + def test_rescan_without_uri(self): + result = self.h.handle_request(u'rescan') + self.assert_(u'OK' in result) + self.assert_(u'updating_db: 0' in result) + + def test_rescan_with_uri(self): + result = self.h.handle_request(u'rescan "file:///dev/urandom"') + self.assert_(u'OK' in result) + self.assert_(u'updating_db: 0' in result) + + +class MusicDatabaseFindTest(unittest.TestCase): + def setUp(self): + self.b = DummyBackend(mixer_class=DummyMixer) + self.h = dispatcher.MpdDispatcher(backend=self.b) + def test_find_album(self): result = self.h.handle_request(u'find "album" "what"') self.assert_(u'OK' in result) @@ -48,11 +101,20 @@ class MusicDatabaseHandlerTest(unittest.TestCase): u'find album "album_what" artist "artist_what"') self.assert_(u'OK' in result) - def test_findadd(self): - result = self.h.handle_request(u'findadd "album" "what"') - self.assert_(u'OK' in result) - def test_list_artist(self): +class MusicDatabaseListTest(unittest.TestCase): + def setUp(self): + self.b = DummyBackend(mixer_class=DummyMixer) + self.h = dispatcher.MpdDispatcher(backend=self.b) + + def test_list_foo_returns_ack(self): + result = self.h.handle_request(u'list "foo"') + self.assertEqual(result[0], + u'ACK [2@0] {list} incorrect arguments') + + ### Artist + + def test_list_artist_with_quotes(self): result = self.h.handle_request(u'list "artist"') self.assert_(u'OK' in result) @@ -64,44 +126,85 @@ class MusicDatabaseHandlerTest(unittest.TestCase): result = self.h.handle_request(u'list Artist') self.assert_(u'OK' in result) - def test_list_artist_with_artist_should_fail(self): + def test_list_artist_with_query_of_one_token(self): result = self.h.handle_request(u'list "artist" "anartist"') - self.assertEqual(result[0], u'ACK [2@0] {list} incorrect arguments') + self.assertEqual(result[0], + u'ACK [2@0] {list} should be "Album" for 3 arguments') - def test_list_album_without_artist(self): + def test_list_artist_with_unknown_field_in_query_returns_ack(self): + result = self.h.handle_request(u'list "artist" "foo" "bar"') + self.assertEqual(result[0], + u'ACK [2@0] {list} not able to parse args') + + ### Album + + def test_list_album_with_quotes(self): result = self.h.handle_request(u'list "album"') self.assert_(u'OK' in result) - def test_list_album_with_artist(self): + def test_list_album_without_quotes(self): + result = self.h.handle_request(u'list album') + self.assert_(u'OK' in result) + + def test_list_album_without_quotes_and_capitalized(self): + result = self.h.handle_request(u'list Album') + self.assert_(u'OK' in result) + + def test_list_album_with_artist_name(self): result = self.h.handle_request(u'list "album" "anartist"') self.assert_(u'OK' in result) - def test_list_album_artist_with_artist_without_quotes(self): - result = self.h.handle_request(u'list album artist "anartist"') + def test_list_album_with_artist_query(self): + result = self.h.handle_request(u'list "album" "artist" "anartist"') self.assert_(u'OK' in result) - def test_listall(self): - result = self.h.handle_request(u'listall "file:///dev/urandom"') - self.assert_(u'ACK [0@0] {} Not implemented' in result) + ### Date - def test_listallinfo(self): - result = self.h.handle_request(u'listallinfo "file:///dev/urandom"') - self.assert_(u'ACK [0@0] {} Not implemented' in result) + def test_list_date_with_quotes(self): + result = self.h.handle_request(u'list "date"') + self.assert_(u'OK' in result) - def test_lsinfo_without_path_returns_same_as_listplaylists(self): - lsinfo_result = self.h.handle_request(u'lsinfo') - listplaylists_result = self.h.handle_request(u'listplaylists') - self.assertEqual(lsinfo_result, listplaylists_result) + def test_list_date_without_quotes(self): + result = self.h.handle_request(u'list date') + self.assert_(u'OK' in result) - def test_lsinfo_with_empty_path_returns_same_as_listplaylists(self): - lsinfo_result = self.h.handle_request(u'lsinfo ""') - listplaylists_result = self.h.handle_request(u'listplaylists') - self.assertEqual(lsinfo_result, listplaylists_result) + def test_list_date_without_quotes_and_capitalized(self): + result = self.h.handle_request(u'list Date') + self.assert_(u'OK' in result) - def test_lsinfo_for_root_returns_same_as_listplaylists(self): - lsinfo_result = self.h.handle_request(u'lsinfo "/"') - listplaylists_result = self.h.handle_request(u'listplaylists') - self.assertEqual(lsinfo_result, listplaylists_result) + def test_list_date_with_query_of_one_token(self): + result = self.h.handle_request(u'list "date" "anartist"') + self.assertEqual(result[0], + u'ACK [2@0] {list} should be "Album" for 3 arguments') + + # TODO Tests for the rest of "list date ..." + + ### Genre + + def test_list_genre_with_quotes(self): + result = self.h.handle_request(u'list "genre"') + self.assert_(u'OK' in result) + + def test_list_genre_without_quotes(self): + result = self.h.handle_request(u'list genre') + self.assert_(u'OK' in result) + + def test_list_genre_without_quotes_and_capitalized(self): + result = self.h.handle_request(u'list Genre') + self.assert_(u'OK' in result) + + def test_list_genre_with_query_of_one_token(self): + result = self.h.handle_request(u'list "genre" "anartist"') + self.assertEqual(result[0], + u'ACK [2@0] {list} should be "Album" for 3 arguments') + + # TODO Tests for the rest of "list genre ..." + + +class MusicDatabaseSearchTest(unittest.TestCase): + def setUp(self): + self.b = DummyBackend(mixer_class=DummyMixer) + self.h = dispatcher.MpdDispatcher(backend=self.b) def test_search_album(self): result = self.h.handle_request(u'search "album" "analbum"') @@ -147,22 +250,4 @@ class MusicDatabaseHandlerTest(unittest.TestCase): result = self.h.handle_request(u'search "sometype" "something"') self.assertEqual(result[0], u'ACK [2@0] {search} incorrect arguments') - def test_update_without_uri(self): - result = self.h.handle_request(u'update') - self.assert_(u'OK' in result) - self.assert_(u'updating_db: 0' in result) - def test_update_with_uri(self): - result = self.h.handle_request(u'update "file:///dev/urandom"') - self.assert_(u'OK' in result) - self.assert_(u'updating_db: 0' in result) - - def test_rescan_without_uri(self): - result = self.h.handle_request(u'rescan') - self.assert_(u'OK' in result) - self.assert_(u'updating_db: 0' in result) - - def test_rescan_with_uri(self): - result = self.h.handle_request(u'rescan "file:///dev/urandom"') - self.assert_(u'OK' in result) - self.assert_(u'updating_db: 0' in result) From 9b73cbb18deba6a5a70c12ab37d342fd03cc8044 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 16:20:25 +0200 Subject: [PATCH 05/40] Add more tests to prove the new 'list' query hendling --- tests/frontends/mpd/music_db_test.py | 98 +++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 3 deletions(-) diff --git a/tests/frontends/mpd/music_db_test.py b/tests/frontends/mpd/music_db_test.py index 408961b2..05b8ebd0 100644 --- a/tests/frontends/mpd/music_db_test.py +++ b/tests/frontends/mpd/music_db_test.py @@ -136,6 +136,31 @@ class MusicDatabaseListTest(unittest.TestCase): self.assertEqual(result[0], u'ACK [2@0] {list} not able to parse args') + def test_list_artist_by_artist(self): + result = self.h.handle_request(u'list "artist" "artist" "anartist"') + self.assert_(u'OK' in result) + + def test_list_artist_by_album(self): + result = self.h.handle_request(u'list "artist" "album" "analbum"') + self.assert_(u'OK' in result) + + def test_list_artist_by_full_date(self): + result = self.h.handle_request(u'list "artist" "date" "2001-01-01"') + self.assert_(u'OK' in result) + + def test_list_artist_by_year(self): + result = self.h.handle_request(u'list "artist" "date" "2001"') + self.assert_(u'OK' in result) + + def test_list_artist_by_genre(self): + result = self.h.handle_request(u'list "artist" "genre" "agenre"') + self.assert_(u'OK' in result) + + def test_list_artist_by_artist_and_album(self): + result = self.h.handle_request( + u'list "artist" "artist" "anartist" "album" "analbum"') + self.assert_(u'OK' in result) + ### Album def test_list_album_with_quotes(self): @@ -154,10 +179,31 @@ class MusicDatabaseListTest(unittest.TestCase): result = self.h.handle_request(u'list "album" "anartist"') self.assert_(u'OK' in result) - def test_list_album_with_artist_query(self): + def test_list_album_by_artist(self): result = self.h.handle_request(u'list "album" "artist" "anartist"') self.assert_(u'OK' in result) + def test_list_album_by_album(self): + result = self.h.handle_request(u'list "album" "album" "analbum"') + self.assert_(u'OK' in result) + + def test_list_album_by_full_date(self): + result = self.h.handle_request(u'list "album" "date" "2001-01-01"') + self.assert_(u'OK' in result) + + def test_list_album_by_year(self): + result = self.h.handle_request(u'list "album" "date" "2001"') + self.assert_(u'OK' in result) + + def test_list_album_by_genre(self): + result = self.h.handle_request(u'list "album" "genre" "agenre"') + self.assert_(u'OK' in result) + + def test_list_album_by_artist_and_album(self): + result = self.h.handle_request( + u'list "album" "artist" "anartist" "album" "analbum"') + self.assert_(u'OK' in result) + ### Date def test_list_date_with_quotes(self): @@ -177,7 +223,30 @@ class MusicDatabaseListTest(unittest.TestCase): self.assertEqual(result[0], u'ACK [2@0] {list} should be "Album" for 3 arguments') - # TODO Tests for the rest of "list date ..." + def test_list_date_by_artist(self): + result = self.h.handle_request(u'list "date" "artist" "anartist"') + self.assert_(u'OK' in result) + + def test_list_date_by_album(self): + result = self.h.handle_request(u'list "date" "album" "analbum"') + self.assert_(u'OK' in result) + + def test_list_date_by_full_date(self): + result = self.h.handle_request(u'list "date" "date" "2001-01-01"') + self.assert_(u'OK' in result) + + def test_list_date_by_year(self): + result = self.h.handle_request(u'list "date" "date" "2001"') + self.assert_(u'OK' in result) + + def test_list_date_by_genre(self): + result = self.h.handle_request(u'list "date" "genre" "agenre"') + self.assert_(u'OK' in result) + + def test_list_date_by_artist_and_album(self): + result = self.h.handle_request( + u'list "date" "artist" "anartist" "album" "analbum"') + self.assert_(u'OK' in result) ### Genre @@ -198,7 +267,30 @@ class MusicDatabaseListTest(unittest.TestCase): self.assertEqual(result[0], u'ACK [2@0] {list} should be "Album" for 3 arguments') - # TODO Tests for the rest of "list genre ..." + def test_list_genre_by_artist(self): + result = self.h.handle_request(u'list "genre" "artist" "anartist"') + self.assert_(u'OK' in result) + + def test_list_genre_by_album(self): + result = self.h.handle_request(u'list "genre" "album" "analbum"') + self.assert_(u'OK' in result) + + def test_list_genre_by_full_date(self): + result = self.h.handle_request(u'list "genre" "date" "2001-01-01"') + self.assert_(u'OK' in result) + + def test_list_genre_by_year(self): + result = self.h.handle_request(u'list "genre" "date" "2001"') + self.assert_(u'OK' in result) + + def test_list_genre_by_genre(self): + result = self.h.handle_request(u'list "genre" "genre" "agenre"') + self.assert_(u'OK' in result) + + def test_list_genre_by_artist_and_album(self): + result = self.h.handle_request( + u'list "genre" "artist" "anartist" "album" "analbum"') + self.assert_(u'OK' in result) class MusicDatabaseSearchTest(unittest.TestCase): From dafd5ac9ecc6e9f863e712d46b9b46a8d308f324 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 16:59:14 +0200 Subject: [PATCH 06/40] Add 'list date' support --- mopidy/frontends/mpd/protocol/music_db.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/music_db.py b/mopidy/frontends/mpd/protocol/music_db.py index 2bccab3d..4c2031aa 100644 --- a/mopidy/frontends/mpd/protocol/music_db.py +++ b/mopidy/frontends/mpd/protocol/music_db.py @@ -175,9 +175,9 @@ def list_(frontend, field, mpd_query=None): elif field == u'album': return _list_album(frontend, query) elif field == u'date': - pass # TODO + return _list_date(frontend, query) elif field == u'genre': - pass # TODO + pass # TODO We don't have genre in our internal data structures yet def _list_build_query(field, mpd_query): """Converts a ``list`` query to a Mopidy query.""" @@ -224,6 +224,14 @@ def _list_album(frontend, query): albums.add((u'Album', track.album.name)) return albums +def _list_date(frontend, query): + dates = set() + playlist = frontend.backend.library.find_exact(**query) + for track in playlist.tracks: + if track.date is not None: + dates.add((u'Date', track.date.strftime('%Y-%m-%d'))) + return dates + @handle_pattern(r'^listall "(?P[^"]+)"') def listall(frontend, uri): """ From 5f95ebf9dcd2ca051611c3b338755c6c2efb3a7a Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Aug 2010 17:00:09 +0200 Subject: [PATCH 07/40] Add 'year:1997' search filter support to libspotify backend --- mopidy/backends/libspotify/library.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/mopidy/backends/libspotify/library.py b/mopidy/backends/libspotify/library.py index 68512ffa..7e545af8 100644 --- a/mopidy/backends/libspotify/library.py +++ b/mopidy/backends/libspotify/library.py @@ -33,13 +33,18 @@ class LibspotifyLibraryController(BaseLibraryController): return Playlist(tracks=tracks) spotify_query = [] for (field, values) in query.iteritems(): + if field == u'track': + field = u'title' + if field == u'date': + field = u'year' if not hasattr(values, '__iter__'): values = [values] for value in values: - if field == u'track': - field = u'title' if field == u'any': spotify_query.append(value) + elif field == u'year': + value = int(value.split('-')[0]) # Extract year + spotify_query.append(u'%s:%d' % (field, value)) else: spotify_query.append(u'%s:"%s"' % (field, value)) spotify_query = u' '.join(spotify_query) From 4ad476e1e5442e8b4a1c70960e72789b2badc419 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 12 Sep 2010 16:38:18 +0200 Subject: [PATCH 08/40] Fix '[Errno 22] Invalid argument' caused by IPv6 socket without IPv4 support --- mopidy/frontends/mpd/server.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mopidy/frontends/mpd/server.py b/mopidy/frontends/mpd/server.py index db13e516..39a0e682 100644 --- a/mopidy/frontends/mpd/server.py +++ b/mopidy/frontends/mpd/server.py @@ -24,6 +24,9 @@ class MpdServer(asyncore.dispatcher): try: if socket.has_ipv6: self.create_socket(socket.AF_INET6, socket.SOCK_STREAM) + # Explicitly configure socket to work for both IPv4 and IPv6 + self.socket.setsockopt( + socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) else: self.create_socket(socket.AF_INET, socket.SOCK_STREAM) self.set_reuse_addr() From 690cddf451ee22d879a48932e6d6f74f149d855c Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 4 Oct 2010 22:30:49 +0200 Subject: [PATCH 09/40] Update changelog after merge of feature/fix-mpd-list --- docs/changes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index dadbb6b9..fc6fc165 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -30,6 +30,8 @@ No description yet. - MPD frontend: - ``add ""`` and ``addid ""`` now behaves as expected. + - ``list`` now supports queries by artist, album name, and date, as used by + e.g. the Ario client. (Fixes: :issue:`20`) 0.1.0 (2010-08-23) From 5cdfbce122ec0248104049760dadc1c83a01f7fb Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 4 Oct 2010 23:18:32 +0200 Subject: [PATCH 10/40] Add regression test for GH-18 --- tests/frontends/mpd/regression_test.py | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 tests/frontends/mpd/regression_test.py diff --git a/tests/frontends/mpd/regression_test.py b/tests/frontends/mpd/regression_test.py new file mode 100644 index 00000000..0959e2a4 --- /dev/null +++ b/tests/frontends/mpd/regression_test.py @@ -0,0 +1,41 @@ +import unittest + +from mopidy.backends.dummy import DummyBackend +from mopidy.frontends.mpd import dispatcher +from mopidy.mixers.dummy import DummyMixer +from mopidy.models import Track + +class IssueGH18RegressionTest(unittest.TestCase): + """ + The issue: http://github.com/jodal/mopidy/issues#issue/18 + + How to reproduce: + + Play, random on, next, random off, next, next. + + At this point it gives the same song over and over. + """ + + 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): + self.mpd.handle_request(u'play') + self.mpd.handle_request(u'random "1"') + self.mpd.handle_request(u'next') + self.mpd.handle_request(u'random "0"') + self.mpd.handle_request(u'next') + + self.mpd.handle_request(u'next') + cp_track_1 = self.backend.playback.current_cp_track + self.mpd.handle_request(u'next') + cp_track_2 = self.backend.playback.current_cp_track + self.mpd.handle_request(u'next') + cp_track_3 = self.backend.playback.current_cp_track + + self.assertNotEqual(cp_track_1, cp_track_2) + self.assertNotEqual(cp_track_2, cp_track_3) From 428436681f58777d7c6bf9dab8a5bab670493a26 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 16 Oct 2010 15:32:39 +0200 Subject: [PATCH 11/40] libspotify: The search callback may be called without userdata --- mopidy/backends/libspotify/session_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/backends/libspotify/session_manager.py b/mopidy/backends/libspotify/session_manager.py index 9554fa3f..b3e71c27 100644 --- a/mopidy/backends/libspotify/session_manager.py +++ b/mopidy/backends/libspotify/session_manager.py @@ -97,7 +97,7 @@ class LibspotifySessionManager(SpotifySessionManager, BaseThread): def search(self, query, connection): """Search method used by Mopidy backend""" - def callback(results, userdata): + def callback(results, userdata=None): # TODO Include results from results.albums(), etc. too playlist = Playlist(tracks=[ LibspotifyTranslator.to_mopidy_track(t) From c4e277a5fd25a39b0a79ea95e2b7ce1296a46bb0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 17 Oct 2010 20:03:57 +0200 Subject: [PATCH 12/40] Tweak log message --- mopidy/frontends/lastfm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/frontends/lastfm.py b/mopidy/frontends/lastfm.py index bba69a5b..42dd16c7 100644 --- a/mopidy/frontends/lastfm.py +++ b/mopidy/frontends/lastfm.py @@ -84,7 +84,7 @@ class LastfmFrontendThread(BaseThread): CLIENT_ID, CLIENT_VERSION) logger.info(u'Connected to Last.fm') except SettingsError as e: - logger.info(u'Last.fm scrobbler did not start.') + logger.info(u'Last.fm scrobbler not started') logger.debug(u'Last.fm settings error: %s', e) except (pylast.WSError, socket.error) as e: logger.error(u'Last.fm connection error: %s', e) From e69f168819cd33162a8406cb37b7dd974da21cb0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 17 Oct 2010 21:08:24 +0200 Subject: [PATCH 13/40] GstreamerOutput: name appsrc 'appsrc' instead of 'src' to not confuse with src pads --- mopidy/outputs/gstreamer.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mopidy/outputs/gstreamer.py b/mopidy/outputs/gstreamer.py index 346f6254..ebcf0ccf 100644 --- a/mopidy/outputs/gstreamer.py +++ b/mopidy/outputs/gstreamer.py @@ -142,7 +142,7 @@ class GStreamerPlayerThread(BaseThread): uri_bin.connect('pad-added', self.process_new_pad, pad) self.gst_pipeline.add(uri_bin) else: - app_src = gst.element_factory_make('appsrc', 'src') + app_src = gst.element_factory_make('appsrc', 'appsrc') self.gst_pipeline.add(app_src) app_src.get_pad('src').link(pad) @@ -208,12 +208,12 @@ class GStreamerPlayerThread(BaseThread): def deliver_data(self, caps_string, data): """Deliver audio data to be played""" - data_src = self.gst_pipeline.get_by_name('src') + app_src = self.gst_pipeline.get_by_name('appsrc') caps = gst.caps_from_string(caps_string) buffer_ = gst.Buffer(buffer(data)) buffer_.set_caps(caps) - data_src.set_property('caps', caps) - data_src.emit('push-buffer', buffer_) + app_src.set_property('caps', caps) + app_src.emit('push-buffer', buffer_) def end_of_data_stream(self): """ @@ -222,7 +222,7 @@ class GStreamerPlayerThread(BaseThread): We will get a GStreamer message when the stream playback reaches the token, and can then do any end-of-stream related tasks. """ - self.gst_pipeline.get_by_name('src').emit('end-of-stream') + self.gst_pipeline.get_by_name('appsrc').emit('end-of-stream') def set_state(self, state_name): """ From 88636982aecac804d5683b6991d2d225825044e7 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 18 Oct 2010 23:15:30 +0200 Subject: [PATCH 14/40] Use sample_rate and channels from libspotify in buffer caps, assert that the sample_type doesn't change --- mopidy/backends/libspotify/session_manager.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mopidy/backends/libspotify/session_manager.py b/mopidy/backends/libspotify/session_manager.py index b3e71c27..3b6f9f0b 100644 --- a/mopidy/backends/libspotify/session_manager.py +++ b/mopidy/backends/libspotify/session_manager.py @@ -69,16 +69,19 @@ class LibspotifySessionManager(SpotifySessionManager, BaseThread): def music_delivery(self, session, frames, frame_size, num_frames, sample_type, sample_rate, channels): """Callback used by pyspotify""" - # TODO Base caps_string on arguments + assert sample_type == 0, u'Expects 16-bit signed integer samples' capabilites = """ audio/x-raw-int, endianness=(int)1234, - channels=(int)2, + channels=(int)%(channels)d, width=(int)16, depth=(int)16, - signed=True, - rate=(int)44100 - """ + signed=(boolean)true, + rate=(int)%(sample_rate)d + """ % { + 'sample_rate': sample_rate, + 'channels': channels, + } self.output.deliver_data(capabilites, bytes(frames)) def play_token_lost(self, session): From 5bdab113ce29f3039a7f10173c11115570b209d9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 18 Oct 2010 23:28:01 +0200 Subject: [PATCH 15/40] Limit caps on appsrc early on. Fixes sound on Ubuntu 10.10 --- mopidy/outputs/gstreamer.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/mopidy/outputs/gstreamer.py b/mopidy/outputs/gstreamer.py index ebcf0ccf..a53fcd20 100644 --- a/mopidy/outputs/gstreamer.py +++ b/mopidy/outputs/gstreamer.py @@ -143,6 +143,15 @@ class GStreamerPlayerThread(BaseThread): self.gst_pipeline.add(uri_bin) else: app_src = gst.element_factory_make('appsrc', 'appsrc') + app_src_caps = gst.Caps(""" + audio/x-raw-int, + endianness=(int)1234, + channels=(int)2, + width=(int)16, + depth=(int)16, + signed=(boolean)true, + rate=(int)44100""") + app_src.set_property('caps', app_src_caps) self.gst_pipeline.add(app_src) app_src.get_pad('src').link(pad) From 0d613418e6d0ee777e2cb86aa5c113e4b7ca1a8a Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 19 Oct 2010 12:23:05 +0200 Subject: [PATCH 16/40] Remove cleanup taken care of by play() --- mopidy/backends/base/playback.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 933424ad..3f1fa4fb 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -311,11 +311,9 @@ class BasePlaybackController(object): return original_cp_track = self.current_cp_track + if self.cp_track_at_eot: self.play(self.cp_track_at_eot) - - if self.random and self.current_cp_track in self._shuffled: - self._shuffled.remove(self.current_cp_track) else: self.stop() self.current_cp_track = None @@ -351,9 +349,6 @@ class BasePlaybackController(object): self.stop() self.current_cp_track = None - if self.random and self.current_cp_track in self._shuffled: - self._shuffled.remove(self.current_cp_track) - def pause(self): """Pause playback.""" if self.state == self.PLAYING and self._pause(): From 3a951ca948ec0d179cdf1fb1a6a5d47f90ef318b Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 19 Oct 2010 12:41:18 +0200 Subject: [PATCH 17/40] Seed the random function to make the test predictable --- tests/frontends/mpd/regression_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/frontends/mpd/regression_test.py b/tests/frontends/mpd/regression_test.py index 0959e2a4..0e8ca386 100644 --- a/tests/frontends/mpd/regression_test.py +++ b/tests/frontends/mpd/regression_test.py @@ -1,3 +1,4 @@ +import random import unittest from mopidy.backends.dummy import DummyBackend @@ -24,6 +25,7 @@ class IssueGH18RegressionTest(unittest.TestCase): 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'next') From 9188dcdd4b385cf91c5faed13ba084eee7fe2700 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 19 Oct 2010 12:42:06 +0200 Subject: [PATCH 18/40] Fix GH-18 by only using the internal shuffled playlist when random mode is on --- mopidy/backends/base/playback.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 3f1fa4fb..3a32ce07 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -142,7 +142,7 @@ class BasePlaybackController(object): random.shuffle(self._shuffled) self._first_shuffle = False - if self._shuffled: + if self.random and self._shuffled: return self._shuffled[0] if self.current_cp_track is None: @@ -195,7 +195,7 @@ class BasePlaybackController(object): random.shuffle(self._shuffled) self._first_shuffle = False - if self._shuffled: + if self.random and self._shuffled: return self._shuffled[0] if self.current_cp_track is None: From d975945eb063fcaa6169ccba6e710b78181b5766 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 19 Oct 2010 12:47:06 +0200 Subject: [PATCH 19/40] Update changelog with GH-18 fix --- docs/changes.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index d061f892..7714408f 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -16,6 +16,9 @@ No description yet. - ``add ""`` and ``addid ""`` now behaves as expected. +- Fix wrong behavior on end of track and next after random mode has been used. + (Fixes: :issue:`18`) + 0.1.0 (2010-08-23) ================== From f8885e3bb50e2089802cf75d05493c842eedd015 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 19 Oct 2010 20:16:27 +0200 Subject: [PATCH 20/40] Add link to develop tarball to PyPI long_description to get support for 'pip install mopidy==dev' --- README.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 6855135e..c6187119 100644 --- a/README.rst +++ b/README.rst @@ -11,8 +11,9 @@ platforms, including Windows, Mac OS X, Linux, and iPhone and Android phones. To install Mopidy, check out `the installation docs `_. -* `Documentation `_ +* `Documentation (latest release) `_ * `Documentation (development version) `_ * `Source code `_ * `Issue tracker `_ * IRC: ``#mopidy`` at `irc.freenode.net `_ +* `Download development snapshot `_ From ada3fcd72631ab08bdd3dce265fb8fff19047cab Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 22:01:18 +0200 Subject: [PATCH 21/40] Extend DummyPlaybackController to be able to return False on _play, _next, _previous --- mopidy/backends/dummy/__init__.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/mopidy/backends/dummy/__init__.py b/mopidy/backends/dummy/__init__.py index 98257f18..a9c00631 100644 --- a/mopidy/backends/dummy/__init__.py +++ b/mopidy/backends/dummy/__init__.py @@ -44,16 +44,19 @@ class DummyLibraryController(BaseLibraryController): class DummyPlaybackController(BasePlaybackController): def _next(self, track): - return True + """Pass None as track to force failure""" + return track is not None def _pause(self): return True def _play(self, track): - return True + """Pass None as track to force failure""" + return track is not None def _previous(self, track): - return True + """Pass None as track to force failure""" + return track is not None def _resume(self): return True From e2a4aaada71c64ef1ffcb48de80ae9e3ea154bce Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 22:01:34 +0200 Subject: [PATCH 22/40] Add regression test for GH-17 --- tests/frontends/mpd/regression_test.py | 34 ++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/frontends/mpd/regression_test.py b/tests/frontends/mpd/regression_test.py index 0e8ca386..29656ac4 100644 --- a/tests/frontends/mpd/regression_test.py +++ b/tests/frontends/mpd/regression_test.py @@ -6,6 +6,40 @@ from mopidy.frontends.mpd import dispatcher from mopidy.mixers.dummy import DummyMixer from mopidy.models import Track +class IssueGH17RegressionTest(unittest.TestCase): + """ + The issue: http://github.com/jodal/mopidy/issues#issue/17 + + How to reproduce: + + - Play a playlist where one track cannot be played + - Turn on random mode + - Press next until you get to the unplayable track + """ + + def setUp(self): + self.backend = DummyBackend(mixer_class=DummyMixer) + self.backend.current_playlist.append([ + Track(uri='a'), Track(uri='b'), None, + Track(uri='d'), Track(uri='e'), Track(uri='f')]) + self.mpd = dispatcher.MpdDispatcher(backend=self.backend) + + def test(self): + random.seed(1) # Playlist order: abcfde + self.mpd.handle_request(u'play') + self.assertEquals('a', self.backend.playback.current_track.uri) + self.mpd.handle_request(u'random "1"') + self.mpd.handle_request(u'next') + self.assertEquals('b', self.backend.playback.current_track.uri) + self.mpd.handle_request(u'next') + # Should now be at track 'c', but playback fails and it skips ahead + self.assertEquals('f', self.backend.playback.current_track.uri) + self.mpd.handle_request(u'next') + self.assertEquals('d', self.backend.playback.current_track.uri) + self.mpd.handle_request(u'next') + self.assertEquals('e', self.backend.playback.current_track.uri) + + class IssueGH18RegressionTest(unittest.TestCase): """ The issue: http://github.com/jodal/mopidy/issues#issue/18 From cc4abf509794010b3cbd008fe9f81f4c10d915a5 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 22:07:27 +0200 Subject: [PATCH 23/40] Add fix for GH-17 --- mopidy/backends/base/playback.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 3a32ce07..aead26af 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -387,6 +387,9 @@ class BasePlaybackController(object): self.current_cp_track = cp_track self.state = self.PLAYING if not self._play(cp_track[1]): + # Track is not playable + if self.random and self._shuffled: + self._shuffled.remove(cp_track) if on_error_step == 1: self.next() elif on_error_step == -1: From c457ae644b916a45a9178a306056a72d81acd5b6 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 22:24:34 +0200 Subject: [PATCH 24/40] Update changelog --- docs/changes.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index 7714408f..91ac4531 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -12,12 +12,12 @@ No description yet. **Changes** -- MPD frontend: - - - ``add ""`` and ``addid ""`` now behaves as expected. - +- MPD frontend: ``add ""`` and ``addid ""`` now behaves as expected. (Fixes + :issue:`16`) - Fix wrong behavior on end of track and next after random mode has been used. (Fixes: :issue:`18`) +- Fix infinite recursion loop crash on playback of non-playable tracks when in + random mode. (Fixes :issue:`17`) 0.1.0 (2010-08-23) From 9895afbfd358d87ed42162b48524d639ae48afe2 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 22:34:22 +0200 Subject: [PATCH 25/40] Merge changes lists, as we won't do a 0.1.1 release --- docs/changes.rst | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index ea25d041..61089e24 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -27,18 +27,9 @@ No description yet. :attr:`mopidy.settings.DEBUG_LOG_FILENAME`. - Switched from using subprocesses to threads. This partly fixes the OS X support. See :issue:`14` for details. -- MPD frontend: ``list`` now supports queries by artist, album name, and date, - as used by e.g. the Ario client. (Fixes: :issue:`20`) - - -0.1.1 (in development) -====================== - -No description yet. - -**Changes** - -- MPD frontend: ``add ""`` and ``addid ""`` now behaves as expected. (Fixes +- MPD command ``list`` now supports queries by artist, album name, and date, as + used by e.g. the Ario client. (Fixes: :issue:`20`) +- MPD command ``add ""`` and ``addid ""`` now behaves as expected. (Fixes :issue:`16`) - Fix wrong behavior on end of track and next after random mode has been used. (Fixes: :issue:`18`) From 3cfc282acca31d781781007db6c7650be8ec48b0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:11:36 +0200 Subject: [PATCH 26/40] 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 27/40] 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 28/40] 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 29/40] 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 30/40] 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 31/40] 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): """ From ea654e3aa67f1e3ac8e2bf5b6d9a40c29f442248 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:42:26 +0200 Subject: [PATCH 32/40] Update changelog --- docs/changes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index 61089e24..db5c24a6 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -35,6 +35,8 @@ No description yet. (Fixes: :issue:`18`) - Fix infinite recursion loop crash on playback of non-playable tracks when in random mode. (Fixes :issue:`17`) +- Fix assertion error that happened if one removed tracks from the current + playlist, while in random mode. (Fixes :issue:`22`) 0.1.0 (2010-08-23) From 733db5d24121de56b6ddc6124abbff53587079e9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:53:43 +0200 Subject: [PATCH 33/40] Add GH-21/24 fix to changelog --- docs/changes.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index db5c24a6..ed3cb080 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -37,6 +37,10 @@ No description yet. random mode. (Fixes :issue:`17`) - Fix assertion error that happened if one removed tracks from the current playlist, while in random mode. (Fixes :issue:`22`) +- GStreamerOutput: Set ``caps`` on the ``appsrc`` bin before use. This makes + sound output work with GStreamer >= 0.10.29, which includes the versions used + in Ubuntu 10.10 and on OS X if using Homebrew. (Fixes: :issue:`21`, + :issue:`24`, contributes to :issue:`14`) 0.1.0 (2010-08-23) From 1734a2e2f015341608bb7a2c6908082a2a465728 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 21 Oct 2010 23:56:26 +0200 Subject: [PATCH 34/40] Next version will be 0.2.0 --- mopidy/__init__.py | 2 +- tests/version_test.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/mopidy/__init__.py b/mopidy/__init__.py index 7d3052c4..5e1b26de 100644 --- a/mopidy/__init__.py +++ b/mopidy/__init__.py @@ -3,7 +3,7 @@ if not (2, 6) <= sys.version_info < (3,): sys.exit(u'Mopidy requires Python >= 2.6, < 3') def get_version(): - return u'0.2.0a1' + return u'0.2.0' class MopidyException(Exception): def __init__(self, message, *args, **kwargs): diff --git a/tests/version_test.py b/tests/version_test.py index b2ef1fce..fcc95c4c 100644 --- a/tests/version_test.py +++ b/tests/version_test.py @@ -13,6 +13,5 @@ class VersionTest(unittest.TestCase): self.assert_(SV('0.1.0a2') < SV('0.1.0a3')) self.assert_(SV('0.1.0a3') < SV('0.1.0')) self.assert_(SV('0.1.0') < SV(get_version())) - self.assert_(SV(get_version()) < SV('0.2.0')) - self.assert_(SV('0.1.1') < SV('0.2.0')) + self.assert_(SV(get_version()) < SV('0.2.1')) self.assert_(SV('0.2.0') < SV('1.0.0')) From f5903e9aa7023c12f9cf23ed47cdb5f69fcd0c01 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 23 Oct 2010 22:11:33 +0200 Subject: [PATCH 35/40] GH-26: Use 'string' as dict key --- mopidy/frontends/mpd/protocol/music_db.py | 1 + 1 file changed, 1 insertion(+) diff --git a/mopidy/frontends/mpd/protocol/music_db.py b/mopidy/frontends/mpd/protocol/music_db.py index 4c2031aa..fb3a3a09 100644 --- a/mopidy/frontends/mpd/protocol/music_db.py +++ b/mopidy/frontends/mpd/protocol/music_db.py @@ -196,6 +196,7 @@ def _list_build_query(field, mpd_query): query = {} while tokens: key = tokens[0].lower() + key = str(key) # Needed for kwargs keys on OS X and Windows value = tokens[1] tokens = tokens[2:] if key not in (u'artist', u'album', u'date', u'genre'): From b3fea05ef02f9db817fee745451897b363f84d89 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 24 Oct 2010 17:37:48 +0200 Subject: [PATCH 36/40] Use BaseThread instead of BaseProcess everywhere --- mopidy/core.py | 4 ++-- mopidy/mixers/nad.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/mopidy/core.py b/mopidy/core.py index 5351e2a5..885fd105 100644 --- a/mopidy/core.py +++ b/mopidy/core.py @@ -6,12 +6,12 @@ from mopidy import get_version, settings, OptionalDependencyError from mopidy.utils import get_class from mopidy.utils.log import setup_logging from mopidy.utils.path import get_or_create_folder, get_or_create_file -from mopidy.utils.process import BaseProcess +from mopidy.utils.process import BaseThread from mopidy.utils.settings import list_settings_optparse_callback logger = logging.getLogger('mopidy.core') -class CoreProcess(BaseProcess): +class CoreProcess(BaseThread): def __init__(self): super(CoreProcess, self).__init__(name='CoreProcess') self.core_queue = multiprocessing.Queue() diff --git a/mopidy/mixers/nad.py b/mopidy/mixers/nad.py index 929d2e1d..7a8f006e 100644 --- a/mopidy/mixers/nad.py +++ b/mopidy/mixers/nad.py @@ -4,7 +4,7 @@ from multiprocessing import Pipe from mopidy import settings from mopidy.mixers import BaseMixer -from mopidy.utils.process import BaseProcess +from mopidy.utils.process import BaseThread logger = logging.getLogger('mopidy.mixers.nad') @@ -50,7 +50,7 @@ class NadMixer(BaseMixer): self._pipe.send({'command': 'set_volume', 'volume': volume}) -class NadTalker(BaseProcess): +class NadTalker(BaseThread): """ Independent process which does the communication with the NAD device. From 11e48083ee0f3d938c59a69e081296c67571159d Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 24 Oct 2010 19:35:04 +0200 Subject: [PATCH 37/40] Give all threads a reference to core_queue --- mopidy/backends/libspotify/session_manager.py | 2 +- mopidy/core.py | 3 ++- mopidy/frontends/lastfm.py | 6 +++--- mopidy/frontends/mpd/thread.py | 2 +- mopidy/outputs/gstreamer.py | 8 ++++---- mopidy/utils/process.py | 8 ++++++++ 6 files changed, 19 insertions(+), 10 deletions(-) diff --git a/mopidy/backends/libspotify/session_manager.py b/mopidy/backends/libspotify/session_manager.py index 3b6f9f0b..61780166 100644 --- a/mopidy/backends/libspotify/session_manager.py +++ b/mopidy/backends/libspotify/session_manager.py @@ -19,7 +19,7 @@ class LibspotifySessionManager(SpotifySessionManager, BaseThread): def __init__(self, username, password, core_queue, output): SpotifySessionManager.__init__(self, username, password) - BaseThread.__init__(self) + BaseThread.__init__(self, core_queue) self.name = 'LibspotifySMThread' # Run as a daemon thread, so Mopidy won't wait for this thread to exit # before Mopidy exits. diff --git a/mopidy/core.py b/mopidy/core.py index 885fd105..f54a3826 100644 --- a/mopidy/core.py +++ b/mopidy/core.py @@ -13,8 +13,9 @@ logger = logging.getLogger('mopidy.core') class CoreProcess(BaseThread): def __init__(self): - super(CoreProcess, self).__init__(name='CoreProcess') self.core_queue = multiprocessing.Queue() + super(CoreProcess, self).__init__(self.core_queue) + self.name = 'CoreProcess' self.options = self.parse_options() self.output = None self.backend = None diff --git a/mopidy/frontends/lastfm.py b/mopidy/frontends/lastfm.py index 42dd16c7..a227aa0e 100644 --- a/mopidy/frontends/lastfm.py +++ b/mopidy/frontends/lastfm.py @@ -45,7 +45,7 @@ class LastfmFrontend(BaseFrontend): def __init__(self, *args, **kwargs): super(LastfmFrontend, self).__init__(*args, **kwargs) (self.connection, other_end) = multiprocessing.Pipe() - self.thread = LastfmFrontendThread(other_end) + self.thread = LastfmFrontendThread(self.core_queue, other_end) def start(self): self.thread.start() @@ -58,8 +58,8 @@ class LastfmFrontend(BaseFrontend): class LastfmFrontendThread(BaseThread): - def __init__(self, connection): - super(LastfmFrontendThread, self).__init__() + def __init__(self, core_queue, connection): + super(LastfmFrontendThread, self).__init__(core_queue) self.name = u'LastfmFrontendThread' self.daemon = True self.connection = connection diff --git a/mopidy/frontends/mpd/thread.py b/mopidy/frontends/mpd/thread.py index 0fb048ec..e8f0be70 100644 --- a/mopidy/frontends/mpd/thread.py +++ b/mopidy/frontends/mpd/thread.py @@ -8,7 +8,7 @@ logger = logging.getLogger('mopidy.frontends.mpd.thread') class MpdThread(BaseThread): def __init__(self, core_queue): - super(MpdThread, self).__init__() + super(MpdThread, self).__init__(core_queue) self.name = u'MpdThread' self.daemon = True self.core_queue = core_queue diff --git a/mopidy/outputs/gstreamer.py b/mopidy/outputs/gstreamer.py index a53fcd20..513f5f82 100644 --- a/mopidy/outputs/gstreamer.py +++ b/mopidy/outputs/gstreamer.py @@ -29,7 +29,7 @@ class GStreamerOutput(BaseOutput): def __init__(self, *args, **kwargs): super(GStreamerOutput, self).__init__(*args, **kwargs) # Start a helper thread that can run the gobject.MainLoop - self.messages_thread = GStreamerMessagesThread() + self.messages_thread = GStreamerMessagesThread(self.core_queue) # Start a helper thread that can process the output_queue self.output_queue = multiprocessing.Queue() @@ -91,8 +91,8 @@ class GStreamerOutput(BaseOutput): class GStreamerMessagesThread(BaseThread): - def __init__(self): - super(GStreamerMessagesThread, self).__init__() + def __init__(self, core_queue): + super(GStreamerMessagesThread, self).__init__(core_queue) self.name = u'GStreamerMessagesThread' self.daemon = True @@ -113,7 +113,7 @@ class GStreamerPlayerThread(BaseThread): """ def __init__(self, core_queue, output_queue): - super(GStreamerPlayerThread, self).__init__() + super(GStreamerPlayerThread, self).__init__(core_queue) self.name = u'GStreamerPlayerThread' self.daemon = True self.core_queue = core_queue diff --git a/mopidy/utils/process.py b/mopidy/utils/process.py index 0acccb4d..e4ef2484 100644 --- a/mopidy/utils/process.py +++ b/mopidy/utils/process.py @@ -19,6 +19,10 @@ def unpickle_connection(pickled_connection): class BaseProcess(multiprocessing.Process): + def __init__(self, core_queue): + super(BaseProcess, self).__init__() + self.core_queue = core_queue + def run(self): logger.debug(u'%s: Starting process', self.name) try: @@ -44,6 +48,10 @@ class BaseProcess(multiprocessing.Process): class BaseThread(multiprocessing.dummy.Process): + def __init__(self, core_queue): + super(BaseThread, self).__init__() + self.core_queue = core_queue + def run(self): logger.debug(u'%s: Starting thread', self.name) try: From a10c36d8ec1464b3dec0931261aede2f8efe18bb Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 24 Oct 2010 19:52:13 +0200 Subject: [PATCH 38/40] When catching an exception, ask main thread to sys.exit --- mopidy/core.py | 14 +++++++++++++- mopidy/utils/process.py | 30 ++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/mopidy/core.py b/mopidy/core.py index f54a3826..69760094 100644 --- a/mopidy/core.py +++ b/mopidy/core.py @@ -1,6 +1,7 @@ import logging import multiprocessing import optparse +import sys from mopidy import get_version, settings, OptionalDependencyError from mopidy.utils import get_class @@ -80,7 +81,9 @@ class CoreProcess(BaseThread): return frontends def process_message(self, message): - if message.get('to') == 'output': + if message.get('to') == 'core': + self.process_message_to_core(message) + elif message.get('to') == 'output': self.output.process_message(message) elif message.get('to') == 'frontend': for frontend in self.frontends: @@ -93,3 +96,12 @@ class CoreProcess(BaseThread): self.backend.stored_playlists.playlists = message['playlists'] else: logger.warning(u'Cannot handle message: %s', message) + + def process_message_to_core(self, message): + assert message['to'] == 'core', u'Message recipient must be "core".' + if message['command'] == 'exit': + if message['reason'] is not None: + logger.info(u'Exiting (%s)', message['reason']) + sys.exit(message['status']) + else: + logger.warning(u'Cannot handle message: %s', message) diff --git a/mopidy/utils/process.py b/mopidy/utils/process.py index e4ef2484..9af6fbf5 100644 --- a/mopidy/utils/process.py +++ b/mopidy/utils/process.py @@ -28,17 +28,17 @@ class BaseProcess(multiprocessing.Process): try: self.run_inside_try() except KeyboardInterrupt: - logger.info(u'%s: Interrupted by user', self.name) - sys.exit(0) + logger.info(u'Interrupted by user') + self.exit(0, u'Interrupted by user') except SettingsError as e: logger.error(e.message) - sys.exit(1) + self.exit(1, u'Settings error') except ImportError as e: logger.error(e) - sys.exit(1) + self.exit(2, u'Import error') except Exception as e: logger.exception(e) - raise e + self.exit(3, u'Unknown error') def run_inside_try(self): raise NotImplementedError @@ -46,6 +46,11 @@ class BaseProcess(multiprocessing.Process): def destroy(self): self.terminate() + def exit(self, status=0, reason=None): + self.core_queue.put({'to': 'core', 'command': 'exit', + 'status': status, 'reason': reason}) + self.destroy() + class BaseThread(multiprocessing.dummy.Process): def __init__(self, core_queue): @@ -57,20 +62,25 @@ class BaseThread(multiprocessing.dummy.Process): try: self.run_inside_try() except KeyboardInterrupt: - logger.info(u'%s: Interrupted by user', self.name) - sys.exit(0) + logger.info(u'Interrupted by user') + self.exit(0, u'Interrupted by user') except SettingsError as e: logger.error(e.message) - sys.exit(1) + self.exit(1, u'Settings error') except ImportError as e: logger.error(e) - sys.exit(1) + self.exit(2, u'Import error') except Exception as e: logger.exception(e) - raise e + self.exit(3, u'Unknown error') def run_inside_try(self): raise NotImplementedError def destroy(self): pass + + def exit(self, status=0, reason=None): + self.core_queue.put({'to': 'core', 'command': 'exit', + 'status': status, 'reason': reason}) + self.destroy() From 0398193d34d2f17f544b296d32033613284671b1 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 24 Oct 2010 20:07:27 +0200 Subject: [PATCH 39/40] Make all threads daemon threads per default --- mopidy/backends/libspotify/session_manager.py | 4 ---- mopidy/frontends/lastfm.py | 1 - mopidy/frontends/mpd/thread.py | 2 -- mopidy/outputs/gstreamer.py | 3 --- mopidy/utils/process.py | 2 ++ 5 files changed, 2 insertions(+), 10 deletions(-) diff --git a/mopidy/backends/libspotify/session_manager.py b/mopidy/backends/libspotify/session_manager.py index 61780166..7f541236 100644 --- a/mopidy/backends/libspotify/session_manager.py +++ b/mopidy/backends/libspotify/session_manager.py @@ -21,10 +21,6 @@ class LibspotifySessionManager(SpotifySessionManager, BaseThread): SpotifySessionManager.__init__(self, username, password) BaseThread.__init__(self, core_queue) self.name = 'LibspotifySMThread' - # Run as a daemon thread, so Mopidy won't wait for this thread to exit - # before Mopidy exits. - self.daemon = True - self.core_queue = core_queue self.output = output self.connected = threading.Event() self.session = None diff --git a/mopidy/frontends/lastfm.py b/mopidy/frontends/lastfm.py index a227aa0e..e91dd272 100644 --- a/mopidy/frontends/lastfm.py +++ b/mopidy/frontends/lastfm.py @@ -61,7 +61,6 @@ class LastfmFrontendThread(BaseThread): def __init__(self, core_queue, connection): super(LastfmFrontendThread, self).__init__(core_queue) self.name = u'LastfmFrontendThread' - self.daemon = True self.connection = connection self.lastfm = None self.scrobbler = None diff --git a/mopidy/frontends/mpd/thread.py b/mopidy/frontends/mpd/thread.py index e8f0be70..0ad5ee68 100644 --- a/mopidy/frontends/mpd/thread.py +++ b/mopidy/frontends/mpd/thread.py @@ -10,8 +10,6 @@ class MpdThread(BaseThread): def __init__(self, core_queue): super(MpdThread, self).__init__(core_queue) self.name = u'MpdThread' - self.daemon = True - self.core_queue = core_queue def run_inside_try(self): logger.debug(u'Starting MPD server thread') diff --git a/mopidy/outputs/gstreamer.py b/mopidy/outputs/gstreamer.py index 513f5f82..3714fed6 100644 --- a/mopidy/outputs/gstreamer.py +++ b/mopidy/outputs/gstreamer.py @@ -94,7 +94,6 @@ class GStreamerMessagesThread(BaseThread): def __init__(self, core_queue): super(GStreamerMessagesThread, self).__init__(core_queue) self.name = u'GStreamerMessagesThread' - self.daemon = True def run_inside_try(self): gobject.MainLoop().run() @@ -115,8 +114,6 @@ class GStreamerPlayerThread(BaseThread): def __init__(self, core_queue, output_queue): super(GStreamerPlayerThread, self).__init__(core_queue) self.name = u'GStreamerPlayerThread' - self.daemon = True - self.core_queue = core_queue self.output_queue = output_queue self.gst_pipeline = None diff --git a/mopidy/utils/process.py b/mopidy/utils/process.py index 9af6fbf5..7855d69c 100644 --- a/mopidy/utils/process.py +++ b/mopidy/utils/process.py @@ -56,6 +56,8 @@ class BaseThread(multiprocessing.dummy.Process): def __init__(self, core_queue): super(BaseThread, self).__init__() self.core_queue = core_queue + # No thread should block process from exiting + self.daemon = True def run(self): logger.debug(u'%s: Starting thread', self.name) From 6ee7d56ec2d1b45a6179f541f14826cacb37ca37 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 24 Oct 2010 20:08:58 +0200 Subject: [PATCH 40/40] Update changelog --- docs/changes.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index ed3cb080..ed05050c 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -41,6 +41,8 @@ No description yet. sound output work with GStreamer >= 0.10.29, which includes the versions used in Ubuntu 10.10 and on OS X if using Homebrew. (Fixes: :issue:`21`, :issue:`24`, contributes to :issue:`14`) +- Improved handling of uncaught exceptions in threads. The entire process + should now exit immediately. 0.1.0 (2010-08-23)