Creates a new FileLoader class to separate the logic of watching files #121
Conversation
|
|
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 3 7 +4
Lines 297 418 +121
======================================
- Misses 297 418 +121
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
It would be great to have a way to swap loaders so extensions can register new document types and the logic to load those documents. For example, JupyterCAD registers a new document type that uses the FreeCAD format. This format is a compressed folder with multiple XML files. Jupyter Server needs to learn how to open those documents, and the contents service needs a way to plug new loaders. I have looked into the contents service of Jupyter Server, trying to find the best solution for this problem. Currently, the ContentsManager only allows registering pre/post save hooks. We could use these hooks to load the content of a specific file type, but we can not ensure that the hook we register will be executed the last one. In addition, there are only hooks for saving the file. There are no hooks for loading the file. I'm not trying to solve the problem in this PR, but I want to redesign the architecture of jupyter_collaboration mindful of this problem to solve it in a follow-up PR. At the moment, I see two options: There is a caveat with the second option. If jupyter_collaboration is installed with another extension that also swaps the default content manager, it may not work. |
|
Hi @Zsailer. I'm pinging you since your thoughts here would be appreciated, and you might be interested in the comment I posted above. I will try to join the jupyter server team meeting this week to discuss this topic. Jupyter Server's collaborators should be aware of and participate in the decisions taken here in the jupyter_collaboration extension since this involves some parts of the jupyter server. Thanks in advance for your comments and your time! |
|
Thanks for pinging me here, @hbcarlos. I'll give this a more thorough review over the next couple of days. I don't know if I'll make it to tomorrow's Jupyter Server meeting—I have a family commitment that conflicts with part of the meeting. If I don't see you then, let's connect over gitter and maybe set up a time to meet separately? |
|
Thanks, @Zsailer!
It's fine, we can continue the discussion here and discuss also it at next week's meeting. |
|
With the current implementation, the content is not synced between different rooms. It doesn't sync because there is only one property, We could change this logic and keep a timestamp by room, allowing to sync between rooms. However, I prefer not to sync between rooms for now. If we sync the content between rooms, each time the content changes in one room, on every other room, we will create a Y update replacing the entire content. This could increase memory usage, so I would like to experiment before allowing rooms to sync. |
You mean with this PR? Before this PR, the content is synced between rooms. |
|
Yes, I meant in this PR. |
|
So you mean that this PR breaks document synchronization between rooms? |
There was a problem hiding this comment.
Thanks @hbcarlos
Here are some suggestions. It will be good to add doc string to document the code and unit tests to at least test for the correctness of the instantiation and clean actions.
|
|
||
| def check_origin(self, origin): | ||
| return True | ||
|
|
||
| @classmethod | ||
| def clean_up(cls): |
There was a problem hiding this comment.
This is another proof that the architecture is not yet great - but let put it aside for this PR.
There was a problem hiding this comment.
I know I'm doing my best, but there are already a lot of changes in this PR.
There was a problem hiding this comment.
Feel free to open an issue, and I'll work on it in another PR
| prefix_dir = "jupyter_ystore_" | ||
|
|
||
|
|
||
| class SQLiteYStoreMetaclass(type(LoggingConfigurable), type(_SQLiteYStore)): # type: ignore |
There was a problem hiding this comment.
Could you add a comment why we need this complex code based on metaclass rather than the classical direct inheritance?
There was a problem hiding this comment.
I just moved it to a new file. I'm not sure why we need it. I have not looked into the stores yet.
@davidbrochart Could you open a PR adding documentation?
I've not been following all the commits, has this been addressed? |
I'm not restoring it. This PR removes the synchronization between rooms in favor of simplicity and stability. I'll add it back once the extension is stable and we have a pleasant RTC experience. |
|
I think it is a big regression. |
|
Users are notified that they might lose changes if they open the same document with multiple views. If you edit in one room, it won't sync with the other one. The last edited room is the one saved to disk. |
|
I think it needs to be discussed, as it is a significant regression and it could lead to a lot of user frustration. |
That is supported |
|
So there is something I don't understand. If the frontend reacts to file changes made in the backend, and you have one user with a notebook opened as a notebook document, and another user with the same notebook opened as a JSON document, what is the behavior? |
|
I just tried this PR and I'm seeing weird behaviors while synchronizing rooms, like a notebook being reverted to a previous state: room_sync.mp4 |
|
Yes, because I prioritize what is in the disk instead of automatically overwriting it whenever a change occurs. I'm not in favor of the synchronization between rooms or the auto-save. It is unpredictable, and I'm sure it is related to some of the issues we are having. |
|
It is unclear to me what you do in this PR regarding room synchronization. Commit 1ee3848 says "Fixes sync between rooms", but it doesn't seem to work. Can you clarify what is fixed? |
|
@fcollonval I was not done testing this PR, and there was still pending questions. |
|
The synchronization has been fixed. We can address following question in follow-up. But we should move forward to unlock the file loader. |
It's hard to tell, I think we should give more time for reviewers to get their questions answered before merging. |
Creates a new FileLoader class to separate the logic of watching files from the WebSocketHandler.
Code changes:
Multiple rooms can get an instance of this class and use it to access the content safely since we can create a single lock by file to ensure two rooms are not accessing the same file simultaneously.
TODO: