avoid bumping mtime when nothing has changed

+ update sync indicator after every op
+ skip mtime bump on undo/redo

qt/aqt/main.py

@@ -738,6 +738,9 @@ class AnkiQt(QMainWindow):
         if not focused and dirty:
             self.fade_out_webview()
 
+        if changes.mtime:
+            self.toolbar.update_sync_status()
+
     def on_focus_did_change(
         self, new_focus: Optional[QWidget], _old: Optional[QWidget]
     ) -> None:

rslib/backend.proto

@@ -1571,6 +1571,7 @@ message OpChanges {
   bool notetype = 5;
   bool config = 6;
   bool deck_config = 11;
+  bool mtime = 12;
 
   bool browser_table = 7;
   bool browser_sidebar = 8;

rslib/src/backend/ops.rs

@@ -18,6 +18,7 @@ impl From<OpChanges> for pb::OpChanges {
             notetype: c.changes.notetype,
             config: c.changes.config,
             deck_config: c.changes.deck_config,
+            mtime: c.changes.mtime,
             browser_table: c.requires_browser_table_redraw(),
             browser_sidebar: c.requires_browser_sidebar_redraw(),
             editor: c.requires_editor_redraw(),

rslib/src/collection/transact.rs

@@ -13,18 +13,22 @@ impl Collection {
         self.storage.begin_rust_trx()?;
         self.begin_undoable_operation(op);
 
-        let mut res = func(self);
-
-        if res.is_ok() {
-            if let Err(e) = self.set_modified() {
-                res = Err(e);
-            } else if let Err(e) = self.storage.commit_rust_trx() {
-                res = Err(e);
-            }
-        }
-
-        match res {
-            Ok(output) => {
+        func(self)
+            // any changes mean an mtime bump
+            .and_then(|out| {
+                if !have_op || (self.current_undo_step_has_changes() && !self.undoing_or_redoing())
+                {
+                    self.set_modified()?;
+                }
+                Ok(out)
+            })
+            // then commit
+            .and_then(|out| {
+                self.storage.commit_rust_trx()?;
+                Ok(out)
+            })
+            // finalize undo step
+            .map(|output| {
                 let changes = if have_op {
                     let changes = self.op_changes();
                     self.maybe_clear_study_queues_after_op(&changes);
@@ -32,22 +36,21 @@ impl Collection {
                     changes
                 } else {
                     self.clear_study_queues();
-                    // dummy value, not used by transact_no_undo(). only required
-                    // until we can migrate all the code to undoable ops
+                    // dummy value for transact_no_undo() case
                     OpChanges {
                         op: Op::SetFlag,
                         changes: StateChanges::default(),
                     }
                 };
                 self.end_undoable_operation();
-                Ok(OpOutput { output, changes })
-            }
-            Err(err) => {
+                OpOutput { output, changes }
+            })
+            // roll back on error
+            .or_else(|err| {
                 self.discard_undo_and_study_queues();
                 self.storage.rollback_rust_trx()?;
                 Err(err)
-            }
-        }
+            })
     }
 
     /// Execute the provided closure in a transaction, rolling back if

rslib/src/config/bool.rs

@@ -79,7 +79,7 @@ impl Collection {
         self.transact(Op::UpdateConfig, |col| {
             col.set_config(key, &value)?;
             if !undoable {
-                col.clear_current_undo_step_changes();
+                col.clear_current_undo_step_changes()?;
             }
             Ok(())
         })

rslib/src/config/mod.rs

@@ -79,7 +79,7 @@ impl Collection {
         self.transact(Op::UpdateConfig, |col| {
             col.set_config(key, val)?;
             if !undoable {
-                col.clear_current_undo_step_changes();
+                col.clear_current_undo_step_changes()?;
             }
             Ok(())
         })

rslib/src/config/string.rs

@@ -32,7 +32,7 @@ impl Collection {
         self.transact(Op::UpdateConfig, |col| {
             col.set_config_string_inner(key, val)?;
             if !undoable {
-                col.clear_current_undo_step_changes();
+                col.clear_current_undo_step_changes()?;
             }
             Ok(())
         })

rslib/src/ops.rs

@@ -96,6 +96,7 @@ pub struct StateChanges {
     pub notetype: bool,
     pub config: bool,
     pub deck_config: bool,
+    pub mtime: bool,
 }
 
 #[derive(Debug, Clone)]
@@ -125,7 +126,7 @@ impl OpChanges {
     #[cfg(test)]
     pub fn had_change(&self) -> bool {
         let c = &self.changes;
-        c.card || c.config || c.deck || c.deck_config || c.note || c.notetype || c.tag
+        c.card || c.config || c.deck || c.deck_config || c.note || c.notetype || c.tag || c.mtime
     }
     // These routines should return true even if the GUI may have
     // special handling for an action, since we need to do the right

rslib/src/undo/mod.rs

@@ -9,7 +9,6 @@ pub(crate) use changes::UndoableChange;
 
 pub use crate::ops::Op;
 use crate::{
-    collection::undo::UndoableCollectionChange,
     ops::{OpChanges, StateChanges},
     prelude::*,
 };
@@ -25,16 +24,9 @@ pub(crate) struct UndoableOp {
 }
 
 impl UndoableOp {
-    /// True if changes empty, or only the collection mtime has changed.
-    /// Always true in the case of custom steps.
+    /// True if changes non-empty, or a custom undo step.
     fn has_changes(&self) -> bool {
-        matches!(self.kind, Op::Custom(_))
-            || !matches!(
-                &self.changes[..],
-                &[] | &[UndoableChange::Collection(
-                    UndoableCollectionChange::Modified(_)
-                )]
-            )
+        !self.changes.is_empty() || matches!(self.kind, Op::Custom(_))
     }
 }
 
@@ -262,9 +254,14 @@ impl Collection {
     }
 
     /// Forget any recorded changes in the current transaction, allowing
-    /// a minor change like a config update to bypass undo.
-    pub(crate) fn clear_current_undo_step_changes(&mut self) {
-        self.state.undo.clear_current_changes()
+    /// a minor change like a config update to bypass undo. Bumps mtime if
+    /// there were pending changes.
+    pub(crate) fn clear_current_undo_step_changes(&mut self) -> Result<()> {
+        if self.current_undo_step_has_changes() {
+            self.set_modified()?;
+        }
+        self.state.undo.clear_current_changes();
+        Ok(())
     }
 
     pub(crate) fn current_undo_op(&self) -> Option<&UndoableOp> {
@@ -275,6 +272,18 @@ impl Collection {
         self.state.undo.previous_op()
     }
 
+    pub(crate) fn undoing_or_redoing(&self) -> bool {
+        self.state.undo.mode != UndoMode::NormalOp
+    }
+
+    pub(crate) fn current_undo_step_has_changes(&self) -> bool {
+        self.state
+            .undo
+            .current_op()
+            .map(|op| op.has_changes())
+            .unwrap_or_default()
+    }
+
     /// Used for coalescing successive note updates.
     pub(crate) fn pop_last_change(&mut self) -> Option<UndoableChange> {
         self.state
@@ -317,6 +326,9 @@ impl Collection {
 impl From<&[UndoableChange]> for StateChanges {
     fn from(changes: &[UndoableChange]) -> Self {
         let mut out = StateChanges::default();
+        if !changes.is_empty() {
+            out.mtime = true;
+        }
         for change in changes {
             match change {
                 UndoableChange::Card(_) => out.card = true,
@@ -511,4 +523,42 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn undo_mtime_bump() -> Result<()> {
+        let mut col = open_test_collection();
+        let mtime = col.storage.get_collection_timestamps()?.collection_change;
+
+        // a no-op change should not bump mtime
+        let out = col.set_config_bool(BoolKey::AddingDefaultsToCurrentDeck, true, true)?;
+        assert_eq!(
+            mtime,
+            col.storage.get_collection_timestamps()?.collection_change
+        );
+        assert_eq!(out.changes.had_change(), false);
+
+        // if there is an undoable step, mtime should change
+        let out = col.set_config_bool(BoolKey::AddingDefaultsToCurrentDeck, false, true)?;
+        let new_mtime = col.storage.get_collection_timestamps()?.collection_change;
+        assert_ne!(mtime, new_mtime);
+        assert_eq!(out.changes.had_change(), true);
+
+        // when skipping undo, mtime should still only be bumped on a change
+        let out = col.set_config_bool(BoolKey::AddingDefaultsToCurrentDeck, false, false)?;
+        assert_eq!(
+            new_mtime,
+            col.storage.get_collection_timestamps()?.collection_change
+        );
+        assert_eq!(out.changes.had_change(), false);
+
+        // op output won't reflect changes were made
+        let out = col.set_config_bool(BoolKey::AddingDefaultsToCurrentDeck, true, false)?;
+        assert_ne!(
+            new_mtime,
+            col.storage.get_collection_timestamps()?.collection_change
+        );
+        assert_eq!(out.changes.had_change(), false);
+
+        Ok(())
+    }
 }