From 6d47015f6e193d522765462d5c7bb6fd17735174 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Thu, 6 Jun 2024 16:49:36 +0800 Subject: [PATCH] Refactor undo cursor (#377) * refactor: make cursor transformation in undo/redo better * chore: add release info --- .changeset/sharp-rules-report.md | 6 +++ crates/loro-internal/src/undo.rs | 86 +++++++++++++++++++++++++++++--- crates/loro-wasm/deno.lock | 8 +++ 3 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 .changeset/sharp-rules-report.md diff --git a/.changeset/sharp-rules-report.md b/.changeset/sharp-rules-report.md new file mode 100644 index 00000000..061e047c --- /dev/null +++ b/.changeset/sharp-rules-report.md @@ -0,0 +1,6 @@ +--- +"loro-wasm": patch +"loro-crdt": patch +--- + +Make cursors transformation better in undo/redo loop diff --git a/crates/loro-internal/src/undo.rs b/crates/loro-internal/src/undo.rs index 14dc32ca..886d09d1 100644 --- a/crates/loro-internal/src/undo.rs +++ b/crates/loro-internal/src/undo.rs @@ -9,7 +9,7 @@ use loro_common::{ ContainerID, Counter, CounterSpan, HasCounterSpan, HasIdSpan, IdSpan, LoroError, LoroResult, LoroValue, PeerID, }; -use tracing::{debug_span, info_span, instrument, trace}; +use tracing::{debug_span, info_span, instrument}; use crate::{ change::get_sys_timestamp, @@ -71,7 +71,7 @@ fn transform_cursor( container_remap: &FxHashMap, ) { let mut cid = &cursor_with_pos.cursor.container; - while let Some(new_cid) = container_remap.get(&cid) { + while let Some(new_cid) = container_remap.get(cid) { cid = new_cid; } @@ -140,6 +140,8 @@ impl UndoOrRedo { } } +/// When a undo/redo item is pushed, the undo manager will call the on_push callback to get the meta data of the undo item. +/// The returned cursors will be recorded for a new pushed undo item. pub type OnPush = Box UndoItemMeta + Send + Sync>; pub type OnPop = Box; @@ -152,6 +154,7 @@ struct UndoManagerInner { merge_interval: i64, max_stack_size: usize, exclude_origin_prefixes: Vec>, + last_popped_selection: Option>, on_push: Option, on_pop: Option, } @@ -187,13 +190,13 @@ struct StackItem { /// /// The cursors inside the metadata will be transformed by remote operations as well. /// So that when the item is popped, users can restore the cursors position correctly. -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub struct UndoItemMeta { pub value: LoroValue, pub cursors: Vec, } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct CursorWithPos { pub cursor: Cursor, pub pos: AbsolutePosition, @@ -370,6 +373,7 @@ impl UndoManagerInner { last_undo_time: 0, max_stack_size: usize::MAX, exclude_origin_prefixes: vec![], + last_popped_selection: None, on_pop: None, on_push: None, } @@ -388,6 +392,7 @@ impl UndoManagerInner { .as_ref() .map(|x| x(UndoOrRedo::Undo, span)) .unwrap_or_default(); + if !self.undo_stack.is_empty() && now - self.last_undo_time < self.merge_interval { self.undo_stack.push_with_merge(span, meta, true); } else { @@ -522,6 +527,60 @@ impl UndoManager { get_opposite: impl Fn(&mut UndoManagerInner) -> &mut Stack, kind: UndoOrRedo, ) -> LoroResult { + // When in the undo/redo loop, the new undo/redo stack item should restore the selection + // to the state it was in before the item that was popped two steps ago from the stack. + // + // ┌────────────┐ + // │Selection 1 │ + // └─────┬──────┘ + // │ Some + // ▼ ops + // ┌────────────┐ + // │Selection 2 │ + // └─────┬──────┘ + // │ Some + // ▼ ops + // ┌────────────┐ + // │Selection 3 │◁ ─ ─ ─ ─ ─ ─ ─ Restore ─ ─ ─ + // └─────┬──────┘ │ + // │ + // │ │ + // │ ┌ ─ ─ ─ ─ ─ ─ ─ + // Enter the │ Undo ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─▶ Push Redo │ + // undo/redo ─ ─ ─ ▶ ▼ └ ─ ─ ─ ─ ─ ─ ─ + // loop ┌────────────┐ │ + // │Selection 2 │◁─ ─ ─ Restore ─ + // └─────┬──────┘ │ │ + // │ + // │ │ │ + // │ ┌ ─ ─ ─ ─ ─ ─ ─ + // │ Undo ─ ─ ─ ─ ▶ Push Redo │ │ + // ▼ └ ─ ─ ─ ─ ─ ─ ─ + // ┌────────────┐ │ │ + // │Selection 1 │ + // └─────┬──────┘ │ │ + // │ Redo ◀ ─ ─ ─ ─ ─ ─ ─ ─ + // ▼ │ + // ┌────────────┐ + // ┌ Restore ─ ▷│Selection 2 │ │ + // └─────┬──────┘ + // │ │ │ + // ┌ ─ ─ ─ ─ ─ ─ ─ │ + // Push Undo │◀─ ─ ─ ─ ─ ─ ─ │ Redo ◀ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┘ + // └ ─ ─ ─ ─ ─ ─ ─ ▼ + // │ ┌────────────┐ + // │Selection 3 │ + // │ └─────┬──────┘ + // ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ▶ │ Undo + // ▼ + // ┌────────────┐ + // │Selection 2 │ + // └────────────┘ + // + // Because users may change the selections during the undo/redo loop, it's + // more stable to keep the selection stored in the last stack item + // rather than using the current selection directly. + self.record_new_checkpoint(doc)?; let end_counter = get_counter_end(doc, self.peer); let mut top = { @@ -532,6 +591,7 @@ impl UndoManager { let mut executed = false; while let Some((mut span, remote_diff)) = top { + let mut next_push_selection = None; { let inner = self.inner.clone(); // We need to clone this because otherwise will be applied to the same remote diff @@ -552,7 +612,8 @@ impl UndoManager { }, )?; drop(commit); - if let Some(x) = self.inner.try_lock().unwrap().on_pop.as_ref() { + let mut inner = self.inner.try_lock().unwrap(); + if let Some(x) = inner.on_pop.as_ref() { for cursor in span.meta.cursors.iter_mut() { // We need to transform cursor here. // Note that right now is already done, @@ -564,17 +625,28 @@ impl UndoManager { &self.container_remap, ); } - x(kind, span.span, span.meta) + + x(kind, span.span, span.meta.clone()); + let take = inner.last_popped_selection.take(); + next_push_selection = take; + inner.last_popped_selection = Some(span.meta.cursors); } } let new_counter = get_counter_end(doc, self.peer); if end_counter != new_counter { let mut inner = self.inner.try_lock().unwrap(); - let meta = inner + let mut meta = inner .on_push .as_ref() .map(|x| x(kind.opposite(), CounterSpan::new(end_counter, new_counter))) .unwrap_or_default(); + if matches!(kind, UndoOrRedo::Undo) && get_opposite(&mut inner).is_empty() { + // If it's the first undo, we use the cursors from the users + } else if let Some(inner) = next_push_selection.take() { + // Otherwise, we use the cursors from the undo/redo loop + meta.cursors = inner; + } + get_opposite(&mut inner).push(CounterSpan::new(end_counter, new_counter), meta); inner.latest_counter = new_counter; executed = true; diff --git a/crates/loro-wasm/deno.lock b/crates/loro-wasm/deno.lock index 67ad4605..ef38bfde 100644 --- a/crates/loro-wasm/deno.lock +++ b/crates/loro-wasm/deno.lock @@ -27,5 +27,13 @@ "https://lra6z45nakk5lnu3yjchp7tftsdnwwikwr65ocha5eojfnlgu4sa.arweave.net/XEHs860CldW2m8JEd_5lnIbbWQq0fdcI4OkckrVmpyQ/path/separator.ts": "9dd15d46ff84a16e13554f56af7fee1f85f8d0f379efbbe60ac066a60561f036", "https://lra6z45nakk5lnu3yjchp7tftsdnwwikwr65ocha5eojfnlgu4sa.arweave.net/XEHs860CldW2m8JEd_5lnIbbWQq0fdcI4OkckrVmpyQ/path/win32.ts": "47114c941681ecbabab4ec355cb41d525fb5a14175cc47a5c76fdc5eaee2685a", "https://raw.githubusercontent.com/zxch3n/dirname/master/mod.ts": "79768be98329cc670a80bd4f5ce9800412944aba27f563a6314ae2c4589be229" + }, + "workspace": { + "packageJson": { + "dependencies": [ + "npm:vite-plugin-top-level-await@^1.2.2", + "npm:vite-plugin-wasm@^3.1.0" + ] + } } }