diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index a714d94922..82371fb171 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -242,8 +242,9 @@ def __getitem__(self, key): else: raise KeyError(key) - def __setitem__(self, key, value): - """Assign the value for a field. + def _setitem(self, key, value): + """Assign the value for a field, return whether new and old value + differ. """ # Choose where to place the value. if key in self._fields: @@ -257,9 +258,17 @@ def __setitem__(self, key, value): # Assign value and possibly mark as dirty. old_value = source.get(key) source[key] = value - if self._always_dirty or old_value != value: + changed = old_value != value + if self._always_dirty or changed: self._dirty.add(key) + return changed + + def __setitem__(self, key, value): + """Assign the value for a field. + """ + self._setitem(key, value) + def __delitem__(self, key): """Remove a flexible attribute from the model. """ diff --git a/beets/library.py b/beets/library.py index f294e6d4f5..988628a5c5 100644 --- a/beets/library.py +++ b/beets/library.py @@ -547,10 +547,10 @@ def __setitem__(self, key, value): elif isinstance(value, BLOB_TYPE): value = bytes(value) - if key in MediaFile.fields(): - self.mtime = 0 # Reset mtime on dirty. + changed = super(Item, self)._setitem(key, value) - super(Item, self).__setitem__(key, value) + if changed and key in MediaFile.fields(): + self.mtime = 0 # Reset mtime on dirty. def update(self, values): """Set all key/value pairs in the mapping. If mtime is diff --git a/docs/changelog.rst b/docs/changelog.rst index 8326877334..5e0391f220 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -29,6 +29,10 @@ Fixes: shown or beets could crash with a traceback. :bug:`2659` * :doc:`/plugins/kodiupdate`: Fix server URL and add better error reporting. :bug:`2662` +* Fixed a problem where "no-op" modifications would reset files' mtimes, + resulting in unnecessary writes. This most prominently affected the + :doc:`/plugins/edit` when saving the text file without making changes to some + music. :bug:`2667` For developers: diff --git a/test/_common.py b/test/_common.py index f3213ec31f..fc7b650b32 100644 --- a/test/_common.py +++ b/test/_common.py @@ -90,6 +90,7 @@ def item(lib=None): mb_artistid='someID-3', mb_albumartistid='someID-4', album_id=None, + mtime=12345, ) if lib: lib.add(i) diff --git a/test/helper.py b/test/helper.py index 795fd2ac07..5af742e12b 100644 --- a/test/helper.py +++ b/test/helper.py @@ -315,6 +315,8 @@ def create_item(self, **values): item = Item(**values_) if 'path' not in values: item['path'] = 'audio.' + item['format'].lower() + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 return item def add_item(self, **values): @@ -365,6 +367,8 @@ def add_item_fixtures(self, ext='mp3', count=1): item = Item.from_path(path) item.album = u'\u00e4lbum {0}'.format(i) # Check unicode paths item.title = u't\u00eftle {0}'.format(i) + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 item.add(self.lib) item.move(copy=True) item.store() @@ -380,6 +384,8 @@ def add_album_fixture(self, track_count=1, ext='mp3'): item = Item.from_path(path) item.album = u'\u00e4lbum' # Check unicode paths item.title = u't\u00eftle {0}'.format(i) + # mtime needs to be set last since other assignments reset it. + item.mtime = 12345 item.add(self.lib) item.move(copy=True) item.store() diff --git a/test/test_edit.py b/test/test_edit.py index 2900d092ad..0a6a286f5c 100644 --- a/test/test_edit.py +++ b/test/test_edit.py @@ -162,7 +162,7 @@ def test_title_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT, title_starts_with=u'modified t\u00eftle') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['title']) + ['title', 'mtime']) def test_single_title_edit_apply(self, mock_write): """Edit title for one item in the library, then apply changes.""" @@ -202,7 +202,7 @@ def test_album_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['album']) + ['album', 'mtime']) # Ensure album is *not* modified. self.album.load() self.assertEqual(self.album.album, u'\u00e4lbum') @@ -210,13 +210,17 @@ def test_album_edit_apply(self, mock_write): def test_single_edit_add_field(self, mock_write): """Edit the yaml file appending an extra field to the first item, then apply changes.""" - # Append "foo: bar" to item with id == 1. - self.run_mocked_command({'replacements': {u"id: 1": - u"id: 1\nfoo: bar"}}, + # Append "foo: bar" to item with id == 2. ("id: 1" would match both + # "id: 1" and "id: 10") + self.run_mocked_command({'replacements': {u"id: 2": + u"id: 2\nfoo: bar"}}, # Apply changes. ['a']) - self.assertEqual(self.lib.items(u'id:1')[0].foo, 'bar') + self.assertEqual(self.lib.items(u'id:2')[0].foo, 'bar') + # Even though a flexible attribute was written (which is not directly + # written to the tags), write should still be called since templates + # might use it. self.assertCounts(mock_write, write_call_count=1, title_starts_with=u't\u00eftle') @@ -232,7 +236,7 @@ def test_a_album_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertEqual(self.album.album, u'modified \u00e4lbum') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['album']) + ['album', 'mtime']) def test_a_albumartist_edit_apply(self, mock_write): """Album query (-a), edit albumartist field, apply changes.""" @@ -246,7 +250,7 @@ def test_a_albumartist_edit_apply(self, mock_write): self.assertCounts(mock_write, write_call_count=self.TRACK_COUNT) self.assertEqual(self.album.albumartist, u'the modified album artist') self.assertItemFieldsModified(self.album.items(), self.items_orig, - ['albumartist']) + ['albumartist', 'mtime']) def test_malformed_yaml(self, mock_write): """Edit the yaml file incorrectly (resulting in a malformed yaml