-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Update jupyter_ydoc and pycrdt_websocket dependencies #367
Open
brichet
wants to merge
7
commits into
jupyterlab:main
Choose a base branch
from
brichet:ydoc_awareness
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+34
−30
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9eed731
Update jupyter_ydoc and pycrdt_websocket dependencies (and indirectly…
brichet a6aeb86
linting
brichet a38c80e
Remove useless enumerate
brichet 8371341
Update the connected users on global awareness changes
brichet c1f8ebd
Apply suggestions from code review
brichet 080e51a
Test if the transcient room is the global awareness to observe it
brichet 218179b
typing
brichet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about
"updated"
users?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.
Actually I wonder if the
connected_users
of the_websocket_server
is even used.I copied that code from the previous message handler, but I don't know if we need that function.
If we keep it, we should indeed handle the "updated" users (removing the former name and add the new one).
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.
Thinking again about that, we don't have the previous name, the changes contain only the client ids.
We should rebuild the full list from the global awareness when a user is updated, because we don't know if the user name has been updated.
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.
Right, the goal of this code was to check in the backend that the awareness information from the frontend is correct. For instance, we don't want a student to take the user name of the teacher. That's why there is this skip variable that was supposed to filter out an awareness message, but this was never put to actual use.
I'm wondering how we can filter out a message with your changes though?
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 don't know if we can filter out a message with the current information we have.
For example when you reload the page, it adds a new client id with the same user information. The old client id is removed in a second step (probably when a client has a lack of update from it ?).
The same user is duplicated over a period of time, with 2 different client IDs. If we allow this behavior, I don't know how we can filter out someone trying to cheat.
The following image shows some logs when a remote client reloads the page. When a change is received, the change and the current users in the awareness are printed.
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.
The backend is the authority for user identities, and the frontend can get its identity at /api/me, so we should be able to check that they match.
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 agree that it's out of scope for this PR, but I'm wondering if we still get the possibility to discard an awareness update?