Broadcast server awareness to all clients #73
Conversation
|
Thanks @davidbrochart for the review and live debugging. As suggested in jupyter-server/jupyter_ydoc#277 (comment) I moved the |
brichet
changed the title
Co-authored-by: David Brochart <[email protected]>
Co-authored-by: David Brochart <[email protected]>
for more information, see https://pre-commit.ci
brichet
force-pushed
the
server_awareness
branch
from
5411d4d
to
52c80ed
Compare
brichet
changed the title
|
Finally that's a lot of commits and comments for minimal changes 😄 |
for more information, see https://pre-commit.ci
pycrdt_websocket/yroom.py
Outdated
| """ | ||
| Callback to broadcast the server awareness to clients. | ||
| """ | ||
| if not changes[1] == "local": |
There was a problem hiding this comment.
| if not changes[1] == "local": | |
| if changes[1] != "local": |
pycrdt_websocket/yroom.py
Outdated
| self.ready_event = Event() | ||
| self.ready = ready | ||
| self.ystore = ystore | ||
| self.log = log or getLogger(__name__) | ||
| self.awareness = Awareness(self.ydoc) | ||
| self.awareness.observe(self.local_update_awareness) |
There was a problem hiding this comment.
I'm not sure about the callback name local_update_awareness, maybe send_server_awareness?
pycrdt_websocket/yroom.py
Outdated
| updated_clients = [ | ||
| *changes[0].get("added", []), | ||
| *changes[0].get("filtered_updated", []), | ||
| ] |
There was a problem hiding this comment.
The changes consist of "added", "updated" and "removed" entries only, not "filtered_updates". Should we include "removed"?
| updated_clients = [ | |
| *changes[0].get("added", []), | |
| *changes[0].get("filtered_updated", []), | |
| ] | |
| updated_clients = [v for value in changes[0].values() for v in value] |
There was a problem hiding this comment.
Also I added a test to broadcast only if there is an effective change (not 'update').
Co-authored-by: David Brochart <[email protected]>
pycrdt_websocket/yroom.py
Outdated
| """ | ||
| Callback to broadcast the server awareness to clients. | ||
| """ | ||
| if type != "change" or changes[1] != "local": |
There was a problem hiding this comment.
Should we actually broadcast every update?
| if type != "change" or changes[1] != "local": | |
| if type != "update" or changes[1] != "local": |
There was a problem hiding this comment.
Your right, we should broadcast all events.
Currently the update event is triggered only when the change event is triggered.
Should we trigger an update every X sec ?
There was a problem hiding this comment.
I think we need this logic in pycrdt. I'll work on it.
There was a problem hiding this comment.
Yes, that's what I meant.
Co-authored-by: David Brochart <[email protected]>
Co-authored-by: David Brochart <[email protected]>
pyproject.toml
Outdated
| @@ -30,7 +30,7 @@ classifiers = [ | |||
| dependencies = [ | |||
| "anyio >=3.6.2,<5", | |||
| "sqlite-anyio >=0.2.3,<0.3.0", | |||
| "pycrdt >=0.9.16,<0.10.0", | |||
| "pycrdt >=0.10.0,<0.11.0", | |||
There was a problem hiding this comment.
Let's take the latest bugfix.
I'm also wondering if we should wait for the implementation of periodic awareness updates in pycrdt?
| "pycrdt >=0.10.0,<0.11.0", | |
| "pycrdt >=0.10.1,<0.11.0", |
There was a problem hiding this comment.
I don't think it is required for this change.
But we should probably remove the type != "update" condition to broadcast all the event.
There was a problem hiding this comment.
But an "update" change is sent for all events, right? So we should keep it, to deduplicate "change" events. Or am I missing something?
There was a problem hiding this comment.
Right, sorry again for the confusion. The change event is only for observers, but not for clients...
There was a problem hiding this comment.
OK, I'll merge then.
Co-authored-by: David Brochart <[email protected]>
This PR remove the Awareness to use the one from pycrdt (jupyter-server/pycrdt#170)This PR add broadcasting changes of state in the local awareness.
Draft until jupyter-server/pycrdt#170 is merged and released.