From ecb9ba97be5113f1f23faa4297d5e6e3973b97ec Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 20:07:44 +0200 Subject: [PATCH 01/38] Add SingletonImportTask The goal of this class is to eliminate all checks for `task.is_album`. This is already accomplished for the methods. Next, we want to tackle the plugin stages. --- beets/importer.py | 92 ++++++++++++++++++++++++++-------------------- test/test_ihate.py | 5 +-- 2 files changed, 54 insertions(+), 43 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index f8134870e..7b9c1b33f 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -402,19 +402,10 @@ class ImportTask(object): obj.sentinel = True return obj - @classmethod - def item_task(cls, item): - """Creates an ImportTask for a single item.""" - obj = cls() - obj.item = item - obj.is_album = False - return obj - def set_candidates(self, cur_artist, cur_album, candidates, rec): """Sets the candidates for this album matched by the `autotag.tag_album` method. """ - assert self.is_album assert not self.sentinel self.cur_artist = cur_artist self.cur_album = cur_album @@ -430,11 +421,7 @@ class ImportTask(object): self.rec = None def set_item_candidates(self, candidates, rec): - """Set the match for a single-item task.""" - assert not self.is_album - assert self.item is not None - self.candidates = candidates - self.rec = rec + raise NotImplementedError def set_choice(self, choice): """Given an AlbumMatch or TrackMatch object or an action constant, @@ -462,7 +449,7 @@ class ImportTask(object): if self.sentinel and self.paths is None: # "Done" sentinel. progress_set(self.toppath, None) - elif self.sentinel or self.is_album: + elif self.sentinel: # "Directory progress" sentinel for singletons or a real # album task, which implies the same. progress_set(self.toppath, self.paths) @@ -470,7 +457,7 @@ class ImportTask(object): def save_history(self): """Save the directory in the history for incremental imports. """ - if self.is_album and self.paths and not self.sentinel: + if self.paths and not self.sentinel: history_add(self.paths) # Logical decisions. @@ -499,17 +486,10 @@ class ImportTask(object): (in which case the data comes from the files' current metadata) or APPLY (data comes from the choice). """ - assert self.choice_flag in (action.ASIS, action.APPLY) - if self.is_album: - if self.choice_flag is action.ASIS: - return (self.cur_artist, self.cur_album) - elif self.choice_flag is action.APPLY: - return (self.match.info.artist, self.match.info.album) - else: - if self.choice_flag is action.ASIS: - return (self.item.artist, self.item.title) - elif self.choice_flag is action.APPLY: - return (self.match.info.artist, self.match.info.title) + if self.choice_flag is action.ASIS: + return (self.cur_artist, self.cur_album) + elif self.choice_flag is action.APPLY: + return (self.match.info.artist, self.match.info.album) def imported_items(self): """Return a list of Items that should be added to the library. @@ -517,15 +497,12 @@ class ImportTask(object): selected match or everything if the choice is ASIS. If this is a singleton task, return a list containing the item. """ - if self.is_album: - if self.choice_flag == action.ASIS: - return list(self.items) - elif self.choice_flag == action.APPLY: - return self.match.mapping.keys() - else: - assert False + if self.choice_flag == action.ASIS: + return list(self.items) + elif self.choice_flag == action.APPLY: + return self.match.mapping.keys() else: - return [self.item] + assert False def cleanup(self): """Perform clean up during `finalize` stage. @@ -619,6 +596,43 @@ class ArchiveImportTask(ImportTask): self.toppath = extract_to +class SingletonImportTask(ImportTask): + """ImportTask for a single track that is not associated to an album. + """ + + def __init__(self, item): + super(SingletonImportTask, self).__init__() + self.item = item + self.is_album = False + + def chosen_ident(self): + assert self.choice_flag in (action.ASIS, action.APPLY) + if self.choice_flag is action.ASIS: + return (self.item.artist, self.item.title) + elif self.choice_flag is action.APPLY: + return (self.match.info.artist, self.match.info.title) + + def imported_items(self): + return [self.item] + + def save_progress(self): + # TODO we should also save progress for singletons + pass + + def save_history(self): + # TODO we should also save history for singletons + pass + + def set_item_candidates(self, candidates, rec): + """Set the match for a single-item task.""" + assert self.item is not None + self.candidates = candidates + self.rec = rec + + def set_candidates(self, cur_artist, cur_album, candidates, rec): + raise NotImplementedError + + # Full-album pipeline stages. def read_tasks(session): @@ -682,7 +696,7 @@ def read_tasks(session): )) continue if config['import']['singletons']: - yield ImportTask.item_task(item) + yield SingletonImportTask(item) else: yield ImportTask(toppath, [toppath], [item]) continue @@ -720,7 +734,7 @@ def read_tasks(session): # Yield all the necessary tasks. if config['import']['singletons']: for item in items: - yield ImportTask.item_task(item) + yield SingletonImportTask(item) yield ImportTask.progress_sentinel(toppath, paths) else: yield ImportTask(toppath, paths, items) @@ -746,7 +760,7 @@ def query_tasks(session): if config['import']['singletons']: # Search for items. for item in session.lib.items(session.query): - yield ImportTask.item_task(item) + yield SingletonImportTask(item) else: # Search for albums. @@ -808,7 +822,7 @@ def user_query(session): # Set up a little pipeline for dealing with the singletons. def emitter(task): for item in task.items: - yield ImportTask.item_task(item) + yield SingletonImportTask(item) yield ImportTask.progress_sentinel(task.toppath, task.paths) ipl = pipeline.Pipeline([ diff --git a/test/test_ihate.py b/test/test_ihate.py index 0b7f9affc..b9c6eb114 100644 --- a/test/test_ihate.py +++ b/test/test_ihate.py @@ -15,10 +15,7 @@ class IHatePluginTest(unittest.TestCase): genre='TestGenre', album=u'TestAlbum', artist=u'TestArtist') - task = importer.ImportTask() - task.items = [test_item] - task.item = test_item - task.is_album = False + task = importer.SingletonImportTask(test_item) # Empty query should let it pass. self.assertFalse(IHatePlugin.do_i_hate_this(task, match_pattern)) From 6284547d5567ec19959eaf2f4cf5a6947f2a7ff6 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 21:52:23 +0200 Subject: [PATCH 02/38] Add task.apply_metadata() method --- beets/importer.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 7b9c1b33f..4f92c35ff 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -295,7 +295,7 @@ class ImportSession(object): ``duplicate``, then this is a secondary choice after a duplicate was detected and a decision was made. """ - paths = task.paths if task.is_album else [task.item.path] + paths = task.paths if duplicate: # Duplicate: log all three choices (skip, keep both, and trump). if task.remove_duplicates: @@ -380,6 +380,7 @@ class ImportTask(object): self.items = items self.sentinel = False self.remove_duplicates = False + # TODO remove this eventually self.is_album = True self.choice_flag = None @@ -435,10 +436,6 @@ class ImportTask(object): self.choice_flag = choice self.match = None else: - if self.is_album: - assert isinstance(choice, autotag.AlbumMatch) - else: - assert isinstance(choice, autotag.TrackMatch) self.choice_flag = action.APPLY # Implicit choice. self.match = choice @@ -509,6 +506,11 @@ class ImportTask(object): """ pass + def apply_metadata(self): + """Copy metadata from match info to the items. + """ + autotag.apply_metadata(self.match.info, self.match.mapping) + # Utilities. def prune(self, filename): @@ -601,7 +603,7 @@ class SingletonImportTask(ImportTask): """ def __init__(self, item): - super(SingletonImportTask, self).__init__() + super(SingletonImportTask, self).__init__(paths=[item.path]) self.item = item self.is_album = False @@ -632,6 +634,9 @@ class SingletonImportTask(ImportTask): def set_candidates(self, cur_artist, cur_album, candidates, rec): raise NotImplementedError + def apply_metadata(self): + autotag.apply_item_metadata(self.item, self.match.info) + # Full-album pipeline stages. @@ -895,12 +900,7 @@ def apply_choices(session): # Change metadata. if task.should_write_tags(): - if task.is_album: - autotag.apply_metadata( - task.match.info, task.match.mapping - ) - else: - autotag.apply_item_metadata(task.item, task.match.info) + task.apply_metadata() plugins.send('import_task_apply', session=session, task=task) # Infer album-level fields. From 8dd5e6a62d20c262b5b26e9aacde35177841d7e7 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 22:13:22 +0200 Subject: [PATCH 03/38] Album tasks always record their progress --- beets/importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/importer.py b/beets/importer.py index 4f92c35ff..1426da1e3 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -446,7 +446,7 @@ class ImportTask(object): if self.sentinel and self.paths is None: # "Done" sentinel. progress_set(self.toppath, None) - elif self.sentinel: + elif self.sentinel or self.is_album: # "Directory progress" sentinel for singletons or a real # album task, which implies the same. progress_set(self.toppath, self.paths) From ce28c2a95b4c4987efe1d20480d4c66ab61d8780 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 22:25:18 +0200 Subject: [PATCH 04/38] Add SentinelImportTask and move code into it --- beets/importer.py | 175 ++++++++++++++++++++++++---------------------- 1 file changed, 91 insertions(+), 84 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 1426da1e3..9ba6a8a18 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -378,36 +378,15 @@ class ImportTask(object): self.toppath = toppath self.paths = paths self.items = items - self.sentinel = False - self.remove_duplicates = False - # TODO remove this eventually - self.is_album = True self.choice_flag = None - - @classmethod - def done_sentinel(cls, toppath): - """Create an ImportTask that indicates the end of a top-level - directory import. - """ - obj = cls(toppath) - obj.sentinel = True - return obj - - @classmethod - def progress_sentinel(cls, toppath, paths): - """Create a task indicating that a single directory in a larger - import has finished. This is only required for singleton - imports; progress is implied for album imports. - """ - obj = cls(toppath, paths) - obj.sentinel = True - return obj + # TODO remove this eventually + self.remove_duplicates = False + self.is_album = True def set_candidates(self, cur_artist, cur_album, candidates, rec): """Sets the candidates for this album matched by the `autotag.tag_album` method. """ - assert not self.sentinel self.cur_artist = cur_artist self.cur_album = cur_album self.candidates = candidates @@ -428,7 +407,6 @@ class ImportTask(object): """Given an AlbumMatch or TrackMatch object or an action constant, indicates that an action has been selected for this task. """ - assert not self.sentinel # Not part of the task structure: assert choice not in (action.MANUAL, action.MANUAL_ID) assert choice != action.APPLY # Only used internally. @@ -443,18 +421,12 @@ class ImportTask(object): """Updates the progress state to indicate that this album has finished. """ - if self.sentinel and self.paths is None: - # "Done" sentinel. - progress_set(self.toppath, None) - elif self.sentinel or self.is_album: - # "Directory progress" sentinel for singletons or a real - # album task, which implies the same. - progress_set(self.toppath, self.paths) + progress_set(self.toppath, self.paths) def save_history(self): """Save the directory in the history for incremental imports. """ - if self.paths and not self.sentinel: + if self.paths: history_add(self.paths) # Logical decisions. @@ -469,10 +441,7 @@ class ImportTask(object): assert False def should_skip(self): - """After a choice has been made, returns True if this is a - sentinel or it has been marked for skipping. - """ - return self.sentinel or self.choice_flag == action.SKIP + return self.choice_flag == action.SKIP # Convenient data. @@ -526,7 +495,86 @@ class ImportTask(object): clutter=config['clutter'].as_str_seq()) -class ArchiveImportTask(ImportTask): +class SingletonImportTask(ImportTask): + """ImportTask for a single track that is not associated to an album. + """ + + def __init__(self, item): + super(SingletonImportTask, self).__init__(paths=[item.path]) + self.item = item + self.is_album = False + + def chosen_ident(self): + assert self.choice_flag in (action.ASIS, action.APPLY) + if self.choice_flag is action.ASIS: + return (self.item.artist, self.item.title) + elif self.choice_flag is action.APPLY: + return (self.match.info.artist, self.match.info.title) + + def imported_items(self): + return [self.item] + + def save_progress(self): + # TODO we should also save progress for singletons + pass + + def save_history(self): + # TODO we should also save history for singletons + pass + + def set_item_candidates(self, candidates, rec): + """Set the match for a single-item task.""" + assert self.item is not None + self.candidates = candidates + self.rec = rec + + def set_candidates(self, cur_artist, cur_album, candidates, rec): + raise NotImplementedError + + def apply_metadata(self): + autotag.apply_item_metadata(self.item, self.match.info) + + +class SentinelImportTask(ImportTask): + """This class marks the progress of an import and does not import + any items itself. + + If only `toppath` is set the task indicats the end of a top-level + directory import. If the `paths` argument is givent, too, the task + indicates the progress in the `toppath` import. + """ + + def __init__(self, toppath=None, paths=None): + self.toppath = toppath + self.paths = paths + # TODO Remove the remaining attributes eventually + self.items = None + self.remove_duplicates = False + self.is_album = True + self.choice_flag = None + + def save_history(self): + pass + + def save_progress(self): + if self.paths is None: + # "Done" sentinel. + progress_set(self.toppath, None) + else: + # "Directory progress" sentinel for singletons + progress_set(self.toppath, self.paths) + + def should_skip(self): + return True + + def set_choice(self, choice): + raise NotImplementedError + + def set_candidates(self, cur_artist, cur_album, candidates, rec): + raise NotImplementedError + + +class ArchiveImportTask(SentinelImportTask): """Additional methods for handling archives. Use when `toppath` points to a `zip`, `tar`, or `rar` archive. @@ -534,7 +582,6 @@ class ArchiveImportTask(ImportTask): def __init__(self, toppath): super(ArchiveImportTask, self).__init__(toppath) - self.sentinel = True self.extracted = False @classmethod @@ -598,46 +645,6 @@ class ArchiveImportTask(ImportTask): self.toppath = extract_to -class SingletonImportTask(ImportTask): - """ImportTask for a single track that is not associated to an album. - """ - - def __init__(self, item): - super(SingletonImportTask, self).__init__(paths=[item.path]) - self.item = item - self.is_album = False - - def chosen_ident(self): - assert self.choice_flag in (action.ASIS, action.APPLY) - if self.choice_flag is action.ASIS: - return (self.item.artist, self.item.title) - elif self.choice_flag is action.APPLY: - return (self.match.info.artist, self.match.info.title) - - def imported_items(self): - return [self.item] - - def save_progress(self): - # TODO we should also save progress for singletons - pass - - def save_history(self): - # TODO we should also save history for singletons - pass - - def set_item_candidates(self, candidates, rec): - """Set the match for a single-item task.""" - assert self.item is not None - self.candidates = candidates - self.rec = rec - - def set_candidates(self, cur_artist, cur_album, candidates, rec): - raise NotImplementedError - - def apply_metadata(self): - autotag.apply_item_metadata(self.item, self.match.info) - - # Full-album pipeline stages. def read_tasks(session): @@ -712,7 +719,7 @@ def read_tasks(session): for _, items in autotag.albums_in_dir(toppath): all_items += items yield ImportTask(toppath, [toppath], all_items) - yield ImportTask.done_sentinel(toppath) + yield SentinelImportTask(toppath) continue # Produce paths under this directory. @@ -740,14 +747,14 @@ def read_tasks(session): if config['import']['singletons']: for item in items: yield SingletonImportTask(item) - yield ImportTask.progress_sentinel(toppath, paths) + yield SentinelImportTask(toppath, paths) else: yield ImportTask(toppath, paths, items) # Indicate the directory is finished. # FIXME hack to delete extracted archives if archive_task is None: - yield ImportTask.done_sentinel(toppath) + yield SentinelImportTask(toppath) else: yield archive_task @@ -828,7 +835,7 @@ def user_query(session): def emitter(task): for item in task.items: yield SingletonImportTask(item) - yield ImportTask.progress_sentinel(task.toppath, task.paths) + yield SentinelImportTask(task.toppath, task.paths) ipl = pipeline.Pipeline([ emitter(task), @@ -1165,6 +1172,6 @@ def group_albums(session): tasks = [] for _, items in itertools.groupby(task.items, group): tasks.append(ImportTask(items=list(items))) - tasks.append(ImportTask.progress_sentinel(task.toppath, task.paths)) + tasks.append(SentinelImportTask(task.toppath, task.paths)) task = pipeline.multiple(tasks) From 4e1b33e125e9fd649b6f330e91042d65401099fe Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 22:44:58 +0200 Subject: [PATCH 05/38] Make _resume() session local and refactor resume in read stage Functionaly, this should not change anything. --- beets/importer.py | 49 ++++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 9ba6a8a18..3709401e3 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -152,13 +152,6 @@ def _infer_album_fields(task): setattr(item, k, v) -def _resume(): - """Check whether an import should resume and return a boolean or the - string 'ask' indicating that the user should be queried. - """ - return config['import']['resume'].as_choice([True, False, 'ask']) - - def _open_state(): """Reads the state file, returning a dictionary.""" try: @@ -281,6 +274,8 @@ class ImportSession(object): if not iconfig['copy']: iconfig['delete'] = False + self.want_resume = iconfig['resume'].as_choice([True, False, 'ask']) + def tag_log(self, status, paths): """Log a message about a given album to logfile. The status should reflect the reason the album couldn't be tagged. @@ -653,24 +648,7 @@ def read_tasks(session): import, yields single-item tasks instead. """ # Look for saved progress. - if _resume(): - resume_dirs = {} - for path in session.paths: - resume_dir = progress_get(path) - if resume_dir: - - # Either accept immediately or prompt for input to decide. - if _resume() is True: - do_resume = True - log.warn('Resuming interrupted import of %s' % path) - else: - do_resume = session.should_resume(path) - - if do_resume: - resume_dirs[path] = resume_dir - else: - # Clear progress; we're starting from the top. - progress_set(path, None) + resume_dirs = {} # Look for saved incremental directories. if config['import']['incremental']: @@ -722,12 +700,23 @@ def read_tasks(session): yield SentinelImportTask(toppath) continue + resume_dir = None + if session.want_resume: + resume_dir = progress_get(toppath) + if resume_dir: + # Either accept immediately or prompt for input to decide. + if session.want_resume is True or \ + session.should_resume(toppath): + log.warn('Resuming interrupted import of %s' % toppath) + else: + # Clear progress; we're starting from the top. + resume_dir = None + progress_set(toppath, None) + # Produce paths under this directory. - if _resume(): - resume_dir = resume_dirs.get(toppath) for paths, items in autotag.albums_in_dir(toppath): # Skip according to progress. - if _resume() and resume_dir: + if session.want_resume and resume_dir: # We're fast-forwarding to resume a previous tagging. if paths == resume_dir: # We've hit the last good path! Turn off the @@ -1057,7 +1046,7 @@ def finalize(session): while True: task = yield if task.should_skip(): - if _resume(): + if session.want_resume: task.save_progress() if config['import']['incremental']: task.save_history() @@ -1082,7 +1071,7 @@ def finalize(session): task.prune(old_path) # Update progress. - if _resume(): + if session.want_resume: task.save_progress() if config['import']['incremental']: task.save_history() From 1c68bbb854b7289993cf338c6e37091493c36a6b Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 17 Apr 2014 23:16:07 +0200 Subject: [PATCH 06/38] Refactor finalize stage into Task class --- beets/importer.py | 103 +++++++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 48 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 3709401e3..5c750b862 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -475,6 +475,46 @@ class ImportTask(object): """ autotag.apply_metadata(self.match.info, self.match.mapping) + def finalize(self, session): + """Move files, save progress and emit plugin event. + """ + # FIXME the session argument is unfortunate. It should be + # present as an attribute of the task. + + # Update progress. + if session.want_resume: + self.save_progress() + if config['import']['incremental']: + self.save_history() + self.cleanup() + + # FIXME This shouldn't be here. Skipped tasks should be removed from + # the pipeline. + if not self.should_skip(): + self._move_files() + self._emit_imported(session) + + def _move_files(self): + items = self.imported_items() + + # When copying and deleting originals, delete old files. + if config['import']['copy'] and config['import']['delete']: + new_paths = [os.path.realpath(item.path) for item in items] + for old_path in self.old_paths: + # Only delete files that were actually copied. + if old_path not in new_paths: + util.remove(syspath(old_path), False) + self.prune(old_path) + + # When moving, prune empty directories containing the original files. + elif config['import']['move']: + for old_path in self.old_paths: + self.prune(old_path) + + def _emit_imported(self, session): + album = session.lib.get_album(self.album_id) + plugins.send('album_imported', lib=session.lib, album=album) + # Utilities. def prune(self, filename): @@ -529,7 +569,14 @@ class SingletonImportTask(ImportTask): def apply_metadata(self): autotag.apply_item_metadata(self.item, self.match.info) + def _emit_imported(self, session): + for item in self.imported_items(): + plugins.send('item_imported', lib=session.lib, item=item) + +# FIXME The inheritance relationships are inverted. This is why there +# are so many methods which pass. We should introduce a new +# BaseImportTask class. class SentinelImportTask(ImportTask): """This class marks the progress of an import and does not import any items itself. @@ -568,6 +615,12 @@ class SentinelImportTask(ImportTask): def set_candidates(self, cur_artist, cur_album, candidates, rec): raise NotImplementedError + def _move_files(self): + pass + + def _emit_imported(self): + pass + class ArchiveImportTask(SentinelImportTask): """Additional methods for handling archives. @@ -647,9 +700,6 @@ def read_tasks(session): in the user-specified list of paths. In the case of a singleton import, yields single-item tasks instead. """ - # Look for saved progress. - resume_dirs = {} - # Look for saved incremental directories. if config['import']['incremental']: incremental_skipped = 0 @@ -1038,54 +1088,11 @@ def manipulate_files(session): plugins.send('import_task_files', session=session, task=task) +# TODO Get rid of this. def finalize(session): - """A coroutine that finishes up importer tasks. In particular, the - coroutine sends plugin events, deletes old files, and saves - progress. This is a "terminal" coroutine (it yields None). - """ while True: task = yield - if task.should_skip(): - if session.want_resume: - task.save_progress() - if config['import']['incremental']: - task.save_history() - task.cleanup() - continue - - items = task.imported_items() - - # When copying and deleting originals, delete old files. - if config['import']['copy'] and config['import']['delete']: - new_paths = [os.path.realpath(item.path) for item in items] - for old_path in task.old_paths: - # Only delete files that were actually copied. - if old_path not in new_paths: - util.remove(syspath(old_path), False) - task.prune(old_path) - - # When moving, prune empty directories containing the original - # files. - elif config['import']['move']: - for old_path in task.old_paths: - task.prune(old_path) - - # Update progress. - if session.want_resume: - task.save_progress() - if config['import']['incremental']: - task.save_history() - task.cleanup() - - # Announce that we've added an album. - if task.is_album: - album = session.lib.get_album(task.album_id) - plugins.send('album_imported', - lib=session.lib, album=album) - else: - for item in items: - plugins.send('item_imported', - lib=session.lib, item=item) + task.finalize(session) # Singleton pipeline stages. From f396244055390b7b74b4c8e54ca11b6ac7c81748 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 13:07:37 +0200 Subject: [PATCH 07/38] Refactor task.finalize() --- beets/importer.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 5c750b862..e8eb80bec 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -465,11 +465,6 @@ class ImportTask(object): else: assert False - def cleanup(self): - """Perform clean up during `finalize` stage. - """ - pass - def apply_metadata(self): """Copy metadata from match info to the items. """ @@ -487,14 +482,15 @@ class ImportTask(object): if config['import']['incremental']: self.save_history() self.cleanup() + self._emit_imported(session) + def cleanup(self): + """Remove and prune imported paths. + """ # FIXME This shouldn't be here. Skipped tasks should be removed from # the pipeline. - if not self.should_skip(): - self._move_files() - self._emit_imported(session) - - def _move_files(self): + if self.should_skip(): + return items = self.imported_items() # When copying and deleting originals, delete old files. @@ -512,6 +508,10 @@ class ImportTask(object): self.prune(old_path) def _emit_imported(self, session): + # FIXME This shouldn't be here. Skipped tasks should be removed from + # the pipeline. + if self.should_skip(): + return album = session.lib.get_album(self.album_id) plugins.send('album_imported', lib=session.lib, album=album) @@ -570,6 +570,10 @@ class SingletonImportTask(ImportTask): autotag.apply_item_metadata(self.item, self.match.info) def _emit_imported(self, session): + # FIXME This shouldn't be here. Skipped tasks should be removed from + # the pipeline. + if self.should_skip(): + return for item in self.imported_items(): plugins.send('item_imported', lib=session.lib, item=item) @@ -615,10 +619,10 @@ class SentinelImportTask(ImportTask): def set_candidates(self, cur_artist, cur_album, candidates, rec): raise NotImplementedError - def _move_files(self): + def cleanup(self): pass - def _emit_imported(self): + def _emit_imported(self, session): pass From 19fdf069f935564d36360e848b4168997ebb46da Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 13:35:57 +0200 Subject: [PATCH 08/38] Merge singleton and album lookup stages --- beets/importer.py | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index e8eb80bec..667396c90 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -331,7 +331,7 @@ class ImportSession(object): if config['import']['singletons']: # Singleton importer. if config['import']['autotag']: - stages += [item_lookup(self), item_query(self)] + stages += [lookup_candidates(self), item_query(self)] else: stages += [item_progress(self)] else: @@ -341,7 +341,7 @@ class ImportSession(object): stages += [group_albums(self)] if config['import']['autotag']: # Only look up and query the user when autotagging. - stages += [initial_lookup(self), user_query(self)] + stages += [lookup_candidates(self), user_query(self)] else: # When not autotagging, just display progress. stages += [show_progress(self)] @@ -515,6 +515,12 @@ class ImportTask(object): album = session.lib.get_album(self.album_id) plugins.send('album_imported', lib=session.lib, album=album) + def lookup_candidates(self): + """Retrieve and store candidates for this album. + """ + self.set_candidates(*autotag.tag_album(self.items)) + + # Utilities. def prune(self, filename): @@ -538,6 +544,7 @@ class SingletonImportTask(ImportTask): super(SingletonImportTask, self).__init__(paths=[item.path]) self.item = item self.is_album = False + self.paths = [item.path] def chosen_ident(self): assert self.choice_flag in (action.ASIS, action.APPLY) @@ -577,6 +584,11 @@ class SingletonImportTask(ImportTask): for item in self.imported_items(): plugins.send('item_imported', lib=session.lib, item=item) + def lookup_candidates(self): + self.set_item_candidates(*autotag.tag_item(self.item)) + + + # FIXME The inheritance relationships are inverted. This is why there # are so many methods which pass. We should introduce a new @@ -826,7 +838,7 @@ def query_tasks(session): yield ImportTask(None, [album.item_dir()], items) -def initial_lookup(session): +def lookup_candidates(session): """A coroutine for performing the initial MusicBrainz lookup for an album. It accepts lists of Items and yields (items, cur_artist, cur_album, candidates, rec) tuples. If no match @@ -839,11 +851,8 @@ def initial_lookup(session): continue plugins.send('import_task_start', session=session, task=task) - log.debug('Looking up: %s' % displayable_path(task.paths)) - task.set_candidates( - *autotag.tag_album(task.items) - ) + task.lookup_candidates() def user_query(session): @@ -882,7 +891,7 @@ def user_query(session): ipl = pipeline.Pipeline([ emitter(task), - item_lookup(session), + lookup_candidates(session), item_query(session), ]) task = pipeline.multiple(ipl.pull()) @@ -896,7 +905,7 @@ def user_query(session): ipl = pipeline.Pipeline([ emitter(task), group_albums(session), - initial_lookup(session), + lookup_candidates(session), user_query(session) ]) task = pipeline.multiple(ipl.pull()) @@ -1101,21 +1110,6 @@ def finalize(session): # Singleton pipeline stages. -def item_lookup(session): - """A coroutine used to perform the initial MusicBrainz lookup for - an item task. - """ - task = None - while True: - task = yield task - if task.should_skip(): - continue - - plugins.send('import_task_start', session=session, task=task) - - task.set_item_candidates(*autotag.tag_item(task.item)) - - def item_query(session): """A coroutine that queries the user for input on single-item lookups. From ec01590df6c8a4125d4fd52200b336a660e5f20f Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 14:03:37 +0200 Subject: [PATCH 09/38] Merge duplicate detection into find_duplicates method --- beets/importer.py | 91 ++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 667396c90..ebd4fc0ff 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -58,50 +58,6 @@ class ImportAbort(Exception): # Utilities. -def _duplicate_check(lib, task): - """Check whether an album already exists in the library. Returns a - list of Album objects (empty if no duplicates are found). - """ - assert task.choice_flag in (action.ASIS, action.APPLY) - artist, album = task.chosen_ident() - - if artist is None: - # As-is import with no artist. Skip check. - return [] - - found_albums = [] - cur_paths = set(i.path for i in task.items if i) - for album_cand in lib.albums(dbcore.MatchQuery('albumartist', artist)): - if album_cand.album == album: - # Check whether the album is identical in contents, in which - # case it is not a duplicate (will be replaced). - other_paths = set(i.path for i in album_cand.items()) - if other_paths == cur_paths: - continue - found_albums.append(album_cand) - return found_albums - - -def _item_duplicate_check(lib, task): - """Check whether an item already exists in the library. Returns a - list of Item objects. - """ - assert task.choice_flag in (action.ASIS, action.APPLY) - artist, title = task.chosen_ident() - - found_items = [] - query = dbcore.AndQuery(( - dbcore.MatchQuery('artist', artist), - dbcore.MatchQuery('title', title), - )) - for other_item in lib.items(query): - # Existing items not considered duplicates. - if other_item.path == task.item.path: - continue - found_items.append(other_item) - return found_items - - def _infer_album_fields(task): """Given an album and an associated import task, massage the album-level metadata. This ensures that the album artist is set @@ -520,6 +476,30 @@ class ImportTask(object): """ self.set_candidates(*autotag.tag_album(self.items)) + def find_duplicates(self, lib): + """Return a list of albums from `lib` with the same artist and + album name as the task. + """ + artist, album = self.chosen_ident() + + if artist is None: + # As-is import with no artist. Skip check. + return [] + + duplicates = [] + task_paths = set(i.path for i in self.items if i) + duplicate_query = dbcore.AndQuery(( + dbcore.MatchQuery('albumartist', artist), + dbcore.MatchQuery('album', album), + )) + + for album in lib.albums(duplicate_query): + # Check whether the album is identical in contents, in which + # case it is not a duplicate (will be replaced). + album_paths = set(i.path for i in album.items()) + if album_paths != task_paths: + duplicates.append(album) + return duplicates # Utilities. @@ -587,7 +567,22 @@ class SingletonImportTask(ImportTask): def lookup_candidates(self): self.set_item_candidates(*autotag.tag_item(self.item)) + def find_duplicates(self, lib): + """Return a list of items from `lib` that have the same artist + and title as the task. + """ + artist, title = self.chosen_ident() + found_items = [] + query = dbcore.AndQuery(( + dbcore.MatchQuery('artist', artist), + dbcore.MatchQuery('title', title), + )) + for other_item in lib.items(query): + # Existing items not considered duplicates. + if other_item.path != self.item.path: + found_items.append(other_item) + return found_items # FIXME The inheritance relationships are inverted. This is why there @@ -917,7 +912,7 @@ def user_query(session): # The "recent" set keeps track of identifiers for recently # imported albums -- those that haven't reached the database # yet. - if ident in recent or _duplicate_check(session.lib, task): + if ident in recent or task.find_duplicates(session.lib): session.resolve_duplicate(task) session.log_choice(task, True) recent.add(ident) @@ -986,10 +981,10 @@ def apply_choices(session): duplicate_items = [] if task.remove_duplicates: if task.is_album: - for album in _duplicate_check(session.lib, task): + for album in task.find_duplicates(session.lib): duplicate_items += album.items() else: - duplicate_items = _item_duplicate_check(session.lib, task) + duplicate_items = task.find_dupliates(session.lib) log.debug('removing %i old duplicated items' % len(duplicate_items)) @@ -1129,7 +1124,7 @@ def item_query(session): # Duplicate check. if task.choice_flag in (action.ASIS, action.APPLY): ident = task.chosen_ident() - if ident in recent or _item_duplicate_check(session.lib, task): + if ident in recent or task.find_duplicates(session.lib): session.resolve_duplicate(task) session.log_choice(task, True) recent.add(ident) From 6f504f553795e195b259ef2496513892840f28dc Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 14:06:02 +0200 Subject: [PATCH 10/38] Add resolve_duplicates stage --- beets/importer.py | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index ebd4fc0ff..b9b3b9e3a 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -287,7 +287,8 @@ class ImportSession(object): if config['import']['singletons']: # Singleton importer. if config['import']['autotag']: - stages += [lookup_candidates(self), item_query(self)] + stages += [lookup_candidates(self), item_query(self), + resolve_duplicates(self)] else: stages += [item_progress(self)] else: @@ -297,7 +298,8 @@ class ImportSession(object): stages += [group_albums(self)] if config['import']['autotag']: # Only look up and query the user when autotagging. - stages += [lookup_candidates(self), user_query(self)] + stages += [lookup_candidates(self), user_query(self), + resolve_duplicates(self)] else: # When not autotagging, just display progress. stages += [show_progress(self)] @@ -863,7 +865,6 @@ def user_query(session): acces to the choice via the ``taks.choice_flag`` property and may choose to change it. """ - recent = set() task = None while True: task = yield task @@ -890,10 +891,9 @@ def user_query(session): item_query(session), ]) task = pipeline.multiple(ipl.pull()) - continue # As albums: group items by albums and create task for each album - if task.choice_flag is action.ALBUMS: + elif task.choice_flag is action.ALBUMS: def emitter(task): yield task @@ -904,14 +904,22 @@ def user_query(session): user_query(session) ]) task = pipeline.multiple(ipl.pull()) - continue - # Check for duplicates if we have a match (or ASIS). + +def resolve_duplicates(session): + """Check if a task conflicts with items or albums already imported + and ask the session to resolve this. + + Two separate stages have to be created for albums and singletons + since `chosen_ident()` returns different types of data. + """ + task = None + recent = set() + while True: + task = yield task + if task.choice_flag in (action.ASIS, action.APPLY): ident = task.chosen_ident() - # The "recent" set keeps track of identifiers for recently - # imported albums -- those that haven't reached the database - # yet. if ident in recent or task.find_duplicates(session.lib): session.resolve_duplicate(task) session.log_choice(task, True) @@ -984,7 +992,7 @@ def apply_choices(session): for album in task.find_duplicates(session.lib): duplicate_items += album.items() else: - duplicate_items = task.find_dupliates(session.lib) + duplicate_items = task.find_duplicates(session.lib) log.debug('removing %i old duplicated items' % len(duplicate_items)) @@ -1110,7 +1118,6 @@ def item_query(session): lookups. """ task = None - recent = set() while True: task = yield task if task.should_skip(): @@ -1121,14 +1128,6 @@ def item_query(session): session.log_choice(task) plugins.send('import_task_choice', session=session, task=task) - # Duplicate check. - if task.choice_flag in (action.ASIS, action.APPLY): - ident = task.chosen_ident() - if ident in recent or task.find_duplicates(session.lib): - session.resolve_duplicate(task) - session.log_choice(task, True) - recent.add(ident) - def item_progress(session): """Skips the lookup and query stages in a non-autotagged singleton From 09ef565cfcb3d962f8693fd5ba08922a1cdd60db Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 16:22:34 +0200 Subject: [PATCH 11/38] Make _infer_album_fields a method --- beets/importer.py | 98 +++++++++++++++++++++---------------------- test/test_importer.py | 21 ++++------ 2 files changed, 56 insertions(+), 63 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index b9b3b9e3a..49042111e 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -58,56 +58,6 @@ class ImportAbort(Exception): # Utilities. -def _infer_album_fields(task): - """Given an album and an associated import task, massage the - album-level metadata. This ensures that the album artist is set - and that the "compilation" flag is set automatically. - """ - assert task.is_album - assert task.items - - changes = {} - - if task.choice_flag == action.ASIS: - # Taking metadata "as-is". Guess whether this album is VA. - plur_albumartist, freq = util.plurality( - [i.albumartist or i.artist for i in task.items] - ) - if freq == len(task.items) or \ - (freq > 1 and - float(freq) / len(task.items) >= SINGLE_ARTIST_THRESH): - # Single-artist album. - changes['albumartist'] = plur_albumartist - changes['comp'] = False - else: - # VA. - changes['albumartist'] = VARIOUS_ARTISTS - changes['comp'] = True - - elif task.choice_flag == action.APPLY: - # Applying autotagged metadata. Just get AA from the first - # item. - for item in task.items: - if item is not None: - first_item = item - break - else: - assert False, "all items are None" - if not first_item.albumartist: - changes['albumartist'] = first_item.artist - if not first_item.mb_albumartistid: - changes['mb_albumartistid'] = first_item.mb_artistid - - else: - assert False - - # Apply new metadata. - for item in task.items: - if item is not None: - for k, v in changes.iteritems(): - setattr(item, k, v) - - def _open_state(): """Reads the state file, returning a dictionary.""" try: @@ -503,6 +453,49 @@ class ImportTask(object): duplicates.append(album) return duplicates + def infer_album_fields(self): + """Make the some album fields equal across `self.items` + """ + changes = {} + + if self.choice_flag == action.ASIS: + # Taking metadata "as-is". Guess whether this album is VA. + plur_albumartist, freq = util.plurality( + [i.albumartist or i.artist for i in self.items] + ) + if freq == len(self.items) or \ + (freq > 1 and + float(freq) / len(self.items) >= SINGLE_ARTIST_THRESH): + # Single-artist album. + changes['albumartist'] = plur_albumartist + changes['comp'] = False + else: + # VA. + changes['albumartist'] = VARIOUS_ARTISTS + changes['comp'] = True + + elif self.choice_flag == action.APPLY: + # Applying autotagged metadata. Just get AA from the first + # item. + # FIXME this is overly complicated. Can we assume that + # `self.items` contains only elements that are not none and + # at least one of them? + for item in self.items: + if item is not None: + first_item = item + break + else: + assert False, "all items are None" + if not first_item.albumartist: + changes['albumartist'] = first_item.artist + if not first_item.mb_albumartistid: + changes['mb_albumartistid'] = first_item.mb_artistid + + # Apply new metadata. + for item in self.items: + if item is not None: + item.update(changes) + # Utilities. def prune(self, filename): @@ -586,6 +579,9 @@ class SingletonImportTask(ImportTask): found_items.append(other_item) return found_items + def infer_album_fields(self): + raise NotImplementedError + # FIXME The inheritance relationships are inverted. This is why there # are so many methods which pass. We should introduce a new @@ -967,7 +963,7 @@ def apply_choices(session): # Infer album-level fields. if task.is_album: - _infer_album_fields(task) + task.infer_album_fields() # Find existing item entries that these are replacing (for # re-imports). Old album structures are automatically cleaned up diff --git a/test/test_importer.py b/test/test_importer.py index 50a8c1d3b..bf37b21b4 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -867,12 +867,9 @@ class InferAlbumDataTest(_common.TestCase): items=self.items) self.task.set_null_candidates() - def _infer(self): - importer._infer_album_fields(self.task) - def test_asis_homogenous_single_artist(self): self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() self.assertFalse(self.items[0].comp) self.assertEqual(self.items[0].albumartist, self.items[2].artist) @@ -881,7 +878,7 @@ class InferAlbumDataTest(_common.TestCase): self.items[1].artist = 'some other artist' self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() self.assertTrue(self.items[0].comp) self.assertEqual(self.items[0].albumartist, 'Various Artists') @@ -891,7 +888,7 @@ class InferAlbumDataTest(_common.TestCase): self.items[1].artist = 'some other artist' self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() for item in self.items: self.assertTrue(item.comp) @@ -901,7 +898,7 @@ class InferAlbumDataTest(_common.TestCase): self.items[0].artist = 'another artist' self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() self.assertFalse(self.items[0].comp) self.assertEqual(self.items[0].albumartist, self.items[2].artist) @@ -914,7 +911,7 @@ class InferAlbumDataTest(_common.TestCase): item.mb_albumartistid = 'some album artist id' self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() self.assertEqual(self.items[0].albumartist, 'some album artist') @@ -924,7 +921,7 @@ class InferAlbumDataTest(_common.TestCase): def test_apply_gets_artist_and_id(self): self.task.set_choice(AlbumMatch(0, None, {}, set(), set())) # APPLY - self._infer() + self.task.infer_album_fields() self.assertEqual(self.items[0].albumartist, self.items[0].artist) self.assertEqual(self.items[0].mb_albumartistid, @@ -936,7 +933,7 @@ class InferAlbumDataTest(_common.TestCase): item.mb_albumartistid = 'some album artist id' self.task.set_choice(AlbumMatch(0, None, {}, set(), set())) # APPLY - self._infer() + self.task.infer_album_fields() self.assertEqual(self.items[0].albumartist, 'some album artist') @@ -947,13 +944,13 @@ class InferAlbumDataTest(_common.TestCase): self.items = [self.items[0]] self.task.items = self.items self.task.set_choice(importer.action.ASIS) - self._infer() + self.task.infer_album_fields() self.assertFalse(self.items[0].comp) def test_first_item_null_apply(self): self.items[0] = None self.task.set_choice(AlbumMatch(0, None, {}, set(), set())) # APPLY - self._infer() + self.task.infer_album_fields() self.assertFalse(self.items[1].comp) self.assertEqual(self.items[1].albumartist, self.items[2].artist) From b9d92d44f330f88d00133c5e3b82cb9609a69354 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 16:28:59 +0200 Subject: [PATCH 12/38] Merge show progress and item progress tasks --- beets/importer.py | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 49042111e..b480b7ea1 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -239,8 +239,6 @@ class ImportSession(object): if config['import']['autotag']: stages += [lookup_candidates(self), item_query(self), resolve_duplicates(self)] - else: - stages += [item_progress(self)] else: # Whole-album importer. if config['import']['group_albums']: @@ -250,9 +248,9 @@ class ImportSession(object): # Only look up and query the user when autotagging. stages += [lookup_candidates(self), user_query(self), resolve_duplicates(self)] - else: - # When not autotagging, just display progress. - stages += [show_progress(self)] + + if not config['import']['autotag']: + stages += [import_asis(self)] stages += [apply_choices(self)] for stage_func in plugins.import_stages(): stages.append(plugin_stage(self, stage_func)) @@ -922,10 +920,11 @@ def resolve_duplicates(session): recent.add(ident) -def show_progress(session): - """This stage replaces the initial_lookup and user_query stages - when the importer is run without autotagging. It displays the album - name and artist as the files are added. +def import_asis(session): + """Select the `action.ASIS` choice for all tasks. + + This stage replaces the initial_lookup and user_query stages + when the importer is run without autotagging. """ task = None while True: @@ -1125,22 +1124,6 @@ def item_query(session): plugins.send('import_task_choice', session=session, task=task) -def item_progress(session): - """Skips the lookup and query stages in a non-autotagged singleton - import. Just shows progress. - """ - task = None - log.info('Importing items:') - while True: - task = yield task - if task.should_skip(): - continue - - log.info(displayable_path(task.item.path)) - task.set_null_candidates() - task.set_choice(action.ASIS) - - def group_albums(session): """Group the items of a task by albumartist and album name and create a new task for each album. Yield the tasks as a multi message. From d617e8c786acfa3ce08dd6dfd37cb9524833ca45 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 16:44:40 +0200 Subject: [PATCH 13/38] Add manipulate_files method, store item during finalize --- beets/importer.py | 88 ++++++++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 40 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index b480b7ea1..31d4ee6a8 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -377,7 +377,7 @@ class ImportTask(object): autotag.apply_metadata(self.match.info, self.match.mapping) def finalize(self, session): - """Move files, save progress and emit plugin event. + """Save items to library, save progress and emit plugin event. """ # FIXME the session argument is unfortunate. It should be # present as an attribute of the task. @@ -387,6 +387,12 @@ class ImportTask(object): self.save_progress() if config['import']['incremental']: self.save_history() + + if not self.should_skip(): + with session.lib.transaction(): + for item in self.imported_items(): + item.store() + self.cleanup() self._emit_imported(session) @@ -494,6 +500,41 @@ class ImportTask(object): if item is not None: item.update(changes) + def manipulate_files(self, move=False, copy=False, write=False, session=None): + items = self.imported_items() + # Save the original paths of all items for deletion and pruning + # in the next step (finalization). + self.old_paths = [item.path for item in items] + for item in items: + if config['import']['move']: + # Just move the file. + item.move(False) + elif config['import']['copy']: + # If it's a reimport, move in-library files and copy + # out-of-library files. Otherwise, copy and keep track + # of the old path. + old_path = item.path + if self.replaced_items[item]: + # This is a reimport. Move in-library files and copy + # out-of-library files. + if session.lib.directory in util.ancestry(old_path): + item.move(False) + # We moved the item, so remove the + # now-nonexistent file from old_paths. + self.old_paths.remove(old_path) + else: + item.move(True) + else: + # A normal import. Just copy files and keep track of + # old paths. + item.move(True) + + if config['import']['write'] and self.should_write_tags(): + item.try_write() + + plugins.send('import_task_files', session=session, task=self) + + # Utilities. def prune(self, filename): @@ -1058,45 +1099,12 @@ def manipulate_files(session): util.prune_dirs(os.path.dirname(duplicate_path), session.lib.directory) - # Move/copy/write files. - items = task.imported_items() - # Save the original paths of all items for deletion and pruning - # in the next step (finalization). - task.old_paths = [item.path for item in items] - for item in items: - if config['import']['move']: - # Just move the file. - item.move(False) - elif config['import']['copy']: - # If it's a reimport, move in-library files and copy - # out-of-library files. Otherwise, copy and keep track - # of the old path. - old_path = item.path - if task.replaced_items[item]: - # This is a reimport. Move in-library files and copy - # out-of-library files. - if session.lib.directory in util.ancestry(old_path): - item.move(False) - # We moved the item, so remove the - # now-nonexistent file from old_paths. - task.old_paths.remove(old_path) - else: - item.move(True) - else: - # A normal import. Just copy files and keep track of - # old paths. - item.move(True) - - if config['import']['write'] and task.should_write_tags(): - item.try_write() - - # Save new paths. - with session.lib.transaction(): - for item in items: - item.store() - - # Plugin event. - plugins.send('import_task_files', session=session, task=task) + task.manipulate_files( + move=config['import']['move'], + copy=config['import']['copy'], + write=config['import']['write'], + session=session, + ) # TODO Get rid of this. From 21d749e64b6dcfd46317b3b6376fa7f666d48e89 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 16:52:28 +0200 Subject: [PATCH 14/38] Rename methods to better convey their meaning --- beets/importer.py | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 31d4ee6a8..85676a18c 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -332,16 +332,12 @@ class ImportTask(object): # Logical decisions. - def should_write_tags(self): - """Should new info be written to the files' metadata?""" - if self.choice_flag == action.APPLY: - return True - elif self.choice_flag in (action.ASIS, action.TRACKS, action.SKIP): - return False - else: - assert False + @property + def apply(self): + return self.choice_flag == action.APPLY - def should_skip(self): + @property + def skip(self): return self.choice_flag == action.SKIP # Convenient data. @@ -388,7 +384,7 @@ class ImportTask(object): if config['import']['incremental']: self.save_history() - if not self.should_skip(): + if not self.skip: with session.lib.transaction(): for item in self.imported_items(): item.store() @@ -401,7 +397,7 @@ class ImportTask(object): """ # FIXME This shouldn't be here. Skipped tasks should be removed from # the pipeline. - if self.should_skip(): + if self.skip: return items = self.imported_items() @@ -422,7 +418,7 @@ class ImportTask(object): def _emit_imported(self, session): # FIXME This shouldn't be here. Skipped tasks should be removed from # the pipeline. - if self.should_skip(): + if self.skip: return album = session.lib.get_album(self.album_id) plugins.send('album_imported', lib=session.lib, album=album) @@ -529,7 +525,7 @@ class ImportTask(object): # old paths. item.move(True) - if config['import']['write'] and self.should_write_tags(): + if config['import']['write'] and self.apply: item.try_write() plugins.send('import_task_files', session=session, task=self) @@ -593,7 +589,7 @@ class SingletonImportTask(ImportTask): def _emit_imported(self, session): # FIXME This shouldn't be here. Skipped tasks should be removed from # the pipeline. - if self.should_skip(): + if self.skip: return for item in self.imported_items(): plugins.send('item_imported', lib=session.lib, item=item) @@ -654,7 +650,7 @@ class SentinelImportTask(ImportTask): # "Directory progress" sentinel for singletons progress_set(self.toppath, self.paths) - def should_skip(self): + def skip(self): return True def set_choice(self, choice): @@ -879,7 +875,7 @@ def lookup_candidates(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue plugins.send('import_task_start', session=session, task=task) @@ -903,7 +899,7 @@ def user_query(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue # Ask the user for a choice. @@ -970,7 +966,7 @@ def import_asis(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue log.info(displayable_path(task.paths)) @@ -987,7 +983,7 @@ def apply_choices(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue items = task.imported_items() @@ -997,7 +993,7 @@ def apply_choices(session): item.album_id = None # Change metadata. - if task.should_write_tags(): + if task.apply: task.apply_metadata() plugins.send('import_task_apply', session=session, task=task) @@ -1071,7 +1067,7 @@ def plugin_stage(session, func): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue func(session, task) @@ -1087,7 +1083,7 @@ def manipulate_files(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue # Remove duplicate files marked for deletion. @@ -1123,7 +1119,7 @@ def item_query(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue choice = session.choose_item(task) @@ -1142,7 +1138,7 @@ def group_albums(session): task = None while True: task = yield task - if task.should_skip(): + if task.skip: continue tasks = [] for _, items in itertools.groupby(task.items, group): From a4b5297f4f6a7a460864e00a48829e8c058650ca Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 16:58:19 +0200 Subject: [PATCH 15/38] Add do_remove_method --- beets/importer.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 85676a18c..f6bdf5209 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -372,6 +372,16 @@ class ImportTask(object): """ autotag.apply_metadata(self.match.info, self.match.mapping) + def do_remove_duplicates(self, lib): + # TODO: Bad name. Resolve naming conflict. + if self.remove_duplicates: + for duplicate_path in self.duplicate_paths: + log.debug(u'deleting replaced duplicate %s' % + util.displayable_path(duplicate_path)) + util.remove(duplicate_path) + util.prune_dirs(os.path.dirname(duplicate_path), + lib.directory) + def finalize(self, session): """Save items to library, save progress and emit plugin event. """ @@ -1086,14 +1096,7 @@ def manipulate_files(session): if task.skip: continue - # Remove duplicate files marked for deletion. - if task.remove_duplicates: - for duplicate_path in task.duplicate_paths: - log.debug(u'deleting replaced duplicate %s' % - util.displayable_path(duplicate_path)) - util.remove(duplicate_path) - util.prune_dirs(os.path.dirname(duplicate_path), - session.lib.directory) + task.do_remove_duplicates(session.lib) task.manipulate_files( move=config['import']['move'], From d78c99e7a1f4227f60118f46c68b76e79e0d68ae Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 17:13:05 +0200 Subject: [PATCH 16/38] Update library after changing item paths --- beets/importer.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index f6bdf5209..dc41278fc 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -383,7 +383,7 @@ class ImportTask(object): lib.directory) def finalize(self, session): - """Save items to library, save progress and emit plugin event. + """Save progress clean up files, and emit plugin event. """ # FIXME the session argument is unfortunate. It should be # present as an attribute of the task. @@ -394,11 +394,6 @@ class ImportTask(object): if config['import']['incremental']: self.save_history() - if not self.skip: - with session.lib.transaction(): - for item in self.imported_items(): - item.store() - self.cleanup() self._emit_imported(session) @@ -538,6 +533,10 @@ class ImportTask(object): if config['import']['write'] and self.apply: item.try_write() + with session.lib.transaction(): + for item in self.imported_items(): + item.store() + plugins.send('import_task_files', session=session, task=self) From 20c09bfe8d9ce93d2a5f13c706b66778cf37cdcd Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 17:36:30 +0200 Subject: [PATCH 17/38] Refactor logic for removing duplicate items --- beets/importer.py | 50 +++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index dc41278fc..eaba11435 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -372,14 +372,24 @@ class ImportTask(object): """ autotag.apply_metadata(self.match.info, self.match.mapping) + def duplicate_items(self, lib): + duplicate_items = [] + for album in self.find_duplicates(lib): + duplicate_items += album.items() + return duplicate_items + def do_remove_duplicates(self, lib): # TODO: Bad name. Resolve naming conflict. - if self.remove_duplicates: - for duplicate_path in self.duplicate_paths: - log.debug(u'deleting replaced duplicate %s' % - util.displayable_path(duplicate_path)) - util.remove(duplicate_path) - util.prune_dirs(os.path.dirname(duplicate_path), + duplicate_items = self.duplicate_items(lib) + log.debug('removing %i old duplicated items' % + len(duplicate_items)) + for item in duplicate_items: + item.remove() + if lib.directory in util.ancestry(item.path): + log.debug(u'deleting duplicate %s' % + util.displayable_path(item.path)) + util.remove(item.path) + util.prune_dirs(os.path.dirname(item.path), lib.directory) def finalize(self, session): @@ -623,6 +633,8 @@ class SingletonImportTask(ImportTask): found_items.append(other_item) return found_items + duplicate_items = find_duplicates + def infer_album_fields(self): raise NotImplementedError @@ -1025,27 +1037,6 @@ def apply_choices(session): log.debug('%i of %i items replaced' % (len(task.replaced_items), len(items))) - # Find old items that should be replaced as part of a duplicate - # resolution. - duplicate_items = [] - if task.remove_duplicates: - if task.is_album: - for album in task.find_duplicates(session.lib): - duplicate_items += album.items() - else: - duplicate_items = task.find_duplicates(session.lib) - log.debug('removing %i old duplicated items' % - len(duplicate_items)) - - # Delete duplicate files that are located inside the library - # directory. - task.duplicate_paths = [] - for duplicate_path in [i.path for i in duplicate_items]: - if session.lib.directory in util.ancestry(duplicate_path): - # Mark the path for deletion in the manipulate_files - # stage. - task.duplicate_paths.append(duplicate_path) - # Add items -- before path changes -- to the library. We add the # items now (rather than at the end) so that album structures # are in place before calls to destination(). @@ -1054,8 +1045,6 @@ def apply_choices(session): for replaced in task.replaced_items.itervalues(): for item in replaced: item.remove() - for item in duplicate_items: - item.remove() # Add new ones. if task.is_album: @@ -1095,7 +1084,8 @@ def manipulate_files(session): if task.skip: continue - task.do_remove_duplicates(session.lib) + if task.remove_duplicates: + task.do_remove_duplicates(session.lib) task.manipulate_files( move=config['import']['move'], From 30c9676ef35f4f427484cf98171bad3e5dfbe808 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 17:43:12 +0200 Subject: [PATCH 18/38] Add 'add' method to tasks, remove album_id property --- beets/importer.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index eaba11435..cf4da883e 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -435,8 +435,7 @@ class ImportTask(object): # the pipeline. if self.skip: return - album = session.lib.get_album(self.album_id) - plugins.send('album_imported', lib=session.lib, album=album) + plugins.send('album_imported', lib=session.lib, album=self.album) def lookup_candidates(self): """Retrieve and store candidates for this album. @@ -549,6 +548,10 @@ class ImportTask(object): plugins.send('import_task_files', session=session, task=self) + def add(self, lib): + """Add the items as an album to the library. + """ + self.album = lib.add_album(self.imported_items()) # Utilities. @@ -635,6 +638,10 @@ class SingletonImportTask(ImportTask): duplicate_items = find_duplicates + def add(self, lib): + lib.add(self.item) + + def infer_album_fields(self): raise NotImplementedError @@ -1047,14 +1054,7 @@ def apply_choices(session): item.remove() # Add new ones. - if task.is_album: - # Add an album. - album = session.lib.add_album(items) - task.album_id = album.id - else: - # Add tracks. - for item in items: - session.lib.add(item) + task.add(session.lib) def plugin_stage(session, func): From 4c0554a6c520651354364250f44ea98cb0c09bbf Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 17:56:18 +0200 Subject: [PATCH 19/38] Add 'remove_replaced' method --- beets/importer.py | 58 ++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index cf4da883e..f6eaca686 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -510,7 +510,8 @@ class ImportTask(object): if item is not None: item.update(changes) - def manipulate_files(self, move=False, copy=False, write=False, session=None): + def manipulate_files(self, move=False, copy=False, write=False, + session=None): items = self.imported_items() # Save the original paths of all items for deletion and pruning # in the next step (finalization). @@ -549,9 +550,28 @@ class ImportTask(object): plugins.send('import_task_files', session=session, task=self) def add(self, lib): - """Add the items as an album to the library. + """Add the items as an album to the library and remove replaced items. """ - self.album = lib.add_album(self.imported_items()) + with lib.transaction(): + self.remove_replaced(lib) + self.album = lib.add_album(self.imported_items()) + + def remove_replaced(self, lib): + """Removes all the items from the library that have the same + path as an item from this task. + + Records the replaced items in the `replaced_items` dictionary + """ + self.replaced_items = defaultdict(list) + for item in self.imported_items(): + dup_items = lib.items(dbcore.query.BytesQuery('path', item.path)) + self.replaced_items[item] = dup_items + for dup_item in dup_items: + log.debug('replacing item %i: %s' % + (dup_item.id, displayable_path(item.path))) + dup_item.remove() + log.debug('%i of %i items replaced' % (len(self.replaced_items), + len(self.imported_items()))) # Utilities. @@ -639,8 +659,9 @@ class SingletonImportTask(ImportTask): duplicate_items = find_duplicates def add(self, lib): - lib.add(self.item) - + with lib.transaction(): + self.remove_replaced(lib) + lib.add(self.item) def infer_album_fields(self): raise NotImplementedError @@ -1029,32 +1050,7 @@ def apply_choices(session): if task.is_album: task.infer_album_fields() - # Find existing item entries that these are replacing (for - # re-imports). Old album structures are automatically cleaned up - # when the last item is removed. - task.replaced_items = defaultdict(list) - for item in items: - dup_items = session.lib.items( - dbcore.query.BytesQuery('path', item.path) - ) - for dup_item in dup_items: - task.replaced_items[item].append(dup_item) - log.debug('replacing item %i: %s' % - (dup_item.id, displayable_path(item.path))) - log.debug('%i of %i items replaced' % (len(task.replaced_items), - len(items))) - - # Add items -- before path changes -- to the library. We add the - # items now (rather than at the end) so that album structures - # are in place before calls to destination(). - with session.lib.transaction(): - # Remove old items. - for replaced in task.replaced_items.itervalues(): - for item in replaced: - item.remove() - - # Add new ones. - task.add(session.lib) + task.add(session.lib) def plugin_stage(session, func): From 07ace040fdcb386f7c7c73e890d0732e2ffda597 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 18:01:32 +0200 Subject: [PATCH 20/38] Clear IDs in query stage --- beets/importer.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index f6eaca686..0ef278301 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -912,6 +912,9 @@ def query_tasks(session): log.debug('yielding album %i: %s - %s' % (album.id, album.albumartist, album.album)) items = list(album.items()) + for item in items: + item.id = None + item.album_id = None yield ImportTask(None, [album.item_dir()], items) @@ -1035,12 +1038,6 @@ def apply_choices(session): if task.skip: continue - items = task.imported_items() - # Clear IDs in case the items are being re-tagged. - for item in items: - item.id = None - item.album_id = None - # Change metadata. if task.apply: task.apply_metadata() From 5d7d2fe2c7ebffcdd39ef0910c5df51eeb1c053f Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 18:12:26 +0200 Subject: [PATCH 21/38] Merge item_query and user_query --- beets/importer.py | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 0ef278301..c70f1de54 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -237,7 +237,7 @@ class ImportSession(object): if config['import']['singletons']: # Singleton importer. if config['import']['autotag']: - stages += [lookup_candidates(self), item_query(self), + stages += [lookup_candidates(self), user_query(self), resolve_duplicates(self)] else: # Whole-album importer. @@ -573,6 +573,13 @@ class ImportTask(object): log.debug('%i of %i items replaced' % (len(self.replaced_items), len(self.imported_items()))) + def choose_match(self, session): + """Ask the session which match should apply and apply it. + """ + choice = session.choose_match(self) + self.set_choice(choice) + session.log_choice(self) + # Utilities. def prune(self, filename): @@ -666,6 +673,13 @@ class SingletonImportTask(ImportTask): def infer_album_fields(self): raise NotImplementedError + def choose_match(self, session): + """Ask the session which match should apply and apply it. + """ + choice = session.choose_item(self) + self.set_choice(choice) + session.log_choice(self) + # FIXME The inheritance relationships are inverted. This is why there # are so many methods which pass. We should introduce a new @@ -955,9 +969,7 @@ def user_query(session): continue # Ask the user for a choice. - choice = session.choose_match(task) - task.set_choice(choice) - session.log_choice(task) + task.choose_match(session) plugins.send('import_task_choice', session=session, task=task) # As-tracks: transition to singleton workflow. @@ -971,7 +983,7 @@ def user_query(session): ipl = pipeline.Pipeline([ emitter(task), lookup_candidates(session), - item_query(session), + user_query(session), ]) task = pipeline.multiple(ipl.pull()) @@ -1095,24 +1107,6 @@ def finalize(session): task.finalize(session) -# Singleton pipeline stages. - -def item_query(session): - """A coroutine that queries the user for input on single-item - lookups. - """ - task = None - while True: - task = yield task - if task.skip: - continue - - choice = session.choose_item(task) - task.set_choice(choice) - session.log_choice(task) - plugins.send('import_task_choice', session=session, task=task) - - def group_albums(session): """Group the items of a task by albumartist and album name and create a new task for each album. Yield the tasks as a multi message. From 35ac93728d5c8f6a1ff5774b8b42ceee8623fd99 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 18:15:39 +0200 Subject: [PATCH 22/38] Unify singleton and album stages --- beets/importer.py | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index c70f1de54..db57edc34 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -234,22 +234,16 @@ class ImportSession(object): stages = [read_tasks(self)] else: stages = [query_tasks(self)] - if config['import']['singletons']: - # Singleton importer. - if config['import']['autotag']: - stages += [lookup_candidates(self), user_query(self), - resolve_duplicates(self)] - else: - # Whole-album importer. - if config['import']['group_albums']: - # Split directory tasks into one task for each album - stages += [group_albums(self)] - if config['import']['autotag']: - # Only look up and query the user when autotagging. - stages += [lookup_candidates(self), user_query(self), - resolve_duplicates(self)] - if not config['import']['autotag']: + if config['import']['group_albums'] and \ + not config['import']['singletons']: + # Split directory tasks into one task for each album + stages += [group_albums(self)] + if config['import']['autotag']: + # Only look up and query the user when autotagging. + stages += [lookup_candidates(self), user_query(self), + resolve_duplicates(self)] + else: stages += [import_asis(self)] stages += [apply_choices(self)] for stage_func in plugins.import_stages(): From 48572aead8050df316532ad2b161b89c517498d1 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 18:42:45 +0200 Subject: [PATCH 23/38] Simplify pipeline emitter --- beets/importer.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index db57edc34..308270577 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -241,6 +241,9 @@ class ImportSession(object): stages += [group_albums(self)] if config['import']['autotag']: # Only look up and query the user when autotagging. + + # FIXME We should also resolve duplicates when not + # autotagging. stages += [lookup_candidates(self), user_query(self), resolve_duplicates(self)] else: @@ -983,11 +986,8 @@ def user_query(session): # As albums: group items by albums and create task for each album elif task.choice_flag is action.ALBUMS: - def emitter(task): - yield task - ipl = pipeline.Pipeline([ - emitter(task), + iter([task]), group_albums(session), lookup_candidates(session), user_query(session) From d8362fd03c9a9099a3064a5a6073a267abe23a06 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 18:54:29 +0200 Subject: [PATCH 24/38] Add pipeline.stage decorator --- beets/importer.py | 177 ++++++++++++++++++++--------------------- beets/util/pipeline.py | 23 ++++++ test/test_pipeline.py | 14 ++++ 3 files changed, 122 insertions(+), 92 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 308270577..063e8f326 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -929,24 +929,24 @@ def query_tasks(session): yield ImportTask(None, [album.item_dir()], items) -def lookup_candidates(session): +@pipeline.stage +def lookup_candidates(session, task): """A coroutine for performing the initial MusicBrainz lookup for an album. It accepts lists of Items and yields (items, cur_artist, cur_album, candidates, rec) tuples. If no match is found, all of the yielded parameters (except items) are None. """ - task = None - while True: - task = yield task - if task.skip: - continue + if task.skip: + return task - plugins.send('import_task_start', session=session, task=task) - log.debug('Looking up: %s' % displayable_path(task.paths)) - task.lookup_candidates() + plugins.send('import_task_start', session=session, task=task) + log.debug('Looking up: %s' % displayable_path(task.paths)) + task.lookup_candidates() + return task -def user_query(session): +@pipeline.stage +def user_query(session, task): """A coroutine for interfacing with the user about the tagging process. @@ -959,40 +959,38 @@ def user_query(session): acces to the choice via the ``taks.choice_flag`` property and may choose to change it. """ - task = None - while True: - task = yield task - if task.skip: - continue + if task.skip: + return task - # Ask the user for a choice. - task.choose_match(session) - plugins.send('import_task_choice', session=session, task=task) + # Ask the user for a choice. + task.choose_match(session) + plugins.send('import_task_choice', session=session, task=task) - # As-tracks: transition to singleton workflow. - if task.choice_flag is action.TRACKS: - # Set up a little pipeline for dealing with the singletons. - def emitter(task): - for item in task.items: - yield SingletonImportTask(item) - yield SentinelImportTask(task.toppath, task.paths) + # As-tracks: transition to singleton workflow. + if task.choice_flag is action.TRACKS: + # Set up a little pipeline for dealing with the singletons. + def emitter(task): + for item in task.items: + yield SingletonImportTask(item) + yield SentinelImportTask(task.toppath, task.paths) - ipl = pipeline.Pipeline([ - emitter(task), - lookup_candidates(session), - user_query(session), - ]) - task = pipeline.multiple(ipl.pull()) + ipl = pipeline.Pipeline([ + emitter(task), + lookup_candidates(session), + user_query(session), + ]) + task = pipeline.multiple(ipl.pull()) - # As albums: group items by albums and create task for each album - elif task.choice_flag is action.ALBUMS: - ipl = pipeline.Pipeline([ - iter([task]), - group_albums(session), - lookup_candidates(session), - user_query(session) - ]) - task = pipeline.multiple(ipl.pull()) + # As albums: group items by albums and create task for each album + elif task.choice_flag is action.ALBUMS: + ipl = pipeline.Pipeline([ + iter([task]), + group_albums(session), + lookup_candidates(session), + user_query(session) + ]) + task = pipeline.multiple(ipl.pull()) + return task def resolve_duplicates(session): @@ -1015,90 +1013,85 @@ def resolve_duplicates(session): recent.add(ident) -def import_asis(session): +@pipeline.stage +def import_asis(session, task): """Select the `action.ASIS` choice for all tasks. This stage replaces the initial_lookup and user_query stages when the importer is run without autotagging. """ - task = None - while True: - task = yield task - if task.skip: - continue + if task.skip: + return task - log.info(displayable_path(task.paths)) + log.info(displayable_path(task.paths)) - # Behave as if ASIS were selected. - task.set_null_candidates() - task.set_choice(action.ASIS) + # Behave as if ASIS were selected. + task.set_null_candidates() + task.set_choice(action.ASIS) + return task -def apply_choices(session): +@pipeline.stage +def apply_choices(session, task): """A coroutine for applying changes to albums and singletons during the autotag process. """ - task = None - while True: - task = yield task - if task.skip: - continue + if task.skip: + return task - # Change metadata. - if task.apply: - task.apply_metadata() - plugins.send('import_task_apply', session=session, task=task) + # Change metadata. + if task.apply: + task.apply_metadata() + plugins.send('import_task_apply', session=session, task=task) - # Infer album-level fields. - if task.is_album: - task.infer_album_fields() + # Infer album-level fields. + if task.is_album: + task.infer_album_fields() - task.add(session.lib) + task.add(session.lib) + return task -def plugin_stage(session, func): +@pipeline.stage +def plugin_stage(session, func, task): """A coroutine (pipeline stage) that calls the given function with each non-skipped import task. These stages occur between applying metadata changes and moving/copying/writing files. """ - task = None - while True: - task = yield task - if task.skip: - continue - func(session, task) + if task.skip: + return task + func(session, task) - # Stage may modify DB, so re-load cached item data. - for item in task.imported_items(): - item.load() + # Stage may modify DB, so re-load cached item data. + for item in task.imported_items(): + item.load() + return task -def manipulate_files(session): +@pipeline.stage +def manipulate_files(session, task): """A coroutine (pipeline stage) that performs necessary file manipulations *after* items have been added to the library. """ - task = None - while True: - task = yield task - if task.skip: - continue + if task.skip: + return task - if task.remove_duplicates: - task.do_remove_duplicates(session.lib) + if task.remove_duplicates: + task.do_remove_duplicates(session.lib) - task.manipulate_files( - move=config['import']['move'], - copy=config['import']['copy'], - write=config['import']['write'], - session=session, - ) + task.manipulate_files( + move=config['import']['move'], + copy=config['import']['copy'], + write=config['import']['write'], + session=session, + ) + return task # TODO Get rid of this. -def finalize(session): - while True: - task = yield - task.finalize(session) +@pipeline.stage +def finalize(session, task): + task.finalize(session) def group_albums(session): diff --git a/beets/util/pipeline.py b/beets/util/pipeline.py index 95b77b4da..1591122e6 100644 --- a/beets/util/pipeline.py +++ b/beets/util/pipeline.py @@ -137,6 +137,29 @@ def multiple(messages): return MultiMessage(messages) +def stage(func): + """Decorate a function to become a simple stage. + + >>> @stage + ... def add(n, i): + ... return i + n + >>> pipe = Pipeline([ + ... iter([1, 2, 3]), + ... add(2), + ... ]) + >>> list(pipe.pull()) + [3, 4, 5] + """ + + def coro(*args): + task = None + while True: + task = yield task + task = func(*(args + (task,))) + return coro + + + def _allmsgs(obj): """Returns a list of all the messages encapsulated in obj. If obj is a MultiMessage, returns its enclosed messages. If obj is BUBBLE, diff --git a/test/test_pipeline.py b/test/test_pipeline.py index cd371af12..62a087eb2 100644 --- a/test/test_pipeline.py +++ b/test/test_pipeline.py @@ -208,6 +208,20 @@ class MultiMessageTest(unittest.TestCase): self.assertEqual(list(pl.pull()), [0, 0, 1, -1, 2, -2, 3, -3, 4, -4]) +class StageDecoratorTest(unittest.TestCase): + + def test_decorator(self): + @pipeline.stage + def add(n, i): + return i + n + + pl = pipeline.Pipeline([ + iter([1, 2, 3]), + add(2) + ]) + self.assertEqual(list(pl.pull()), [3, 4, 5]) + + def suite(): return unittest.TestLoader().loadTestsFromName(__name__) From 495c49703c039ab44a32f8c4abd0939f7377957c Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 19 Apr 2014 19:04:15 +0200 Subject: [PATCH 25/38] Add pipeline.mutator_stage --- beets/importer.py | 27 ++++++++++++--------------- beets/util/pipeline.py | 22 ++++++++++++++++++++++ test/test_pipeline.py | 14 +++++++++++++- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 063e8f326..8be2c2c8e 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -929,7 +929,7 @@ def query_tasks(session): yield ImportTask(None, [album.item_dir()], items) -@pipeline.stage +@pipeline.mutator_stage def lookup_candidates(session, task): """A coroutine for performing the initial MusicBrainz lookup for an album. It accepts lists of Items and yields @@ -937,12 +937,11 @@ def lookup_candidates(session, task): is found, all of the yielded parameters (except items) are None. """ if task.skip: - return task + return plugins.send('import_task_start', session=session, task=task) log.debug('Looking up: %s' % displayable_path(task.paths)) task.lookup_candidates() - return task @pipeline.stage @@ -990,6 +989,7 @@ def user_query(session, task): user_query(session) ]) task = pipeline.multiple(ipl.pull()) + return task @@ -1013,7 +1013,7 @@ def resolve_duplicates(session): recent.add(ident) -@pipeline.stage +@pipeline.mutator_stage def import_asis(session, task): """Select the `action.ASIS` choice for all tasks. @@ -1021,23 +1021,22 @@ def import_asis(session, task): when the importer is run without autotagging. """ if task.skip: - return task + return log.info(displayable_path(task.paths)) # Behave as if ASIS were selected. task.set_null_candidates() task.set_choice(action.ASIS) - return task -@pipeline.stage +@pipeline.mutator_stage def apply_choices(session, task): """A coroutine for applying changes to albums and singletons during the autotag process. """ if task.skip: - return task + return # Change metadata. if task.apply: @@ -1049,32 +1048,31 @@ def apply_choices(session, task): task.infer_album_fields() task.add(session.lib) - return task -@pipeline.stage +@pipeline.mutator_stage def plugin_stage(session, func, task): """A coroutine (pipeline stage) that calls the given function with each non-skipped import task. These stages occur between applying metadata changes and moving/copying/writing files. """ if task.skip: - return task + return + func(session, task) # Stage may modify DB, so re-load cached item data. for item in task.imported_items(): item.load() - return task -@pipeline.stage +@pipeline.mutator_stage def manipulate_files(session, task): """A coroutine (pipeline stage) that performs necessary file manipulations *after* items have been added to the library. """ if task.skip: - return task + return if task.remove_duplicates: task.do_remove_duplicates(session.lib) @@ -1085,7 +1083,6 @@ def manipulate_files(session, task): write=config['import']['write'], session=session, ) - return task # TODO Get rid of this. diff --git a/beets/util/pipeline.py b/beets/util/pipeline.py index 1591122e6..d267789c8 100644 --- a/beets/util/pipeline.py +++ b/beets/util/pipeline.py @@ -159,6 +159,28 @@ def stage(func): return coro +def mutator_stage(func): + """Decorate a function that manipulates items in a coroutine to + become a simple stage. + + >>> @mutator_stage + ... def setkey(key, item): + ... item[key] = True + >>> pipe = Pipeline([ + ... iter([{'x': False}, {'a': False}]), + ... setkey('x'), + ... ]) + >>> list(pipe.pull()) + [{'x': True}, {'a': False, 'x': True}] + """ + + def coro(*args): + task = None + while True: + task = yield task + func(*(args + (task,))) + return coro + def _allmsgs(obj): """Returns a list of all the messages encapsulated in obj. If obj diff --git a/test/test_pipeline.py b/test/test_pipeline.py index 62a087eb2..0c4de6836 100644 --- a/test/test_pipeline.py +++ b/test/test_pipeline.py @@ -210,7 +210,7 @@ class MultiMessageTest(unittest.TestCase): class StageDecoratorTest(unittest.TestCase): - def test_decorator(self): + def test_stage_decorator(self): @pipeline.stage def add(n, i): return i + n @@ -221,6 +221,18 @@ class StageDecoratorTest(unittest.TestCase): ]) self.assertEqual(list(pl.pull()), [3, 4, 5]) + def test_mutator_stage_decorator(self): + @pipeline.mutator_stage + def setkey(key, item): + item[key] = True + + pl = pipeline.Pipeline([ + iter([{'x': False}, {'a': False}]), + setkey('x'), + ]) + self.assertEqual(list(pl.pull()), + [{'x': True}, {'a': False, 'x': True}]) + def suite(): return unittest.TestLoader().loadTestsFromName(__name__) From a08ec5b5373310ce41b5617dce1da3ace60d4ccf Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sun, 20 Apr 2014 13:17:07 +0200 Subject: [PATCH 26/38] Attach config to session --- beets/importer.py | 60 ++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 8be2c2c8e..a5d6db6f5 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -155,12 +155,14 @@ class ImportSession(object): if self.paths: self.paths = map(normpath, self.paths) - def _amend_config(self): - """Make implied changes the importer configuration. + def set_config(self, config): + """Set `config` property from global import config and make + implied changes. """ # FIXME: Maybe this function should not exist and should instead # provide "decision wrappers" like "should_resume()", etc. - iconfig = config['import'] + iconfig = dict(config) + self.config = iconfig # Incremental and progress are mutually exclusive. if iconfig['incremental']: @@ -180,7 +182,7 @@ class ImportSession(object): if not iconfig['copy']: iconfig['delete'] = False - self.want_resume = iconfig['resume'].as_choice([True, False, 'ask']) + self.want_resume = config['resume'].as_choice([True, False, 'ask']) def tag_log(self, status, paths): """Log a message about a given album to logfile. The status should @@ -227,7 +229,7 @@ class ImportSession(object): def run(self): """Run the import task. """ - self._amend_config() + self.set_config(config['import']) # Set up the pipeline. if self.query is None: @@ -235,11 +237,11 @@ class ImportSession(object): else: stages = [query_tasks(self)] - if config['import']['group_albums'] and \ - not config['import']['singletons']: + if self.config['group_albums'] and \ + not self.config['singletons']: # Split directory tasks into one task for each album stages += [group_albums(self)] - if config['import']['autotag']: + if self.config['autotag']: # Only look up and query the user when autotagging. # FIXME We should also resolve duplicates when not @@ -398,13 +400,13 @@ class ImportTask(object): # Update progress. if session.want_resume: self.save_progress() - if config['import']['incremental']: + if session.config['incremental']: self.save_history() - self.cleanup() + self.cleanup(session) self._emit_imported(session) - def cleanup(self): + def cleanup(self, session): """Remove and prune imported paths. """ # FIXME This shouldn't be here. Skipped tasks should be removed from @@ -414,7 +416,7 @@ class ImportTask(object): items = self.imported_items() # When copying and deleting originals, delete old files. - if config['import']['copy'] and config['import']['delete']: + if session.config['copy'] and session.config['delete']: new_paths = [os.path.realpath(item.path) for item in items] for old_path in self.old_paths: # Only delete files that were actually copied. @@ -423,7 +425,7 @@ class ImportTask(object): self.prune(old_path) # When moving, prune empty directories containing the original files. - elif config['import']['move']: + elif session.config['move']: for old_path in self.old_paths: self.prune(old_path) @@ -514,10 +516,10 @@ class ImportTask(object): # in the next step (finalization). self.old_paths = [item.path for item in items] for item in items: - if config['import']['move']: + if session.config['move']: # Just move the file. item.move(False) - elif config['import']['copy']: + elif session.config['copy']: # If it's a reimport, move in-library files and copy # out-of-library files. Otherwise, copy and keep track # of the old path. @@ -537,7 +539,7 @@ class ImportTask(object): # old paths. item.move(True) - if config['import']['write'] and self.apply: + if session.config['write'] and self.apply: item.try_write() with session.lib.transaction(): @@ -719,7 +721,7 @@ class SentinelImportTask(ImportTask): def set_candidates(self, cur_artist, cur_album, candidates, rec): raise NotImplementedError - def cleanup(self): + def cleanup(self, session): pass def _emit_imported(self, session): @@ -773,7 +775,7 @@ class ArchiveImportTask(SentinelImportTask): return cls._handlers - def cleanup(self): + def cleanup(self, session): """Removes the temporary directory the archive was extracted to. """ if self.extracted: @@ -805,7 +807,7 @@ def read_tasks(session): import, yields single-item tasks instead. """ # Look for saved incremental directories. - if config['import']['incremental']: + if session.config['incremental']: incremental_skipped = 0 history_dirs = history_get() @@ -813,7 +815,7 @@ def read_tasks(session): # Extract archives. archive_task = None if ArchiveImportTask.is_archive(syspath(toppath)): - if not (config['import']['move'] or config['import']['copy']): + if not (session.config['move'] or session.config['copy']): log.warn("Archive importing requires either " "'copy' or 'move' to be enabled.") continue @@ -839,14 +841,14 @@ def read_tasks(session): util.displayable_path(toppath) )) continue - if config['import']['singletons']: + if session.config['singletons']: yield SingletonImportTask(item) else: yield ImportTask(toppath, [toppath], [item]) continue # A flat album import merges all items into one album. - if config['import']['flat'] and not config['import']['singletons']: + if session.config['flat'] and not session.config['singletons']: all_items = [] for _, items in autotag.albums_in_dir(toppath): all_items += items @@ -879,7 +881,7 @@ def read_tasks(session): continue # When incremental, skip paths in the history. - if config['import']['incremental'] \ + if session.config['incremental'] \ and tuple(paths) in history_dirs: log.debug(u'Skipping previously-imported path: %s' % displayable_path(paths)) @@ -887,7 +889,7 @@ def read_tasks(session): continue # Yield all the necessary tasks. - if config['import']['singletons']: + if session.config['singletons']: for item in items: yield SingletonImportTask(item) yield SentinelImportTask(toppath, paths) @@ -902,7 +904,7 @@ def read_tasks(session): yield archive_task # Show skipped directories. - if config['import']['incremental'] and incremental_skipped: + if session.config['incremental'] and incremental_skipped: log.info(u'Incremental import: skipped %i directories.' % incremental_skipped) @@ -912,7 +914,7 @@ def query_tasks(session): Instead of finding files from the filesystem, a query is used to match items from the library. """ - if config['import']['singletons']: + if session.config['singletons']: # Search for items. for item in session.lib.items(session.query): yield SingletonImportTask(item) @@ -1078,9 +1080,9 @@ def manipulate_files(session, task): task.do_remove_duplicates(session.lib) task.manipulate_files( - move=config['import']['move'], - copy=config['import']['copy'], - write=config['import']['write'], + move=session.config['move'], + copy=session.config['copy'], + write=session.config['write'], session=session, ) From 8b3227b14901c66433eace9adacab6121f2bb3c1 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Thu, 24 Apr 2014 22:18:49 -0700 Subject: [PATCH 27/38] restore a comment --- beets/importer.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/beets/importer.py b/beets/importer.py index a5d6db6f5..a290faad0 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -925,9 +925,13 @@ def query_tasks(session): log.debug('yielding album %i: %s - %s' % (album.id, album.albumartist, album.album)) items = list(album.items()) + + # Clear IDs from re-tagged items so they appear "fresh" when + # we add them back to the library. for item in items: item.id = None item.album_id = None + yield ImportTask(None, [album.item_dir()], items) From 07dbe042f510a7002fbe5040e6c9ed227437e943 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sun, 20 Apr 2014 18:04:45 +0200 Subject: [PATCH 28/38] Move set_candidate logic --- beets/importer.py | 34 ++++++++-------------------------- test/test_ui.py | 26 -------------------------- 2 files changed, 8 insertions(+), 52 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index a290faad0..6d2d9ed2b 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -283,15 +283,6 @@ class ImportTask(object): self.remove_duplicates = False self.is_album = True - def set_candidates(self, cur_artist, cur_album, candidates, rec): - """Sets the candidates for this album matched by the - `autotag.tag_album` method. - """ - self.cur_artist = cur_artist - self.cur_album = cur_album - self.candidates = candidates - self.rec = rec - def set_null_candidates(self): """Set the candidates to indicate no album match was found. """ @@ -300,9 +291,6 @@ class ImportTask(object): self.candidates = None self.rec = None - def set_item_candidates(self, candidates, rec): - raise NotImplementedError - def set_choice(self, choice): """Given an AlbumMatch or TrackMatch object or an action constant, indicates that an action has been selected for this task. @@ -439,7 +427,11 @@ class ImportTask(object): def lookup_candidates(self): """Retrieve and store candidates for this album. """ - self.set_candidates(*autotag.tag_album(self.items)) + artist, album, candidates, recommendation = autotag.tag_album(self.items) + self.cur_artist = artist + self.cur_album = album + self.candidates = candidates + self.rec = recommendation def find_duplicates(self, lib): """Return a list of albums from `lib` with the same artist and @@ -622,15 +614,6 @@ class SingletonImportTask(ImportTask): # TODO we should also save history for singletons pass - def set_item_candidates(self, candidates, rec): - """Set the match for a single-item task.""" - assert self.item is not None - self.candidates = candidates - self.rec = rec - - def set_candidates(self, cur_artist, cur_album, candidates, rec): - raise NotImplementedError - def apply_metadata(self): autotag.apply_item_metadata(self.item, self.match.info) @@ -643,7 +626,9 @@ class SingletonImportTask(ImportTask): plugins.send('item_imported', lib=session.lib, item=item) def lookup_candidates(self): - self.set_item_candidates(*autotag.tag_item(self.item)) + candidates, recommendation = autotag.tag_item(self.item) + self.candidates = candidates + self.rec = recommendation def find_duplicates(self, lib): """Return a list of items from `lib` that have the same artist @@ -718,9 +703,6 @@ class SentinelImportTask(ImportTask): def set_choice(self, choice): raise NotImplementedError - def set_candidates(self, cur_artist, cur_album, candidates, rec): - raise NotImplementedError - def cleanup(self, session): pass diff --git a/test/test_ui.py b/test/test_ui.py index 54cbb533f..d28587c0a 100644 --- a/test/test_ui.py +++ b/test/test_ui.py @@ -473,32 +473,6 @@ class PrintTest(_common.TestCase): del os.environ['LC_CTYPE'] -class AutotagTest(_common.TestCase): - def setUp(self): - super(AutotagTest, self).setUp() - self.io.install() - - def _no_candidates_test(self, result): - task = importer.ImportTask( - 'toppath', - 'path', - [_common.item()], - ) - task.set_candidates('artist', 'album', [], autotag.Recommendation.none) - session = _common.import_session(cli=True) - res = session.choose_match(task) - self.assertEqual(res, result) - self.assertTrue('No match' in self.io.getoutput()) - - def test_choose_match_with_no_candidates_skip(self): - self.io.addinput('s') - self._no_candidates_test(importer.action.SKIP) - - def test_choose_match_with_no_candidates_asis(self): - self.io.addinput('u') - self._no_candidates_test(importer.action.ASIS) - - class ImportTest(_common.TestCase): def test_quiet_timid_disallowed(self): config['import']['quiet'] = True From e38a33a569e0470ad62f18d16ae94a67e05eb796 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Fri, 25 Apr 2014 13:29:37 +0200 Subject: [PATCH 29/38] Clarify comments in importer --- beets/importer.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 6d2d9ed2b..21e7c1233 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -397,8 +397,8 @@ class ImportTask(object): def cleanup(self, session): """Remove and prune imported paths. """ - # FIXME This shouldn't be here. Skipped tasks should be removed from - # the pipeline. + # FIXME This shouldn't be here. Skipping should be handled in + # the stages. if self.skip: return items = self.imported_items() @@ -418,8 +418,8 @@ class ImportTask(object): self.prune(old_path) def _emit_imported(self, session): - # FIXME This shouldn't be here. Skipped tasks should be removed from - # the pipeline. + # FIXME This shouldn't be here. Skipping should be handled in + # the stages. if self.skip: return plugins.send('album_imported', lib=session.lib, album=self.album) @@ -925,6 +925,8 @@ def lookup_candidates(session, task): is found, all of the yielded parameters (except items) are None. """ if task.skip: + # FIXME This gets duplicated a lot. We need a better + # abstraction. return plugins.send('import_task_start', session=session, task=task) @@ -1073,7 +1075,8 @@ def manipulate_files(session, task): ) -# TODO Get rid of this. +# FIXME Boilerplate. Maybe we should move this to the `manipulate_files` +# stage. @pipeline.stage def finalize(session, task): task.finalize(session) From ac9ee8ed66834bb9482684f9572761e616e2c476 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Fri, 25 Apr 2014 13:36:17 +0200 Subject: [PATCH 30/38] Decouple session and import task methods --- beets/importer.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 21e7c1233..76f34036f 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -391,12 +391,15 @@ class ImportTask(object): if session.config['incremental']: self.save_history() - self.cleanup(session) - self._emit_imported(session) + self.cleanup(copy=session.config['copy'], delete=session.config['delete'], + move=session.config['move']) + self._emit_imported(session.lib) - def cleanup(self, session): + def cleanup(self, copy=False, delete=False, move=False): """Remove and prune imported paths. """ + # FIXME Maybe the keywords should be task properties. + # FIXME This shouldn't be here. Skipping should be handled in # the stages. if self.skip: @@ -404,7 +407,7 @@ class ImportTask(object): items = self.imported_items() # When copying and deleting originals, delete old files. - if session.config['copy'] and session.config['delete']: + if copy and delete: new_paths = [os.path.realpath(item.path) for item in items] for old_path in self.old_paths: # Only delete files that were actually copied. @@ -413,16 +416,16 @@ class ImportTask(object): self.prune(old_path) # When moving, prune empty directories containing the original files. - elif session.config['move']: + elif move: for old_path in self.old_paths: self.prune(old_path) - def _emit_imported(self, session): + def _emit_imported(self, lib): # FIXME This shouldn't be here. Skipping should be handled in # the stages. if self.skip: return - plugins.send('album_imported', lib=session.lib, album=self.album) + plugins.send('album_imported', lib=lib, album=self.album) def lookup_candidates(self): """Retrieve and store candidates for this album. @@ -617,13 +620,13 @@ class SingletonImportTask(ImportTask): def apply_metadata(self): autotag.apply_item_metadata(self.item, self.match.info) - def _emit_imported(self, session): + def _emit_imported(self, lib): # FIXME This shouldn't be here. Skipped tasks should be removed from # the pipeline. if self.skip: return for item in self.imported_items(): - plugins.send('item_imported', lib=session.lib, item=item) + plugins.send('item_imported', lib=lib, item=item) def lookup_candidates(self): candidates, recommendation = autotag.tag_item(self.item) @@ -703,7 +706,7 @@ class SentinelImportTask(ImportTask): def set_choice(self, choice): raise NotImplementedError - def cleanup(self, session): + def cleanup(self, **kwargs): pass def _emit_imported(self, session): @@ -757,7 +760,7 @@ class ArchiveImportTask(SentinelImportTask): return cls._handlers - def cleanup(self, session): + def cleanup(self, **kwargs): """Removes the temporary directory the archive was extracted to. """ if self.extracted: From e96753f96a40af6e3428b5a80ef8542fe6d3df85 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Fri, 25 Apr 2014 13:52:03 +0200 Subject: [PATCH 31/38] Plugins use import_task.album --- beets/importer.py | 21 +++++++++++++++++---- beetsplug/fetchart.py | 5 ++--- beetsplug/lastgenre/__init__.py | 2 +- beetsplug/replaygain.py | 3 +-- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 76f34036f..5206562de 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -391,7 +391,8 @@ class ImportTask(object): if session.config['incremental']: self.save_history() - self.cleanup(copy=session.config['copy'], delete=session.config['delete'], + self.cleanup(copy=session.config['copy'], + delete=session.config['delete'], move=session.config['move']) self._emit_imported(session.lib) @@ -430,7 +431,8 @@ class ImportTask(object): def lookup_candidates(self): """Retrieve and store candidates for this album. """ - artist, album, candidates, recommendation = autotag.tag_album(self.items) + artist, album, candidates, recommendation = \ + autotag.tag_album(self.items) self.cur_artist = artist self.cur_album = album self.candidates = candidates @@ -574,6 +576,13 @@ class ImportTask(object): self.set_choice(choice) session.log_choice(self) + def reload(self): + """Reload albums and items from the database. + """ + for item in self.imported_items(): + item.load() + self.album.load() + # Utilities. def prune(self, filename): @@ -667,6 +676,9 @@ class SingletonImportTask(ImportTask): self.set_choice(choice) session.log_choice(self) + def reload(self): + self.item.load() + # FIXME The inheritance relationships are inverted. This is why there # are so many methods which pass. We should introduce a new @@ -1055,8 +1067,9 @@ def plugin_stage(session, func, task): func(session, task) # Stage may modify DB, so re-load cached item data. - for item in task.imported_items(): - item.load() + # FIXME Importer plugins should not modify the database but instead + # the albums and items attached to tasks. + task.reload() @pipeline.mutator_stage diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 1b47e07c3..4b4f53bea 100644 --- a/beetsplug/fetchart.py +++ b/beetsplug/fetchart.py @@ -278,8 +278,7 @@ class FetchArtPlugin(BeetsPlugin): # For any other choices (e.g., TRACKS), do nothing. return - album = session.lib.get_album(task.album_id) - path = art_for_album(album, task.paths, self.maxwidth, local) + path = art_for_album(task.album, task.paths, self.maxwidth, local) if path: self.art_paths[task] = path @@ -290,7 +289,7 @@ class FetchArtPlugin(BeetsPlugin): if task in self.art_paths: path = self.art_paths.pop(task) - album = session.lib.get_album(task.album_id) + album = task.album src_removed = (config['import']['delete'].get(bool) or config['import']['move'].get(bool)) album.set_art(path, not src_removed) diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index eea428dfa..6646e6af3 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -382,7 +382,7 @@ class LastGenrePlugin(plugins.BeetsPlugin): def imported(self, session, task): """Event hook called when an import task finishes.""" if task.is_album: - album = session.lib.get_album(task.album_id) + album = task.album album.genre, src = self._get_genre(album) log.debug(u'added last.fm album genre ({0}): {1}'.format( src, album.genre diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index 87a4eb2dc..acf7afc86 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -601,8 +601,7 @@ class ReplayGainPlugin(BeetsPlugin): return if task.is_album: - album = session.lib.get_album(task.album_id) - self.handle_album(album, False) + self.handle_album(task.album, False) else: self.handle_track(task.item, False) From 7700f19135b94ca49247d8d1640480c8ccc4848d Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Fri, 25 Apr 2014 13:56:01 +0200 Subject: [PATCH 32/38] Added comment on importer ui --- beets/importer.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/beets/importer.py b/beets/importer.py index 5206562de..52977c250 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -246,6 +246,9 @@ class ImportSession(object): # FIXME We should also resolve duplicates when not # autotagging. + # FIXME user_query and resolve_duplicates use the UI. + # Running them parallel in different stages might mess up + # the output. stages += [lookup_candidates(self), user_query(self), resolve_duplicates(self)] else: From cb3354bd6e524bb3fd62285173b3b5af72278303 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 26 Apr 2014 18:19:52 +0200 Subject: [PATCH 33/38] Resolve duplicates in user_query stage --- beets/importer.py | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 52977c250..e306c8031 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -150,6 +150,7 @@ class ImportSession(object): self.logfile = logfile self.paths = paths self.query = query + self.seen_idents = set() # Normalize the paths. if self.paths: @@ -246,11 +247,7 @@ class ImportSession(object): # FIXME We should also resolve duplicates when not # autotagging. - # FIXME user_query and resolve_duplicates use the UI. - # Running them parallel in different stages might mess up - # the output. - stages += [lookup_candidates(self), user_query(self), - resolve_duplicates(self)] + stages += [lookup_candidates(self), user_query(self)] else: stages += [import_asis(self)] stages += [apply_choices(self)] @@ -998,27 +995,21 @@ def user_query(session, task): ]) task = pipeline.multiple(ipl.pull()) + resolve_duplicates(session, task) + return task -def resolve_duplicates(session): +def resolve_duplicates(session, task): """Check if a task conflicts with items or albums already imported and ask the session to resolve this. - - Two separate stages have to be created for albums and singletons - since `chosen_ident()` returns different types of data. """ - task = None - recent = set() - while True: - task = yield task - - if task.choice_flag in (action.ASIS, action.APPLY): - ident = task.chosen_ident() - if ident in recent or task.find_duplicates(session.lib): - session.resolve_duplicate(task) - session.log_choice(task, True) - recent.add(ident) + if task.choice_flag in (action.ASIS, action.APPLY): + ident = task.chosen_ident() + if ident in session.seen_ident or task.find_duplicates(session.lib): + session.resolve_duplicate(task) + session.log_choice(task, True) + session.seen_ident.add(ident) @pipeline.mutator_stage From ba60431319f8284211c68cffadd37a2934ae0e14 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 26 Apr 2014 18:31:54 +0200 Subject: [PATCH 34/38] Fix typo and return early from stage --- beets/importer.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index e306c8031..b62d449fd 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -983,20 +983,20 @@ def user_query(session, task): lookup_candidates(session), user_query(session), ]) - task = pipeline.multiple(ipl.pull()) + return pipeline.multiple(ipl.pull()) # As albums: group items by albums and create task for each album - elif task.choice_flag is action.ALBUMS: + if task.choice_flag is action.ALBUMS: ipl = pipeline.Pipeline([ iter([task]), group_albums(session), lookup_candidates(session), user_query(session) ]) - task = pipeline.multiple(ipl.pull()) + return pipeline.multiple(ipl.pull()) + resolve_duplicates(session, task) - return task @@ -1006,10 +1006,10 @@ def resolve_duplicates(session, task): """ if task.choice_flag in (action.ASIS, action.APPLY): ident = task.chosen_ident() - if ident in session.seen_ident or task.find_duplicates(session.lib): + if ident in session.seen_idents or task.find_duplicates(session.lib): session.resolve_duplicate(task) session.log_choice(task, True) - session.seen_ident.add(ident) + session.seen_idents.add(ident) @pipeline.mutator_stage From 1d787e0b38aefbfe3ac1067cc6240f354d72ce18 Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Sat, 26 Apr 2014 18:51:56 +0200 Subject: [PATCH 35/38] Adapt FetchArt tests to new task api --- beets/importer.py | 1 - test/test_art.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index b62d449fd..ff8e6284b 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -995,7 +995,6 @@ def user_query(session, task): ]) return pipeline.multiple(ipl.pull()) - resolve_duplicates(session, task) return task diff --git a/test/test_art.py b/test/test_art.py index 2a4293a8f..01e786e6e 100644 --- a/test/test_art.py +++ b/test/test_art.py @@ -222,7 +222,7 @@ class ArtImporterTest(_common.TestCase): # Import task for the coroutine. self.task = importer.ImportTask(None, None, [self.i]) self.task.is_album = True - self.task.album_id = self.album.id + self.task.album = self.album info = AlbumInfo( album = 'some album', album_id = 'albumid', From 38eba4af31ac3d65a644621c487021ee936e3e42 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 26 Apr 2014 20:07:26 -0700 Subject: [PATCH 36/38] Remove obsolete null checks Along with their tests. Background: https://github.com/sampsyo/beets/pull/720/files#r12027386 --- beets/importer.py | 20 +++++--------------- test/test_importer.py | 7 ------- 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index ff8e6284b..0fc11f660 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -487,24 +487,14 @@ class ImportTask(object): elif self.choice_flag == action.APPLY: # Applying autotagged metadata. Just get AA from the first # item. - # FIXME this is overly complicated. Can we assume that - # `self.items` contains only elements that are not none and - # at least one of them? - for item in self.items: - if item is not None: - first_item = item - break - else: - assert False, "all items are None" - if not first_item.albumartist: - changes['albumartist'] = first_item.artist - if not first_item.mb_albumartistid: - changes['mb_albumartistid'] = first_item.mb_artistid + if not self.items[0].albumartist: + changes['albumartist'] = self.items[0].artist + if not self.items[0].mb_albumartistid: + changes['mb_albumartistid'] = self.items[0].mb_artistid # Apply new metadata. for item in self.items: - if item is not None: - item.update(changes) + item.update(changes) def manipulate_files(self, move=False, copy=False, write=False, session=None): diff --git a/test/test_importer.py b/test/test_importer.py index bf37b21b4..371e9ab1c 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -947,13 +947,6 @@ class InferAlbumDataTest(_common.TestCase): self.task.infer_album_fields() self.assertFalse(self.items[0].comp) - def test_first_item_null_apply(self): - self.items[0] = None - self.task.set_choice(AlbumMatch(0, None, {}, set(), set())) # APPLY - self.task.infer_album_fields() - self.assertFalse(self.items[1].comp) - self.assertEqual(self.items[1].albumartist, self.items[2].artist) - class ImportDuplicateAlbumTest(unittest.TestCase, TestHelper): From e9355f336b2d9f942696bd804faa2c099da6c624 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 26 Apr 2014 20:21:20 -0700 Subject: [PATCH 37/38] Rename remove_duplicates and do_remove_duplicates --- beets/importer.py | 13 ++++++------- beets/ui/commands.py | 2 +- test/helper.py | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 0fc11f660..4cfe1c7e3 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -202,7 +202,7 @@ class ImportSession(object): paths = task.paths if duplicate: # Duplicate: log all three choices (skip, keep both, and trump). - if task.remove_duplicates: + if task.should_remove_duplicates: self.tag_log('duplicate-replace', paths) elif task.choice_flag in (action.ASIS, action.APPLY): self.tag_log('duplicate-keep', paths) @@ -280,7 +280,7 @@ class ImportTask(object): self.items = items self.choice_flag = None # TODO remove this eventually - self.remove_duplicates = False + self.should_remove_duplicates = False self.is_album = True def set_null_candidates(self): @@ -365,8 +365,7 @@ class ImportTask(object): duplicate_items += album.items() return duplicate_items - def do_remove_duplicates(self, lib): - # TODO: Bad name. Resolve naming conflict. + def remove_duplicates(self, lib): duplicate_items = self.duplicate_items(lib) log.debug('removing %i old duplicated items' % len(duplicate_items)) @@ -687,7 +686,7 @@ class SentinelImportTask(ImportTask): self.paths = paths # TODO Remove the remaining attributes eventually self.items = None - self.remove_duplicates = False + self.should_remove_duplicates = False self.is_album = True self.choice_flag = None @@ -1063,8 +1062,8 @@ def manipulate_files(session, task): if task.skip: return - if task.remove_duplicates: - task.do_remove_duplicates(session.lib) + if task.should_remove_duplicates: + task.remove_duplicates(session.lib) task.manipulate_files( move=session.config['move'], diff --git a/beets/ui/commands.py b/beets/ui/commands.py index 24c798e94..b660cd9e4 100644 --- a/beets/ui/commands.py +++ b/beets/ui/commands.py @@ -740,7 +740,7 @@ class TerminalImportSession(importer.ImportSession): pass elif sel == 'r': # Remove old. - task.remove_duplicates = True + task.should_remove_duplicates = True else: assert False diff --git a/test/helper.py b/test/helper.py index 2f65b2e7d..16cfd11d3 100644 --- a/test/helper.py +++ b/test/helper.py @@ -349,7 +349,7 @@ class TestImportSession(importer.ImportSession): if res == self.Resolution.SKIP: task.set_choice(importer.action.SKIP) elif res == self.Resolution.REMOVE: - task.remove_duplicates = True + task.should_remove_duplicates = True def generate_album_info(album_id, track_ids): From 8c2763ffdcd430ce05164390fa3ce3989d0b40ab Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 26 Apr 2014 20:35:41 -0700 Subject: [PATCH 38/38] Collapse finalize stage into manipulate_files --- beets/importer.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 4cfe1c7e3..344e382dd 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -254,7 +254,6 @@ class ImportSession(object): for stage_func in plugins.import_stages(): stages.append(plugin_stage(self, stage_func)) stages += [manipulate_files(self)] - stages += [finalize(self)] pl = pipeline.Pipeline(stages) # Run the pipeline. @@ -379,7 +378,7 @@ class ImportTask(object): lib.directory) def finalize(self, session): - """Save progress clean up files, and emit plugin event. + """Save progress, clean up files, and emit plugin event. """ # FIXME the session argument is unfortunate. It should be # present as an attribute of the task. @@ -1054,10 +1053,11 @@ def plugin_stage(session, func, task): task.reload() -@pipeline.mutator_stage +@pipeline.stage def manipulate_files(session, task): """A coroutine (pipeline stage) that performs necessary file - manipulations *after* items have been added to the library. + manipulations *after* items have been added to the library and + finalizes each task. """ if task.skip: return @@ -1072,11 +1072,7 @@ def manipulate_files(session, task): session=session, ) - -# FIXME Boilerplate. Maybe we should move this to the `manipulate_files` -# stage. -@pipeline.stage -def finalize(session, task): + # Progress, cleanup, and event. task.finalize(session)