-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
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. |
…nto undo-redo-object
hackerwins
force-pushed
the
main
branch
2 times, most recently
from
October 20, 2023 07:46
a9325d6
to
7baa69f
Compare
20 tasks
…kie-js-sdk into undo-redo-object
…nto undo-redo-object
- Display undo/redo stack count - Prevent script errors when clicking the undo/redo buttons
- Remove duplication between Array.getPositionedAt and Element.getLastExecutedAt - Add more comments
`getByID` is a method used by the system. In the CRDT layer, it should still return the element even if it has been deleted.
hackerwins
approved these changes
Nov 15, 2023
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.
LGTM. 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it?
Implement the reverse operations for
Object.set
andObject.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 samecreatedAt
was assigned to the element. To address this, I modified the comparison during value setting on the object to compareexecutedAt
with the result ofgetPositionedAt()
from the existing node. And if the value is reassigned during undo,movedAt
is set.1-2. Setting nested objects: yorkie-team/yorkie#663
Where the document is
{"shape":{"point":{"x":0,"y":0}}}
, if you update the point usingdoc.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.
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.
3-1. Feature Introduction
yorkie document
shows the document's root object and allows you to check values and createdAt.operations
show all the operations applied to the document.undo stack
andredo 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
andtoJSON
only return value information).3-3. Additional Features to Apply (Todo)
pause
andresume
: Merge changes for rectangle movement into a single undo (pause history on mousedown and resume on mouseup).doc.subscribe('history')
: Activate undo and redo buttons whencanUndo
andcanRedo
are true.array.add
andarray.remove
.Any background context you want to provide?
What are the relevant tickets?
Address yorkie-team/yorkie#652
Checklist