From 43e16ddb6588886debdb3354a507a8e8ecad13ac Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Jan 2014 17:14:32 +0100 Subject: [PATCH] mpd: Switch mpd to use path<->uri mapping for browsing --- mopidy/mpd/dispatcher.py | 16 ++++ mopidy/mpd/protocol/current_playlist.py | 8 +- mopidy/mpd/protocol/music_db.py | 74 ++++++++++------ mopidy/mpd/translator.py | 9 ++ tests/dummy_backend.py | 2 +- tests/mpd/protocol/test_current_playlist.py | 12 +-- tests/mpd/protocol/test_music_db.py | 97 +++++++++++++++------ 7 files changed, 156 insertions(+), 62 deletions(-) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 4ddb4025..c269a5b3 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -292,3 +292,19 @@ class MpdContext(object): if uri not in self._playlist_name_from_uri: self.refresh_playlists_mapping() return self._playlist_name_from_uri[uri] + + # TODO: consider making context.browse(path) which uses this internally. + # advantage would be that all browse requests then go through the same code + # and we could prebuild/cache path->uri relationships instead of having to + # look them up all the time. + def directory_path_to_uri(self, path): + parts = re.findall(r'[^/]+', path) + uri = None + for part in parts: + for ref in self.core.library.browse(uri).get(): + if ref.type == ref.DIRECTORY and part == ref.name: + uri = ref.uri + break + else: + raise exceptions.MpdNoExistError() + return uri diff --git a/mopidy/mpd/protocol/current_playlist.py b/mopidy/mpd/protocol/current_playlist.py index ab799fb4..054fbf3a 100644 --- a/mopidy/mpd/protocol/current_playlist.py +++ b/mopidy/mpd/protocol/current_playlist.py @@ -27,8 +27,12 @@ def add(context, uri): if tl_tracks: return - if not uri.startswith('/'): - uri = '/%s' % uri + try: + uri = context.directory_path_to_uri(translator.normalize_path(uri)) + except MpdNoExistError as e: + e.command = 'add' + e.message = 'directory or file not found' + raise browse_futures = [context.core.library.browse(uri)] lookup_futures = [] diff --git a/mopidy/mpd/protocol/music_db.py b/mopidy/mpd/protocol/music_db.py index 7ef11111..e95303fc 100644 --- a/mopidy/mpd/protocol/music_db.py +++ b/mopidy/mpd/protocol/music_db.py @@ -417,25 +417,33 @@ def listall(context, uri=None): Lists all songs and directories in ``URI``. """ - if uri is None: - uri = '/' - if not uri.startswith('/'): - uri = '/%s' % uri - result = [] - browse_futures = [context.core.library.browse(uri)] + root_path = translator.normalize_path(uri) + # TODO: doesn't the dispatcher._call_handler have enough info to catch + # the error this can produce, set the command and then 'raise'? + try: + uri = context.directory_path_to_uri(root_path) + except MpdNoExistError as e: + e.command = 'listall' + e.message = 'Not found' + raise + browse_futures = [(root_path, context.core.library.browse(uri))] + while browse_futures: - for ref in browse_futures.pop().get(): + base_path, future = browse_futures.pop() + for ref in future.get(): if ref.type == Ref.DIRECTORY: - result.append(('directory', ref.uri)) - browse_futures.append(context.core.library.browse(ref.uri)) + path = '/'.join([base_path, ref.name.replace('/', '')]) + result.append(('directory', path)) + browse_futures.append( + (path, context.core.library.browse(ref.uri))) elif ref.type == Ref.TRACK: result.append(('file', ref.uri)) if not result: raise MpdNoExistError('Not found', command='listall') - return [('directory', uri)] + result + return [('directory', root_path)] + result @handle_request(r'listallinfo$') @@ -449,18 +457,25 @@ def listallinfo(context, uri=None): Same as ``listall``, except it also returns metadata info in the same format as ``lsinfo``. """ - if uri is None: - uri = '/' - if not uri.startswith('/'): - uri = '/%s' % uri - dirs_and_futures = [] - browse_futures = [context.core.library.browse(uri)] + result = [] + root_path = translator.normalize_path(uri) + try: + uri = context.directory_path_to_uri(root_path) + except MpdNoExistError as e: + e.command = 'listallinfo' + e.message = 'Not found' + raise + browse_futures = [(root_path, context.core.library.browse(uri))] + while browse_futures: - for ref in browse_futures.pop().get(): + base_path, future = browse_futures.pop() + for ref in future.get(): if ref.type == Ref.DIRECTORY: - dirs_and_futures.append(('directory', ref.uri)) - browse_futures.append(context.core.library.browse(ref.uri)) + path = '/'.join([base_path, ref.name.replace('/', '')]) + future = context.core.library.browse(ref.uri) + browse_futures.append((path, future)) + dirs_and_futures.append(('directory', path)) elif ref.type == Ref.TRACK: # TODO Lookup tracks in batch for better performance dirs_and_futures.append(context.core.library.lookup(ref.uri)) @@ -476,7 +491,7 @@ def listallinfo(context, uri=None): if not result: raise MpdNoExistError('Not found', command='listallinfo') - return [('directory', uri)] + result + return [('directory', root_path)] + result @handle_request(r'lsinfo$') @@ -498,16 +513,21 @@ def lsinfo(context, uri=None): ""``, and ``lsinfo "/"``. """ result = [] - if uri is None or uri == '/' or uri == '': + root_path = translator.normalize_path(uri, relative=True) + try: + uri = context.directory_path_to_uri(root_path) + except MpdNoExistError as e: + e.command = 'lsinfo' + e.message = 'Not found' + raise + + if uri is None: result.extend(stored_playlists.listplaylists(context)) - uri = '/' - if not uri.startswith('/'): - uri = '/%s' % uri + for ref in context.core.library.browse(uri).get(): if ref.type == Ref.DIRECTORY: - assert ref.uri.startswith('/'), ( - 'Directory URIs must start with /: %r' % ref) - result.append(('directory', ref.uri[1:])) + path = '/'.join([root_path, ref.name.replace('/', '')]) + result.append(('directory', path.lstrip('/'))) elif ref.type == Ref.TRACK: # TODO Lookup tracks in batch for better performance tracks = context.core.library.lookup(ref.uri).get() diff --git a/mopidy/mpd/translator.py b/mopidy/mpd/translator.py index 49ebce35..520e9ac8 100644 --- a/mopidy/mpd/translator.py +++ b/mopidy/mpd/translator.py @@ -1,11 +1,20 @@ from __future__ import unicode_literals +import re import shlex from mopidy.mpd.exceptions import MpdArgError from mopidy.models import TlTrack # TODO: special handling of local:// uri scheme +normalize_path_re = re.compile(r'[^/]+') + + +def normalize_path(path, relative=False): + parts = normalize_path_re.findall(path or '') + if not relative: + parts.insert(0, '') + return '/'.join(parts) def track_to_mpd_format(track, position=None): diff --git a/tests/dummy_backend.py b/tests/dummy_backend.py index 378ccadd..94b01433 100644 --- a/tests/dummy_backend.py +++ b/tests/dummy_backend.py @@ -28,7 +28,7 @@ class DummyBackend(pykka.ThreadingActor, backend.Backend): class DummyLibraryProvider(backend.LibraryProvider): - root_directory = Ref.directory(uri='dummy:directory', name='dummy') + root_directory = Ref.directory(uri='dummy:/', name='dummy') def __init__(self, *args, **kwargs): super(DummyLibraryProvider, self).__init__(*args, **kwargs) diff --git a/tests/mpd/protocol/test_current_playlist.py b/tests/mpd/protocol/test_current_playlist.py index 34221fcd..ff8e198e 100644 --- a/tests/mpd/protocol/test_current_playlist.py +++ b/tests/mpd/protocol/test_current_playlist.py @@ -27,7 +27,7 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): def test_add_with_empty_uri_should_not_add_anything_and_ok(self): self.backend.library.dummy_library = [Track(uri='dummy:/a', name='a')] self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a')]} self.sendRequest('add ""') self.assertEqual(len(self.core.tracklist.tracks.get()), 0) @@ -35,13 +35,13 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): def test_add_with_library_should_recurse(self): tracks = [Track(uri='dummy:/a', name='a'), - Track(uri='dummy:/b', name='b')] + Track(uri='dummy:/foo/b', name='b')] self.backend.library.dummy_library = tracks self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo')], - '/foo': [Ref.track(uri='dummy:/b', name='b')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/foo/b', name='b')]} self.sendRequest('add "/dummy"') self.assertEqual(self.core.tracklist.tracks.get(), tracks) @@ -50,7 +50,7 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): def test_add_root_should_not_add_anything_and_ok(self): self.backend.library.dummy_library = [Track(uri='dummy:/a', name='a')] self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a')]} self.sendRequest('add "/"') self.assertEqual(len(self.core.tracklist.tracks.get()), 0) diff --git a/tests/mpd/protocol/test_music_db.py b/tests/mpd/protocol/test_music_db.py index d2ecd66c..8d74fb95 100644 --- a/tests/mpd/protocol/test_music_db.py +++ b/tests/mpd/protocol/test_music_db.py @@ -124,34 +124,34 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): def test_listall_without_uri(self): tracks = [Track(uri='dummy:/a', name='a'), - Track(uri='dummy:/b', name='b')] + Track(uri='dummy:/foo/b', name='b')] self.backend.library.dummy_library = tracks self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo')], - '/foo': [Ref.track(uri='dummy:/b', name='b')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/foo/b', name='b')]} self.sendRequest('listall') self.assertInResponse('file: dummy:/a') self.assertInResponse('directory: /dummy/foo') - self.assertInResponse('file: dummy:/b') + self.assertInResponse('file: dummy:/foo/b') self.assertInResponse('OK') def test_listall_with_uri(self): tracks = [Track(uri='dummy:/a', name='a'), - Track(uri='dummy:/b', name='b')] + Track(uri='dummy:/foo/b', name='b')] self.backend.library.dummy_library = tracks self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo')], - '/foo': [Ref.track(uri='dummy:/b', name='b')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/foo/b', name='b')]} self.sendRequest('listall "/dummy/foo"') self.assertNotInResponse('file: dummy:/a') self.assertInResponse('directory: /dummy/foo') - self.assertInResponse('file: dummy:/b') + self.assertInResponse('file: dummy:/foo/b') self.assertInResponse('OK') def test_listall_with_unknown_uri(self): @@ -159,39 +159,57 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): self.assertEqualResponse('ACK [50@0] {listall} Not found') + def test_listall_for_dir_with_and_without_leading_slash_is_the_same(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} + + response1 = self.sendRequest('listall "dummy"') + response2 = self.sendRequest('listall "/dummy"') + self.assertEqual(response1, response2) + + def test_listall_for_dir_with_and_without_trailing_slash_is_the_same(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} + + response1 = self.sendRequest('listall "dummy"') + response2 = self.sendRequest('listall "dummy/"') + self.assertEqual(response1, response2) + def test_listallinfo_without_uri(self): tracks = [Track(uri='dummy:/a', name='a'), - Track(uri='dummy:/b', name='b')] + Track(uri='dummy:/foo/b', name='b')] self.backend.library.dummy_library = tracks self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo')], - '/foo': [Ref.track(uri='dummy:/b', name='b')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/foo/b', name='b')]} self.sendRequest('listallinfo') self.assertInResponse('file: dummy:/a') self.assertInResponse('Title: a') self.assertInResponse('directory: /dummy/foo') - self.assertInResponse('file: dummy:/b') + self.assertInResponse('file: dummy:/foo/b') self.assertInResponse('Title: b') self.assertInResponse('OK') def test_listallinfo_with_uri(self): tracks = [Track(uri='dummy:/a', name='a'), - Track(uri='dummy:/b', name='b')] + Track(uri='dummy:/foo/b', name='b')] self.backend.library.dummy_library = tracks self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo')], - '/foo': [Ref.track(uri='dummy:/b', name='b')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/foo/b', name='b')]} self.sendRequest('listallinfo "/dummy/foo"') self.assertNotInResponse('file: dummy:/a') self.assertNotInResponse('Title: a') self.assertInResponse('directory: /dummy/foo') - self.assertInResponse('file: dummy:/b') + self.assertInResponse('file: dummy:/foo/b') self.assertInResponse('Title: b') self.assertInResponse('OK') @@ -200,6 +218,24 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): self.assertEqualResponse('ACK [50@0] {listallinfo} Not found') + def test_listallinfo_for_dir_with_and_without_leading_slash_is_same(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} + + response1 = self.sendRequest('listallinfo "dummy"') + response2 = self.sendRequest('listallinfo "/dummy"') + self.assertEqual(response1, response2) + + def test_listallinfo_for_dir_with_and_without_trailing_slash_is_same(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} + + response1 = self.sendRequest('listallinfo "dummy"') + response2 = self.sendRequest('listallinfo "dummy/"') + self.assertEqual(response1, response2) + def test_lsinfo_without_path_returns_same_as_for_root(self): last_modified = datetime.datetime(2001, 3, 17, 13, 41, 17, 12345) self.backend.playlists.playlists = [ @@ -231,8 +267,8 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): def test_lsinfo_for_root_includes_dirs_for_each_lib_with_content(self): self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo', name='foo')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} self.sendRequest('lsinfo "/"') self.assertInResponse('directory: dummy') @@ -240,19 +276,28 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): def test_lsinfo_for_dir_with_and_without_leading_slash_is_the_same(self): self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a'), - Ref.directory(uri='/foo', name='foo')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} response1 = self.sendRequest('lsinfo "dummy"') response2 = self.sendRequest('lsinfo "/dummy"') self.assertEqual(response1, response2) + def test_lsinfo_for_dir_with_and_without_trailing_slash_is_the_same(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.track(uri='dummy:/a', name='a'), + Ref.directory(uri='dummy:/foo', name='foo')]} + + response1 = self.sendRequest('lsinfo "dummy"') + response2 = self.sendRequest('lsinfo "dummy/"') + self.assertEqual(response1, response2) + def test_lsinfo_for_dir_includes_tracks(self): self.backend.library.dummy_library = [ Track(uri='dummy:/a', name='a'), ] self.backend.library.dummy_browse_result = { - '/': [Ref.track(uri='dummy:/a', name='a')]} + 'dummy:/': [Ref.track(uri='dummy:/a', name='a')]} self.sendRequest('lsinfo "/dummy"') self.assertInResponse('file: dummy:/a') @@ -261,7 +306,7 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): def test_lsinfo_for_dir_includes_subdirs(self): self.backend.library.dummy_browse_result = { - '/': [Ref.directory(uri='/foo', name='foo')]} + 'dummy:/': [Ref.directory(uri='/foo', name='foo')]} self.sendRequest('lsinfo "/dummy"') self.assertInResponse('directory: dummy/foo')