From 1c33e1c021de894dd121e65597ec01365169233a Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Thu, 4 Jan 2024 12:33:29 -0800 Subject: [PATCH 1/8] no absolute positioning, scrolltop compute based on sticky lines. --- .../browser/media/notebookEditorStickyScroll.css | 1 - .../contrib/notebook/browser/notebookEditorWidget.ts | 2 +- .../browser/view/cellParts/cellToolbarStickyScroll.ts | 4 ++-- .../browser/viewParts/notebookEditorStickyScroll.ts | 11 +++++++++++ 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css index 5750b20cd6538..2a8178c07c945 100644 --- a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css +++ b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css @@ -5,7 +5,6 @@ .monaco-workbench .notebookOverlay .notebook-sticky-scroll-container { display: none; - position: absolute; background-color: var(--vscode-notebook-editorBackground); z-index: var(--z-index-notebook-sticky-scroll); width: 100%; diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 424862aeca382..51bef52ade807 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1818,7 +1818,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD this._dimension = dimension; this._position = position; - const newBodyHeight = this.getBodyHeight(dimension.height); + const newBodyHeight = this.getBodyHeight(dimension.height) - this.getLayoutInfo().stickyHeight; DOM.size(this._body, dimension.width, newBodyHeight); const topInserToolbarHeight = this._notebookOptions.computeTopInsertToolbarHeight(this.viewModel?.viewType); diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts index 55d4de6eb4983..ed1a11b1dfbcc 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts @@ -15,10 +15,10 @@ export function registerCellToolbarStickyScroll(notebookEditor: INotebookEditor, if (cell.isInputCollapsed) { element.style.top = ''; } else { - const stickyHeight = notebookEditor.getLayoutInfo().stickyHeight; + // const stickyHeight = notebookEditor.getLayoutInfo().stickyHeight; const scrollTop = notebookEditor.scrollTop; const elementTop = notebookEditor.getAbsoluteTopOfElement(cell); - const diff = scrollTop - elementTop + extraOffset + stickyHeight; + const diff = scrollTop - elementTop + extraOffset; // + stickyHeight; const maxTop = cell.layoutInfo.editorHeight + cell.layoutInfo.statusBarHeight - 45; // subtract roughly the height of the execution order label plus padding const top = maxTop > 20 ? // Don't move the run button if it can only move a very short distance clamp(min, diff, maxTop) : diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index f7b02b2e4657b..1b9b13aa3177f 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -278,6 +278,17 @@ export class NotebookStickyScroll extends Disposable { } private updateContent(newMap: Map) { + if (newMap.size !== this.currentStickyLines.size) { + // update scrollTop for nb Editor to account for sticky scroll height + if (newMap.size < this.currentStickyLines.size) { + // compute scrollTop change based on dif between new and old sticky height + this.notebookEditor.setScrollTop(this.notebookEditor.scrollTop + (this.currentStickyLines.size - newMap.size) * 22); + } else if (newMap.size > this.currentStickyLines.size) { + // compute scrollTop change based on dif between new and old sticky height + this.notebookEditor.setScrollTop(this.notebookEditor.scrollTop - (newMap.size - this.currentStickyLines.size) * 22); + } + } + this.setCurrentStickyLines(newMap); this.updateDisplay(); } From c7da2dc0dbfa4f7b0cec5b5a18aebecc1ecc5d0d Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Thu, 4 Jan 2024 12:47:05 -0800 Subject: [PATCH 2/8] remove z-index var --- build/lib/stylelint/vscode-known-variables.json | 1 - .../notebook/browser/media/notebookEditorStickyScroll.css | 1 - .../workbench/contrib/notebook/browser/notebookEditorWidget.ts | 1 - 3 files changed, 3 deletions(-) diff --git a/build/lib/stylelint/vscode-known-variables.json b/build/lib/stylelint/vscode-known-variables.json index 0d6805f278844..52fe380b02941 100644 --- a/build/lib/stylelint/vscode-known-variables.json +++ b/build/lib/stylelint/vscode-known-variables.json @@ -795,7 +795,6 @@ "--z-index-notebook-progress-bar", "--z-index-notebook-scrollbar", "--z-index-run-button-container", - "--z-index-notebook-sticky-scroll", "--zoom-factor", "--test-bar-width" ] diff --git a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css index 2a8178c07c945..bc7ba5db025a3 100644 --- a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css +++ b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css @@ -6,7 +6,6 @@ .monaco-workbench .notebookOverlay .notebook-sticky-scroll-container { display: none; background-color: var(--vscode-notebook-editorBackground); - z-index: var(--z-index-notebook-sticky-scroll); width: 100%; font-family: var(--notebook-cell-input-preview-font-family); } diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 51bef52ade807..91575fb23149c 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -3142,7 +3142,6 @@ registerZIndex(ZIndex.Base, 28, 'notebook-cell-bottom-toolbar-container'); registerZIndex(ZIndex.Base, 29, 'notebook-run-button-container'); registerZIndex(ZIndex.Base, 29, 'notebook-input-collapse-condicon'); registerZIndex(ZIndex.Base, 30, 'notebook-cell-output-toolbar'); -registerZIndex(ZIndex.Base, 31, 'notebook-sticky-scroll'); registerZIndex(ZIndex.Sash, 1, 'notebook-cell-expand-part-button'); registerZIndex(ZIndex.Sash, 2, 'notebook-cell-toolbar'); registerZIndex(ZIndex.Sash, 3, 'notebook-cell-toolbar-dropdown-active'); From dd31341ecf6f65deb103a3b5dcfdcc9b0abd74a0 Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Tue, 16 Jan 2024 15:58:04 -0800 Subject: [PATCH 3/8] compute re-write, remove init, reduce pop-in --- .../media/notebookEditorStickyScroll.css | 15 +- .../notebook/browser/notebookEditorWidget.ts | 12 + .../view/cellParts/cellToolbarStickyScroll.ts | 3 +- .../viewParts/notebookEditorStickyScroll.ts | 359 +++++++----------- .../test/browser/notebookStickyScroll.test.ts | 2 +- 5 files changed, 161 insertions(+), 230 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css index bc7ba5db025a3..4b4078aecbe8b 100644 --- a/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css +++ b/src/vs/workbench/contrib/notebook/browser/media/notebookEditorStickyScroll.css @@ -7,15 +7,14 @@ display: none; background-color: var(--vscode-notebook-editorBackground); width: 100%; - font-family: var(--notebook-cell-input-preview-font-family); } + .monaco-workbench .notebookOverlay .notebook-sticky-scroll-container .notebook-sticky-scroll-line { background-color: var(--vscode-notebook-editorBackground); position: relative; - z-index: 0; padding-left: 12px; /* transition: margin-top 0.2s ease-in-out; */ } @@ -33,15 +32,3 @@ background-color: var(--vscode-editorStickyScrollHover-background); cursor: pointer; } - -.monaco-workbench - .notebookOverlay - .notebook-sticky-scroll-container - .notebook-shadow { - display: block; - top: 0; - left: 3px; - height: 3px; - width: 100%; - box-shadow: var(--vscode-scrollbar-shadow) 0 6px 6px -6px inset; -} diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 91575fb23149c..0c46f79dcf944 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1065,6 +1065,18 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD private _registerNotebookStickyScroll() { this._notebookStickyScroll = this._register(this.instantiationService.createInstance(NotebookStickyScroll, this._notebookStickyScrollContainer, this, this._notebookOutline, this._list)); + + this._register(this._notebookStickyScroll.onDidChangeNotebookStickyScroll((sizeDelta) => { + if (this._dimension) { // TODO @Yoyokrazy: think this logic is unnecessary in this situation + if (sizeDelta > 0) { // delta > 0 ==> sticky is growing, cell list shrinking + this.layout(this._dimension); + this.setScrollTop(this.scrollTop + sizeDelta); + } else if (sizeDelta < 0) { // delta < 0 ==> sticky is shrinking, cell list growing + this.setScrollTop(this.scrollTop + sizeDelta); + this.layout(this._dimension); + } + } + })); } private _updateOutputRenderers() { diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts index ed1a11b1dfbcc..f425ea10dcbfe 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellToolbarStickyScroll.ts @@ -15,10 +15,9 @@ export function registerCellToolbarStickyScroll(notebookEditor: INotebookEditor, if (cell.isInputCollapsed) { element.style.top = ''; } else { - // const stickyHeight = notebookEditor.getLayoutInfo().stickyHeight; const scrollTop = notebookEditor.scrollTop; const elementTop = notebookEditor.getAbsoluteTopOfElement(cell); - const diff = scrollTop - elementTop + extraOffset; // + stickyHeight; + const diff = scrollTop - elementTop + extraOffset; const maxTop = cell.layoutInfo.editorHeight + cell.layoutInfo.statusBarHeight - 45; // subtract roughly the height of the execution order label plus padding const top = maxTop > 20 ? // Don't move the run button if it can only move a very short distance clamp(min, diff, maxTop) : diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index 1b9b13aa3177f..f7f7fe777c945 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -6,6 +6,8 @@ import { localize } from 'vs/nls'; import * as DOM from 'vs/base/browser/dom'; import { StandardMouseEvent } from 'vs/base/browser/mouseEvent'; +import { Delayer } from 'vs/base/common/async'; +import { Emitter, Event } from 'vs/base/common/event'; import { Disposable, DisposableStore } from 'vs/base/common/lifecycle'; import { ServicesAccessor } from 'vs/editor/browser/editorExtensions'; import { Categories } from 'vs/platform/action/common/actionCommonCategories'; @@ -15,10 +17,9 @@ import { ContextKeyExpr } from 'vs/platform/contextkey/common/contextkey'; import { IContextMenuService } from 'vs/platform/contextview/browser/contextView'; import { INotebookEditor } from 'vs/workbench/contrib/notebook/browser/notebookBrowser'; import { INotebookCellList } from 'vs/workbench/contrib/notebook/browser/view/notebookRenderingCommon'; +import { OutlineEntry } from 'vs/workbench/contrib/notebook/browser/viewModel/OutlineEntry'; import { NotebookCellOutlineProvider } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider'; - import { CellKind } from 'vs/workbench/contrib/notebook/common/notebookCommon'; -import { OutlineEntry } from 'vs/workbench/contrib/notebook/browser/viewModel/OutlineEntry'; export class ToggleNotebookStickyScroll extends Action2 { @@ -81,32 +82,49 @@ export class NotebookStickyLine extends Disposable { } } -// TODO @Yoyokrazy: -// BEHAVIOR -// - [ ] bug with some popping around the cell transition -// - [ ] bug with only bottom most sticky being partially transitioned -// - partial rendering/transition only occuring when the headers shrink against a new section -// - **and only for BOTTOM of that initial sticky tree** -// - issues with HC themes -// UX -// - [ ] render symbols instead of #'s? -// - maybe 'Hx >' where x is the level export class NotebookStickyScroll extends Disposable { private readonly _disposables = new DisposableStore(); private currentStickyLines = new Map(); + private renderedStickyLines: NotebookStickyLine[] = []; + private filteredOutlineEntries: OutlineEntry[] = []; + + private readonly _onDidChangeNotebookStickyScroll = this._register(new Emitter()); + readonly onDidChangeNotebookStickyScroll: Event = this._onDidChangeNotebookStickyScroll.event; + getDomNode(): HTMLElement { return this.domNode; } getCurrentStickyHeight() { - return this.currentStickyLines.size * 22; + let height = 0; + this.currentStickyLines.forEach((value) => { + if (value.rendered) { + height += 22; + } + }); + return height; } private setCurrentStickyLines(newStickyLines: Map) { this.currentStickyLines = newStickyLines; } + private compareStickyLineMaps(mapA: Map, mapB: Map): boolean { + if (mapA.size !== mapB.size) { + return false; + } + + for (const [key, value] of mapA) { + const otherValue = mapB.get(key); + if (!otherValue || value.rendered !== otherValue.rendered) { + return false; + } + } + + return true; + } + constructor( private readonly domNode: HTMLElement, private readonly notebookEditor: INotebookEditor, @@ -124,9 +142,6 @@ export class NotebookStickyScroll extends Disposable { if (e.stickyScroll) { this.updateConfig(); } - if (e.globalToolbar) { - this.setTop(); - } })); this._register(DOM.addDisposableListener(this.domNode, DOM.EventType.CONTEXT_MENU, async (event: MouseEvent) => { @@ -153,36 +168,36 @@ export class NotebookStickyScroll extends Disposable { } } - private setTop() { - if (this.notebookEditor.notebookOptions.getDisplayOptions().globalToolbar) { - this.domNode.style.top = '26px'; - } else { - this.domNode.style.top = '0px'; - } - } - private init() { this.notebookOutline.init(); - this.initializeContent(); + this.filteredOutlineEntries = this.notebookOutline.entries.filter(entry => entry.level !== 7); this._disposables.add(this.notebookOutline.onDidChange(() => { - DOM.clearNode(this.domNode); - this.disposeCurrentStickyLines(); - this.updateContent(computeContent(this.domNode, this.notebookEditor, this.notebookCellList, this.notebookOutline.entries)); + this.filteredOutlineEntries = this.notebookOutline.entries.filter(entry => entry.level !== 7); + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { + this.updateContent(recompute); + } })); this._disposables.add(this.notebookEditor.onDidAttachViewModel(() => { this.notebookOutline.init(); - this.initializeContent(); + this.updateContent(computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines)); })); this._disposables.add(this.notebookEditor.onDidScroll(() => { - DOM.clearNode(this.domNode); - this.disposeCurrentStickyLines(); - this.updateContent(computeContent(this.domNode, this.notebookEditor, this.notebookCellList, this.notebookOutline.entries)); + const deboucer = new Delayer(200); + + deboucer.trigger(() => { + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { + this.updateContent(recompute); + } + }); })); } + // take in an cell index, and get the corresponding outline entry static getVisibleOutlineEntry(visibleIndex: number, notebookOutlineEntries: OutlineEntry[]): OutlineEntry | undefined { let left = 0; let right = notebookOutlineEntries.length - 1; @@ -210,102 +225,38 @@ export class NotebookStickyScroll extends Disposable { return undefined; } - private initializeContent() { - - // find last code cell of section, store bottom scroll position in sectionBottom - const visibleRange = this.notebookEditor.visibleRanges[0]; - if (!visibleRange) { - return; - } - + private updateContent(newMap: Map) { DOM.clearNode(this.domNode); - const editorScrollTop = this.notebookEditor.scrollTop; - - let trackedEntry = undefined; - let sectionBottom = 0; - for (let i = visibleRange.start; i < visibleRange.end; i++) { - if (i === 0) { // don't show headers when you're viewing the top cell - this.updateDisplay(); - this.setCurrentStickyLines(new Map()); - return; - } - const cell = this.notebookEditor.cellAt(i); - if (!cell) { - return; - } - - // if we are here, the cell is a code cell. - // check next cell, if markdown, that means this is the end of the section - // check if cell is within visible range - const nextCell = this.notebookEditor.cellAt(i + 1); - if (nextCell && i + 1 < visibleRange.end) { - if (nextCell.cellKind === CellKind.Markup) { - // this is the end of the section - // store the bottom scroll position of this cell - sectionBottom = this.notebookCellList.getCellViewScrollBottom(cell); - // compute sticky scroll height - const entry = NotebookStickyScroll.getVisibleOutlineEntry(i, this.notebookOutline.entries); - if (!entry) { - return; - } - // using 22 instead of stickyscrollheight, as we don't necessarily render each line. 22 starts rendering sticky when we have space for at least 1 of them - const newStickyHeight = NotebookStickyScroll.computeStickyHeight(entry!); - if (editorScrollTop + newStickyHeight < sectionBottom) { - trackedEntry = entry; - break; - } else { - // if (editorScrollTop + stickyScrollHeight > sectionBottom), then continue to next section - continue; - } - } - } else { - // there is no next cell, so use the bottom of the editor as the sectionBottom, using scrolltop + height - sectionBottom = this.notebookEditor.scrollTop + this.notebookEditor.getLayoutInfo().scrollHeight; - trackedEntry = NotebookStickyScroll.getVisibleOutlineEntry(i, this.notebookOutline.entries); - break; - } - } // cell loop close - - // ------------------------------------------------------------------------------------- - // we now know the cell which the sticky is determined by, and the sectionBottom value to determine how many sticky lines to render - // compute the space available for sticky lines, and render sticky lines + this.disposeCurrentStickyLines(); + this.renderStickyLines(newMap, this.domNode); - const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22); - let newMap: Map = new Map(); - newMap = NotebookStickyScroll.renderStickyLines(trackedEntry?.parent, this.domNode, linesToRender, newMap, this.notebookEditor); + const oldStickyHeight = this.getCurrentStickyHeight(); this.setCurrentStickyLines(newMap); - this.updateDisplay(); - } - - private updateContent(newMap: Map) { - if (newMap.size !== this.currentStickyLines.size) { - // update scrollTop for nb Editor to account for sticky scroll height - if (newMap.size < this.currentStickyLines.size) { - // compute scrollTop change based on dif between new and old sticky height - this.notebookEditor.setScrollTop(this.notebookEditor.scrollTop + (this.currentStickyLines.size - newMap.size) * 22); - } else if (newMap.size > this.currentStickyLines.size) { - // compute scrollTop change based on dif between new and old sticky height - this.notebookEditor.setScrollTop(this.notebookEditor.scrollTop - (newMap.size - this.currentStickyLines.size) * 22); - } + this.renderedStickyLines = Array.from(newMap.values()) + .filter(value => value.rendered) + .map(value => value.line); + + // (+) = sticky height increased + // (-) = sticky height decreased + const sizeDelta = this.getCurrentStickyHeight() - oldStickyHeight; + if (sizeDelta !== 0) { + this._onDidChangeNotebookStickyScroll.fire(sizeDelta); } - - this.setCurrentStickyLines(newMap); this.updateDisplay(); } private updateDisplay() { - const hasSticky = this.currentStickyLines.size > 0; + const hasSticky = this.getCurrentStickyHeight() > 0; if (!hasSticky) { this.domNode.style.display = 'none'; } else { this.domNode.style.display = 'block'; } - this.setTop(); } static computeStickyHeight(entry: OutlineEntry) { let height = 0; - if (entry.cell.cellKind === CellKind.Markup) { + if (entry.cell.cellKind === CellKind.Markup && entry.level !== 7) { height += 22; } while (entry.parent) { @@ -315,8 +266,9 @@ export class NotebookStickyScroll extends Disposable { return height; } - static renderStickyLines(entry: OutlineEntry | undefined, containerElement: HTMLElement, numLinesToRender: number, newMap: Map, notebookEditor: INotebookEditor) { + static checkCollapsedStickyLines(entry: OutlineEntry | undefined, numLinesToRender: number, notebookEditor: INotebookEditor) { let currentEntry = entry; + const newMap = new Map(); const elementsToRender = []; while (currentEntry) { @@ -331,32 +283,27 @@ export class NotebookStickyScroll extends Disposable { currentEntry = currentEntry.parent; } - // TODO: clean up partial cell animation - // [ ] slight pop as lines finish disappearing - // [ ] only actually works when shrunk against new section. **and only for BOTTOM of that initial sticky tree** - // [ ] issues with HC themes - // use negative margins to render the bottom sticky line as a partial element - // todo: partial render logic here - // if (numLinesToRender % 1 !== 0) { - // const partialHeight = 22 - Math.floor((numLinesToRender % 1) * 22); - // elementsToRender[elementsToRender.length - 1].element.style.zIndex = '-1'; - // elementsToRender[elementsToRender.length - 1].element.style.marginTop = `-${partialHeight}px`; - // } - // iterate over elements to render, and append to container // break when we reach numLinesToRender for (let i = 0; i < elementsToRender.length; i++) { if (i >= numLinesToRender) { break; } - containerElement.append(elementsToRender[i].element); newMap.set(elementsToRender[i].entry, { line: elementsToRender[i], rendered: true }); } - - containerElement.append(DOM.$('div', { class: 'notebook-shadow' })); // ensure we have dropShadow at base of sticky scroll return newMap; } + private renderStickyLines(stickyMap: Map, containerElement: HTMLElement) { + const reversedEntries = Array.from(stickyMap.entries()).reverse(); + for (const [, value] of reversedEntries) { + if (!value.rendered) { + continue; + } + containerElement.append(value.line.element); + } + } + static createStickyElement(entry: OutlineEntry, notebookEditor: INotebookEditor) { const stickyElement = document.createElement('div'); stickyElement.classList.add('notebook-sticky-scroll-line'); @@ -377,111 +324,97 @@ export class NotebookStickyScroll extends Disposable { } } -export function computeContent(domNode: HTMLElement, notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[]): Map { - // find first code cell in visible range. this marks the start of the first section - // find the last code cell in the first section of the visible range, store the bottom scroll position in a const sectionBottom - // compute sticky scroll height, and check if editorScrolltop + stickyScrollHeight < sectionBottom - // if that condition is true, break out of the loop with that cell as the tracked cell - // if that condition is false, continue to next cell - - const editorScrollTop = notebookEditor.scrollTop; - - // find last code cell of section, store bottom scroll position in sectionBottom +export function computeContent(notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[], renderedStickyHeight: number, renderedLines: NotebookStickyLine[]): Map { + // get data about the cell list within viewport ---------------------------------------------------------------------------------------- + const editorScrollTop = notebookEditor.scrollTop - renderedStickyHeight; const visibleRange = notebookEditor.visibleRanges[0]; if (!visibleRange) { return new Map(); } - let trackedEntry = undefined; - let sectionBottom = 0; - for (let i = visibleRange.start; i < visibleRange.end; i++) { - const cell = notebookEditor.cellAt(i); + const startIndex = visibleRange.start > 0 ? visibleRange.start - 1 : visibleRange.start; + + // iterate over cells in viewport ------------------------------------------------------------------------------------------------------ + let cell; + let cellEntry; + for (let currentIndex = startIndex; currentIndex < visibleRange.end; currentIndex++) { + // store data for current cell, and next cell + cell = notebookEditor.cellAt(currentIndex); if (!cell) { return new Map(); } + cellEntry = NotebookStickyScroll.getVisibleOutlineEntry(currentIndex, notebookOutlineEntries); + if (!cellEntry) { + return new Map(); + } - const nextCell = notebookEditor.cellAt(i + 1); + const nextCell = notebookEditor.cellAt(currentIndex + 1); + if (!nextCell) { + const sectionBottom = notebookEditor.getLayoutInfo().scrollHeight; + const linesToRender = Math.floor((sectionBottom) / 22); + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); + return newMap; + } + const nextCellEntry = NotebookStickyScroll.getVisibleOutlineEntry(currentIndex + 1, notebookOutlineEntries); + if (!nextCellEntry) { + return new Map(); + } - // account for transitions between top level headers - if (cell.cellKind === CellKind.Markup) { - sectionBottom = notebookCellList.getCellViewScrollBottom(cell); - const entry = NotebookStickyScroll.getVisibleOutlineEntry(i, notebookOutlineEntries); - if (!entry) { - return new Map(); + // check next cell, if markdown with non level 7 entry, that means this is the end of the section (new header) --------------------- + if (nextCell.cellKind === CellKind.Markup && nextCellEntry.level !== 7) { + const sectionBottom = notebookCellList.getCellViewScrollTop(nextCell); + const currentSectionStickyHeight = NotebookStickyScroll.computeStickyHeight(cellEntry); + const nextSectionStickyHeight = NotebookStickyScroll.computeStickyHeight(nextCellEntry); + + // case: we can render the all sticky lines for the current section ------------------------------------------------------------ + if (editorScrollTop + currentSectionStickyHeight < sectionBottom) { + const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22); + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); + return newMap; } - if (!entry.parent) { - // if the cell is a top level header, only render once we have scrolled past the bottom of the cell - // todo: (polish) figure out what padding value to use here. need to account properly for bottom insert cell toolbar, cell toolbar, and md cell bottom padding - if (sectionBottom > editorScrollTop) { - return new Map(); - } + // cases: next section has a parent, so adjust based on find the shared parent ------------------------------------------------- + // shrink until sizes are the same, then render the next section + else if (nextSectionStickyHeight === currentSectionStickyHeight) { + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); // !document why I use 100 and don't compute linesToRender + return newMap; } - } - - // if we are here, the cell is a code cell. - // check next cell, if markdown, that means this is the end of the section - if (nextCell && i + 1 < visibleRange.end) { - if (nextCell.cellKind === CellKind.Markup) { - // this is the end of the section - // store the bottom scroll position of this cell - sectionBottom = notebookCellList.getCellViewScrollBottom(cell); - // compute sticky scroll height - const entry = NotebookStickyScroll.getVisibleOutlineEntry(i, notebookOutlineEntries); - if (!entry) { - return new Map(); + // next section is larger than current section, don't shrink, render another + else if (nextSectionStickyHeight > currentSectionStickyHeight) { + // if the difference is greater than 22, throw an error. this shouldn't be possible + if (nextSectionStickyHeight - currentSectionStickyHeight > 22) { + throw new Error('next > curr, but diff > 22'); } - // check if we can render this section of sticky - const currentSectionStickyHeight = NotebookStickyScroll.computeStickyHeight(entry!); - if (editorScrollTop + currentSectionStickyHeight < sectionBottom) { - const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22); - let newMap: Map = new Map(); - newMap = NotebookStickyScroll.renderStickyLines(entry, domNode, linesToRender, newMap, notebookEditor); - return newMap; - } - - let nextSectionEntry = undefined; - for (let j = 1; j < visibleRange.end - i; j++) { - // find next section after this one - const cellCheck = notebookEditor.cellAt(i + j); - if (cellCheck) { - nextSectionEntry = NotebookStickyScroll.getVisibleOutlineEntry(i + j, notebookOutlineEntries); - if (nextSectionEntry) { - break; - } + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); // !document why I use 100 and don't compute linesToRender + return newMap; + } + // next section is smaller than current section, shrink until the you find the shared node then re-render + else if (nextSectionStickyHeight < currentSectionStickyHeight) { + const availableSpace = sectionBottom - editorScrollTop; + + if (nextSectionStickyHeight === renderedLines.length * 22) { + if (availableSpace > (renderedLines.length + 1) * 22) { + const linesToRender = Math.floor((availableSpace) / 22); + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); + return newMap; + } else { + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); + return newMap; } - } - const nextSectionStickyHeight = NotebookStickyScroll.computeStickyHeight(nextSectionEntry!); - - // recompute section bottom based on the top of the next section - sectionBottom = notebookCellList.getCellViewScrollTop(nextSectionEntry!.cell) - 10; - - // this block of logic cleans transitions between two sections that share a parent. - // if the current section and the next section share a parent, then we can render the next section's sticky lines to avoid pop-in between - if (entry?.parent?.parent === nextSectionEntry?.parent) { - const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22) + 100; - let newMap: Map = new Map(); - newMap = NotebookStickyScroll.renderStickyLines(nextSectionEntry?.parent, domNode, linesToRender, newMap, notebookEditor); - return newMap; - } else if (Math.abs(currentSectionStickyHeight - nextSectionStickyHeight) > 22) { // only shrink sticky - const linesToRender = (sectionBottom - editorScrollTop) / 22; - let newMap: Map = new Map(); - newMap = NotebookStickyScroll.renderStickyLines(entry?.parent, domNode, linesToRender, newMap, notebookEditor); + } else { + const linesToRender = Math.floor((availableSpace) / 22); + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); return newMap; } } - } else { - // there is no next visible cell, so use the bottom of the editor as the sectionBottom, using scrolltop + height - sectionBottom = notebookEditor.getLayoutInfo().scrollHeight; - trackedEntry = NotebookStickyScroll.getVisibleOutlineEntry(i, notebookOutlineEntries); - const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22); - - let newMap: Map = new Map(); - newMap = NotebookStickyScroll.renderStickyLines(trackedEntry?.parent, domNode, linesToRender, newMap, notebookEditor); - return newMap; } - } // for cell loop close - return new Map(); + } // visible range loop close + + // case: all visible cells were non-header cells, so render any headers relevant to their section -------------------------------------- + const sectionBottom = notebookEditor.getLayoutInfo().scrollHeight; + const linesToRender = Math.floor((sectionBottom - editorScrollTop) / 22); + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); + return newMap; } registerAction2(ToggleNotebookStickyScroll); diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts index 5c9682417a99d..7b8ce2f2b94b8 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts @@ -50,7 +50,7 @@ suite('NotebookEditorStickyScroll', () => { } function nbStickyTestHelper(domNode: HTMLElement, notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[], disposables: Pick) { - const output = computeContent(domNode, notebookEditor, notebookCellList, notebookOutlineEntries); + const output = computeContent(notebookEditor, notebookCellList, notebookOutlineEntries, 0, []); for (const stickyLine of output.values()) { disposables.add(stickyLine.line); } From 1b0498b60bda02838d2515cbe1e25f40c2effffe Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Wed, 17 Jan 2024 09:58:53 -0800 Subject: [PATCH 4/8] dispose delayer --- .../browser/viewParts/notebookEditorStickyScroll.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index f7f7fe777c945..ae1913e7d98b0 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -91,6 +91,7 @@ export class NotebookStickyScroll extends Disposable { private readonly _onDidChangeNotebookStickyScroll = this._register(new Emitter()); readonly onDidChangeNotebookStickyScroll: Event = this._onDidChangeNotebookStickyScroll.event; + private readonly deboucer = new Delayer(200); getDomNode(): HTMLElement { return this.domNode; @@ -186,9 +187,7 @@ export class NotebookStickyScroll extends Disposable { })); this._disposables.add(this.notebookEditor.onDidScroll(() => { - const deboucer = new Delayer(200); - - deboucer.trigger(() => { + this.deboucer.trigger(() => { const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { this.updateContent(recompute); @@ -319,6 +318,7 @@ export class NotebookStickyScroll extends Disposable { override dispose() { this._disposables.dispose(); + this.deboucer.dispose(); this.disposeCurrentStickyLines(); super.dispose(); } From 402553c195573c6ad04f77be479886153aa4c5a3 Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Wed, 17 Jan 2024 10:47:47 -0800 Subject: [PATCH 5/8] remove debounce --- .../browser/viewParts/notebookEditorStickyScroll.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index ae1913e7d98b0..c87f5c2c01c47 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -6,7 +6,6 @@ import { localize } from 'vs/nls'; import * as DOM from 'vs/base/browser/dom'; import { StandardMouseEvent } from 'vs/base/browser/mouseEvent'; -import { Delayer } from 'vs/base/common/async'; import { Emitter, Event } from 'vs/base/common/event'; import { Disposable, DisposableStore } from 'vs/base/common/lifecycle'; import { ServicesAccessor } from 'vs/editor/browser/editorExtensions'; @@ -91,7 +90,6 @@ export class NotebookStickyScroll extends Disposable { private readonly _onDidChangeNotebookStickyScroll = this._register(new Emitter()); readonly onDidChangeNotebookStickyScroll: Event = this._onDidChangeNotebookStickyScroll.event; - private readonly deboucer = new Delayer(200); getDomNode(): HTMLElement { return this.domNode; @@ -187,12 +185,10 @@ export class NotebookStickyScroll extends Disposable { })); this._disposables.add(this.notebookEditor.onDidScroll(() => { - this.deboucer.trigger(() => { - const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); - if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { - this.updateContent(recompute); - } - }); + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { + this.updateContent(recompute); + } })); } @@ -318,7 +314,6 @@ export class NotebookStickyScroll extends Disposable { override dispose() { this._disposables.dispose(); - this.deboucer.dispose(); this.disposeCurrentStickyLines(); super.dispose(); } From 86afa29f5d6b956d7a3d2693f07fd290f69d0736 Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Wed, 17 Jan 2024 14:38:35 -0800 Subject: [PATCH 6/8] edge case for cell 0 header, next animation frame instead of debounce --- .../notebook/browser/notebookEditorWidget.ts | 25 +++++++++++++------ .../viewParts/notebookEditorStickyScroll.ts | 13 +++++++++- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 0c46f79dcf944..ed0226a61afa8 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1066,16 +1066,25 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD private _registerNotebookStickyScroll() { this._notebookStickyScroll = this._register(this.instantiationService.createInstance(NotebookStickyScroll, this._notebookStickyScrollContainer, this, this._notebookOutline, this._list)); + const localDisposableStore = this._register(new DisposableStore()); + this._register(this._notebookStickyScroll.onDidChangeNotebookStickyScroll((sizeDelta) => { - if (this._dimension) { // TODO @Yoyokrazy: think this logic is unnecessary in this situation - if (sizeDelta > 0) { // delta > 0 ==> sticky is growing, cell list shrinking - this.layout(this._dimension); - this.setScrollTop(this.scrollTop + sizeDelta); - } else if (sizeDelta < 0) { // delta < 0 ==> sticky is shrinking, cell list growing - this.setScrollTop(this.scrollTop + sizeDelta); - this.layout(this._dimension); + const d = localDisposableStore.add(DOM.scheduleAtNextAnimationFrame(DOM.getWindow(this.getDomNode()), () => { + if (this.isDisposed) { + return; } - } + + if (this._dimension) { // TODO @Yoyokrazy: think this logic is unnecessary in this situation + if (sizeDelta > 0) { // delta > 0 ==> sticky is growing, cell list shrinking + this.layout(this._dimension); + this.setScrollTop(this.scrollTop + sizeDelta); + } else if (sizeDelta < 0) { // delta < 0 ==> sticky is shrinking, cell list growing + this.setScrollTop(this.scrollTop + sizeDelta); + this.layout(this._dimension); + } + } + localDisposableStore.delete(d); + })); })); } diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index c87f5c2c01c47..6bad40a29bf29 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -327,11 +327,22 @@ export function computeContent(notebookEditor: INotebookEditor, notebookCellList return new Map(); } - const startIndex = visibleRange.start > 0 ? visibleRange.start - 1 : visibleRange.start; + // edge case for cell 0 in the notebook is a header ------------------------------------------------------------------------------------ + if (visibleRange.start === 0) { + const firstCell = notebookEditor.cellAt(0); + const firstCellEntry = NotebookStickyScroll.getVisibleOutlineEntry(0, notebookOutlineEntries); + if (firstCell && firstCellEntry && firstCell.cellKind === CellKind.Markup && firstCellEntry.level !== 7) { + if (notebookEditor.scrollTop > 22) { + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(firstCellEntry, 100, notebookEditor); + return newMap; + } + } + } // iterate over cells in viewport ------------------------------------------------------------------------------------------------------ let cell; let cellEntry; + const startIndex = visibleRange.start - 1; // -1 to account for cells hidden "under" sticky lines. for (let currentIndex = startIndex; currentIndex < visibleRange.end; currentIndex++) { // store data for current cell, and next cell cell = notebookEditor.cellAt(currentIndex); From 5156f6f8bc39d0bbce1a61850e0dcfaa5ebd3879 Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Wed, 17 Jan 2024 17:06:36 -0800 Subject: [PATCH 7/8] add delayer back, further improve pop in --- .../notebook/browser/notebookEditorWidget.ts | 2 +- .../viewParts/notebookEditorStickyScroll.ts | 45 +++++++------------ 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index ed0226a61afa8..923c173afc28f 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1074,7 +1074,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD return; } - if (this._dimension) { // TODO @Yoyokrazy: think this logic is unnecessary in this situation + if (this._dimension) { if (sizeDelta > 0) { // delta > 0 ==> sticky is growing, cell list shrinking this.layout(this._dimension); this.setScrollTop(this.scrollTop + sizeDelta); diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index 6bad40a29bf29..d2411d9d9f89d 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -19,6 +19,7 @@ import { INotebookCellList } from 'vs/workbench/contrib/notebook/browser/view/no import { OutlineEntry } from 'vs/workbench/contrib/notebook/browser/viewModel/OutlineEntry'; import { NotebookCellOutlineProvider } from 'vs/workbench/contrib/notebook/browser/viewModel/notebookOutlineProvider'; import { CellKind } from 'vs/workbench/contrib/notebook/common/notebookCommon'; +import { Delayer } from 'vs/base/common/async'; export class ToggleNotebookStickyScroll extends Action2 { @@ -185,10 +186,14 @@ export class NotebookStickyScroll extends Disposable { })); this._disposables.add(this.notebookEditor.onDidScroll(() => { - const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); - if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { - this.updateContent(recompute); - } + const d = new Delayer(100); + d.trigger(() => { + d.dispose(); + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { + this.updateContent(recompute); + } + }); })); } @@ -379,38 +384,22 @@ export function computeContent(notebookEditor: INotebookEditor, notebookCellList return newMap; } - // cases: next section has a parent, so adjust based on find the shared parent ------------------------------------------------- - // shrink until sizes are the same, then render the next section - else if (nextSectionStickyHeight === currentSectionStickyHeight) { - const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); // !document why I use 100 and don't compute linesToRender + // case: next section is the same size or bigger, render next entry ----------------------------------------------------------- + else if (nextSectionStickyHeight >= currentSectionStickyHeight) { + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); return newMap; } - // next section is larger than current section, don't shrink, render another - else if (nextSectionStickyHeight > currentSectionStickyHeight) { - // if the difference is greater than 22, throw an error. this shouldn't be possible - if (nextSectionStickyHeight - currentSectionStickyHeight > 22) { - throw new Error('next > curr, but diff > 22'); - } - const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); // !document why I use 100 and don't compute linesToRender - return newMap; - } - // next section is smaller than current section, shrink until the you find the shared node then re-render + // case: next section is the smaller, shrink until next section height is greater than the available space --------------------- else if (nextSectionStickyHeight < currentSectionStickyHeight) { const availableSpace = sectionBottom - editorScrollTop; - if (nextSectionStickyHeight === renderedLines.length * 22) { - if (availableSpace > (renderedLines.length + 1) * 22) { - const linesToRender = Math.floor((availableSpace) / 22); - const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); - return newMap; - } else { - const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); - return newMap; - } - } else { + if (availableSpace >= nextSectionStickyHeight) { const linesToRender = Math.floor((availableSpace) / 22); const newMap = NotebookStickyScroll.checkCollapsedStickyLines(cellEntry, linesToRender, notebookEditor); return newMap; + } else { + const newMap = NotebookStickyScroll.checkCollapsedStickyLines(nextCellEntry, 100, notebookEditor); + return newMap; } } } From 844016af83fb5bdfeee159a64610e892fff796ad Mon Sep 17 00:00:00 2001 From: Michael Lively Date: Wed, 17 Jan 2024 17:15:28 -0800 Subject: [PATCH 8/8] remove unused param, update testing snapshots --- .../viewParts/notebookEditorStickyScroll.ts | 12 ++++-------- ...lapsing_against_equivalent_level_header.0.snap | 1 - ...lapsing_against_equivalent_level_header.0.snap | 1 + ...0-_2___scrolltop_halfway_through_cell_2.0.snap | 2 +- .../test/browser/notebookStickyScroll.test.ts | 15 +++++---------- 5 files changed, 11 insertions(+), 20 deletions(-) delete mode 100644 src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_1___collapsing_against_equivalent_level_header.0.snap create mode 100644 src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_2___collapsing_against_equivalent_level_header.0.snap diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts index d2411d9d9f89d..0c812230c030b 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorStickyScroll.ts @@ -85,7 +85,6 @@ export class NotebookStickyLine extends Disposable { export class NotebookStickyScroll extends Disposable { private readonly _disposables = new DisposableStore(); private currentStickyLines = new Map(); - private renderedStickyLines: NotebookStickyLine[] = []; private filteredOutlineEntries: OutlineEntry[] = []; private readonly _onDidChangeNotebookStickyScroll = this._register(new Emitter()); @@ -174,7 +173,7 @@ export class NotebookStickyScroll extends Disposable { this._disposables.add(this.notebookOutline.onDidChange(() => { this.filteredOutlineEntries = this.notebookOutline.entries.filter(entry => entry.level !== 7); - const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight()); if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { this.updateContent(recompute); } @@ -182,14 +181,14 @@ export class NotebookStickyScroll extends Disposable { this._disposables.add(this.notebookEditor.onDidAttachViewModel(() => { this.notebookOutline.init(); - this.updateContent(computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines)); + this.updateContent(computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight())); })); this._disposables.add(this.notebookEditor.onDidScroll(() => { const d = new Delayer(100); d.trigger(() => { d.dispose(); - const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight(), this.renderedStickyLines); + const recompute = computeContent(this.notebookEditor, this.notebookCellList, this.filteredOutlineEntries, this.getCurrentStickyHeight()); if (!this.compareStickyLineMaps(recompute, this.currentStickyLines)) { this.updateContent(recompute); } @@ -232,9 +231,6 @@ export class NotebookStickyScroll extends Disposable { const oldStickyHeight = this.getCurrentStickyHeight(); this.setCurrentStickyLines(newMap); - this.renderedStickyLines = Array.from(newMap.values()) - .filter(value => value.rendered) - .map(value => value.line); // (+) = sticky height increased // (-) = sticky height decreased @@ -324,7 +320,7 @@ export class NotebookStickyScroll extends Disposable { } } -export function computeContent(notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[], renderedStickyHeight: number, renderedLines: NotebookStickyLine[]): Map { +export function computeContent(notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[], renderedStickyHeight: number): Map { // get data about the cell list within viewport ---------------------------------------------------------------------------------------- const editorScrollTop = notebookEditor.scrollTop - renderedStickyHeight; const visibleRange = notebookEditor.visibleRanges[0]; diff --git a/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_1___collapsing_against_equivalent_level_header.0.snap b/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_1___collapsing_against_equivalent_level_header.0.snap deleted file mode 100644 index 1bcf0a58d432c..0000000000000 --- a/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_1___collapsing_against_equivalent_level_header.0.snap +++ /dev/null @@ -1 +0,0 @@ -[ "# header a", "## header aa" ] \ No newline at end of file diff --git a/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_2___collapsing_against_equivalent_level_header.0.snap b/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_2___collapsing_against_equivalent_level_header.0.snap new file mode 100644 index 0000000000000..3b3d5999fd8f6 --- /dev/null +++ b/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test3__should_render_0-_2___collapsing_against_equivalent_level_header.0.snap @@ -0,0 +1 @@ +[ "# header a", "## header aa", "### header aab" ] \ No newline at end of file diff --git a/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test5__should_render_0-_2___scrolltop_halfway_through_cell_2.0.snap b/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test5__should_render_0-_2___scrolltop_halfway_through_cell_2.0.snap index cf5583b0ab9e4..2e13ea43822b9 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test5__should_render_0-_2___scrolltop_halfway_through_cell_2.0.snap +++ b/src/vs/workbench/contrib/notebook/test/browser/__snapshots__/NotebookEditorStickyScroll_test5__should_render_0-_2___scrolltop_halfway_through_cell_2.0.snap @@ -1 +1 @@ -[ "# header a", "## header aa", "### header aaa" ] +[ "# header a", "## header aa", "### header aaa" ] \ No newline at end of file diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts index 7b8ce2f2b94b8..0cb027d1beb7f 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookStickyScroll.test.ts @@ -50,7 +50,7 @@ suite('NotebookEditorStickyScroll', () => { } function nbStickyTestHelper(domNode: HTMLElement, notebookEditor: INotebookEditor, notebookCellList: INotebookCellList, notebookOutlineEntries: OutlineEntry[], disposables: Pick) { - const output = computeContent(notebookEditor, notebookCellList, notebookOutlineEntries, 0, []); + const output = computeContent(notebookEditor, notebookCellList, notebookOutlineEntries, 0); for (const stickyLine of output.values()) { disposables.add(stickyLine.line); } @@ -181,7 +181,7 @@ suite('NotebookEditorStickyScroll', () => { }); }); - test('test3: should render 0->1, collapsing against equivalent level header', async function () { + test('test3: should render 0->2, collapsing against equivalent level header', async function () { await withTestNotebook( [ ['# header a', 'markdown', CellKind.Markup, [], {}], // 0 @@ -222,7 +222,7 @@ suite('NotebookEditorStickyScroll', () => { }); // outdated/improper behavior - test.skip('test4: should render 0, scrolltop halfway through cell 0', async function () { + test('test4: should render 0, scrolltop halfway through cell 0', async function () { await withTestNotebook( [ ['# header a', 'markdown', CellKind.Markup, [], {}], @@ -260,8 +260,7 @@ suite('NotebookEditorStickyScroll', () => { }); }); - // outdated/improper behavior - test.skip('test5: should render 0->2, scrolltop halfway through cell 2', async function () { + test('test5: should render 0->2, scrolltop halfway through cell 2', async function () { await withTestNotebook( [ ['# header a', 'markdown', CellKind.Markup, [], {}], @@ -301,8 +300,7 @@ suite('NotebookEditorStickyScroll', () => { }); }); - // outdated/improper behavior - test.skip('test6: should render 6->7, scrolltop halfway through cell 7', async function () { + test('test6: should render 6->7, scrolltop halfway through cell 7', async function () { await withTestNotebook( [ ['# header a', 'markdown', CellKind.Markup, [], {}], @@ -342,7 +340,6 @@ suite('NotebookEditorStickyScroll', () => { }); }); - // waiting on behavior push to fix this. test('test7: should render 0->1, collapsing against next section', async function () { await withTestNotebook( [ @@ -384,6 +381,4 @@ suite('NotebookEditorStickyScroll', () => { outline.dispose(); }); }); - - });