y-monaco icon indicating copy to clipboard operation
y-monaco copied to clipboard

The content is different under different operating systems (can be reproduce in the monaco demo)

Open chenyjade opened this issue 5 years ago • 20 comments

Please save me some time and use the following template. In 90% of all issues I can't reproduce the problem because I don't know what exactly you are doing, in which environment, or which y-* version is responsible. Just use the following template even if you think the problem is obvious.

Checklist

  • [ ] Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/
  • [ ] Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

Describe the bug A clear and concise description of what the bug is. The content is different under different operating systems. It seems that user under Win10 can see only a single new line when user under mac enter two new lines. Bug also appears after the syntax completion, see section below on how to reproduce. After some testing, the content under mac is the same as the content under Android and iOS, just different from the content under win10 and no matter which browser or computer we use.

To Reproduce Steps to reproduce the behavior:

  1. Open https://demos.yjs.dev/monaco/monaco.html under both windows and mac system
  2. Under windows: Type "Text from Windows" then click "Ender" button (new line)
  3. Under Mac: Click "Enter" button (new line) then type "Text from Mac"
  4. The content is different
  5. Clear all the content
  6. Under windows, enter something like "bbb" then enter a new line
  7. Start from the new line, enter "aaa" and click tab to accept the syntax completion (aaa -> AudioParam)
  8. You can see the difference, on Mac it's "aAudioParam" but on Win10 it's "AudioParam"

Expected behavior A clear and concise description of what you expected to happen. The content on both end should look the same.

Screenshots If applicable, add screenshots to help explain your problem. Screen shot from Windows: 2801608183250_ pic 2831608184727_ pic

Screenshot from Mac: 2811608183295_ pic 2821608184430_ pic

Environment Information

Additional context Add any other context about the problem here.

chenyjade avatar Dec 17 '20 06:12 chenyjade

Hi @chenyjade The problem is probably related to y-webrtc. There are know sync-issues between different browsers & platforms. https://github.com/yjs/y-webrtc/issues/19

Another problem is that y-webrtc seems to drop some messages if they are deemed too large (or for some other unknown reasons that I haven't figured out yet). There is help (https://github.com/yjs/y-webrtc/issues/20), but I need time to investigate this problem.

You are welcome to research the problem yourself. I currently don't have enough free time to work on it myself.

dmonad avatar Dec 18 '20 13:12 dmonad

This problem seems to be related to line endings. I debugged this further with a Windows Host machine and a Ubuntu Hyper-V VM:

I inserted the following Key Sequence:

1
2
3
ENTER
ENTER
BACKSPACE

The deltas produced and sent over the wire from windows are the following:

[{"insert":"1"}]
[{"retain":1},{"insert":"2"}]
[{"retain":2},{"insert":"3"}]
[{"retain":3},{"insert":"\r\n"}]
[{"retain":5},{"insert":"\r\n"}]
[{"retain":5},{"delete":2}]

The model text content on a windows patch receiver are the following:

["1"]
["1", "2"]
["1", "2", "3"]
["1", "2", "3", "\r", "\n"]
["1", "2", "3", "\r", "\n", "\r", "\n"]
["1", "2", "3", "\r", "\n"]

However, on the Ubuntu machine the patch receiver model text content is the following:

["1"]
["1", "2"]
["1", "2", "3"]
["1", "2", "3", "\n"]
["1", "2", "3", "\n", "\n"]
["1", "2", "3", "\n", "\n"]

It seems like there is now way to tell monaco to always use \n on all platforms. Thus, I guess we need some translation middleware, that transforms both IModelContentChangedEvent and the YText events.

n1ru4l avatar Jul 02 '21 12:07 n1ru4l

For reference, I inlined and adjusted the code over here: https://github.com/the-guild-org/schemehub/blob/567e24600de7786a0652130cb5aa8d417fa7c89d/lib/yMonaco.tsx and it seems like the issues are no longer present.

n1ru4l avatar Aug 12 '21 12:08 n1ru4l

Thanks for these hints! @n1ru4l & @chenyjade

@n1ru4l What exactly does your fix do? I don't find documentation on the setEOL method. Wouldn't it be better to enforce that everyone is using the same EOL format? It is surprising to me that Monaco is automatically changing the events.

dmonad avatar Aug 12 '21 17:08 dmonad

@dmonad It is impossible to set the EOL that should be used. You can only call setEOL directly for replacing all current line endings in the model. As soon as you hit enter it will use the platform line-specific endings at... So the best way is to call setEOL before applying patches and before producing patches. I did not notice any performance issues.

Another possible solution would be to try translating line endings from each platform, which I tried but failed miserably. setEOL seems to work perfectly.


reference links: https://github.com/microsoft/monaco-editor/issues/1886 https://github.com/microsoft/monaco-editor/issues/2019 https://github.com/FirebaseExtended/firepad/issues/315

n1ru4l avatar Aug 12 '21 19:08 n1ru4l

How did you fix the problem @n1ru4l? The file that includes your fix is too big for me to review entirely. Do you call setEOL every time Yjs applies a change? Wouldn't that change the length of the document?

dmonad avatar Aug 15 '21 14:08 dmonad

@dmonad

https://github.com/the-guild-org/schemehub/blob/567e24600de7786a0652130cb5aa8d417fa7c89d/lib/yMonaco.tsx#L299-L303 https://github.com/the-guild-org/schemehub/blob/567e24600de7786a0652130cb5aa8d417fa7c89d/lib/yMonaco.tsx#L371-L373

I added those two lines for fixing the behavior. Not sure about other implications but seems to work fine in an environment with 4 concurrent cross-platform users.

n1ru4l avatar Aug 15 '21 16:08 n1ru4l

@dmonad @n1ru4l is this fix working? I have the same issue!

yaralahruthik avatar Mar 16 '22 14:03 yaralahruthik

I can't say whether the fix works. It seems to work for @n1ru4l.

I'm not sure whether I want to get this merged in. I hope there is a better solution than this. What is the collaborative VSCode editor doing, for example?

Changing line endings to a specific format might lead to problems. E.g. we probably don't want to change line endings for Windows users that work on native files. I'm just a bit hesitant to merge this..

dmonad avatar Mar 16 '22 15:03 dmonad

@dmonad, I have tried the fix and it definitely works, though this would require you to pass the monaco instance to the binding. https://github.com/yaralahruthik/y-monaco. Yet I have decided to move away from monaco editor to code mirror. As I feel code mirror is more robust solution for my needs.

yaralahruthik avatar Mar 16 '22 17:03 yaralahruthik

Until we find a fix for the bug, I found a solution that I think prevents Monaco from writing '\r':

const editor = monaco.editor.create(document.getElementById("editor"), {
  value:"",
  language: "typescript",
});

// Note: there may be a way to update the existing model instead of swap it with a new one.
const newModel = monaco.editor.createModel("", "typescript");
newModel.setEOL(0); // Sets EOL to \n
editor.setModel(newModel); // Swap models

benhohner avatar Aug 30 '22 04:08 benhohner

Use editor.getModel().setEOL(0);

HARSHJAIN47 avatar Oct 06 '22 10:10 HARSHJAIN47

When testing the demo (https://demos.yjs.dev/monaco/monaco.html) with a colleague, where he's on Windows+Firefox and I am on Mac+Arc(Chromium) we have a consistent offset between his and my browser window. Our edits show up in the wrong place with mine shifted multiple characters left and his multiple to the right.

Based on the above ticket, should I just consider Yjs + Monaco as not working for cross-browser/platform collaboration?

wardweistra avatar Jan 25 '23 11:01 wardweistra

Attempting to set the EOL prior to creating the binding can cause the EOL to get reset due to y-monaco invoking setValue (see https://github.com/microsoft/monaco-editor/issues/1886). Setting it after can cause synchronization issues since the event handlers to propagate changes will have been registered by then.

TheUnlocked avatar Apr 15 '23 16:04 TheUnlocked

Why there are no updates on it since last 10 months, is this issue got solved?

for me this solution, is still not working! editor.getModel().setEOL(0);

yogesh-sirsat avatar Mar 04 '24 07:03 yogesh-sirsat

We also hit this issue recently; we had been using editor.getModel().setEOL(0) previously, but it stopped working once we introduced this package. I spent a great deal of time looking into this, as the above solution also didn't quite seem to work for me. Eventually I found this comment in the monaco-editor repo that noted that calling setValue() can have the side-effect of changing the EOL setting.

Looking at the code in this repo, I found that setValue() is used in one place: https://github.com/yjs/y-monaco/blob/7cecd513d411165dde657b19fde45ead3aefb0c8/src/y-monaco.js#L167-L170

I updated those lines to the following on my local machine, and it seems to have fixed the issue for us.

      const ytextValue = ytext.toString()
      if (monacoModel.getValue() !== ytextValue) {
        const eol = monacoModel.getEndOfLineSequence();
        monacoModel.setValue(ytextValue)
        monacoModel.setEOL(eol);
      }

cbh66 avatar Jun 12 '24 15:06 cbh66

When setting eol, you implicitly change the length of the line-endings. This might break collaboration. Are you sure that editors with different line-endings still sync up correctly?

dmonad avatar Jun 15 '24 18:06 dmonad

It does depend on the assumption that the contents of the ytext were produced with the same eol setting as the monaco model is using. If the ytext doesn't have any carriage returns, then setEOL doesn't have anything to change about the content of the text. It just restores the eol setting so that future edits to the ytext will continue to not use carriage returns.

We do want to test more to be sure it fully works as expected, but at least that's my understanding of it.

cbh66 avatar Jun 17 '24 14:06 cbh66