Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Candidate: Fix nb outline recompute + nb stickyscroll OutlineTarget #211741

Merged
merged 3 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ class NotebookQuickPickProvider implements IQuickPickDataSource<OutlineEntry> {

constructor(
private _getEntries: () => OutlineEntry[],
@IConfigurationService private readonly _configurationService: IConfigurationService,
@IThemeService private readonly _themeService: IThemeService
) { }

Expand All @@ -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...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yoyokrazy
Please do ensure we clean this in next iteration,
I understand this is a quick fix for recovery, but the code is weird, as we're passing in the target as an argument into the method.

Copy link
Contributor Author

@Yoyokrazy Yoyokrazy Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this will get addressed in the restructuring that we discussed after the notebook sync. It'll need work after the structure of the notebookOutlineProvider changes to reflect the rawModel vs viewModel ideas we have talked about.

edit: issue created that references this comment

cachedEntries.forEach((cached) => {
entries.push(new OutlineEntry(index++, cached.level, cell, cached.name, false, false, cached.range, cached.kind));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,10 @@ export class NotebookCellOutlineProvider {
}
}));

this._recomputeActive();
const { changeEventTriggered } = this._recomputeActive();
if (!changeEventTriggered) {
this._onDidChange.fire({});
}
}

private _recomputeActive(): { changeEventTriggered: boolean } {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -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 () {
Expand All @@ -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');
});

});
Loading