diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 61a07fb2..a53d387b 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -288,18 +288,35 @@ class MpdContext(object): 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) + 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 parts: + + for part in path_parts: for ref in self.core.library.browse(uri).get(): if ref.type == ref.DIRECTORY and ref.name == part: uri = ref.uri break else: - raise exceptions.MpdNoExistError() - return uri + raise exceptions.MpdNoExistError('Not found') + + if recursive: + yield (root_path, None) + + path_and_futures = [(root_path, self.core.library.browse(uri))] + while path_and_futures: + base_path, future = path_and_futures.pop() + for ref in future.get(): + path = '/'.join([base_path, ref.name.replace('/', '')]) + if ref.type == ref.DIRECTORY: + yield (path, None) + if recursive: + path_and_futures.append( + (path, self.core.library.browse(ref.uri))) + elif ref.type == ref.TRACK: + if lookup: + yield (path, self.core.library.lookup(ref.uri)) + else: + yield (path, ref) diff --git a/mopidy/mpd/protocol/current_playlist.py b/mopidy/mpd/protocol/current_playlist.py index 39ba9486..e50c1bff 100644 --- a/mopidy/mpd/protocol/current_playlist.py +++ b/mopidy/mpd/protocol/current_playlist.py @@ -20,32 +20,20 @@ def add(context, uri): if not uri.strip('/'): return - tl_tracks = context.core.tracklist.add(uri=uri).get() - if tl_tracks: + if context.core.tracklist.add(uri=uri).get(): return try: - uri = context.directory_path_to_uri(translator.normalize_path(uri)) + tracks = [] + for path, lookup_future in context.browse(uri): + if lookup_future: + tracks.extend(lookup_future.get()) except exceptions.MpdNoExistError as e: e.message = 'directory or file not found' raise - browse_futures = [context.core.library.browse(uri)] - lookup_futures = [] - while browse_futures: - for ref in browse_futures.pop().get(): - if ref.type == ref.DIRECTORY: - browse_futures.append(context.core.library.browse(ref.uri)) - else: - lookup_futures.append(context.core.library.lookup(ref.uri)) - - tracks = [] - for future in lookup_futures: - tracks.extend(future.get()) - if not tracks: raise exceptions.MpdNoExistError('directory or file not found') - context.core.tracklist.add(tracks=tracks) diff --git a/mopidy/mpd/protocol/music_db.py b/mopidy/mpd/protocol/music_db.py index 2472e1d9..2d895d67 100644 --- a/mopidy/mpd/protocol/music_db.py +++ b/mopidy/mpd/protocol/music_db.py @@ -3,7 +3,7 @@ from __future__ import unicode_literals import functools import itertools -from mopidy.models import Ref, Track +from mopidy.models import Track from mopidy.mpd import exceptions, protocol, translator _SEARCH_MAPPING = { @@ -349,7 +349,6 @@ def _list_genre(context, query): return genres -# TODO: see if we can combine listall, listallinfo and lsinfo to one helper @protocol.commands.add('listall') def listall(context, uri=None): """ @@ -360,29 +359,15 @@ def listall(context, uri=None): Lists all songs and directories in ``URI``. """ result = [] - root_path = translator.normalize_path(uri) - try: - uri = context.directory_path_to_uri(root_path) - except exceptions.MpdNoExistError as e: - e.message = 'Not found' - raise - browse_futures = [(root_path, context.core.library.browse(uri))] - - while browse_futures: - base_path, future = browse_futures.pop() - for ref in future.get(): - if ref.type == Ref.DIRECTORY: - 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)) + for path, track_ref in context.browse(uri, lookup=False): + if not track_ref: + result.append(('directory', path)) + else: + result.append(('file', track_ref.uri)) if not result: raise exceptions.MpdNoExistError('Not found') - - return [('directory', root_path)] + result + return result @protocol.commands.add('listallinfo') @@ -395,41 +380,14 @@ def listallinfo(context, uri=None): Same as ``listall``, except it also returns metadata info in the same format as ``lsinfo``. """ - dirs_and_futures = [] result = [] - root_path = translator.normalize_path(uri) - try: - uri = context.directory_path_to_uri(root_path) - except exceptions.MpdNoExistError as e: - e.command = 'listallinfo' - e.message = 'Not found' - raise - browse_futures = [(root_path, context.core.library.browse(uri))] - - while browse_futures: - base_path, future = browse_futures.pop() - for ref in future.get(): - if ref.type == Ref.DIRECTORY: - 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)) - - result = [] - for obj in dirs_and_futures: - if hasattr(obj, 'get'): - for track in obj.get(): - result.extend(translator.track_to_mpd_format(track)) + for path, lookup_future in context.browse(uri): + if not lookup_future: + result.append(('directory', path)) else: - result.append(obj) - - if not result: - raise exceptions.MpdNoExistError('Not found') - - return [('directory', root_path)] + result + for track in lookup_future.get(): + result.extend(translator.track_to_mpd_format(track)) + return result @protocol.commands.add('lsinfo') @@ -450,26 +408,19 @@ def lsinfo(context, uri=None): ""``, and ``lsinfo "/"``. """ result = [] - root_path = translator.normalize_path(uri, relative=True) - try: - uri = context.directory_path_to_uri(root_path) - except exceptions.MpdNoExistError as e: - e.command = 'lsinfo' - e.message = 'Not found' - raise - - if uri is None: + if uri in (None, '', '/'): result.extend(protocol.stored_playlists.listplaylists(context)) - for ref in context.core.library.browse(uri).get(): - if ref.type == Ref.DIRECTORY: - path = '/'.join([root_path, ref.name.replace('/', '')]) + for path, lookup_future in context.browse(uri, recursive=False): + if not lookup_future: 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() + else: + tracks = lookup_future.get() if tracks: result.extend(translator.track_to_mpd_format(tracks[0])) + + if not result: + raise exceptions.MpdNoExistError('Not found') return result diff --git a/tests/mpd/protocol/test_music_db.py b/tests/mpd/protocol/test_music_db.py index d41c05b2..d60a485f 100644 --- a/tests/mpd/protocol/test_music_db.py +++ b/tests/mpd/protocol/test_music_db.py @@ -306,12 +306,33 @@ class MusicDatabaseHandlerTest(protocol.BaseTestCase): def test_lsinfo_for_dir_includes_subdirs(self): self.backend.library.dummy_browse_result = { - 'dummy:/': [Ref.directory(uri='/foo', name='foo')]} + 'dummy:/': [Ref.directory(uri='dummy:/foo', name='foo')]} self.sendRequest('lsinfo "/dummy"') self.assertInResponse('directory: dummy/foo') self.assertInResponse('OK') + def test_lsinfo_for_dir_does_not_recurse(self): + self.backend.library.dummy_library = [ + Track(uri='dummy:/a', name='a'), + ] + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/a', name='a')]} + + self.sendRequest('lsinfo "/dummy"') + self.assertNotInResponse('file: dummy:/a') + self.assertInResponse('OK') + + def test_lsinfo_for_dir_does_not_self(self): + self.backend.library.dummy_browse_result = { + 'dummy:/': [Ref.directory(uri='dummy:/foo', name='foo')], + 'dummy:/foo': [Ref.track(uri='dummy:/a', name='a')]} + + self.sendRequest('lsinfo "/dummy"') + self.assertNotInResponse('directory: dummy') + self.assertInResponse('OK') + def test_update_without_uri(self): self.sendRequest('update') self.assertInResponse('updating_db: 0')