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

Support collaboration feature #13309

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Support collaboration feature #13309

wants to merge 5 commits into from

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 24, 2024

What it does

Related to #2842 (maybe even closes it?)

Makes use of the newly designed open-collaboration-protocol (source code here) to enable collaboration across users on the same workspace. The main ideas have already been outlined in #12677.

This PR implements basic workspace and editor sharing. All guests will work on the host workspace and all edits to a file will be shared across all users. Presence information (i.e. cursor selection of other peers in the collaboration room) is also available.

How to test

  1. Install the open collaboration server instance via npm i -g open-collaboration-server and start it via ocp-server start.
  2. Open Theia and click on the share status bar item. Open a new session (you may need to log in - this is only to give you a name). This will put a room code into your clipboard.
  3. Open another Theia instance click on the status bar item again. Connect to a session and input the code from your clipboard.
  4. Play around with the IDE. Please be aware that not everything is working 100% correctly and not everything might be addressed in this PR.

Follow-ups

There are a lot of improvements on this:

  • Improve the save functionality of guests (it seems to be pretty much broken)
  • Add more editors (like notebook editors)
  • Improve the general UI/UX of this feature
  • Add support for permission changes (especially read-only)
  • Add support for collaborative debugging
  • Add support for collaborative terminals

Review checklist

Reminder for reviewers

@msujew msujew added the collaboration issues related to collaboration label Jan 24, 2024
This was referenced Jan 24, 2024
@msujew msujew force-pushed the msujew/collab-feature branch 2 times, most recently from 89bf3fa to 9e2fa2d Compare January 29, 2024 10:55
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this feature, @msujew, and thanks for your contribution!

During testing, I encountered a few issues beyond those mentioned in the PR description, such as the broken save functionality. Additionally, there seems to be a problem with editor synchronization, as demonstrated in this video: collabsession.webm. I encountered these issues while adding text alternately in different parts of the editor. The setup was using the latest master branch of the open-collaboration-server. Let me know if I can help with providing more specific steps to replicate these problems.

I've also added some minor inline comments regarding the wording and capitalization consistency of UI labels.

Regarding extending support to other types of editors (like notebooks or custom editors), this might be more relevant to the open-collaboration-server repository. However, I'm curious about the approach for integrating additional editors. It seems there are currently no generic message types in the open-collaboration-protocol that one could extend for syncing non-text editors. For Theia, I assume, we would then need to expose collaboration and connection events for other components to respond to new sessions or connection events, enabling them to communicate through these connections.

Looking ahead, should we consider extending the VS Code extension API or creating a VS Code extension that adapts the open-collaboration-server protocol to the VSLS API? What are your thoughts on this?

Thanks again!

@msujew
Copy link
Member Author

msujew commented Feb 15, 2024

Hey @planger thanks for the feedback! It'll probably take a bit until I get to updating the PR, but just to answer your questions:

Regarding extending support to other types of editors (like notebooks or custom editors), this might be more relevant to the open-collaboration-server repository. However, I'm curious about the approach for integrating additional editors. It seems there are currently no generic message types in the open-collaboration-protocol that one could extend for syncing non-text editors. For Theia, I assume, we would then need to expose collaboration and connection events for other components to respond to new sessions or connection events, enabling them to communicate through these connections.

There is a generic Message type (see here) that can be instantiated to support any kind of message for any kind of editor. The current editor messages are just using the class-based signatures for that, see here. Adding a new editor should be as simple as sending and responding to those message types:

connection.onRequest('graphicalEditor', (..args) => { ... });
...
connection.sendRequest('graphicalEditor', ...args);

No need to create a separate connection/session.

Looking ahead, should we consider extending the VS Code extension API or creating a VS Code extension that adapts the open-collaboration-server protocol to the VSLS API? What are your thoughts on this?

Yes, definitely. Though it likely needs to happen by effectively mocking the vscode.extensions.getExtension('ms-vsliveshare.vsliveshare') call, since replacing the import to vsls won't work due to bundling in extensions. It's definitely on my mind!

Let me know if I can help with providing more specific steps to replicate these problems.

That would actually be great. I was aware that this issue could occur, but I wasn't able to replicate it.

@planger
Copy link
Contributor

planger commented Feb 15, 2024

@msujew Thank you for your feedback and clarifying the question about the message!

For Theia, I assume, we would then need to expose collaboration and connection events for other components to respond to new sessions or connection events, enabling them to communicate through these connections.

connection.onRequest('graphicalEditor', (..args) => { ... });
No need to create a separate connection/session.

Great! I was wondering how to get access to the connection and react to session events from a Theia extension. Maybe I missed it, but there doesn't seem to be a way to register session listeners or obtain a connection, or is there?

Let me know if I can help with providing more specific steps to replicate these problems.

That would actually be great. I was aware that this issue could occur, but I wasn't able to replicate it.

Sure, I'll try to reproduce it in the next days and get back to you.

Thanks again for pushing this excellent new feature forward!

@planger
Copy link
Contributor

planger commented Feb 16, 2024

@msujew Hi again, I've now been looking into reproducing this issue and for me it really seems very easy to reproduce. I've recorded a video for reproducing this with a fresh server session:
collabsession2.webm

I don't see anything suspicious in the logs of the collaboration server or Theia. If you want me to enable or add some logging, please let me know. I'm happy to help out in any way!

@msujew msujew force-pushed the msujew/collab-feature branch 2 times, most recently from 3398824 to 4a746ed Compare May 14, 2024 10:28
@msujew msujew marked this pull request as ready for review May 14, 2024 10:29
@msujew
Copy link
Member Author

msujew commented May 14, 2024

Hey @planger thanks for the help, I've integrated Yjs properly into the protocol now to support real editor syncing. Desyncs should no longer be possible now. The PR is now ready for a real review as most of the issues I had with it have been adressed.

@jonah-iden
Copy link
Contributor

@msujew One smaller issue i noticed is "cursors" from other users in the session are not removed from a file when switching to/focusing another. The seem to only be removed when the other user is closing the file.

For example the host is working in file1 then switching to file2. The guest is now seeing the host cursor in both file1 and file2.

grafik

@jonah-iden
Copy link
Contributor

Another thing: There is currently no feedback for a guest when joining a room and wating for the host to accept. Would be nice to have a progress message 'connecting to room' or 'waiting for host to let you join'

@jonah-iden
Copy link
Contributor

The dirty state of files is also not shared currently right? But for the first version of this feature i think thats fine

@jonah-iden
Copy link
Contributor

jonah-iden commented May 16, 2024

Files opened by the host before starting the session seem to not be synchronized at all. The user needs to reopen the file before it works

@msujew
Copy link
Member Author

msujew commented May 16, 2024

@jonah-iden Thanks for looking into this!

One smaller issue i noticed is "cursors" from other users in the session are not removed from a file when switching to/focusing another. The seem to only be removed when the other user is closing the file.

Oh, yeah. I was wondering why my architecture for keeping track of all the peer cursors became so complicated. Having just a single cursor per peer makes stuff way easier :D

Another thing: There is currently no feedback for a guest when joining a room and wating for the host to accept. Would be nice to have a progress message 'connecting to room' or 'waiting for host to let you join'

Good idea 👍

The dirty state of files is also not shared currently right? But for the first version of this feature i think thats fine

Right, I'm waiting for #13683 to be merged before tackling this.

Files opened by the host before starting the session seem to not be synchronized at all. The user needs to reopen the file before it works

Cannot reproduce on my system:

2024-05-16.17-11-47.mp4

@jonah-iden
Copy link
Contributor

Files opened by the host before starting the session seem to not be synchronized at all. The user needs to reopen the file before it works

seems to only happen on files that are automaticly opened on application startup and are visible when the guest connects. But im not totally sure.

Im also getting a weird effect sometimes when opening a file the host has already edited its just empty for the guest and only contains the content after the second time opening it

@planger
Copy link
Contributor

planger commented May 17, 2024

@msujew Thank you very much for this excellent iteration! I can confirm that I can't reproduce the issue of the two editors getting out of sync anymore with plain consecutive editing on both sides 👍

I understand that save on the guest doesn't work yet (PR description). What seems to be a rather severe bug though is:

  • Open file on host
  • Open file on guest
  • Host make some changes -> editor becomes dirty for both (great!)
  • Guest closes the editor and selects "Don't save" -> file is reset to state on disk also for the host (pretty bad)
    I'd expect the state to remain unchanged for the host.

Also, I observed some issues with undo/redo. I managed to get into an inconsistent state between the two editors if I do a change on the left and a change on the right, then go back to left and undo, it seems to undo the change left and right (I feel it only should undo the change from the left). If now the right editor does an undo, I ended up in an inconsistent state. Can you reproduce this too?

@msujew
Copy link
Member Author

msujew commented May 17, 2024

Thank you very much for this excellent iteration! I can confirm that I can't reproduce the issue of the two editors getting out of sync anymore with plain consecutive editing on both sides 👍

Glad to hear that! It took me quite a while to get that right 😅

Also, I observed some issues with undo/redo. I managed to get into an inconsistent state between the two editors if I do a change on the left and a change on the right, then go back to left and undo, it seems to undo the change left and right (I feel it only should undo the change from the left). If now the right editor does an undo, I ended up in an inconsistent state. Can you reproduce this too?

Yes, perfect reproduction steps, thank you :D I've tried replicating the behavior of VSCode Live Share, which actually undoes the latest changes (even if they're from someone else), but that doesn't work as well as I hoped it would. Anyway, I've adjusted it so that users can now only undo their own changes. I think that behavior is fine until I find a better solution for that.

I understand that save on the guest doesn't work yet (PR description). What seems to be a rather severe bug though is:

Hm, I see. It took me a bit to figure out how to fix that but I think I found a well-working solution :)

I've pushed some changes that should improve on these problem areas. @jonah-iden I might have also fixed the first-load issue that you were experiencing as a drive-by, but can't guarantee anything.

@planger
Copy link
Contributor

planger commented May 20, 2024

@msujew Thank you very much! a727293 indeed fixes the two raised issues:

  1. Loss of all unsaved changes when the guest closes the dirty editor
  2. The inconsistent state when one side is using undo

Excellent work!

I still feel that the undo/redo behavior -- even though it is consistent now -- is not ideal. I don't know how VS Code handles the undo/redo in editing sessions, but I personally find it very confusing if all participants share a common command stack and don't have separate command stacks (ie. undo/redoing only their changes), as it seems to be the case now.

Think of working on a larger file, making a change and then use undo. If someone else happens to have made a change between your last change and the undo (which easily happens if two or more persons work on a file), undo seemingly didn't do anything, even though it actually undid a foreign change out of sight (similarly it is weird for the other person whose change has been reverted and taken off their command stack). Even worse if you undo a few times... the first may undo your change, the second may undo another change that may have happened in the meantime.

Also, in the current implementation it also seems that the "undo granularity" is different for the one who made the change and for the others. The one who made the change has a word-by-word undo (as usual in Monaco), while all others seem to have a character by character undo (a bit cumbersome :-)).

However, feel free to leave any undo/redo improvements open for future PRs. Given the breadth of this change, the undo/redo behavior certainly may not be a blocker for having this merged, as long as it is consistent. :-)

@msujew
Copy link
Member Author

msujew commented May 20, 2024

I still feel that the undo/redo behavior -- even though it is consistent now -- is not ideal. I don't know how VS Code handles the undo/redo in editing sessions, but I personally find it very confusing if all participants share a common command stack and don't have separate command stacks (ie. undo/redoing only their changes), as it seems to be the case now.

Actually, VSCode handles this the same way. It's not possible to influence the undo/redo buffer via the VSCode API and so there is a bit of desync within each users undo/redo buffer. Undoing an action as one user (i.e. removing the top element from the stack) leads to all other users adding the inverse action to their own undo/redo buffer. It's pretty weird, and it seems like the VSCode people also weren't able to solve this in a satisfactory manner. For simplicities sake, I've aligned Theia's behavior exactly to VSCode for now. This also makes it easier later on to, for example, have people from Theia and VSCode join the same collaboration room.

It's still something that I would like to revisit eventually, but I think it's good enough for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collaboration issues related to collaboration
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

None yet

3 participants