Merge pull request #762 from trygveaa/fix/mpd-browse-duplicate

mpd: Create unique names for all items in browse

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._uri_from_name = {}
-        self._playlist_name_from_uri = {}
+        self._name_from_uri = {}
         self.refresh_playlists_mapping()
 
-    def create_unique_name(self, playlist_name):
+    def create_unique_name(self, name, uri):
-        stripped_name = self._invalid_playlist_chars.sub(' ', playlist_name)
+        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)
+                name = self._invalid_playlist_chars.sub(' ', playlist.name)
-                self._playlist_uri_from_name[name] = playlist.uri
+                self.insert_name_uri_mapping(name, playlist.uri)
-                self._playlist_name_from_uri[playlist.uri] = name
 
     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:
+        if root_path not in self._uri_from_name:
-            for ref in self.core.library.browse(uri).get():
+            uri = None
-                if (ref.type in (ref.DIRECTORY, ref.ALBUM, ref.PLAYLIST) and
+            for part in path_parts:
-                        ref.name == part):
+                for ref in self.core.library.browse(uri).get():
-                    uri = ref.uri
+                    if (ref.type in (ref.DIRECTORY, ref.ALBUM, ref.PLAYLIST)
-                    break
+                            and ref.name == part):
-            else:
+                        uri = ref.uri
-                raise exceptions.MpdNoExistError('Not found')
+                        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:

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')