Fix save as in collaborative mode

binder/environment.yml

@@ -56,4 +56,4 @@ dependencies:
   # extra conda stuff
   - jsonschema-with-format-nongpl
   - pip:
-    - jupyter-collaboration >=1.0.0a3
+    - jupyter-collaboration >=1.0.0a4

packages/docmanager-extension/src/index.tsx

@@ -955,7 +955,28 @@ function addCommands(
             buttons: [Dialog.okButton()]
           });
         }
-        return context.saveAs();
+
+        const onChange = (
+          sender: Contents.IManager,
+          args: Contents.IChangedArgs
+        ) => {
+          if (
+            args.type === 'save' &&
+            args.newValue &&
+            args.newValue.path !== context.path
+          ) {
+            void commands.execute(CommandIDs.open, {
+              path: args.newValue.path
+            });
+          }
+        };
+        docManager.services.contents.fileChanged.connect(onChange);
+        context
+          .saveAs()
+          .then(() => void docManager.closeFile(context.path))
+          .finally(() =>
+            docManager.services.contents.fileChanged.disconnect(onChange)
+          );
       }
     }
   });

packages/docregistry/src/context.ts

@@ -1,6 +1,7 @@
 // Copyright (c) Jupyter Development Team.
 // Distributed under the terms of the Modified BSD License.
 
