Refactor undo cursor (#377)

* refactor: make cursor transformation in undo/redo better

* chore: add release info

.changeset/sharp-rules-report.md

@@ -0,0 +1,6 @@
+---
+"loro-wasm": patch
+"loro-crdt": patch
+---
+
+Make cursors transformation better in undo/redo loop

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<ContainerID, ContainerID>,
 ) {
     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<dyn Fn(UndoOrRedo, CounterSpan) -> UndoItemMeta + Send + Sync>;
 pub type OnPop = Box<dyn Fn(UndoOrRedo, CounterSpan, UndoItemMeta) + Send + Sync>;
 
@@ -152,6 +154,7 @@ struct UndoManagerInner {
     merge_interval: i64,
     max_stack_size: usize,
     exclude_origin_prefixes: Vec<Box<str>>,
+    last_popped_selection: Option<Vec<CursorWithPos>>,
     on_push: Option<OnPush>,
     on_pop: Option<OnPop>,
 }
@@ -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<CursorWithPos>,
 }
 
-#[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<bool> {
+        // 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 <transform_delta> 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() {
                         // <cursor_transform> We need to transform cursor here.
                         // Note that right now <transform_delta> 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;

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"
+      ]
+    }
   }
 }