From d4c695ac75018ba8845fe7b2ccc8867ef4ee355b Mon Sep 17 00:00:00 2001
From: Thomas Adamcik <thomas@adamcik.no>
Date: Thu, 23 Apr 2015 23:08:25 +0200
Subject: [PATCH] mpd: Split browse and playlist name to uri caching

---
 docs/changelog.rst                    |  3 +++
 mopidy/mpd/uri_mapper.py              | 20 ++++++++++++-----
 tests/mpd/protocol/test_regression.py | 32 ++++++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/docs/changelog.rst b/docs/changelog.rst
index 0c470386..647ca651 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -27,6 +27,9 @@ Bug fix release.
 
 - HTTP: Fix threading bug that would cause duplicate delivery of WS messages.
 
+- MPD: Fix case where a playlist that is present in both browse and as a listed
+  playlist breaks the MPD frontend protocol output. (Fixes :issue:`1120`)
+
 
 v1.0.0 (2015-03-25)
 ===================
diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py
index 08c7f689..89dfd619 100644
--- a/mopidy/mpd/uri_mapper.py
+++ b/mopidy/mpd/uri_mapper.py
@@ -2,8 +2,12 @@ from __future__ import absolute_import, unicode_literals
 
 import re
 
+# TOOD: refactor this into a generic mapper that does not know about browse
+# or playlists and then use one instance for each case?
+
 
 class MpdUriMapper(object):
+
     """
     Maintains the mappings between uniquified MPD names and URIs.
     """
@@ -17,7 +21,8 @@ class MpdUriMapper(object):
     def __init__(self, core=None):
         self.core = core
         self._uri_from_name = {}
-        self._name_from_uri = {}
+        self._browse_name_from_uri = {}
+        self._playlist_name_from_uri = {}
 
     def _create_unique_name(self, name, uri):
         stripped_name = self._invalid_browse_chars.sub(' ', name)
@@ -30,13 +35,16 @@ class MpdUriMapper(object):
             i += 1
         return name
 
-    def insert(self, name, uri):
+    def insert(self, name, uri, playlist=False):
         """
         Create a unique and MPD compatible name that maps to the given URI.
         """
         name = self._create_unique_name(name, uri)
         self._uri_from_name[name] = uri
-        self._name_from_uri[uri] = name
+        if playlist:
+            self._playlist_name_from_uri[uri] = name
+        else:
+            self._browse_name_from_uri[uri] = name
         return name
 
     def uri_from_name(self, name):
@@ -56,7 +64,7 @@ class MpdUriMapper(object):
                 if not playlist_ref.name:
                     continue
                 name = self._invalid_playlist_chars.sub('|', playlist_ref.name)
-                self.insert(name, playlist_ref.uri)
+                self.insert(name, playlist_ref.uri, playlist=True)
 
     def playlist_uri_from_name(self, name):
         """
@@ -70,6 +78,6 @@ class MpdUriMapper(object):
         """
         Helper function to retrieve the unique MPD playlist name from its URI.
         """
-        if uri not in self._name_from_uri:
+        if uri not in self._playlist_name_from_uri:
             self.refresh_playlists_mapping()
-        return self._name_from_uri[uri]
+        return self._playlist_name_from_uri[uri]
diff --git a/tests/mpd/protocol/test_regression.py b/tests/mpd/protocol/test_regression.py
index 6fb59afd..60a71ee8 100644
--- a/tests/mpd/protocol/test_regression.py
+++ b/tests/mpd/protocol/test_regression.py
@@ -2,7 +2,7 @@ from __future__ import absolute_import, unicode_literals
 
 import random
 
-from mopidy.models import Track
+from mopidy.models import Playlist, Ref, Track
 
 from tests.mpd import protocol
 
@@ -175,3 +175,33 @@ class IssueGH137RegressionTest(protocol.BaseTestCase):
             u'Album "This Is Remixed Hits - Mashups & Rare 12" Mixes"')
 
         self.assertInResponse('ACK [2@0] {list} Invalid unquoted character')
+
+
+class IssueGH1120RegressionTest(protocol.BaseTestCase):
+    """
+    The issue: https://github.com/mopidy/mopidy/issues/1120
+
+    How to reproduce:
+
+    - A playlist must be in both browse results and playlists
+    - Call for instance ``lsinfo "/"`` to populate the cache with the
+      playlist name from the playlist backend.
+    - Call ``lsinfo "/dummy"`` to override the playlist name with the browse
+      name.
+    - Call ``lsinfo "/"`` and we now have an invalid name with ``/`` in it.
+
+    """
+
+    def test(self):
+        self.backend.library.dummy_browse_result = {
+            'dummy:/': [Ref.playlist(name='Top 100 tracks', uri='dummy:/1')],
+        }
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(name='Top 100 tracks', uri='dummy:/1'),
+        ])
+
+        response1 = self.send_request('lsinfo "/"')
+        self.send_request('lsinfo "/dummy"')
+
+        response2 = self.send_request('lsinfo "/"')
+        self.assertEqual(response1, response2)