-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
if type != "change": | ||
return | ||
added_users = changes[0]["added"] | ||
removed_users = changes[0]["removed"] |
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.
If we keep it, we should indeed handle the "updated" users (removing the former name and add the new one).
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 logic to filter out messages is here already, but how can we connect it to the changes here?
I saw that part, but on my side it is never reached, self.on_message
is never set. I don't understand what was the expected logic here and what should be in the self.on_message
function.
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 think I never finished the work, but self.on_message
should be a callback that would basically be this.
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.
We should not apply the awareness in the first place if it's not correct.
This is what I meant with "the simplest is probably to do it in the YRoom directly, before updating the awareness"
I think that what's wrong is that with this PR, we apply the awareness updates coming from clients to the awareness in the backend, and then we observe the changes and decide if the updates should be forwarded to other clients, but it's already too late, right?
For what I understand the message is forwarded to the clients at the same time the awareness in the backend is updated, here, and then the user list is updated in the _websocket_server
.
But probably I miss something...
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.
It's actually used in jupyverse.
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.
For what I understand the message is forwarded to the clients at the same time the awareness in the backend is updated, here, and then the user list is updated in the
_websocket_server
.
Yes but we should not execute this code if the awareness update is "invalid".
Co-authored-by: David Brochart <[email protected]>
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.
Thanks @brichet, let's merge for now?
…#376) * Backport 'Update jupyter_ydoc and pycrdt_websocket dependencies' #367 * Update pyproject.toml Co-authored-by: David Brochart <[email protected]> --------- Co-authored-by: David Brochart <[email protected]>
This PR updates dependencies:
jupyter_ydoc >=2.1.2
pycrdt_websocket >=0.15.0
pycrdt >=0.10.1,<0.11.0
These changes allow the use of document awareness on the server side, to handle states on server.
YDocument
(after Doc awareness jupyter-server/jupyter_ydoc#277)