From 989845199bfa4139374c8e93f13feb590243dd45 Mon Sep 17 00:00:00 2001 From: wordofglass Date: Thu, 24 Aug 2017 13:07:20 +0200 Subject: [PATCH 1/6] edit: Correctly reset the old object, do not reload it from the tags Previously, if continuing to edit (i.e. invoking the $EDITOR) multiple times in one invocation of EditPlugin.edit_objects, the plugin would reload the old state from the file tags. The initial 'old state' is usually only loaded from the database, though. As a consequence, if database and tags were not in sync, the diffs from first and all subsequent edits could differ unexpectedly. --- beetsplug/edit.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/beetsplug/edit.py b/beetsplug/edit.py index 8feb28f27..eba2db659 100644 --- a/beetsplug/edit.py +++ b/beetsplug/edit.py @@ -302,9 +302,14 @@ class EditPlugin(plugins.BeetsPlugin): elif choice == u'c': # Cancel. return False elif choice == u'e': # Keep editing. - # Reset the temporary changes to the objects. + # Reset the temporary changes to the objects. I we have a + # deepcopy from above, use that, else reload from the + # database. + objs = [(old_obj or obj) + for old_obj, obj in zip(objs_old, objs)] for obj in objs: - obj.read() + if obj._db: + obj.load() continue # Remove the temporary file before returning. From 33f7e679432f4d31f2dcfbbd604e3544977c2d1c Mon Sep 17 00:00:00 2001 From: wordofglass Date: Thu, 24 Aug 2017 13:15:15 +0200 Subject: [PATCH 2/6] edit: Fix a regression from #2659 when re-tagging albums --- beetsplug/edit.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/beetsplug/edit.py b/beetsplug/edit.py index eba2db659..7b59546cc 100644 --- a/beetsplug/edit.py +++ b/beetsplug/edit.py @@ -283,7 +283,7 @@ class EditPlugin(plugins.BeetsPlugin): # Show the changes. # If the objects are not on the DB yet, we need a copy of their # original state for show_model_changes. - objs_old = [deepcopy(obj) if not obj._db else None + objs_old = [deepcopy(obj) if obj.id < 0 else None for obj in objs] self.apply_data(objs, old_data, new_data) changed = False @@ -308,7 +308,7 @@ class EditPlugin(plugins.BeetsPlugin): objs = [(old_obj or obj) for old_obj, obj in zip(objs_old, objs)] for obj in objs: - if obj._db: + if not obj.id < 0: obj.load() continue @@ -374,7 +374,8 @@ class EditPlugin(plugins.BeetsPlugin): # yet. By using negative values, no clash with items in the database # can occur. for i, obj in enumerate(task.items, start=1): - if not obj._db: + # The importer may set the id to None when re-importing albums. + if not obj._db or obj.id is None: obj.id = -i # Present the YAML to the user and let her change it. @@ -383,7 +384,7 @@ class EditPlugin(plugins.BeetsPlugin): # Remove temporary ids. for obj in task.items: - if not obj._db: + if obj.id < 0: obj.id = None # Save the new data. From a9a2276774bbc88f354c126d1397c31c90a93435 Mon Sep 17 00:00:00 2001 From: wordofglass Date: Thu, 24 Aug 2017 14:42:04 +0200 Subject: [PATCH 3/6] Implement beets.dbcore.db.Model.copy() --- beets/dbcore/db.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index a714d9492..073b228c9 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -217,6 +217,18 @@ class Model(object): if need_id and not self.id: raise ValueError(u'{0} has no id'.format(type(self).__name__)) + def copy(self): + """Create a semi-deep copy of the item. In particular, the _db object + is not duplicated. A simple copy.deepcopy wouldn't work due to the + sqlite objects in _db. + """ + new = self.__class__() + new._db = self._db + new._values_fixed = self._values_fixed.copy() + new._values_flex = self._values_flex.copy() + new._dirty = self._dirty.copy() + return new + # Essential field accessors. @classmethod From 352d99cccdbb6b53b051d5642ae64cdf965599d1 Mon Sep 17 00:00:00 2001 From: wordofglass Date: Thu, 24 Aug 2017 14:43:56 +0200 Subject: [PATCH 4/6] edit: Do not deepcopy objects, finally fixes the regression from #2659 Deepcopying fails if a database is attached as Model._db since the sqlite connection objects it contains are non-copyable --- beetsplug/edit.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beetsplug/edit.py b/beetsplug/edit.py index 7b59546cc..5e0559bbf 100644 --- a/beetsplug/edit.py +++ b/beetsplug/edit.py @@ -23,7 +23,6 @@ from beets import ui from beets.dbcore import types from beets.importer import action from beets.ui.commands import _do_query, PromptChoice -from copy import deepcopy import codecs import subprocess import yaml @@ -283,7 +282,7 @@ class EditPlugin(plugins.BeetsPlugin): # Show the changes. # If the objects are not on the DB yet, we need a copy of their # original state for show_model_changes. - objs_old = [deepcopy(obj) if obj.id < 0 else None + objs_old = [obj.copy() if obj.id < 0 else None for obj in objs] self.apply_data(objs, old_data, new_data) changed = False From 5f21bd3ccd232014cd604062071eb40bc442d1b1 Mon Sep 17 00:00:00 2001 From: wordofglass Date: Thu, 24 Aug 2017 14:52:41 +0200 Subject: [PATCH 5/6] test_edit: Add a test for retagging. --- test/test_edit.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/test_edit.py b/test/test_edit.py index 2900d092a..b4eea4abb 100644 --- a/test/test_edit.py +++ b/test/test_edit.py @@ -22,6 +22,7 @@ from test import _common from test.helper import TestHelper, control_stdin from test.test_ui_importer import TerminalImportSessionSetup from test.test_importer import ImportHelper, AutotagStub +from beets.dbcore.query import TrueQuery from beets.library import Item from beetsplug.edit import EditPlugin @@ -359,6 +360,34 @@ class EditDuringImporterTest(TerminalImportSessionSetup, unittest.TestCase, # Ensure album is fetched from a candidate. self.assertIn('albumid', self.lib.albums()[0].mb_albumid) + def test_edit_retag_apply(self): + """Import the album using a candidate, then retag and edit and apply + changes. + """ + self._setup_import_session() + self.run_mocked_interpreter({}, + # 1, Apply changes. + ['1', 'a']) + + # Retag and edit track titles. On retag, the importer will reset items + # ids but not the db connections. + self.importer.paths = [] + self.importer.query = TrueQuery() + self.run_mocked_interpreter({'replacements': {u'Applied Title': + u'Edited Title'}}, + # eDit, Apply changes. + ['d', 'a']) + + # Check that 'title' field is modified, and other fields come from + # the candidate. + self.assertTrue(all('Edited Title ' in i.title + for i in self.lib.items())) + self.assertTrue(all('match ' in i.mb_trackid + for i in self.lib.items())) + + # Ensure album is fetched from a candidate. + self.assertIn('albumid', self.lib.albums()[0].mb_albumid) + def test_edit_discard_candidate(self): """Edit the album field for all items in the library, discard changes, using a candidate. From 7f12cc0c8813c56baa576d5e82fd7471b0a0e45d Mon Sep 17 00:00:00 2001 From: wordofglass Date: Fri, 25 Aug 2017 15:47:07 +0200 Subject: [PATCH 6/6] edit, Model.copy: documentation improvements --- beets/dbcore/db.py | 9 ++++++--- beetsplug/edit.py | 3 +-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 073b228c9..a45e6dc3d 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -218,9 +218,12 @@ class Model(object): raise ValueError(u'{0} has no id'.format(type(self).__name__)) def copy(self): - """Create a semi-deep copy of the item. In particular, the _db object - is not duplicated. A simple copy.deepcopy wouldn't work due to the - sqlite objects in _db. + """Create a copy of the model object. + + The field values and other state is duplicated, but the new copy + remains associated with the same database as the old object. + (A simple `copy.deepcopy` will not work because it would try to + duplicate the SQLite connection.) """ new = self.__class__() new._db = self._db diff --git a/beetsplug/edit.py b/beetsplug/edit.py index 5e0559bbf..631a1b584 100644 --- a/beetsplug/edit.py +++ b/beetsplug/edit.py @@ -302,8 +302,7 @@ class EditPlugin(plugins.BeetsPlugin): return False elif choice == u'e': # Keep editing. # Reset the temporary changes to the objects. I we have a - # deepcopy from above, use that, else reload from the - # database. + # copy from above, use that, else reload from the database. objs = [(old_obj or obj) for old_obj, obj in zip(objs_old, objs)] for obj in objs: