Backport PR jupyter-server#148: Fix notebook undo scope

fcollonval · fcollonval · commit 924ce7b93563 · 2023-02-22T10:07:19.000+01:00
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -30,6 +30,7 @@ jobs:
   test:
     name: Run tests on ${{ matrix.os }}
     runs-on: ${{ matrix.os }}
+    timeout-minutes: 10
 
     strategy:
       matrix:
diff --git a/javascript/src/api.ts b/javascript/src/api.ts
@@ -62,8 +62,11 @@ export interface ISharedBase extends IDisposable {
   /**
    * Perform a transaction. While the function f is called, all changes to the shared
    * document are bundled into a single event.
+   *
+   * @param f Transaction to execute
+   * @param undoable Whether to track the change in the action history or not (default `true`)
    */
-  transact(f: () => void): void;
+  transact(f: () => void, undoable?: boolean): void;
 }
 
 /**
diff --git a/javascript/src/ymodels.ts b/javascript/src/ymodels.ts
@@ -777,7 +777,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
       }
       this.transact(() => {
         this.ymodel.set('metadata', clone);
-      });
+      }, false);
     } else {
       const clone = JSONExt.deepCopy(metadata) as any;
       if (clone.collapsed != null) {
@@ -789,7 +789,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
       if (!JSONExt.deepEqual(clone, this.getMetadata())) {
         this.transact(() => {
           this.ymodel.set('metadata', clone);
-        });
+        }, false);
       }
     }
   }
@@ -811,11 +811,11 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
    * document are bundled into a single event.
    */
   transact(f: () => void, undoable = true): void {
-    this.notebook && undoable
-      ? this.notebook.transact(f)
-      : this.ymodel.doc == null
-      ? f()
-      : this.ymodel.doc.transact(f, this);
+    !this.notebook || this.notebook.disableDocumentWideUndoRedo
+      ? this.ymodel.doc == null
+        ? f()
+        : this.ymodel.doc.transact(f, undoable ? this : null)
+      : this.notebook.transact(f, undoable);
   }
 
   /**
@@ -971,7 +971,7 @@ export class YCodeCell
     if (this.ymodel.get('execution_count') !== count) {
       this.transact(() => {
         this.ymodel.set('execution_count', count);
-      });
+      }, false);
     }
   }
 
@@ -1106,7 +1106,7 @@ class YAttachmentCell
       } else {
         this.ymodel.set('attachments', attachments);
       }
-    });
+    }, false);
   }
 
   /**
diff --git a/javascript/test/ycell.spec.ts b/javascript/test/ycell.spec.ts
@@ -0,0 +1,316 @@
+/* eslint-disable camelcase */
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import { IMapChange, YCodeCell, YNotebook } from '../src';
+
+describe('@jupyter/ydoc', () => {
+  describe('YCell standalone', () => {
+    test('should set source', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      expect(codeCell.getSource()).toBe('test');
+      codeCell.dispose();
+    });
+
+    test('should update source', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      codeCell.updateSource(0, 0, 'hello');
+      expect(codeCell.getSource()).toBe('hellotest');
+      codeCell.dispose();
+    });
+
+    test('should get metadata', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+
+      cell.setMetadata(metadata);
+
+      expect(cell.metadata).toEqual({
+        ...metadata,
+        jupyter: { outputs_hidden: true }
+      });
+      cell.dispose();
+    });
+
+    test('should get all metadata', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        jupyter: { outputs_hidden: true },
+        editable: false,
+        name: 'cell-name'
+      };
+
+      cell.setMetadata(metadata);
+
+      expect(cell.getMetadata()).toEqual({ ...metadata, collapsed: true });
+      cell.dispose();
+    });
+
+    test('should get one metadata', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+
+      cell.setMetadata(metadata);
+
+      expect(cell.getMetadata('editable')).toEqual(metadata.editable);
+      cell.dispose();
+    });
+
+    it.each([null, undefined, 1, true, 'string', { a: 1 }, [1, 2]])(
+      'should get single metadata %s',
+      value => {
+        const cell = YCodeCell.create();
+        const metadata = {
+          collapsed: true,
+          editable: false,
+          name: 'cell-name',
+          test: value
+        };
+
+        cell.setMetadata(metadata);
+
+        expect(cell.getMetadata('test')).toEqual(value);
+        cell.dispose();
+      }
+    );
+
+    test('should set one metadata', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+
+      cell.setMetadata(metadata);
+      cell.setMetadata('test', 'banana');
+
+      expect(cell.getMetadata('test')).toEqual('banana');
+      cell.dispose();
+    });
+
+    test('should emit all metadata changes', () => {
+      const notebook = YNotebook.create();
+
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+
+      const changes: IMapChange[] = [];
+      notebook.metadataChanged.connect((_, c) => {
+        changes.push(c);
+      });
+      notebook.metadata = metadata;
+
+      expect(changes).toHaveLength(3);
+      expect(changes).toEqual([
+        {
+          type: 'add',
+          key: 'collapsed',
+          newValue: metadata.collapsed,
+          oldValue: undefined
+        },
+        {
+          type: 'add',
+          key: 'editable',
+          newValue: metadata.editable,
+          oldValue: undefined
+        },
+        {
+          type: 'add',
+          key: 'name',
+          newValue: metadata.name,
+          oldValue: undefined
+        }
+      ]);
+
+      notebook.dispose();
+    });
+
+    test('should emit a add metadata change', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+      cell.metadata = metadata;
+
+      const changes: IMapChange[] = [];
+      cell.metadataChanged.connect((_, c) => {
+        changes.push(c);
+      });
+      cell.setMetadata('test', 'banana');
+
+      try {
+        expect(changes).toHaveLength(1);
+        expect(changes).toEqual([
+          { type: 'add', key: 'test', newValue: 'banana', oldValue: undefined }
+        ]);
+      } finally {
+        cell.dispose();
+      }
+    });
+
+    test('should emit a delete metadata change', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+      cell.metadata = metadata;
+
+      const changes: IMapChange[] = [];
+      cell.setMetadata('test', 'banana');
+
+      cell.metadataChanged.connect((_, c) => {
+        changes.push(c);
+      });
+      cell.deleteMetadata('test');
+
+      try {
+        expect(changes).toHaveLength(1);
+        expect(changes).toEqual([
+          {
+            type: 'remove',
+            key: 'test',
+            newValue: undefined,
+            oldValue: 'banana'
+          }
+        ]);
+      } finally {
+        cell.dispose();
+      }
+    });
+
+    test('should emit an update metadata change', () => {
+      const cell = YCodeCell.create();
+      const metadata = {
+        collapsed: true,
+        editable: false,
+        name: 'cell-name'
+      };
+      cell.metadata = metadata;
+
+      const changes: IMapChange[] = [];
+      cell.setMetadata('test', 'banana');
+
+      cell.metadataChanged.connect((_, c) => {
+        changes.push(c);
+      });
+      cell.setMetadata('test', 'orange');
+
+      try {
+        expect(changes).toHaveLength(1);
+        expect(changes).toEqual([
+          {
+            type: 'change',
+            key: 'test',
+            newValue: 'orange',
+            oldValue: 'banana'
+          }
+        ]);
+      } finally {
+        cell.dispose();
+      }
+    });
+  });
+
+  describe('#undo', () => {
+    test('should undo source change', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      codeCell.undoManager?.stopCapturing();
+      codeCell.updateSource(0, 0, 'hello');
+
+      codeCell.undo();
+
+      expect(codeCell.getSource()).toEqual('test');
+    });
+
+    test('should not undo execution count change', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      codeCell.undoManager?.stopCapturing();
+      codeCell.execution_count = 22;
+      codeCell.undoManager?.stopCapturing();
+      codeCell.execution_count = 42;
+
+      codeCell.undo();
+
+      expect(codeCell.execution_count).toEqual(42);
+    });
+
+    test('should not undo output change', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      codeCell.undoManager?.stopCapturing();
+      const outputs = [
+        {
+          data: {
+            'application/geo+json': {
+              geometry: {
+                coordinates: [-118.4563712, 34.0163116],
+                type: 'Point'
+              },
+              type: 'Feature'
+            },
+            'text/plain': ['<IPython.display.GeoJSON object>']
+          },
+          metadata: {
+            'application/geo+json': {
+              expanded: false
+            }
+          },
+          output_type: 'display_data'
+        },
+        {
+          data: {
+            'application/vnd.jupyter.widget-view+json': {
+              model_id: '4619c172d65e496baa5d1230894b535a',
+              version_major: 2,
+              version_minor: 0
+            },
+            'text/plain': [
+              "HBox(children=(Text(value='text input', layout=Layout(border='1px dashed red', width='80px')), Button(descript…"
+            ]
+          },
+          metadata: {},
+          output_type: 'display_data'
+        }
+      ];
+      codeCell.setOutputs(outputs);
+      codeCell.undoManager?.stopCapturing();
+
+      codeCell.undo();
+
+      expect(codeCell.getOutputs()).toEqual(outputs);
+    });
+
+    test('should not undo metadata change', () => {
+      const codeCell = YCodeCell.create();
+      codeCell.setSource('test');
+      codeCell.undoManager?.stopCapturing();
+      codeCell.setMetadata({ collapsed: false });
+      codeCell.undoManager?.stopCapturing();
+      codeCell.setMetadata({ collapsed: true });
+
+      codeCell.undo();
+
+      expect(codeCell.getMetadata('collapsed')).toEqual(true);
+    });
+  });
+});
diff --git a/javascript/test/ymodels.spec.ts b/javascript/test/ymodels.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -777,7 +777,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>`
`777`	`777`	`}`
`778`	`778`	`this.transact(() => {`
`779`	`779`	`this.ymodel.set('metadata', clone);`
`780`		`- });`
	`780`	`+ }, false);`
`781`	`781`	`} else {`
`782`	`782`	`const clone = JSONExt.deepCopy(metadata) as any;`
`783`	`783`	`if (clone.collapsed != null) {`
`@@ -789,7 +789,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>`
`789`	`789`	`if (!JSONExt.deepEqual(clone, this.getMetadata())) {`
`790`	`790`	`this.transact(() => {`
`791`	`791`	`this.ymodel.set('metadata', clone);`
`792`		`- });`
	`792`	`+ }, false);`
`793`	`793`	`}`
`794`	`794`	`}`
`795`	`795`	`}`
`@@ -811,11 +811,11 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>`
`811`	`811`	`* document are bundled into a single event.`
`812`	`812`	`*/`
`813`	`813`	`transact(f: () => void, undoable = true): void {`
`814`		`- this.notebook && undoable`
`815`		`- ? this.notebook.transact(f)`
`816`		`- : this.ymodel.doc == null`
`817`		`- ? f()`
`818`		`- : this.ymodel.doc.transact(f, this);`
	`814`	`+ !this.notebook \|\| this.notebook.disableDocumentWideUndoRedo`
	`815`	`+ ? this.ymodel.doc == null`
	`816`	`+ ? f()`
	`817`	`+ : this.ymodel.doc.transact(f, undoable ? this : null)`
	`818`	`+ : this.notebook.transact(f, undoable);`
`819`	`819`	`}`
`820`	`820`
`821`	`821`	`/**`
`@@ -971,7 +971,7 @@ export class YCodeCell`
`971`	`971`	`if (this.ymodel.get('execution_count') !== count) {`
`972`	`972`	`this.transact(() => {`
`973`	`973`	`this.ymodel.set('execution_count', count);`
`974`		`- });`
	`974`	`+ }, false);`
`975`	`975`	`}`
`976`	`976`	`}`
`977`	`977`
`@@ -1106,7 +1106,7 @@ class YAttachmentCell`
`1106`	`1106`	`} else {`
`1107`	`1107`	`this.ymodel.set('attachments', attachments);`
`1108`	`1108`	`}`
`1109`		`- });`
	`1109`	`+ }, false);`
`1110`	`1110`	`}`
`1111`	`1111`
`1112`	`1112`	`/**`