Skip to content

Commit bfc54ef

Browse files
fcollonvalpre-commit-ci[bot]
fcollonval
and
pre-commit-ci[bot]
authored
Fix notebook undo scope (#148)
* Track only source change in undo history * Add tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Enforce job timeout --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 parent 292781f commit bfc54ef

File tree

5 files changed

+233
-38
lines changed

5 files changed

+233
-38
lines changed

‎.github/workflows/test.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ jobs:
3030
test:
3131
name: Run tests on ${{ matrix.os }}
3232
runs-on: ${{ matrix.os }}
33+
timeout-minutes: 10
3334

3435
strategy:
3536
matrix:

‎javascript/src/api.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,11 @@ export interface ISharedBase extends IObservableDisposable {
7373
/**
7474
* Perform a transaction. While the function f is called, all changes to the shared
7575
* document are bundled into a single event.
76+
*
77+
* @param f Transaction to execute
78+
* @param undoable Whether to track the change in the action history or not (default `true`)
7679
*/
77-
transact(f: () => void): void;
80+
transact(f: () => void, undoable?: boolean): void;
7881
}
7982

8083
/**

‎javascript/src/ycell.ts‎

Lines changed: 44 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -431,21 +431,23 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
431431
return;
432432
}
433433

434-
this._ymetadata.delete(key);
434+
this.transact(() => {
435+
this._ymetadata.delete(key);
435436

436-
const jupyter = this.getMetadata('jupyter') as any;
437-
if (key === 'collapsed' && jupyter) {
438-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
439-
const { outputs_hidden, ...others } = jupyter;
437+
const jupyter = this.getMetadata('jupyter') as any;
438+
if (key === 'collapsed' && jupyter) {
439+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
440+
const { outputs_hidden, ...others } = jupyter;
440441

441-
if (Object.keys(others).length === 0) {
442-
this._ymetadata.delete('jupyter');
443-
} else {
444-
this._ymetadata.set('jupyter', others);
442+
if (Object.keys(others).length === 0) {
443+
this._ymetadata.delete('jupyter');
444+
} else {
445+
this._ymetadata.set('jupyter', others);
446+
}
447+
} else if (key === 'jupyter') {
448+
this._ymetadata.delete('collapsed');
445449
}
446-
} else if (key === 'jupyter') {
447-
this._ymetadata.delete('collapsed');
448-
}
450+
}, false);
449451
}
450452

451453
/**
@@ -524,7 +526,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
524526
this.deleteMetadata('collapsed');
525527
}
526528
}
527-
});
529+
}, false);
528530
} else {
529531
const clone = JSONExt.deepCopy(metadata) as any;
530532
if (clone.collapsed != null) {
@@ -538,7 +540,7 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
538540
for (const [key, value] of Object.entries(clone)) {
539541
this._ymetadata.set(key, value);
540542
}
541-
});
543+
}, false);
542544
}
543545
}
544546
}
@@ -558,13 +560,16 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
558560
/**
559561
* Perform a transaction. While the function f is called, all changes to the shared
560562
* document are bundled into a single event.
563+
*
564+
* @param f Transaction to execute
565+
* @param undoable Whether to track the change in the action history or not (default `true`)
561566
*/
562567
transact(f: () => void, undoable = true): void {
563-
this.notebook && undoable
564-
? this.notebook.transact(f)
565-
: this.ymodel.doc == null
566-
? f()
567-
: this.ymodel.doc.transact(f, this);
568+
!this.notebook || this.notebook.disableDocumentWideUndoRedo
569+
? this.ymodel.doc == null
570+
? f()
571+
: this.ymodel.doc.transact(f, undoable ? this : null)
572+
: this.notebook.transact(f, undoable);
568573
}
569574

570575
/**
@@ -606,22 +611,24 @@ export class YBaseCell<Metadata extends nbformat.IBaseCellMetadata>
606611
});
607612
break;
608613
case 'update':
609-
const newValue = this._ymetadata.get(key);
610-
const oldValue = change.oldValue;
611-
let equal = true;
612-
if (typeof oldValue == 'object' && typeof newValue == 'object') {
613-
equal = JSONExt.deepEqual(oldValue, newValue);
614-
} else {
615-
equal = oldValue === newValue;
616-
}
617-
618-
if (!equal) {
619-
this._metadataChanged.emit({
620-
key,
621-
type: 'change',
622-
oldValue,
623-
newValue
624-
});
614+
{
615+
const newValue = this._ymetadata.get(key);
616+
const oldValue = change.oldValue;
617+
let equal = true;
618+
if (typeof oldValue == 'object' && typeof newValue == 'object') {
619+
equal = JSONExt.deepEqual(oldValue, newValue);
620+
} else {
621+
equal = oldValue === newValue;
622+
}
623+
624+
if (!equal) {
625+
this._metadataChanged.emit({
626+
key,
627+
type: 'change',
628+
oldValue,
629+
newValue
630+
});
631+
}
625632
}
626633
break;
627634
}
@@ -732,7 +739,7 @@ export class YCodeCell
732739
if (this.ymodel.get('execution_count') !== count) {
733740
this.transact(() => {
734741
this.ymodel.set('execution_count', count);
735-
});
742+
}, false);
736743
}
737744
}
738745

@@ -865,7 +872,7 @@ class YAttachmentCell
865872
} else {
866873
this.ymodel.set('attachments', attachments);
867874
}
868-
});
875+
}, false);
869876
}
870877

871878
/**

‎javascript/test/ycell.spec.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
/* eslint-disable camelcase */
12
// Copyright (c) Jupyter Development Team.
23
// Distributed under the terms of the Modified BSD License.
34

@@ -227,4 +228,89 @@ describe('@jupyter/ydoc', () => {
227228
}
228229
});
229230
});
231+
232+
describe('#undo', () => {
233+
test('should undo source change', () => {
234+
const codeCell = YCodeCell.create();
235+
codeCell.setSource('test');
236+
codeCell.undoManager?.stopCapturing();
237+
codeCell.updateSource(0, 0, 'hello');
238+
239+
codeCell.undo();
240+
241+
expect(codeCell.getSource()).toEqual('test');
242+
});
243+
244+
test('should not undo execution count change', () => {
245+
const codeCell = YCodeCell.create();
246+
codeCell.setSource('test');
247+
codeCell.undoManager?.stopCapturing();
248+
codeCell.execution_count = 22;
249+
codeCell.undoManager?.stopCapturing();
250+
codeCell.execution_count = 42;
251+
252+
codeCell.undo();
253+
254+
expect(codeCell.execution_count).toEqual(42);
255+
});
256+
257+
test('should not undo output change', () => {
258+
const codeCell = YCodeCell.create();
259+
codeCell.setSource('test');
260+
codeCell.undoManager?.stopCapturing();
261+
const outputs = [
262+
{
263+
data: {
264+
'application/geo+json': {
265+
geometry: {
266+
coordinates: [-118.4563712, 34.0163116],
267+
type: 'Point'
268+
},
269+
type: 'Feature'
270+
},
271+
'text/plain': ['<IPython.display.GeoJSON object>']
272+
},
273+
metadata: {
274+
'application/geo+json': {
275+
expanded: false
276+
}
277+
},
278+
output_type: 'display_data'
279+
},
280+
{
281+
data: {
282+
'application/vnd.jupyter.widget-view+json': {
283+
model_id: '4619c172d65e496baa5d1230894b535a',
284+
version_major: 2,
285+
version_minor: 0
286+
},
287+
'text/plain': [
288+
"HBox(children=(Text(value='text input', layout=Layout(border='1px dashed red', width='80px')), Button(descript…"
289+
]
290+
},
291+
metadata: {},
292+
output_type: 'display_data'
293+
}
294+
];
295+
codeCell.setOutputs(outputs);
296+
codeCell.undoManager?.stopCapturing();
297+
298+
codeCell.undo();
299+
300+
expect(codeCell.getOutputs()).toEqual(outputs);
301+
});
302+
303+
test('should not undo metadata change', () => {
304+
const codeCell = YCodeCell.create();
305+
codeCell.setSource('test');
306+
codeCell.undoManager?.stopCapturing();
307+
codeCell.setMetadata({ collapsed: false });
308+
codeCell.undoManager?.stopCapturing();
309+
codeCell.setMetadata({ collapsed: true });
310+
311+
codeCell.undo();
312+
313+
expect(codeCell.getMetadata('collapsed')).toEqual(true);
314+
});
315+
});
230316
});

