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

Use Y.UndoManager #10

Closed
wants to merge 5 commits into from
Closed

Use Y.UndoManager #10

wants to merge 5 commits into from

Conversation

domagojk
Copy link

Resolves #9

I have reverted the change made in #8 and added Y.UndoManager.

It works for the most part, but there is an issue if someone makes a change at nearly the same time the other person triggers undo/redo operation. Here is a video to demonstrate:

y-monaco-undo.mov

On the left tab, I've created a function that inserts number "2" every 500ms, and on the right tab I'm triggering undo/redo command. You can see how undo affects the remote change.

I was trying to figure out why this happens, and it seems to me that in afterTransaction handler transaction.afterState includes the change made by other person that is then pushed in undo stack. Shouldn't beforeState and afterState be isolated to a single transation?

@domagojk
Copy link
Author

domagojk commented Mar 10, 2021

Btw, here is the function I've used for inserting text (in monaco-demo.js)

window.monacoStart = interval => {
  window.startinterval = setInterval(
    () =>
      editor.getModel().pushEditOperations(
        [
          {
            endColumn: 32,
            endLineNumber: 6,
            positionColumn: 32,
            positionLineNumber: 6,
            selectionStartColumn: 32,
            selectionStartLineNumber: 6,
            startColumn: 32,
            startLineNumber: 6,
          },
        ],
        [
          {
            text: '2',
            range: {
              endColumn: 32,
              endLineNumber: 6,
              startColumn: 32,
              startLineNumber: 6,
            },
          },
        ],
        () => null,
      ),
    interval,
  );
};

window.monacoEnd = () => {
  clearInterval(window.startinterval);
};

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

I didn't finish the Y.UndoManager integration. You should be able to implement it though. First, you should probably read the documentation on the Y.UndoManager here: https://docs.yjs.dev/api/undo-manager and have a look at a working implementation that uses Y.UndoManager (https://github.com/yjs/y-codemirror).

The idea is that every change from the editor (local changes) are associated with the binding instance (the transaction origin is set to the binding instance). The UndoManager should only track changes associated with the binding instance.

As a next step, you probably want to restore the previous selection as I do in the y-codemirror binding.

@domagojk
Copy link
Author

The idea is that every change from the editor (local changes) are associated with the binding instance (the transaction origin is set to the binding instance). The UndoManager should only track changes associated with the binding instance.

Yea, but I've added:

this.yUndoManager.trackedOrigins.add(this) // track changes performed by this editor binding

just like in y-codemirror. And it works, but only when changes happen at the same time this bug occurs.

I haven't restored previous selection tho, but that wouldn't fix the issue, right?

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

Oh okay, this looks like the Y.UndoManager is not being used. Are you sure that your undo command takes precedence?

@domagojk
Copy link
Author

Yea, I'm sure. This is triggered when executing cmd-z:

Screen Shot 2021-03-10 at 15 10 52

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

Could you please check if the other monaco-UndoManager also triggered?

Instead, you could also verify that Y.UndoManager is doing something wrong by emitting the Y.Text events.

If Y.UndoManager undo's remote changes, then that would mean that these changes are somehow tracked in Y.UndoManager (using the transaction origin).

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

Also, you probably don't want to track the null origin. So instead you should initialize the trackedOrigins property properly:

const undoManager = new Y.UndoManager(ytext, {
  trackedOrigins: new Set([this]) // the default is Set([null])
})

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

If you are using a provider that doesn't specify an origin, then you can probably fix the issue by removing the null origin from trackedOrigins

@domagojk
Copy link
Author

Could you please check if the other monaco-UndoManager also triggered?

I did. Right now the only way to create custom monaco undo/redo is by overriding the shortcut (microsoft/monaco-editor#2005). If you override it with an empty function, it wont react on those shortcuts, so I'm pretty sure it is disabled.

Also, you probably don't want to track the null origin. So instead you should initialize the trackedOrigins property properly:

I've added initial tracked origin as suggested, but it's still the same.
Btw I guess, that should also be done in y-codemirror then? https://github.com/yjs/y-codemirror/blob/master/demo/codemirror.js#L13

Just for context, we have been using yjs in a company I work for, and it was great so far! This was the issue that pop-up while two users were working together on a file. My initial assumption was also that we were using origins in a wrong way, so just to be sure I have even tried adding

if (!transaction.local) return;

at this line (just to test it) but it didn't help.

I wasn't sure if it was the bug on our end or something in yjs, so I've tried to replicate it here and it is the same.

As I said it seems to me that in transaction.afterState somehow picks up remote change if it happens at nearly the same time undo is triggered. But I'm not sure about that 🙂

@dmonad
Copy link
Member

dmonad commented Mar 10, 2021

As I said it seems to me that in transaction.afterState somehow picks up remote change if it happens at nearly the same time undo is triggered. But I'm not sure about that 🙂

It works correctly in y-codemirror (and other bindings), so there is probably some issue with how y-monaco interacts with the editor.

I usually start by tracking what exactly happens in each event (monaco & Y.Undomanager & Y.Text events).

@domagojk
Copy link
Author

Ok, thanks!

@domagojk
Copy link
Author

It works correctly in y-codemirror (and other bindings), so there is probably some issue with how y-monaco interacts with the editor.

I was able to reproduce this in y-codemirror. I haven't changed anything in the codebase, I've just added this function that inserts a character every 500ms so I can test it:

window.startTyping = (interval) => {
  window.starttimeout = setInterval(() => {
    editor.replaceRange("2", { ch: 1, line: 1 }, { ch: 1, line: 1 });
  }, interval);
};

steps to reproduce:

  1. open two browser tabs
  2. execute startTyping(500) in first tab
  3. start typing in second tab and then execute undo/redo (try to do it at the same time as the character is inserted)

you should be able to modify text originating from the first tab

y-codemirror-demo.mov

So it seems the issue is in UndoManager. I will try to fix it but if you have any ideas what could be the issue, let me know :)

@domagojk
Copy link
Author

I can confirm that it works when I reverted UndoManager to this commit
so the issue seems to be introduced in yjs/yjs@dab172f

dmonad added a commit to yjs/yjs that referenced this pull request Mar 11, 2021
@dmonad
Copy link
Member

dmonad commented Mar 11, 2021

I think I found the issue. Can you try again with [email protected]?

@domagojk
Copy link
Author

It works now! Thank you!! 🙂 🙏

@dmonad
Copy link
Member

dmonad commented Mar 14, 2021

Great 👍

Then I'm going to close this PR.

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.

Use Yjs UndoManager
2 participants