From c062dd21c43c36012dc6a166e5e501ac1fb60d28 Mon Sep 17 00:00:00 2001 From: Trygve Aaberge Date: Tue, 24 Jun 2014 00:15:38 +0200 Subject: [PATCH] mpd: Create unique names for all items in browse Replace the playlist uri/name mapping with a mapping for all the items in browse. Check the mapping when browsing a path. This has the added benefit of serving as a cache for browse, so we don't need to traverse the path parts and lookup each of them for each call to browse. --- mopidy/mpd/dispatcher.py | 64 +++++++++++++++++------------ tests/mpd/protocol/test_music_db.py | 27 ++++++++++++ 2 files changed, 65 insertions(+), 26 deletions(-) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index b28140f2..84550698 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -227,7 +227,8 @@ class MpdContext(object): #: The subsytems that we want to be notified about in idle mode. subscriptions = None - _invalid_playlist_chars = re.compile(r'[\n\r/]') + _invalid_browse_chars = re.compile(r'[\n\r]') + _invalid_playlist_chars = re.compile(r'[/]') def __init__(self, dispatcher, session=None, config=None, core=None): self.dispatcher = dispatcher @@ -237,68 +238,77 @@ class MpdContext(object): self.core = core self.events = set() self.subscriptions = set() - self._playlist_uri_from_name = {} - self._playlist_name_from_uri = {} + self._uri_from_name = {} + self._name_from_uri = {} self.refresh_playlists_mapping() - def create_unique_name(self, playlist_name): - stripped_name = self._invalid_playlist_chars.sub(' ', playlist_name) + def create_unique_name(self, name, uri): + stripped_name = self._invalid_browse_chars.sub(' ', name) name = stripped_name i = 2 - while name in self._playlist_uri_from_name: + while name in self._uri_from_name: + if self._uri_from_name[name] == uri: + return name name = '%s [%d]' % (stripped_name, i) i += 1 return name + def insert_name_uri_mapping(self, name, uri): + name = self.create_unique_name(name, uri) + self._uri_from_name[name] = uri + self._name_from_uri[uri] = name + return name + def refresh_playlists_mapping(self): """ Maintain map between playlists and unique playlist names to be used by MPD """ if self.core is not None: - self._playlist_uri_from_name.clear() - self._playlist_name_from_uri.clear() for playlist in self.core.playlists.playlists.get(): if not playlist.name: continue # TODO: add scheme to name perhaps 'foo (spotify)' etc. - name = self.create_unique_name(playlist.name) - self._playlist_uri_from_name[name] = playlist.uri - self._playlist_name_from_uri[playlist.uri] = name + name = self._invalid_playlist_chars.sub(' ', playlist.name) + self.insert_name_uri_mapping(name, playlist.uri) def lookup_playlist_from_name(self, name): """ Helper function to retrieve a playlist from its unique MPD name. """ - if not self._playlist_uri_from_name: + if not self._uri_from_name: self.refresh_playlists_mapping() - if name not in self._playlist_uri_from_name: + if name not in self._uri_from_name: return None - uri = self._playlist_uri_from_name[name] + uri = self._uri_from_name[name] return self.core.playlists.lookup(uri).get() def lookup_playlist_name_from_uri(self, uri): """ Helper function to retrieve the unique MPD playlist name from its uri. """ - if uri not in self._playlist_name_from_uri: + if uri not in self._name_from_uri: self.refresh_playlists_mapping() - return self._playlist_name_from_uri[uri] + return self._name_from_uri[uri] def browse(self, path, recursive=True, lookup=True): - # TODO: consider caching lookups for less work mapping path->uri path_parts = re.findall(r'[^/]+', path or '') root_path = '/'.join([''] + path_parts) - uri = None - for part in path_parts: - for ref in self.core.library.browse(uri).get(): - if (ref.type in (ref.DIRECTORY, ref.ALBUM, ref.PLAYLIST) and - ref.name == part): - uri = ref.uri - break - else: - raise exceptions.MpdNoExistError('Not found') + if root_path not in self._uri_from_name: + uri = None + for part in path_parts: + for ref in self.core.library.browse(uri).get(): + if (ref.type in (ref.DIRECTORY, ref.ALBUM, ref.PLAYLIST) + and ref.name == part): + uri = ref.uri + break + else: + raise exceptions.MpdNoExistError('Not found') + root_path = self.insert_name_uri_mapping(root_path, uri) + + else: + uri = self._uri_from_name[root_path] if recursive: yield (root_path, None) @@ -308,6 +318,8 @@ class MpdContext(object): base_path, future = path_and_futures.pop() for ref in future.get(): path = '/'.join([base_path, ref.name.replace('/', '')]) + path = self.insert_name_uri_mapping(path, ref.uri) + if ref.type in (ref.DIRECTORY, ref.ALBUM, ref.PLAYLIST): yield (path, None) if recursive: diff --git a/tests/mpd/protocol/test_music_db.py b/tests/mpd/protocol/test_music_db.py index 3d68f2b7..55ab75c5 100644 --- a/tests/mpd/protocol/test_music_db.py +++ b/tests/mpd/protocol/test_music_db.py @@ -190,6 +190,15 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): response2 = self.sendRequest('listall "dummy/"') self.assertEqual(response1, response2) + def test_listall_duplicate(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.directory(uri='dummy:/a1', name='a'), + Ref.directory(uri='dummy:/a2', name='a')]} + + self.sendRequest('listall') + self.assertInResponse('directory: /dummy/a') + self.assertInResponse('directory: /dummy/a [2]') + def test_listallinfo_without_uri(self): tracks = [Track(uri='dummy:/a', name='a'), Track(uri='dummy:/foo/b', name='b')] @@ -253,6 +262,15 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): response2 = self.sendRequest('listallinfo "dummy/"') self.assertEqual(response1, response2) + def test_listallinfo_duplicate(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.directory(uri='dummy:/a1', name='a'), + Ref.directory(uri='dummy:/a2', name='a')]} + + self.sendRequest('listallinfo') + self.assertInResponse('directory: /dummy/a') + self.assertInResponse('directory: /dummy/a [2]') + def test_lsinfo_without_path_returns_same_as_for_root(self): last_modified = 1390942873222 self.backend.playlists.playlists = [ @@ -362,6 +380,15 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): self.assertLess(response.index('directory: dummy'), response.index('playlist: a')) + def test_lsinfo_duplicate(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.directory(uri='dummy:/a1', name='a'), + Ref.directory(uri='dummy:/a2', name='a')]} + + self.sendRequest('lsinfo "/dummy"') + self.assertInResponse('directory: dummy/a') + self.assertInResponse('directory: dummy/a [2]') + def test_update_without_uri(self): self.sendRequest('update') self.assertInResponse('updating_db: 0')