From ef07cc9d5e2bc52a827ca1f9b613c5cd3ccbe926 Mon Sep 17 00:00:00 2001 From: aamunger Date: Tue, 3 Oct 2023 10:42:12 -0700 Subject: [PATCH] fix undo notebook action leak --- .../common/model/notebookTextModel.ts | 1 + .../browser/contrib/notebookUndoRedo.test.ts | 31 ++++++++----------- .../test/browser/testNotebookEditor.ts | 2 +- 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts index 3a1d9faaddf..f2b71a50826 100644 --- a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts +++ b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts @@ -735,6 +735,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel this._bindCellContentHandler(cell, e); }); this._cellListeners.set(cell.handle, dirtyStateListener); + this._register(cell); return cell; }); diff --git a/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookUndoRedo.test.ts b/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookUndoRedo.test.ts index 3f1a6f021fa..98628d0c1b9 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookUndoRedo.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookUndoRedo.test.ts @@ -4,20 +4,20 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; -import { DisposableStore } from 'vs/base/common/lifecycle'; -import { ILanguageService } from 'vs/editor/common/languages/language'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; import { CellEditType, CellKind, SelectionStateType } from 'vs/workbench/contrib/notebook/common/notebookCommon'; -import { createNotebookCellList, TestCell, withTestNotebook } from 'vs/workbench/contrib/notebook/test/browser/testNotebookEditor'; +import { createNotebookCellList, withTestNotebook } from 'vs/workbench/contrib/notebook/test/browser/testNotebookEditor'; suite('Notebook Undo/Redo', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + test('Basics', async function () { await withTestNotebook( [ ['# header 1', 'markdown', CellKind.Markup, [], {}], ['body', 'markdown', CellKind.Markup, [], {}], ], - async (editor, viewModel, _ds, accessor) => { - const languageService = accessor.get(ILanguageService); + async (editor, viewModel, _ds, _accessor) => { assert.strictEqual(viewModel.length, 2); assert.strictEqual(viewModel.getVersionId(), 0); assert.strictEqual(viewModel.getAlternativeId(), '0_0,1;1,1'); @@ -41,7 +41,7 @@ suite('Notebook Undo/Redo', () => { editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 0, count: 0, cells: [ - new TestCell(viewModel.viewType, 3, '# header 2', 'markdown', CellKind.Code, [], languageService), + { source: '# header 3', language: 'markdown', cellKind: CellKind.Markup, outputs: [], mime: undefined } ] }], true, undefined, () => undefined, undefined, true); assert.strictEqual(viewModel.getVersionId(), 4); @@ -60,8 +60,7 @@ suite('Notebook Undo/Redo', () => { ['# header 1', 'markdown', CellKind.Markup, [], {}], ['body', 'markdown', CellKind.Markup, [], {}], ], - async (editor, viewModel, _ds, accessor) => { - const languageService = accessor.get(ILanguageService); + async (editor, _viewModel, _ds, _accessor) => { editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 0, count: 2, cells: [] }], true, undefined, () => undefined, undefined, true); @@ -69,7 +68,7 @@ suite('Notebook Undo/Redo', () => { assert.doesNotThrow(() => { editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 0, count: 2, cells: [ - new TestCell(viewModel.viewType, 3, '# header 2', 'markdown', CellKind.Code, [], languageService), + { source: '# header 2', language: 'markdown', cellKind: CellKind.Markup, outputs: [], mime: undefined } ] }], true, undefined, () => undefined, undefined, true); }); @@ -101,15 +100,14 @@ suite('Notebook Undo/Redo', () => { ['# header 1', 'markdown', CellKind.Markup, [], {}], ['body', 'markdown', CellKind.Markup, [], {}], ], - async (editor, viewModel, _ds, accessor) => { - const languageService = accessor.get(ILanguageService); + async (editor, viewModel, _ds, _accessor) => { editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 0, count: 2, cells: [] }], true, undefined, () => undefined, undefined, true); editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 0, count: 2, cells: [ - new TestCell(viewModel.viewType, 3, '# header 2', 'markdown', CellKind.Code, [], languageService), + { source: '# header 2', language: 'markdown', cellKind: CellKind.Markup, outputs: [], mime: undefined } ] }], true, undefined, () => undefined, undefined, true); @@ -134,14 +132,13 @@ suite('Notebook Undo/Redo', () => { ['body', 'markdown', CellKind.Markup, [], {}], ], async (editor, viewModel, _ds, accessor) => { - const languageService = accessor.get(ILanguageService); - const cellList = createNotebookCellList(accessor, new DisposableStore()); + const cellList = createNotebookCellList(accessor, disposables); cellList.attachViewModel(viewModel); cellList.setFocus([1]); editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 2, count: 0, cells: [ - new TestCell(viewModel.viewType, 3, '# header 2', 'markdown', CellKind.Code, [], languageService) + { source: '# header 2', language: 'markdown', cellKind: CellKind.Markup, outputs: [], mime: undefined } ] }], true, { focus: { start: 1, end: 2 }, selections: [{ start: 1, end: 2 }], kind: SelectionStateType.Index }, () => { return { @@ -175,11 +172,9 @@ suite('Notebook Undo/Redo', () => { ['body', 'markdown', CellKind.Markup, [], {}], ], async (editor, viewModel, _ds, accessor) => { - const languageService = accessor.get(ILanguageService); - editor.textModel.applyEdits([{ editType: CellEditType.Replace, index: 2, count: 0, cells: [ - new TestCell(viewModel.viewType, 3, '# header 2', 'markdown', CellKind.Code, [], languageService) + { source: '# header 2', language: 'markdown', cellKind: CellKind.Markup, outputs: [], mime: undefined } ] }, { editType: CellEditType.Metadata, index: 0, metadata: { inputCollapsed: false } diff --git a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts index b67c8750640..d20676cf47e 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts @@ -416,7 +416,7 @@ export async function withTestNotebook(cells: [source: string, lang: st }); } -export function createNotebookCellList(instantiationService: TestInstantiationService, disposables: DisposableStore, viewContext?: ViewContext) { +export function createNotebookCellList(instantiationService: TestInstantiationService, disposables: Pick, viewContext?: ViewContext) { const delegate: IListVirtualDelegate = { getHeight(element: CellViewModel) { return element.getHeight(17); }, getTemplateId() { return 'template'; }