yjs / yjs

Shared data types for building collaborative software
https://docs.yjs.dev
Other
16.48k stars 593 forks source link

Allow undoing YMap keys overwritten by other clients #390

Closed doodlewind closed 2 years ago

doodlewind commented 2 years ago

Checklist

Related Problem

For now when undoing a YMap key that is overwritten by another user, Yjs intentionally disabled this key from being undone (see https://github.com/yjs/yjs/blob/6403bc2bb5/tests/undo-redo.tests.js#L96 and https://github.com/yjs/yjs/blob/6403bc2bb5/src/structs/Item.js#L160-L164). This may bring unexpected result, for example in an graphics editor, we have two users dragging same element:

// element dragged by A
yElementA.set('left', 0);

// same element dragged by B, this field is overwritten
yElementB.set('left', 100);

// same element dragged by A, field overwritten again
yElementA.set('left', 200);

// same element dragged by B again
yElementB.set('left', 300);

// undoing by A would have no effect
undoManagerA.undo();

// undoing by B would also have no effect
undoManagerB.undo();

We can see in this case, neither A nor B could undo their state change, because the key was overwritten by both of them. But if we use the mental model that always simply apply the reverse operation of a client's previous change, both of the clients should be able to recover to their previous state.

Proposed Solution

Since this behavior was controlled by an if block explicitly, we can trivially making it adjustable via an option for UndoManager, whose possible name can be ignoreOverwrittenKey: boolean, with true as its default value.

With ignoreOverwrittenKey set to false, we can get following behavior:

// element dragged by A
yElementA.set('left', 0);

// same element dragged by B
yElementB.set('left', 100);

// same element dragged by A again
yElementA.set('left', 200);

// same element dragged by B again
yElementB.set('left', 300);

// undoing by A would bring element to the previous state created by A
undoManagerA.undo(); // left: 0

// undoing by B would bring element to the previous state created by B
undoManagerB.undo(); // left: 100

// redoing by A or B would both bring element back to its last state before undoing
undoManagerA.redo(); // left: 300

undoManagerB.redo(); // left: 300

In this case, an undo buffer that doesn't prevent user from undoing would make the user experience more alike with the implementation shared by Figma's blog:

If you undo a lot, copy something, and redo back to the present (a common operation), the document should not change.

To explain this principle, we can notice that after being redone by A, the state is left: 300 created by B, instead of the left: 200 one created by A. This is perfectly expected because we were undoing from left: 300, so redoing back to left: 300 looks good.

We have tweaked the code above for enabling this feature in our forked earlier Yjs version, which works well. Thanks again for this awesome library!

Possible Alternatives

Personally I would suggest using ignoreOverwrittenKey: false as default, but this may become a breaking change.

Huly®: YJS-279

dmonad commented 2 years ago

Hi @doodlewind, I implemented this behavior as you suggested (just using a different option name). It's released in Yjs@13.5.32.

new Y.UndoManager(type, { ignoreRemoteMapChanges: true })

Would love to hear your feedback!