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
This commit is contained in:
Damien Elmes 2021-06-25 09:13:19 +10:00
parent d82b87e643
commit 73d9391f64
8 changed files with 36 additions and 52 deletions

View File

@ -9,6 +9,7 @@ impl Collection {
F: FnOnce(&mut Collection) -> Result<R>,
{
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

View File

@ -76,11 +76,13 @@ impl Collection {
value: bool,
undoable: bool,
) -> Result<OpOutput<()>> {
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(())
})
}

View File

@ -76,11 +76,13 @@ impl Collection {
val: &T,
undoable: bool,
) -> Result<OpOutput<()>> {
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(())
})
}

View File

@ -31,11 +31,13 @@ impl Collection {
val: &str,
undoable: bool,
) -> Result<OpOutput<()>> {
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(())
})
}

View File

@ -307,7 +307,7 @@ impl Collection {
collapsed: bool,
scope: DeckCollapseScope,
) -> Result<OpOutput<()>> {
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;

View File

@ -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
}

View File

@ -17,7 +17,7 @@ impl Collection {
}
pub fn set_tag_collapsed(&mut self, tag: &str, collapsed: bool) -> Result<OpOutput<()>> {
self.transact(Op::ExpandCollapse, |col| {
self.transact(Op::SkipUndo, |col| {
col.set_tag_collapsed_inner(tag, collapsed, col.usn()?)
})
}

View File

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