+import { DocumentChange, ISharedDocument } from '@jupyter/ydoc';
 import {
   Dialog,
   ISessionContext,
@@ -17,7 +18,6 @@ import {
   ServerConnection,
   ServiceManager
 } from '@jupyterlab/services';
-import { DocumentChange, ISharedDocument } from '@jupyter/ydoc';
 import {
   ITranslator,
   nullTranslator,
@@ -270,34 +270,39 @@ export class Context<
 
   /**
    * Save the document to a different path chosen by the user.
+   *
+   * It will be rejected if the user abort providing a new path.
    */
-  saveAs(): Promise<void> {
-    return this.ready
-      .then(() => {
-        return Private.getSavePath(this._path);
-      })
-      .then(newPath => {
-        if (this.isDisposed || !newPath) {
-          return;
-        }
-        if (newPath === this._path) {
-          return this.save();
-        }
-        // Make sure the path does not exist.
-        return this._manager.ready
-          .then(() => {
-            return this._manager.contents.get(newPath);
-          })
-          .then(() => {
-            return this._maybeOverWrite(newPath);
-          })
-          .catch(err => {
-            if (!err.response || err.response.status !== 404) {
-              throw err;
-            }
-            return this._finishSaveAs(newPath);
-          });
-      });
+  async saveAs(): Promise<void> {
+    await this.ready;
+    const localPath = this._manager.contents.localPath(this.path);
+    const newLocalPath = await Private.getSavePath(localPath);
+
+    if (!newLocalPath) {
+      return Promise.reject('Save as cancelled by user.');
+    }
+    if (this.isDisposed) {
+      return;
+    }
+
+    const drive = this._manager.contents.driveName(this.path);
+    const newPath = drive == '' ? newLocalPath : `${drive}:${newLocalPath}`;
+
+    if (newPath === this._path) {
+      return this.save();
+    }
+
+    // Make sure the path does not exist.
+    try {
+      await this._manager.ready;
+      await this._manager.contents.get(newPath);
+      await this._maybeOverWrite(newPath);
+    } catch (err) {
+      if (!err.response || err.response.status !== 404) {
+        throw err;
+      }
+      await this._finishSaveAs(newPath);
+    }
   }
 
   /**
@@ -592,22 +597,8 @@ export class Context<
    */
   private async _save(): Promise<void> {
     this._saveState.emit('started');
-    const model = this._model;
-    let content: PartialJSONValue = null;
-    if (this._factory.fileFormat === 'json') {
-      content = model.toJSON();
-    } else {
-      content = model.toString();
-      if (this._lineEnding) {
-        content = content.replace(/\n/g, this._lineEnding);
-      }
-    }
+    const options = this._createSaveOptions();
 
-    const options = {
-      type: this._factory.contentType,
-      format: this._factory.fileFormat,
-      content
-    };
     try {
       let value: Contents.IModel;
       await this._manager.ready;
@@ -616,7 +607,7 @@ export class Context<
         return;
       }
 
-      model.dirty = false;
+      this._model.dirty = false;
       this._updateContentsModel(value);
 
       if (!this._isPopulated) {
@@ -876,18 +867,56 @@ or load the version on disk (revert)?`,
    * Finish a saveAs operation given a new path.
    */
   private async _finishSaveAs(newPath: string): Promise<void> {
-    this._path = newPath;
-    await this.sessionContext.session?.setPath(newPath);
-    await this.sessionContext.session?.setName(newPath.split('/').pop()!);
-    // we must rename the document before saving with the new path
-    const localPath = this._manager.contents.localPath(this._path);
-    (this.urlResolver as RenderMimeRegistry.UrlResolver).path = newPath;
-    this._model.sharedModel.setState('path', localPath);
-    this._pathChanged.emit(newPath);
+    this._saveState.emit('started');
+    try {
+      await this._manager.ready;
+      const options = this._createSaveOptions();
+      await this._manager.contents.save(newPath, options);
+      await this._maybeCheckpoint(true);
+
+      // Emit completion.
+      this._saveState.emit('completed');
+    } catch (err) {
+      // If the save has been canceled by the user,
+      // throw the error so that whoever called save()
+      // can decide what to do.
+      if (
+        err.message === 'Cancel' ||
+        err.message === 'Modal is already displayed'
+      ) {
+        throw err;
+      }
 
-    // save triggers a fileChanged which updates the contents model
-    await this.save();
-    await this._maybeCheckpoint(true);
+      // Otherwise show an error message and throw the error.
+      const localPath = this._manager.contents.localPath(this._path);
+      const name = PathExt.basename(localPath);
+      void this._handleError(
+        err,
+        this._trans.__('File Save Error for %1', name)
+      );
+
+      // Emit failure.
+      this._saveState.emit('failed');
+      return;
+    }
+  }
+
+  private _createSaveOptions(): Partial<Contents.IModel> {
+    let content: PartialJSONValue = null;
+    if (this._factory.fileFormat === 'json') {
+      content = this._model.toJSON();
+    } else {
+      content = this._model.toString();
+      if (this._lineEnding) {
+        content = content.replace(/\n/g, this._lineEnding);
+      }
+    }
+
+    return {
+      type: this._factory.contentType,
+      format: this._factory.fileFormat,
+      content
+    };
   }
 
   protected translator: ITranslator;

packages/docregistry/test/context.spec.ts

@@ -376,11 +376,17 @@ describe('docregistry/context', () => {
         const oldPath = context.path;
         await context.saveAs();
         await promise;
-        expect(context.path).toBe(newPath);
+        // We no longer rename the current document
+        //expect(context.path).toBe(newPath);
         // Make sure the both files are there now.
         const model = await manager.contents.get('', { content: true });
         expect(model.content.find((x: any) => x.name === oldPath)).toBeTruthy();
         expect(model.content.find((x: any) => x.name === newPath)).toBeTruthy();
+
+        // Make sure both files are equal
+        const model1 = await manager.contents.get(oldPath, { content: true });
+        const model2 = await manager.contents.get(newPath, { content: true });
+        expect(model1.content).toEqual(model2.content);
       });
 
       it('should bring up a conflict dialog', async () => {
@@ -401,9 +407,22 @@ describe('docregistry/context', () => {
         });
         await context.initialize(true);
         const promise = func();
+
+        const oldPath = context.path;
         await context.saveAs();
         await promise;
-        expect(context.path).toBe(newPath);
+
+        // We no longer rename the current document
+        //expect(context.path).toBe(newPath);
+        // Make sure the both files are there now.
+        const model = await manager.contents.get('', { content: true });
+        expect(model.content.find((x: any) => x.name === oldPath)).toBeTruthy();
+        expect(model.content.find((x: any) => x.name === newPath)).toBeTruthy();
+
+        // Make sure both files are equal
+        const model1 = await manager.contents.get(oldPath, { content: true });
+        const model2 = await manager.contents.get(newPath, { content: true });
+        expect(model1.content).toEqual(model2.content);
       });
 
       it('should keep the file if overwrite is aborted', async () => {
@@ -437,6 +456,15 @@ describe('docregistry/context', () => {
         await promise;
         expect(context.path).toBe(path);
       });
+
+      it('should be rejected if the user cancel the dialog', async () => {
+        await context.initialize(true);
+
+        const promise = context.saveAs();
+        await dismissDialog();
+
+        await expect(promise).rejects.toEqual('Save as cancelled by user.');
+      });
     });
 
     describe('#revert()', () => {