-
-
Notifications
You must be signed in to change notification settings - Fork 164
feat: add pause()/resume() to UndoManager #1053
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
acd36bb
a306b5c
183a397
6b30bea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| "loro-crdt": minor | ||
| "loro-js": minor | ||
| --- | ||
|
|
||
| Add `pause()`, `resume()`, and `isPaused()` to `UndoManager`. While paused, | ||
| local edits are not recorded as undo steps and checkout events do not clear the | ||
| stacks. Import events (remote changes) are still processed so that the stacks | ||
| remain correctly transformed against concurrent edits. Use this to preserve | ||
| undo/redo history across temporary checkouts such as read-only history previews. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,6 +203,7 @@ struct UndoManagerInner { | |
| undo_stack: Stack, | ||
| redo_stack: Stack, | ||
| processing_undo: bool, | ||
| paused: bool, | ||
| last_undo_time: i64, | ||
| merge_interval_in_ms: i64, | ||
| max_stack_size: usize, | ||
|
|
@@ -502,6 +503,7 @@ impl UndoManagerInner { | |
| undo_stack: Default::default(), | ||
| redo_stack: Default::default(), | ||
| processing_undo: false, | ||
| paused: false, | ||
| merge_interval_in_ms: 0, | ||
| last_undo_time: 0, | ||
| max_stack_size: usize::MAX, | ||
|
|
@@ -607,7 +609,7 @@ impl UndoManager { | |
| // TODO: PERF undo can be significantly faster if we can get | ||
| // the DiffBatch for undo here | ||
| let lock = inner_clone.lock(); | ||
| if lock.borrow().processing_undo { | ||
| if lock.borrow().processing_undo || lock.borrow().paused { | ||
| return; | ||
| } | ||
| if let Some(id) = event | ||
|
|
@@ -666,6 +668,9 @@ impl UndoManager { | |
| } | ||
| EventTriggerKind::Checkout => { | ||
| let lock = inner_clone.lock(); | ||
| if lock.borrow().paused { | ||
| return; | ||
| } | ||
| let mut inner = lock.borrow_mut(); | ||
| inner.undo_stack.clear(); | ||
| inner.redo_stack.clear(); | ||
|
|
@@ -977,6 +982,32 @@ impl UndoManager { | |
| self.inner.lock().borrow_mut().undo_stack.clear(); | ||
| } | ||
|
|
||
| /// Pause the UndoManager so that local edits and checkout events are ignored. | ||
| /// | ||
| /// While paused, local edits are not recorded as undo steps and checkout | ||
| /// events do not clear the stacks. Import events (remote changes) are still | ||
| /// processed so that the stacks remain correctly transformed against | ||
| /// concurrent edits. | ||
| /// | ||
| /// Use this before temporary checkouts (e.g. read-only history preview) that | ||
| /// should not disturb the undo/redo history. Close any open group (via | ||
| /// [`Self::group_end`]) before pausing. | ||
| /// | ||
| /// Call [`Self::resume`] after returning the document to its original state. | ||
| pub fn pause(&self) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Repro in the review body ( Suggested fix: early-return
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — perform() returns Ok(false) when paused, added a ProcessingUndoGuard drop guard, and the popped item is pushed back on error. |
||
| self.inner.lock().borrow_mut().paused = true; | ||
| } | ||
|
|
||
| /// Resume the UndoManager after a [`Self::pause`]. | ||
| pub fn resume(&self) { | ||
| self.inner.lock().borrow_mut().paused = false; | ||
| } | ||
|
|
||
| /// Returns whether the UndoManager is currently paused. | ||
| pub fn is_paused(&self) -> bool { | ||
| self.inner.lock().borrow().paused | ||
| } | ||
|
|
||
| pub fn set_top_undo_meta(&self, meta: UndoItemMeta) { | ||
| self.inner.lock().borrow_mut().undo_stack.set_top_meta(meta); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1761,3 +1761,63 @@ fn undo_with_custom_commit_options() -> anyhow::Result<()> { | |
| assert_eq!(trigger_times.load(atomic::Ordering::SeqCst), 2); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn pause_preserves_undo_stacks_across_checkout() -> LoroResult<()> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both new tests are single-peer checkout round-trips — they pass with or without the Import-while-paused distinction, so the central claim of the second commit is currently untested. Suggest adding (here and mirrored in #[test]
fn import_while_paused_keeps_stacks_transformed() -> LoroResult<()> {
let doc_a = LoroDoc::new();
doc_a.set_peer_id(1)?;
let doc_b = LoroDoc::new();
doc_b.set_peer_id(2)?;
let mut undo = UndoManager::new(&doc_a);
let text_a = doc_a.get_text("text");
text_a.insert(0, "local")?;
doc_a.commit();
undo.pause();
// concurrent remote edit arrives while paused
doc_b.import(&doc_a.export(loro::ExportMode::all_updates())?)?;
doc_b.get_text("text").insert(0, "remote-")?;
doc_b.commit();
doc_a.import(&doc_b.export(loro::ExportMode::all_updates())?)?;
undo.resume();
// undo must remove only the local edit, at the transformed position
undo.undo()?;
assert_eq!(text_a.to_string(), "remote-");
Ok(())
}This is the regression test that would catch a future refactor back to a blanket early-return guard.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — added import_during_pause_transforms_undo_stack_correctly in both Rust and TS. |
||
| let doc = LoroDoc::new(); | ||
| doc.set_peer_id(1)?; | ||
| let mut undo = UndoManager::new(&doc); | ||
| let text = doc.get_text("text"); | ||
|
|
||
| text.insert(0, "Hello")?; | ||
| doc.commit(); | ||
| text.insert(5, " World")?; | ||
| doc.commit(); | ||
| assert_eq!(text.to_string(), "Hello World"); | ||
| assert!(undo.can_undo()); | ||
|
|
||
| // Pause before checkout so undo stacks survive | ||
| undo.pause(); | ||
| assert!(undo.is_paused()); | ||
| doc.checkout(&Frontiers::default())?; | ||
| assert_eq!(text.to_string(), ""); | ||
| doc.attach(); | ||
| assert_eq!(text.to_string(), "Hello World"); | ||
| undo.resume(); | ||
| assert!(!undo.is_paused()); | ||
|
|
||
| // Undo stacks should still be intact | ||
| assert!(undo.can_undo()); | ||
| undo.undo()?; | ||
| assert_eq!(text.to_string(), "Hello"); | ||
| undo.undo()?; | ||
| assert_eq!(text.to_string(), ""); | ||
|
|
||
| // Redo should also work | ||
| assert!(undo.can_redo()); | ||
| undo.redo()?; | ||
| assert_eq!(text.to_string(), "Hello"); | ||
| undo.redo()?; | ||
| assert_eq!(text.to_string(), "Hello World"); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn checkout_without_pause_clears_undo_stacks() -> LoroResult<()> { | ||
| let doc = LoroDoc::new(); | ||
| doc.set_peer_id(1)?; | ||
| let undo = UndoManager::new(&doc); | ||
| let text = doc.get_text("text"); | ||
|
|
||
| text.insert(0, "Hello")?; | ||
| doc.commit(); | ||
| assert!(undo.can_undo()); | ||
|
|
||
| // Checkout without pausing should still clear stacks (existing behavior) | ||
| doc.checkout(&Frontiers::default())?; | ||
| assert!(!undo.can_undo()); | ||
| assert!(!undo.can_redo()); | ||
|
|
||
| Ok(()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ export class UndoManager { | |
| #onPush: UndoConfig["onPush"]; | ||
| #onPop: UndoConfig["onPop"]; | ||
| #applying = false; | ||
| #paused = false; | ||
| #groupDepth = 0; | ||
| #unsubscribe: () => void; | ||
|
|
||
|
|
@@ -186,6 +187,18 @@ export class UndoManager { | |
| this.#redo.length = 0; | ||
| } | ||
|
|
||
| pause(): void { | ||
| this.#paused = true; | ||
| } | ||
|
|
||
| resume(): void { | ||
| this.#paused = false; | ||
| } | ||
|
|
||
| isPaused(): boolean { | ||
| return this.#paused; | ||
| } | ||
|
|
||
| destroy(): void { | ||
| this.#unsubscribe(); | ||
| this.clear(); | ||
|
|
@@ -195,13 +208,14 @@ export class UndoManager { | |
| if (this.#applying) return; | ||
| const targets = new Set(event.events.map(({ target }) => target)); | ||
| if (event.by === "checkout") { | ||
| this.clear(); | ||
| if (!this.#paused) this.clear(); | ||
| return; | ||
| } | ||
| if (event.by === "import") { | ||
| for (const target of targets) this.#remoteTargets.add(target); | ||
| return; | ||
| } | ||
| if (this.#paused) return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: this early return gives the runtime different semantics from the Rust side for local edits made while paused — here they're dropped forever (never undoable), while Rust folds them into the next undo item (see the divergence table in the review body). Whichever semantics wins in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — paused local edits now add to remoteTargets (matching the excluded-origin path). Both layers produce the same result. |
||
| if ( | ||
| event.origin !== undefined && | ||
| [...this.#excludeOriginPrefixes].some((prefix) => event.origin!.startsWith(prefix)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping the Local event entirely means
next_counteris not advanced, so the paused ops get folded into the span of the nextrecord_checkpoint— they do become undoable, contradicting the doc comment, and diverging from the TS runtime which genuinely drops them (see review body for the three-way repro).Suggest handling
pausedlike theshould_excludebranch below instead of returning early:That's the established pattern for "local edit that must not become an undo step," it keeps the stacks correctly transformed, and it makes the documented semantics true on both packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — paused now shares the
should_excludepath.