diff --git a/qt/aqt/main.py b/qt/aqt/main.py index b433cb0ca..004de77b4 100644 --- a/qt/aqt/main.py +++ b/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: diff --git a/rslib/backend.proto b/rslib/backend.proto index 6747073f4..e61514214 100644 --- a/rslib/backend.proto +++ b/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; diff --git a/rslib/src/backend/ops.rs b/rslib/src/backend/ops.rs index 41ae739b7..0db69b92f 100644 --- a/rslib/src/backend/ops.rs +++ b/rslib/src/backend/ops.rs @@ -18,6 +18,7 @@ impl From 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(), diff --git a/rslib/src/collection/transact.rs b/rslib/src/collection/transact.rs index 59fc5ccac..6713249fb 100644 --- a/rslib/src/collection/transact.rs +++ b/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 diff --git a/rslib/src/config/bool.rs b/rslib/src/config/bool.rs index b97c83184..d6bbd25fc 100644 --- a/rslib/src/config/bool.rs +++ b/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(()) }) diff --git a/rslib/src/config/mod.rs b/rslib/src/config/mod.rs index 93d660e6d..6cc6bcf5f 100644 --- a/rslib/src/config/mod.rs +++ b/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(()) }) diff --git a/rslib/src/config/string.rs b/rslib/src/config/string.rs index 553b5255e..a2dc43016 100644 --- a/rslib/src/config/string.rs +++ b/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(()) }) diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index 95503a54c..97812a5fb 100644 --- a/rslib/src/ops.rs +++ b/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 diff --git a/rslib/src/undo/mod.rs b/rslib/src/undo/mod.rs index 1c503915d..d5581a47f 100644 --- a/rslib/src/undo/mod.rs +++ b/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 { 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(()) + } }