From 73d9391f64f74bfbf05d1e02d28647f5f963930a Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 25 Jun 2021 09:13:19 +1000 Subject: [PATCH] update undo skipping; exclude deck/tag expand/collapse Instead of calling a method inside the transaction body, routines can now pass Op::SkipUndo if they wish the changes to be discarded at the end of the transaction. The advantage of doing it this way is that the list of changes can still be returned, allowing the sync indicator to update immediately. Closes #1252 --- rslib/src/collection/transact.rs | 3 ++- rslib/src/config/bool.rs | 10 ++++++---- rslib/src/config/mod.rs | 10 ++++++---- rslib/src/config/string.rs | 10 ++++++---- rslib/src/decks/tree.rs | 2 +- rslib/src/ops.rs | 18 +++++++---------- rslib/src/tags/tree.rs | 2 +- rslib/src/undo/mod.rs | 33 +++++++------------------------- 8 files changed, 36 insertions(+), 52 deletions(-) diff --git a/rslib/src/collection/transact.rs b/rslib/src/collection/transact.rs index 5c196286f..e40892616 100644 --- a/rslib/src/collection/transact.rs +++ b/rslib/src/collection/transact.rs @@ -9,6 +9,7 @@ impl Collection { F: FnOnce(&mut Collection) -> Result, { let have_op = op.is_some(); + let skip_undo_queue = op == Some(Op::SkipUndo); self.storage.begin_rust_trx()?; self.begin_undoable_operation(op); @@ -36,7 +37,7 @@ impl Collection { changes: StateChanges::default(), } }; - self.end_undoable_operation(); + self.end_undoable_operation(skip_undo_queue); Ok(OpOutput { output, changes }) }) // roll back on error diff --git a/rslib/src/config/bool.rs b/rslib/src/config/bool.rs index d6bbd25fc..848918f37 100644 --- a/rslib/src/config/bool.rs +++ b/rslib/src/config/bool.rs @@ -76,11 +76,13 @@ impl Collection { value: bool, undoable: bool, ) -> Result> { - self.transact(Op::UpdateConfig, |col| { + let op = if undoable { + Op::UpdateConfig + } else { + Op::SkipUndo + }; + self.transact(op, |col| { col.set_config(key, &value)?; - if !undoable { - col.clear_current_undo_step_changes()?; - } Ok(()) }) } diff --git a/rslib/src/config/mod.rs b/rslib/src/config/mod.rs index 6cc6bcf5f..72490e1ac 100644 --- a/rslib/src/config/mod.rs +++ b/rslib/src/config/mod.rs @@ -76,11 +76,13 @@ impl Collection { val: &T, undoable: bool, ) -> Result> { - self.transact(Op::UpdateConfig, |col| { + let op = if undoable { + Op::UpdateConfig + } else { + Op::SkipUndo + }; + self.transact(op, |col| { col.set_config(key, val)?; - if !undoable { - col.clear_current_undo_step_changes()?; - } Ok(()) }) } diff --git a/rslib/src/config/string.rs b/rslib/src/config/string.rs index edf42c868..ea7d5c739 100644 --- a/rslib/src/config/string.rs +++ b/rslib/src/config/string.rs @@ -31,11 +31,13 @@ impl Collection { val: &str, undoable: bool, ) -> Result> { - self.transact(Op::UpdateConfig, |col| { + let op = if undoable { + Op::UpdateConfig + } else { + Op::SkipUndo + }; + self.transact(op, |col| { col.set_config_string_inner(key, val)?; - if !undoable { - col.clear_current_undo_step_changes()?; - } Ok(()) }) } diff --git a/rslib/src/decks/tree.rs b/rslib/src/decks/tree.rs index 8e1922cf6..c40b065d9 100644 --- a/rslib/src/decks/tree.rs +++ b/rslib/src/decks/tree.rs @@ -307,7 +307,7 @@ impl Collection { collapsed: bool, scope: DeckCollapseScope, ) -> Result> { - self.transact(Op::ExpandCollapse, |col| { + self.transact(Op::SkipUndo, |col| { if let Some(mut deck) = col.storage.get_deck(did)? { let original = deck.clone(); let c = &mut deck.common; diff --git a/rslib/src/ops.rs b/rslib/src/ops.rs index ddf94de33..394f2d310 100644 --- a/rslib/src/ops.rs +++ b/rslib/src/ops.rs @@ -15,7 +15,6 @@ pub enum Op { ChangeNotetype, ClearUnusedTags, EmptyFilteredDeck, - ExpandCollapse, FindAndReplace, RebuildFilteredDeck, RemoveDeck, @@ -42,6 +41,9 @@ pub enum Op { UpdateTag, UpdateNotetype, SetCurrentDeck, + /// Does not register changes in undo queue, but does not clear the current + /// queue either. + SkipUndo, } impl Op { @@ -75,7 +77,6 @@ impl Op { Op::BuildFilteredDeck => tr.actions_build_filtered_deck(), Op::RebuildFilteredDeck => tr.actions_build_filtered_deck(), Op::EmptyFilteredDeck => tr.studying_empty(), - Op::ExpandCollapse => tr.actions_expand_collapse(), Op::SetCurrentDeck => tr.browsing_change_deck(), Op::UpdateDeckConfig => tr.deck_config_title(), Op::AddNotetype => tr.actions_add_notetype(), @@ -84,6 +85,7 @@ impl Op { Op::UpdateConfig => tr.actions_update_config(), Op::Custom(name) => name.into(), Op::ChangeNotetype => tr.browsing_change_notetype(), + Op::SkipUndo => return "".to_string(), } .into() } @@ -133,17 +135,11 @@ impl OpChanges { // These routines should return true even if the GUI may have // special handling for an action, since we need to do the right // thing when undoing, and if multiple windows of the same type are - // open. For example, while toggling the expand/collapse state - // in the sidebar will not normally trigger a full sidebar refresh, - // requires_browser_sidebar_redraw() should still return true. + // open. pub fn requires_browser_table_redraw(&self) -> bool { let c = &self.changes; - c.card - || c.notetype - || c.config - || (c.note && self.op != Op::AddNote) - || (c.deck && self.op != Op::ExpandCollapse) + c.card || c.notetype || c.config || (c.note && self.op != Op::AddNote) || c.deck } pub fn requires_browser_sidebar_redraw(&self) -> bool { @@ -159,7 +155,7 @@ impl OpChanges { pub fn requires_study_queue_rebuild(&self) -> bool { let c = &self.changes; c.card - || (c.deck && self.op != Op::ExpandCollapse) + || c.deck || (c.config && matches!(self.op, Op::SetCurrentDeck | Op::UpdatePreferences)) || c.deck_config } diff --git a/rslib/src/tags/tree.rs b/rslib/src/tags/tree.rs index 20ed15524..a4e4ff9fa 100644 --- a/rslib/src/tags/tree.rs +++ b/rslib/src/tags/tree.rs @@ -17,7 +17,7 @@ impl Collection { } pub fn set_tag_collapsed(&mut self, tag: &str, collapsed: bool) -> Result> { - self.transact(Op::ExpandCollapse, |col| { + self.transact(Op::SkipUndo, |col| { col.set_tag_collapsed_inner(tag, collapsed, col.usn()?) }) } diff --git a/rslib/src/undo/mod.rs b/rslib/src/undo/mod.rs index ed16cd5cf..42961734d 100644 --- a/rslib/src/undo/mod.rs +++ b/rslib/src/undo/mod.rs @@ -94,17 +94,15 @@ impl UndoManager { }); } - fn end_step(&mut self) { + fn end_step(&mut self, skip_undo: bool) { if let Some(step) = self.current_step.take() { - if step.has_changes() { + if step.has_changes() && !skip_undo { if self.mode == UndoMode::Undoing { self.redo_steps.push(step); } else { self.undo_steps.truncate(UNDO_LIMIT - 1); self.undo_steps.push_front(step); } - } else { - println!("no undo changes, discarding step"); } } } @@ -171,15 +169,9 @@ impl UndoManager { /// counter value, which can be used with `merge_undoable_ops`. fn add_custom_step(&mut self, name: String) -> usize { self.begin_step(Some(Op::Custom(name))); - self.end_step(); + self.end_step(false); self.counter } - - fn clear_current_changes(&mut self) { - if let Some(op) = &mut self.current_step { - op.changes.clear(); - } - } } impl Collection { @@ -234,8 +226,8 @@ impl Collection { /// Called at the end of a successful transaction. /// In most instances, this will also clear the study queues. - pub(crate) fn end_undoable_operation(&mut self) { - self.state.undo.end_step(); + pub(crate) fn end_undoable_operation(&mut self, skip_undo: bool) { + self.state.undo.end_step(skip_undo); } pub(crate) fn discard_undo_and_study_queues(&mut self) { @@ -253,17 +245,6 @@ impl Collection { self.state.undo.save(item.into()); } - /// Forget any recorded changes in the current transaction, allowing - /// 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> { self.state.undo.current_op() } @@ -554,13 +535,13 @@ mod test { ); assert!(!out.changes.had_change()); - // op output won't reflect changes were made + // op output will reflect changes were made let out = col.set_config_bool(BoolKey::AddingDefaultsToCurrentDeck, true, false)?; assert_ne!( col.storage.get_collection_timestamps()?.collection_change.0, 0 ); - assert!(!out.changes.had_change()); + assert!(out.changes.had_change()); Ok(()) }