Candidate: Fix nb outline recompute + nb stickyscroll OutlineTarget (microsoft#211741)

* fix outline recompute event + nb stickyscroll target

* exclude code cell from quickpick, maintain stable behavior

Author: Yoyokrazy

src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts

@@ -288,6 +288,7 @@ class NotebookQuickPickProvider implements IQuickPickDataSource<OutlineEntry> {
 
 	constructor(
 		private _getEntries: () => OutlineEntry[],
+		@IConfigurationService private readonly _configurationService: IConfigurationService,
 		@IThemeService private readonly _themeService: IThemeService
 	) { }
 
@@ -299,7 +300,25 @@ class NotebookQuickPickProvider implements IQuickPickDataSource<OutlineEntry> {
 		const result: IQuickPickOutlineElement<OutlineEntry>[] = [];
 		const { hasFileIcons } = this._themeService.getFileIconTheme();
 
-		for (const element of bucket) {
+		const showSymbols = this._configurationService.getValue<boolean>(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...

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));
 					});

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 } {

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()));
 

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');
 	});
 
 });