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

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

Open
2 tasks
chenyjade opened this issue Dec 17, 2020 · 20 comments
Assignees
Labels
bug Something isn't working

Comments

@chenyjade
Copy link

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

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 chenyjade added the bug Something isn't working label Dec 17, 2020
@dmonad
Copy link
Member

dmonad commented Dec 18, 2020

Hi @chenyjade The problem is probably related to y-webrtc. There are know sync-issues between different browsers & platforms. yjs/y-webrtc#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 (yjs/y-webrtc#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.

@n1ru4l
Copy link

n1ru4l commented Jul 2, 2021

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
Copy link

n1ru4l commented Aug 12, 2021

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.

@dmonad
Copy link
Member

dmonad commented Aug 12, 2021

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.

@n1ru4l
Copy link

n1ru4l commented Aug 12, 2021

@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:
microsoft/monaco-editor#1886
microsoft/monaco-editor#2019
FirebaseExtended/firepad#315

@dmonad
Copy link
Member

dmonad commented Aug 15, 2021

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?

@n1ru4l
Copy link

n1ru4l commented Aug 15, 2021

@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.

@yaralahruthik
Copy link

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

@dmonad
Copy link
Member

dmonad commented Mar 16, 2022

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..

@yaralahruthik
Copy link

@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.

@benhohner
Copy link

benhohner commented Aug 30, 2022

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

@HARSHJAIN47
Copy link

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

@wardweistra
Copy link

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?

@TheUnlocked
Copy link

TheUnlocked commented Apr 15, 2023

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

@yogesh-sirsat
Copy link

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);

@cbh66
Copy link

cbh66 commented Jun 12, 2024

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:

y-monaco/src/y-monaco.js

Lines 167 to 170 in 7cecd51

const ytextValue = ytext.toString()
if (monacoModel.getValue() !== ytextValue) {
monacoModel.setValue(ytextValue)
}

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);
      }

@dmonad
Copy link
Member

dmonad commented Jun 15, 2024

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?

@cbh66
Copy link

cbh66 commented Jun 17, 2024

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.

@ijevtic
Copy link

ijevtic commented Oct 23, 2024

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:

y-monaco/src/y-monaco.js

Lines 167 to 170 in 7cecd51

const ytextValue = ytext.toString()
if (monacoModel.getValue() !== ytextValue) {
monacoModel.setValue(ytextValue)
}

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);
      }

Do we have a working solution? I've tried with this, but the issue persists.

@cbh66
Copy link

cbh66 commented Oct 28, 2024

@ijevtic Yes, we've had this patch running in production for a few months now, and it's been working fine. I don't know the details of the rest of your project, but there are two other things to check:

  1. Make sure you actually are calling setEOL. It seems obvious, but our code used to sometimes fail to set it properly, since that happened in a react effect that wasn't always re-triggered. When we failed to set EOL, windows users continued to get the default line endings.
  2. Make sure that anywhere else outside of monaco that your y.js contents are modified, a \r cannot be inserted. For us, for example, we allowed users to upload text files which would initialize some y.js text. This allowed \rs to sneak in on files that were uploaded from windows machines. So we had to replace line endings manually before initializing our y.js text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests