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

Text/Cursor desynchronized when typing at the same time. #2

Closed
rafeautie opened this issue Aug 1, 2020 · 8 comments
Closed

Text/Cursor desynchronized when typing at the same time. #2

rafeautie opened this issue Aug 1, 2020 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@rafeautie
Copy link

rafeautie commented Aug 1, 2020

Describe the bug
Behavior 1: When two users are typing at the same time. The cursor that is farthest down the page moves up with every key press.

Behavior 2: When two users are typing at the same time. The text gets out of sync. (i.e. one user has many lines of line breaks, the other does not)

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'https://rafe-dev-bp41knwx5.vercel.app/code'
  2. Click on 'Connect'
  3. Connect two users to the same room
  4. Type at the same time

Expected behavior
Behavior 1 and 2 do not happen. I expect one person type to not affect another persons cursor

Environment Information

  • Chrome
  • yjs ^13.3.0
  • y-webrtc ^10.1.6
  • y-monaco ^0.0.1
  • react-monaco-editor ^0.39.1

Additional context
Im willing to assist in reproducing the issue. It might be difficult to do alone. Let me know so we can coordinate a time.

@rafeautie rafeautie added the bug Something isn't working label Aug 1, 2020
@akosyakov
Copy link

Behavior 1: When two users are typing at the same time. The cursor that is farthest down the page moves up with every key press.

try to remove this code:

y-monaco/src/y-monaco.js

Lines 150 to 156 in e17d9bc

this._savedSelections.forEach((rsel, editor) => {
const sel = createMonacoSelectionFromRelativeSelection(editor, ytext, rsel, this.doc)
if (sel !== null) {
editor.setSelection(sel)
}
})
})
Monaco can handle how cursor should be moved while editing. For me it gives better results.

Behavior 2: When two users are typing at the same time. The text gets out of sync. (i.e. one user has many lines of line breaks, the other does not)

I've observed the same issue with yjs in another context. Not sure that it is related to Moanco integration.

@akosyakov
Copy link

I wonder about this line:

event.changes.sort((change1, change2) => change2.rangeOffset - change1.rangeOffset).forEach(change => {
Monaco provides edits which should be applied to the previous version in the given order to get new results, changing order of edits can give different result.

@dmonad Could you clarify please why it is important to reorder them fro yjs? 🙏

dmonad added a commit that referenced this issue Aug 7, 2020
@dmonad
Copy link
Member

dmonad commented Aug 7, 2020

Hi @akosyakov

Monaco used to have a very confusing representation of changes.

Starting with the doc "ab" and the changes object [{ pos: 0, insert: '1' }, { pos: 1, insert: '2' }], the resulting document should be "1a2b". You are expected to apply the changes as one atomic operation. I reorder the operations by decreasing positions to ensure that the operations can be applied on the Yjs model.

Nowadays, Monaco already sorts the operations and my sort implementation won't have any effect.

I might be wrong about this. But this is how I understood monaco changes.

I fixed the cursor issue. Thanks for notifying me. I only test that the documents synchronize. The most recent yjs release has a breaking change that breaks cursor synchronization in monaco and prosemirror.

But I can't reproduce any syncing issues. I opened your app and I also tried locally. Please open a separate ticket with a description on how to reproduce it. If you don't know how to exactly reproduce it, please try to make a recording of the issue. I already have several automatic tests for the monaco binding. There is probably an edge case (on the event object?) I'm not considering.

dmonad added a commit that referenced this issue Aug 7, 2020
@dmonad
Copy link
Member

dmonad commented Aug 7, 2020

I published the fix in y-monaco@0.1.0

@stefan-egorov
Copy link

stefan-egorov commented Aug 11, 2020

There still seems to be issues, If both collaborators start with a blank state(not on different lines), editor selects whole blocks of text. Will try to make a test-case for it.

@akosyakov
Copy link

akosyakov commented Aug 15, 2020

Starting with the doc "ab" and the changes object [{ pos: 0, insert: '1' }, { pos: 1, insert: '2' }], the resulting document should be "1a2b". You are expected to apply the changes as one atomic operation. I reorder the operations by decreasing positions to ensure that the operations can be applied on the Yjs model.

@dmonad Monaco expects to apply a consequent change to a document resulted from an application of a previous change. Do I understand correctly that Yjs expects to apply all changes to the original content?

I'm not sure reordering monaco changes like that is safe. (Although it should be probably alright if all changes are happening from typing, not from programmatic edits) If one does not use transact then changes are applied in the same manner as by Monaco but each change result in an update event?

@dmonad
Copy link
Member

dmonad commented Aug 15, 2020

Reordering is still necessary in Monaco pre version 17. I implemented multiple teats that confirm that Monaco also orders changes by position. So the current approach should still work.

I will stop reordering only if it causes bugs to keep supporting older versions.

Changes within a Yjs transaction only produce a single event. Otherwise there is no difference whether you use a transaction or several smaller updates.

@stefan-egorov
Copy link

There still seems to be issues, If both collaborators start with a blank state(not on different lines), editor selects whole blocks of text. Will try to make a test-case for it.

Ignore this. I noticed beforeAllTransactions handler wasn't being invoked, apparently it was very recently introduced in yjs and I wasn't on the latest version. All good now, cheers!

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

4 participants