‎javascript/test/ynotebook.spec.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,5 +482,103 @@ describe('@jupyter/ydoc', () => {
482482
notebook.dispose();
483483
});
484484
});
485+
486+
describe('#undo', () => {
487+
describe('globally', () => {
488+
test('should undo cell addition', () => {
489+
const notebook = YNotebook.create();
490+
notebook.addCell({ cell_type: 'code' });
491+
notebook.undoManager.stopCapturing();
492+
notebook.addCell({ cell_type: 'markdown' });
493+
494+
expect(notebook.cells.length).toEqual(2);
495+
496+
notebook.undo();
497+
498+
expect(notebook.cells.length).toEqual(1);
499+
});
500+
501+
test('should undo cell source update', () => {
502+
const notebook = YNotebook.create();
503+
const codeCell = notebook.addCell({ cell_type: 'code' });
504+
notebook.undoManager.stopCapturing();
505+
notebook.addCell({ cell_type: 'markdown' });
506+
notebook.undoManager.stopCapturing();
507+
codeCell.updateSource(0, 0, 'print(hello);');
508+
509+
notebook.undo();
510+
511+
expect(notebook.cells.length).toEqual(2);
512+
expect(notebook.getCell(0).getSource()).toEqual('');
513+
});
514+
515+
test('should undo at global level when called locally', () => {
516+
const notebook = YNotebook.create();
517+
const codeCell = notebook.addCell({ cell_type: 'code' });
518+
notebook.undoManager.stopCapturing();
519+
const markdownCell = notebook.addCell({ cell_type: 'markdown' });
520+
notebook.undoManager.stopCapturing();
521+
codeCell.updateSource(0, 0, 'print(hello);');
522+
notebook.undoManager.stopCapturing();
523+
markdownCell.updateSource(0, 0, '# Title');
524+
525+
codeCell.undo();
526+
527+
expect(notebook.cells.length).toEqual(2);
528+
expect(notebook.getCell(0).getSource()).toEqual('print(hello);');
529+
expect(notebook.getCell(1).getSource()).toEqual('');
530+
});
531+
});
532+
533+
describe('per cells', () => {
534+
test('should undo cell addition', () => {
535+
const notebook = YNotebook.create({
536+
disableDocumentWideUndoRedo: true
537+
});
538+
notebook.addCell({ cell_type: 'code' });
539+
notebook.undoManager.stopCapturing();
540+
notebook.addCell({ cell_type: 'markdown' });
541+
542+
expect(notebook.cells.length).toEqual(2);
543+
544+
notebook.undo();
545+
546+
expect(notebook.cells.length).toEqual(1);
547+
});
548+
549+
test('should not undo cell source update', () => {
550+
const notebook = YNotebook.create({
551+
disableDocumentWideUndoRedo: true
552+
});
553+
const codeCell = notebook.addCell({ cell_type: 'code' });
554+
notebook.undoManager.stopCapturing();
555+
notebook.addCell({ cell_type: 'markdown' });
556+
557+
codeCell.updateSource(0, 0, 'print(hello);');
558+
559+
notebook.undo();
560+
561+
expect(notebook.cells.length).toEqual(1);
562+
expect(notebook.getCell(0).getSource()).toEqual('print(hello);');
563+
});
564+
565+
test('should only undo cell source update', () => {
566+
const notebook = YNotebook.create({
567+
disableDocumentWideUndoRedo: true
568+
});
569+
const codeCell = notebook.addCell({ cell_type: 'code' });
570+
notebook.undoManager.stopCapturing();
571+
const markdownCell = notebook.addCell({ cell_type: 'markdown' });
572+
codeCell.updateSource(0, 0, 'print(hello);');
573+
markdownCell.updateSource(0, 0, '# Title');
574+
575+
codeCell.undo();
576+
577+
expect(notebook.cells.length).toEqual(2);
578+
expect(notebook.getCell(0).getSource()).toEqual('');
579+
expect(notebook.getCell(1).getSource()).toEqual('# Title');
580+
});
581+
});
582+
});
485583
});
486584
});

0 commit comments

Comments
 (0)