From 1b74c437fec5475548e8cf08233c0992dcfdd4f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krassowski?= <5832902+krassowski@users.noreply.github.com> Date: Thu, 22 Aug 2024 12:06:12 +0100 Subject: [PATCH] Fix spurious "File Changed" dialog in JupyterLab 4.1+/Notebook 7.1+ (#337) * Save current hash on the document * Use public API now that PR 262 in `jupyter_ydoc` is merged * Use a clean solution which requires a change in JupyterLab * `jupyter-server-ydoc`: require `jupyter-server` 2.11.1 or newer As only 2.11.1 added `require_hash` argument --- packages/docprovider/src/ydrive.ts | 46 ++++++++++++++++++- .../jupyter_server_ydoc/loaders.py | 20 ++++++-- .../jupyter_server_ydoc/rooms.py | 4 +- projects/jupyter-server-ydoc/pyproject.toml | 2 +- 4 files changed, 66 insertions(+), 6 deletions(-) diff --git a/packages/docprovider/src/ydrive.ts b/packages/docprovider/src/ydrive.ts index 0f4ce8bf..08d137dd 100644 --- a/packages/docprovider/src/ydrive.ts +++ b/packages/docprovider/src/ydrive.ts @@ -4,6 +4,7 @@ import { PageConfig, URLExt } from '@jupyterlab/coreutils'; import { TranslationBundle } from '@jupyterlab/translation'; import { Contents, Drive, User } from '@jupyterlab/services'; +import { ISignal, Signal } from '@lumino/signaling'; import { DocumentChange, ISharedDocument, YDocument } from '@jupyter/ydoc'; @@ -45,6 +46,10 @@ export class YDrive extends Drive implements ICollaborativeDrive { this._providers = new Map(); this.sharedModelFactory = new SharedModelFactory(this._onCreate); + super.fileChanged.connect((_, change) => { + // pass through any events from the Drive superclass + this._ydriveFileChanged.emit(change); + }); } /** @@ -84,7 +89,7 @@ export class YDrive extends Drive implements ICollaborativeDrive { const provider = this._providers.get(key); if (provider) { - // If the document does't exist, `super.get` will reject with an + // If the document doesn't exist, `super.get` will reject with an // error and the provider will never be resolved. // Use `Promise.all` to reject as soon as possible. The Context will // show a dialog to the user. @@ -132,6 +137,13 @@ export class YDrive extends Drive implements ICollaborativeDrive { return super.save(localPath, options); } + /** + * A signal emitted when a file operation takes place. + */ + get fileChanged(): ISignal { + return this._ydriveFileChanged; + } + private _onCreate = ( options: Contents.ISharedFactoryOptions, sharedModel: YDocument @@ -161,6 +173,37 @@ export class YDrive extends Drive implements ICollaborativeDrive { const key = `${options.format}:${options.contentType}:${options.path}`; this._providers.set(key, provider); + sharedModel.changed.connect(async (_, change) => { + if (!change.stateChange) { + return; + } + const hashChanges = change.stateChange.filter( + change => change.name === 'hash' + ); + if (hashChanges.length === 0) { + return; + } + if (hashChanges.length > 1) { + console.error( + 'Unexpected multiple changes to hash value in a single transaction' + ); + } + const hashChange = hashChanges[0]; + + // A change in hash signifies that a save occurred on the server-side + // (e.g. a collaborator performed the save) - we want to notify the + // observers about this change so that they can store the new hash value. + const model = await this.get(options.path, { content: false }); + + this._ydriveFileChanged.emit({ + type: 'save', + newValue: { ...model, hash: hashChange.newValue }, + // we do not have the old model because it was discarded when server made the change, + // we only have the old hash here (which may be empty if the file was newly created!) + oldValue: { hash: hashChange.oldValue } + }); + }); + sharedModel.disposed.connect(() => { const provider = this._providers.get(key); if (provider) { @@ -190,6 +233,7 @@ export class YDrive extends Drive implements ICollaborativeDrive { private _trans: TranslationBundle; private _providers: Map; private _globalAwareness: Awareness | null; + private _ydriveFileChanged = new Signal(this); } /** diff --git a/projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py b/projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py index a2622cca..745baed7 100644 --- a/projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py +++ b/projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py @@ -117,7 +117,7 @@ async def load_content(self, format: str, file_type: str) -> dict[str, Any]: self.last_modified = model["last_modified"] return model - async def maybe_save_content(self, model: dict[str, Any]) -> None: + async def maybe_save_content(self, model: dict[str, Any]) -> dict[str, Any] | None: """ Save the content of the file. @@ -149,20 +149,34 @@ async def maybe_save_content(self, model: dict[str, Any]) -> None: # otherwise it could corrupt the file done_saving = asyncio.Event() task = asyncio.create_task(self._save_content(model, done_saving)) + saved_model = None try: - await asyncio.shield(task) + saved_model = await asyncio.shield(task) except asyncio.CancelledError: pass await done_saving.wait() + return saved_model else: # file changed on disk, raise an error self.last_modified = m["last_modified"] raise OutOfBandChanges - async def _save_content(self, model: dict[str, Any], done_saving: asyncio.Event) -> None: + async def _save_content( + self, model: dict[str, Any], done_saving: asyncio.Event + ) -> dict[str, Any]: try: m = await ensure_async(self._contents_manager.save(model, self.path)) self.last_modified = m["last_modified"] + # TODO, get rid of the extra `get` here once upstream issue: + # https://github.com/jupyter-server/jupyter_server/issues/1453 is resolved + model_with_hash = await ensure_async( + self._contents_manager.get( + self.path, + content=False, + require_hash=True, + ) + ) + return {**m, "hash": model_with_hash["hash"]} finally: done_saving.set() diff --git a/projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py b/projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py index 873fe113..888f2de4 100644 --- a/projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py +++ b/projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py @@ -271,7 +271,7 @@ async def _maybe_save_document(self, saving_document: asyncio.Task | None) -> No await asyncio.sleep(self._save_delay) self.log.info("Saving the content from room %s", self._room_id) - await self._file.maybe_save_content( + saved_model = await self._file.maybe_save_content( { "format": self._file_format, "type": self._file_type, @@ -280,6 +280,8 @@ async def _maybe_save_document(self, saving_document: asyncio.Task | None) -> No ) async with self._update_lock: self._document.dirty = False + if saved_model: + self._document.hash = saved_model["hash"] self._emit(LogLevel.INFO, "save", "Content saved.") diff --git a/projects/jupyter-server-ydoc/pyproject.toml b/projects/jupyter-server-ydoc/pyproject.toml index 6cd6538a..c88891ef 100644 --- a/projects/jupyter-server-ydoc/pyproject.toml +++ b/projects/jupyter-server-ydoc/pyproject.toml @@ -28,7 +28,7 @@ authors = [ { name = "Jupyter Development Team", email = "jupyter@googlegroups.com" }, ] dependencies = [ - "jupyter_server>=2.4.0,<3.0.0", + "jupyter_server>=2.11.1,<3.0.0", "jupyter_ydoc>=2.0.0,<4.0.0", "pycrdt", "pycrdt-websocket>=0.14.0,<0.15.0",