-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
89bf3fa
to
9e2fa2d
Compare
There was a problem hiding this 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!
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
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:
There is a generic connection.onRequest('graphicalEditor', (..args) => { ... });
...
connection.sendRequest('graphicalEditor', ...args); No need to create a separate connection/session.
Yes, definitely. Though it likely needs to happen by effectively mocking the
That would actually be great. I was aware that this issue could occur, but I wasn't able to replicate it. |
@msujew Thank you for your feedback and clarifying the question about the message!
Great! I was wondering how to get access to the
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! |
@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: 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! |
3398824
to
4a746ed
Compare
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. |
@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. |
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' |
The dirty state of files is also not shared currently right? But for the first version of this feature i think thats fine |
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 |
@jonah-iden Thanks for looking into this!
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
Good idea 👍
Right, I'm waiting for #13683 to be merged before tackling this.
Cannot reproduce on my system: 2024-05-16.17-11-47.mp4 |
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 |
@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:
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? |
Glad to hear that! It took me quite a while to get that right 😅
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.
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. |
@msujew Thank you very much! a727293 indeed fixes the two raised issues:
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. :-) |
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. |
a727293
to
6ae8175
Compare
@msujew When do we plan to merge this? |
6ae8175
to
56a846e
Compare
@JonasHelming I've just updated this PR to the latest version of the collaboration packages. It no longer needs a local server but runs on the public instance that we're currently running on google cloud. All that's needed to merge this is a final review and approval. |
45e464d
to
38d180f
Compare
38d180f
to
b596f15
Compare
@tsmaeder This is ready for another review. Note that the current version is a Regarding the license: It seems like the 3PP check passes successfully - there should be no issues with the license we're using. |
@msujew I can now log in, but on Electron/Windows, the login windows still open as extra Electron windows and can't be closed. Could you maybe rebase the PR so we can test that it works with the now merged PR about external links and Electron? One UI suggestion: it would be neat if there was an indication which one of the users is the local user. If you have multiple users with the same name (thomas1, thomas2), it might be useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test the collaboration support extensively, but I spent a day working with these code changes and they do not seem to introduce any breakage when not used.
I have some comments which we should discuss and maybe address in a follow-up.
packages/collaboration/src/browser/collaboration-file-system-provider.ts
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-file-system-provider.ts
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-file-system-provider.ts
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Show resolved
Hide resolved
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
b596f15
to
8429fdc
Compare
I forgot to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that the sign-in now opens a browser window. Let's merge this and continually improve as we go along.
packages/collaboration/src/browser/collaboration-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
@msujew Congrats! |
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
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.Follow-ups
There are a lot of improvements on this:
WorkspaceService
API to enable virtual workspaces #14080Review checklist
Reminder for reviewers