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

Support Undo/Redo for object.set and object.remove operations #658

Merged
merged 42 commits into from
Nov 15, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Sep 26, 2023

What this PR does / why we need it?

Implement the reverse operations for Object.set and Object.remove operations. (follow-up to #650)

1. Define reverse operations

1-1. Setting an element with the same createdAt

Set and Remove operations are based on the createdAt. When setting a value on the object, there was an issue where the new value couldn't be set during undo because the same createdAt was assigned to the element. To address this, I modified the comparison during value setting on the object to compare executedAt with the result of getPositionedAt() from the existing node. And if the value is reassigned during undo, movedAt is set.

public set(
    key: string,
    value: CRDTElement,
    executedAt: TimeTicket,
): CRDTElement | undefined {
    // ...
    // as-is 
    if (
        node == null ||
        value.getCreatedAt().after(node.getValue().getCreatedAt())
    ) {
        this.nodeMapByKey.set(key, newNode);
    }

    // to-be
    if (
        node == null ||
        executedAt.after(node.getValue().getPositionedAt())
    ) {
        this.nodeMapByKey.set(key, newNode);
        value.setMovedAt(executedAt);
    }
}

CRDTElement Life cycle

1-2. Setting nested objects: yorkie-team/yorkie#663

Where the document is {"shape":{"point":{"x":0,"y":0}}}, if you update the point using doc.update((root) => {root.shape.point = { x: 1, y: 1 }}), the undo operation results in [ 'Remove x', 'Remove y', 'Set point: {x:0, y:0}' ]. Currently, the set operation is designed to only create an empty object when the value is an object, so it cannot handle nested objects. Therefore, it is necessary to modify the set operation to be able to set nested objects. Refer to the test code(Should ensure convergence of peer's document after undoing nested objects) for more information.

2. Handle edge cases

2-1. When the target element is deleted by other peers

In set and remove operations, if the target of the operation has been deleted by another peer, the operation cannot be executed. Therefore, when executing operations during undo and redo, I implemented handling to skip the execution of reverse operations if the element has been deleted. Additionally, reverse operations for such operations are not generated, and further discussion on this matter can be considered in issue.

public execute(
  root: CRDTRoot,
  source?: OpSource,
): ExecutionResult | undefined {
  // ...

  // NOTE (chacha912): Handle cases where the operation cannot be executed during undo and redo.
  if (source === OpSource.UndoRedo && obj.getRemovedAt()) {
    return;
  }
}

2-2. When the target element is garbage-collected(purged): yorkie-team/yorkie#664

The current behavior of garbage collection involves deleting elements that document replicas can no longer reference. In other words, if an element has been deleted and it occurred before minSyncedTicket, it can be considered as not referable by each replica. However, after the introduction of Undo/Redo, even deleted elements can be referenced through the undo/redo stack, so they shouldn't be garbage collected. Currently, we are testing with the {disableGC: true} option. We plan to resolve this by considering the elements referenced by operations stored in the stack when performing GC.

3. Add whiteboard example

I added the whiteboard example to test the undo redo object.

whiteboard

3-1. Feature Introduction

  • You can add, delete rectangles, and change the color.
  • You can perform undo and redo for object set and remove operations.
    • I.E. you can undo and redo for rectangle color changes and position movements.
    • Undo and redo for adding/deleting rectangles can be tested after implementing the reverse operation for array.add and array.remove (as shapes are managed as an array).
  • At the bottom of the example, you can view logs related to the yorkie data.
    • The yorkie document shows the document's root object and allows you to check values and createdAt.
    • The operations show all the operations applied to the document.
    • The undo stack and redo stack show the operations they contain.

3-2. Variables Added for Displaying Yorkie Data

  • CRDTRoot.opsForTest: A variable for monitoring all operations, accumulating operations only in the development environment.
  • CRDTElement.toJSForTest: Information including value and id(createdAt) (Note: toJS and toJSON only return value information).

3-3. Additional Features to Apply (Todo)

  • Implement history pause and resume: Merge changes for rectangle movement into a single undo (pause history on mousedown and resume on mouseup).
  • Apply doc.subscribe('history'): Activate undo and redo buttons when canUndo and canRedo are true.
  • Apply reverse operations for array.add and array.remove.

Any background context you want to provide?

What are the relevant tickets?

Address yorkie-team/yorkie#652

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (7c8313a) 67.90% compared to head (79eca12) 67.94%.
Report is 5 commits behind head on main.

Files Patch % Lines
src/document/crdt/array.ts 44.44% 5 Missing ⚠️
src/document/crdt/object.ts 58.33% 5 Missing ⚠️
src/document/json/object.ts 57.14% 2 Missing and 1 partial ⚠️
src/document/document.ts 90.47% 1 Missing and 1 partial ⚠️
src/document/operation/operation.ts 71.42% 1 Missing and 1 partial ⚠️
src/document/operation/remove_operation.ts 87.50% 0 Missing and 2 partials ⚠️
src/document/crdt/counter.ts 0.00% 1 Missing ⚠️
src/document/crdt/primitive.ts 0.00% 1 Missing ⚠️
src/document/crdt/text.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
+ Coverage   67.90%   67.94%   +0.04%     
==========================================
  Files          58       58              
  Lines        8737     8783      +46     
  Branches      785      795      +10     
==========================================
+ Hits         5933     5968      +35     
- Misses       2546     2555       +9     
- Partials      258      260       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chacha912 chacha912 marked this pull request as ready for review November 6, 2023 15:54
@chacha912 chacha912 requested a review from hackerwins November 7, 2023 02:08
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

@hackerwins hackerwins merged commit 859c019 into main Nov 15, 2023
1 check passed
@hackerwins hackerwins deleted the undo-redo-object branch November 15, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants