From e5eb53ac7057c1ccac36cae30eca40d52d6b910f Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Tue, 30 Apr 2024 17:12:33 -0700 Subject: [PATCH] Candidate: Fix nb outline recompute + nb stickyscroll OutlineTarget (#211741) * fix outline recompute event + nb stickyscroll target * exclude code cell from quickpick, maintain stable behavior --- .../contrib/outline/notebookOutline.ts | 21 +++++++++- .../viewModel/notebookOutlineEntryFactory.ts | 6 +-- .../viewModel/notebookOutlineProvider.ts | 5 ++- .../viewParts/notebookEditorStickyScroll.ts | 2 +- .../browser/contrib/notebookSymbols.test.ts | 42 ++++++++++--------- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts b/src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts index 5cf1550482081..9374cc3661c16 100644 --- a/src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts +++ b/src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts @@ -288,6 +288,7 @@ class NotebookQuickPickProvider implements IQuickPickDataSource { constructor( private _getEntries: () => OutlineEntry[], + @IConfigurationService private readonly _configurationService: IConfigurationService, @IThemeService private readonly _themeService: IThemeService ) { } @@ -299,7 +300,25 @@ class NotebookQuickPickProvider implements IQuickPickDataSource { const result: IQuickPickOutlineElement[] = []; const { hasFileIcons } = this._themeService.getFileIconTheme(); - for (const element of bucket) { + const showSymbols = this._configurationService.getValue(NotebookSetting.gotoSymbolsAllSymbols); + for (let i = 0; i < bucket.length; i++) { + const element = bucket[i]; + const nextElement = bucket[i + 1]; + + // this logic controls the following for code cells entries in quick pick: + if (element.cell.cellKind === CellKind.Code) { + // if we are showing all symbols, and + // - the next entry is a symbol, we DO NOT include the code cell entry (ie continue) + // - the next entry is not a symbol, we DO include the code cell entry (ie push as normal in the loop) + if (showSymbols && element.level === NotebookOutlineConstants.NonHeaderOutlineLevel && (nextElement?.level > NotebookOutlineConstants.NonHeaderOutlineLevel)) { + continue; + } + // if we are not showing all symbols, skip all entries with level > NonHeaderOutlineLevel (ie 8+) + else if (!showSymbols && element.level > NotebookOutlineConstants.NonHeaderOutlineLevel) { + continue; + } + } + const useFileIcon = hasFileIcons && !element.symbolKind; // todo@jrieken it is fishy that codicons cannot be used with iconClasses // but file icons can... diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory.ts index ded82f216173b..8655d423a6a9a 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineEntryFactory.ts @@ -87,10 +87,8 @@ export class NotebookOutlineEntryFactory { // Gathering symbols from the model is an async operation, but this provider is syncronous. // So symbols need to be precached before this function is called to get the full list. if (cachedEntries) { - // push code cell that is a parent of cached symbols if we are targeting the outlinePane - if (target === OutlineTarget.OutlinePane) { - entries.push(new OutlineEntry(index++, NotebookOutlineConstants.NonHeaderOutlineLevel, cell, preview, !!exeState, exeState ? exeState.isPaused : false)); - } + // push code cell entry that is a parent of cached symbols, always necessary. filtering done elsewhere. + entries.push(new OutlineEntry(index++, NotebookOutlineConstants.NonHeaderOutlineLevel, cell, preview, !!exeState, exeState ? exeState.isPaused : false)); cachedEntries.forEach((cached) => { entries.push(new OutlineEntry(index++, cached.level, cell, cached.name, false, false, cached.range, cached.kind)); }); diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider.ts index 999113306e6c9..daa78ad6cf3df 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider.ts @@ -260,7 +260,10 @@ export class NotebookCellOutlineProvider { } })); - this._recomputeActive(); + const { changeEventTriggered } = this._recomputeActive(); + if (!changeEventTriggered) { + this._onDidChange.fire({}); + } } private _recomputeActive(): { changeEventTriggered: boolean } { diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index 521b8d908f735..52e6889c4bc76 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -202,7 +202,7 @@ export class NotebookStickyScroll extends Disposable { } private init() { - const { object: notebookOutlineReference } = this.notebookOutlineReference = this.instantiationService.invokeFunction((accessor) => accessor.get(INotebookCellOutlineProviderFactory).getOrCreate(this.notebookEditor, OutlineTarget.QuickPick)); + const { object: notebookOutlineReference } = this.notebookOutlineReference = this.instantiationService.invokeFunction((accessor) => accessor.get(INotebookCellOutlineProviderFactory).getOrCreate(this.notebookEditor, OutlineTarget.OutlinePane)); this._register(this.notebookOutlineReference); this.updateContent(computeContent(this.notebookEditor, this.notebookCellList, notebookOutlineReference.entries, this.getCurrentStickyHeight())); diff --git a/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookSymbols.test.ts b/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookSymbols.test.ts index fdb19d202424e..15d3843cdea5c 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookSymbols.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/contrib/notebookSymbols.test.ts @@ -81,14 +81,15 @@ suite('Notebook Symbols', function () { await entryFactory.cacheSymbols(cell, outlineModelService, CancellationToken.None); const entries = entryFactory.getOutlineEntries(cell, OutlineTarget.QuickPick, 0); - assert.equal(entries.length, 2, 'wrong number of outline entries'); - assert.equal(entries[0].label, 'var1'); + assert.equal(entries.length, 3, 'wrong number of outline entries'); + assert.equal(entries[0].label, '# code'); + assert.equal(entries[1].label, 'var1'); // 6 levels for markdown, all code symbols are greater than the max markdown level - assert.equal(entries[0].level, 8); - assert.equal(entries[0].index, 0); - assert.equal(entries[1].label, 'var2'); assert.equal(entries[1].level, 8); assert.equal(entries[1].index, 1); + assert.equal(entries[2].label, 'var2'); + assert.equal(entries[2].level, 8); + assert.equal(entries[2].index, 2); }); test('Cell with nested symbols', async function () { @@ -102,17 +103,18 @@ suite('Notebook Symbols', function () { await entryFactory.cacheSymbols(cell, outlineModelService, CancellationToken.None); const entries = entryFactory.getOutlineEntries(createCellViewModel(), OutlineTarget.QuickPick, 0); - assert.equal(entries.length, 5, 'wrong number of outline entries'); - assert.equal(entries[0].label, 'root1'); - assert.equal(entries[0].level, 8); - assert.equal(entries[1].label, 'nested1'); - assert.equal(entries[1].level, 9); - assert.equal(entries[2].label, 'nested2'); + assert.equal(entries.length, 6, 'wrong number of outline entries'); + assert.equal(entries[0].label, '# code'); + assert.equal(entries[1].label, 'root1'); + assert.equal(entries[1].level, 8); + assert.equal(entries[2].label, 'nested1'); assert.equal(entries[2].level, 9); - assert.equal(entries[3].label, 'root2'); - assert.equal(entries[3].level, 8); - assert.equal(entries[4].label, 'nested1'); - assert.equal(entries[4].level, 9); + assert.equal(entries[3].label, 'nested2'); + assert.equal(entries[3].level, 9); + assert.equal(entries[4].label, 'root2'); + assert.equal(entries[4].level, 8); + assert.equal(entries[5].label, 'nested1'); + assert.equal(entries[5].level, 9); }); test('Multiple Cells with symbols', async function () { @@ -129,10 +131,12 @@ suite('Notebook Symbols', function () { const entries2 = entryFactory.getOutlineEntries(createCellViewModel(1, '$2'), OutlineTarget.QuickPick, 0); - assert.equal(entries1.length, 1, 'wrong number of outline entries'); - assert.equal(entries1[0].label, 'var1'); - assert.equal(entries2.length, 1, 'wrong number of outline entries'); - assert.equal(entries2[0].label, 'var2'); + assert.equal(entries1.length, 2, 'wrong number of outline entries'); + assert.equal(entries1[0].label, '# code'); + assert.equal(entries1[1].label, 'var1'); + assert.equal(entries2.length, 2, 'wrong number of outline entries'); + assert.equal(entries2[0].label, '# code'); + assert.equal(entries2[1].label, 'var2'); }); });