Merge pull request #2667 from wordofglass/mtime_fixes

Mtime fixes

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.
         """

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

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:
 

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)

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()

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,21 +202,25 @@ 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')
 
     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