Skip to content
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

Fix spurious "File Changed" dialog in JupyterLab 4.1+/Notebook 7.1+ #337

Merged
merged 4 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion packages/docprovider/src/ydrive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -45,6 +46,10 @@ export class YDrive extends Drive implements ICollaborativeDrive {
this._providers = new Map<string, WebSocketProvider>();

this.sharedModelFactory = new SharedModelFactory(this._onCreate);
super.fileChanged.connect((_, change) => {
// pass through any events from the Drive superclass
this._ydriveFileChanged.emit(change);
});
}

/**
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<this, Contents.IChangedArgs> {
return this._ydriveFileChanged;
}

private _onCreate = (
options: Contents.ISharedFactoryOptions,
sharedModel: YDocument<DocumentChange>
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -190,6 +233,7 @@ export class YDrive extends Drive implements ICollaborativeDrive {
private _trans: TranslationBundle;
private _providers: Map<string, WebSocketProvider>;
private _globalAwareness: Awareness | null;
private _ydriveFileChanged = new Signal<this, Contents.IChangedArgs>(this);
}

/**
Expand Down
20 changes: 17 additions & 3 deletions projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
davidbrochart marked this conversation as resolved.
Show resolved Hide resolved
"""
Save the content of the file.

Expand Down Expand Up @@ -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, # TODO require version supporting hash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this one before merging.

)
)
return {**m, "hash": model_with_hash["hash"]}
finally:
done_saving.set()

Expand Down
4 changes: 3 additions & 1 deletion projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"]
davidbrochart marked this conversation as resolved.
Show resolved Hide resolved

self._emit(LogLevel.INFO, "save", "Content saved.")

Expand Down
Loading