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.

Sign up for GitHub

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

Jump to bottom

adjust focused cell anchor behavior when resizing cells #195309

Merged
merged 2 commits into from
Oct 10, 2023
Merged

adjust focused cell anchor behavior when resizing cells #195309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
538d93e
auto anchoring to focused cell
amunger Oct 10, 2023
eec21df
unit tests for new behavior
amunger Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,14 +1207,20 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
const focused = this.getFocus();
const focus = focused.length ? focused[0] : null;

const anchorFocusedSetting = this.configurationService.getValue(NotebookSetting.anchorToFocusedCell);
const allowScrolling = this.configurationService.getValue(NotebookSetting.scrollToRevealCell) !== 'none';
const scrollHeuristic = allowScrolling && anchorFocusedSetting === 'auto' && this.view.elementTop(index) < this.view.getScrollTop();
if (focused && (anchorFocusedSetting === 'on' || scrollHeuristic)) {
this.view.updateElementHeight(index, size, focus);
// If the cell is growing, we should favor anchoring to the focused cell
if (focused) {
Copy link
Member

Choose a reason for hiding this comment

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

@amunger should we check focus other than focused: number[]?

const cellEditorIsFocused = this.view.element(focused[0]).focusMode === CellFocusMode.Editor;
const anchorFocusedSetting = this.configurationService.getValue(NotebookSetting.anchorToFocusedCell);
const growing = this.view.elementHeight(index) < size;
const allowScrolling = this.configurationService.getValue(NotebookSetting.scrollToRevealCell) !== 'none';
const autoAnchor = allowScrolling && growing && anchorFocusedSetting !== 'off';

if (cellEditorIsFocused || autoAnchor || anchorFocusedSetting === 'on') {
return this.view.updateElementHeight(index, size, focus);
}
}

this.view.updateElementHeight(index, size, null);
return this.view.updateElementHeight(index, size, null);
}

// override
Expand Down
112 changes: 106 additions & 6 deletions src/vs/workbench/contrib/notebook/test/browser/notebookCellList.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ suite('NotebookCellList', () => {

ensureNoDisposablesAreLeakedInTestSuite();

let config: TestConfigurationService;
setup(() => {
testDisposables = new DisposableStore();
instantiationService = setupInstantiationService(testDisposables);
const config = new TestConfigurationService({
config = new TestConfigurationService({
[NotebookSetting.anchorToFocusedCell]: 'auto'
});

instantiationService.stub(IConfigurationService, config);
});

Expand Down Expand Up @@ -201,7 +203,7 @@ suite('NotebookCellList', () => {
});
});

test('updateElementHeight with anchor #121723', async function () {
test('updateElementHeight with anchor', async function () {
await withTestNotebook(
[
['# header a', 'markdown', CellKind.Markup, [], {}],
Expand Down Expand Up @@ -231,6 +233,7 @@ suite('NotebookCellList', () => {
assert.deepStrictEqual(cellList.getViewScrollBottom(), 210);

// scroll to 5
cellList.updateElementHeight2(viewModel.cellAt(0)!, 50);
cellList.scrollTop = 5;
assert.deepStrictEqual(cellList.scrollTop, 5);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 215);
Expand All @@ -239,13 +242,14 @@ suite('NotebookCellList', () => {
cellList.updateElementHeight2(viewModel.cellAt(0)!, 100);
assert.deepStrictEqual(cellList.scrollHeight, 400);

// the first cell grows, but it's partially visible, so we won't push down the focused cell
// the first cell grows, but we anchor to the focused cell, so the notebook will scroll down
assert.deepStrictEqual(cellList.scrollTop, 55);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 265);

// We don't anchor to the focused cell when cells shrink
cellList.updateElementHeight2(viewModel.cellAt(0)!, 50);
assert.deepStrictEqual(cellList.scrollTop, 5);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 215);
assert.deepStrictEqual(cellList.scrollTop, 55);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 265);

// focus won't be visible after cell 0 grow to 250, so let's try to keep the focused cell visible
cellList.updateElementHeight2(viewModel.cellAt(0)!, 250);
Expand All @@ -254,7 +258,103 @@ suite('NotebookCellList', () => {
});
});

test('updateElementHeight with anchor #121723: focus element out of viewport', async function () {
test('updateElementHeight with no scrolling', async function () {
config.setUserConfiguration(NotebookSetting.scrollToRevealCell, 'none');
await withTestNotebook(
[
['# header a', 'markdown', CellKind.Markup, [], {}],
['var b = 1;', 'javascript', CellKind.Code, [], {}],
['# header b', 'markdown', CellKind.Markup, [], {}],
['var b = 2;', 'javascript', CellKind.Code, [], {}],
['# header c', 'markdown', CellKind.Markup, [], {}]
],
async (editor, viewModel, disposables) => {
viewModel.restoreEditorViewState({
editingCells: [false, false, false, false, false],
editorViewStates: [null, null, null, null, null],
cellTotalHeights: [50, 100, 50, 100, 50],
cellLineNumberStates: {},
collapsedInputCells: {},
collapsedOutputCells: {},
});
const cellList = createNotebookCellList(instantiationService, disposables);
cellList.attachViewModel(viewModel);

// render height 210, it can render 3 full cells and 1 partial cell
cellList.layout(210, 100);

// init scrollTop and scrollBottom
assert.deepStrictEqual(cellList.scrollTop, 0);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 210);

// scroll to 5
cellList.updateElementHeight2(viewModel.cellAt(0)!, 50);
cellList.scrollTop = 5;
assert.deepStrictEqual(cellList.scrollTop, 5);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 215);

cellList.setFocus([1]);
cellList.updateElementHeight2(viewModel.cellAt(0)!, 100);
assert.deepStrictEqual(cellList.scrollHeight, 400);

// Any change in cell size should not affect the scroll height with scrollToReveal set to none
assert.deepStrictEqual(cellList.scrollTop, 5);

cellList.updateElementHeight2(viewModel.cellAt(0)!, 50);
assert.deepStrictEqual(cellList.scrollTop, 5);

cellList.updateElementHeight2(viewModel.cellAt(0)!, 250);
assert.deepStrictEqual(cellList.scrollTop, 5);
});
});

test('updateElementHeight with no scroll setting and cell editor focused', async function () {
config.setUserConfiguration(NotebookSetting.scrollToRevealCell, 'none');
await withTestNotebook(
[
['# header a', 'markdown', CellKind.Markup, [], {}],
['var b = 1;', 'javascript', CellKind.Code, [], {}],
['# header b', 'markdown', CellKind.Markup, [], {}],
['var b = 2;', 'javascript', CellKind.Code, [], {}],
['# header c', 'markdown', CellKind.Markup, [], {}]
],
async (editor, viewModel, disposables) => {
viewModel.restoreEditorViewState({
editingCells: [false, false, false, false, false],
editorViewStates: [null, null, null, null, null],
cellTotalHeights: [50, 100, 50, 100, 50],
cellLineNumberStates: {},
collapsedInputCells: {},
collapsedOutputCells: {},
});
const cellList = createNotebookCellList(instantiationService, disposables);
cellList.attachViewModel(viewModel);

// render height 210, it can render 3 full cells and 1 partial cell
cellList.layout(210, 100);

// init scrollTop and scrollBottom
assert.deepStrictEqual(cellList.scrollTop, 0);
assert.deepStrictEqual(cellList.getViewScrollBottom(), 210);

cellList.setFocus([1]);

editor.focusNotebookCell(cellList.viewModel?.cellAt(1)!, 'editor');
cellList.updateElementHeight2(viewModel.cellAt(0)!, 100);
assert.deepStrictEqual(cellList.scrollHeight, 400);

// We have the cell editor focused, so we should anchor to that cell
assert.deepStrictEqual(cellList.scrollTop, 50);

cellList.updateElementHeight2(viewModel.cellAt(0)!, 50);
assert.deepStrictEqual(cellList.scrollTop, 0);

cellList.updateElementHeight2(viewModel.cellAt(0)!, 250);
assert.deepStrictEqual(cellList.scrollTop, 250 + 100 - cellList.renderHeight);
});
});

test('updateElementHeight with focused element out of viewport', async function () {
await withTestNotebook(
[
['# header a', 'markdown', CellKind.Markup, [], {}],
Expand Down
8 changes: 6 additions & 2 deletions src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { UndoRedoService } from 'vs/platform/undoRedo/common/undoRedoService';
import { IWorkspaceTrustRequestService } from 'vs/platform/workspace/common/workspaceTrust';
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
import { EditorModel } from 'vs/workbench/common/editor/editorModel';
import { CellFindMatchWithIndex, IActiveNotebookEditorDelegate, IBaseCellEditorOptions, ICellViewModel, INotebookEditorDelegate } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
import { CellFindMatchWithIndex, CellFocusMode, IActiveNotebookEditorDelegate, IBaseCellEditorOptions, ICellViewModel, INotebookEditorDelegate } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
import { NotebookCellStateChangedEvent, NotebookLayoutInfo } from 'vs/workbench/contrib/notebook/browser/notebookViewEvents';
import { NotebookCellStatusBarService } from 'vs/workbench/contrib/notebook/browser/services/notebookCellStatusBarServiceImpl';
import { ListViewInfoAccessor, NotebookCellList } from 'vs/workbench/contrib/notebook/browser/view/notebookCellList';
Expand Down Expand Up @@ -276,7 +276,11 @@ function _createTestNotebookEditor(instantiationService: TestInstantiationServic
override async revealRangeInCenterIfOutsideViewportAsync() { }
override async layoutNotebookCell() { }
override async removeInset() { }
override async focusNotebookCell() { }
override async focusNotebookCell(cell: ICellViewModel, focusItem: 'editor' | 'container' | 'output') {
cell.focusMode = focusItem === 'editor' ? CellFocusMode.Editor
: focusItem === 'output' ? CellFocusMode.Output
: CellFocusMode.Container;
}
override cellAt(index: number) { return viewModel.cellAt(index)!; }
override getCellIndex(cell: ICellViewModel) { return viewModel.getCellIndex(cell); }
override getCellsInRange(range?: ICellRange) { return viewModel.getCellsInRange(range); }
Expand Down