Edit plugin logic: Regression from previous PR; incorrect diffs #2668
Conversation
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.
|
Ok, this still doesn't work because deepcopy doesn't like |
|
Should be fine now, I ended up implementing |
…box#2659 Deepcopying fails if a database is attached as Model._db since the sqlite connection objects it contains are non-copyable
There was a problem hiding this comment.
Aha; good catch. Adding a copy method seems like a reasonable fix—and you're right that deepcopy was the wrong thing. (In fact, it's surprising that it stuck around for so long in the edit plugin…)
I have a couple of documentation suggestions; then this should be good to go.
beetsplug/edit.py
Outdated
| @@ -302,9 +301,14 @@ def edit_objects(self, objs, fields): | |||
| 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 | |||
There was a problem hiding this comment.
Perhaps just call it a "copy" here, since we're not using deepcopy anymore.
There was a problem hiding this comment.
Right, that's a leftover from before, thanks for spotting it!
beets/dbcore/db.py
Outdated
| """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. | ||
| """ |
There was a problem hiding this comment.
I know this is extremely low-level, but sometimes I like structuring docstrings so there is a quick summary at the top followed by more detail after a blank line. This can help when navigating a big codebase for the first time. For example, I might suggest:
"""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.)
"""
|
Well, it could persist because the plugin didn't copy any items with a database connection, but let |
Edit plugin logic: Regression from previous PR; incorrect diffs
Contains two fixes:
idwhenobject._dbis set. Crashed beets when re-tagging albums (since the yaml file would containid: ''which can obviously not be coerced to an integer)