Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor undo cursor #377

Merged
merged 2 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/sharp-rules-report.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"loro-wasm": patch
"loro-crdt": patch
---

Make cursors transformation better in undo/redo loop
86 changes: 79 additions & 7 deletions crates/loro-internal/src/undo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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>;

Expand All @@ -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>,
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 = {
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions crates/loro-wasm/deno.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading