Skip to content

Commit d1dbd72

Browse files
roblourensroblourens
committed
Revamp cursor navigation to do the right type of cell reveal when moving to/from markdown cells
Fix microsoft#147527
1 parent 432e18c commit d1dbd72

File tree

5 files changed

+44
-71
lines changed

5 files changed

+44
-71
lines changed

‎src/vs/workbench/contrib/notebook/browser/contrib/navigation/arrow.ts

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation
1414
import { KeybindingWeight } from 'vs/platform/keybinding/common/keybindingsRegistry';
1515
import { Registry } from 'vs/platform/registry/common/platform';
1616
import { INotebookActionContext, INotebookCellActionContext, NotebookAction, NotebookCellAction, NOTEBOOK_EDITOR_WIDGET_ACTION_WEIGHT } from 'vs/workbench/contrib/notebook/browser/controller/coreActions';
17-
import { NOTEBOOK_CELL_HAS_OUTPUTS, NOTEBOOK_EDITOR_FOCUSED, NOTEBOOK_OUTPUT_FOCUSED } from 'vs/workbench/contrib/notebook/common/notebookContextKeys';
17+
import { NOTEBOOK_CELL_HAS_OUTPUTS, NOTEBOOK_CELL_MARKDOWN_EDIT_MODE, NOTEBOOK_CELL_TYPE, NOTEBOOK_CURSOR_NAVIGATION_MODE, NOTEBOOK_EDITOR_FOCUSED, NOTEBOOK_OUTPUT_FOCUSED } from 'vs/workbench/contrib/notebook/common/notebookContextKeys';
1818
import { CellEditState } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
1919
import { CellKind, NOTEBOOK_EDITOR_CURSOR_BOUNDARY } from 'vs/workbench/contrib/notebook/common/notebookCommon';
2020
import { EditorExtensionsRegistry } from 'vs/editor/browser/editorExtensions';
@@ -41,14 +41,23 @@ registerAction2(class FocusNextCellAction extends NotebookCellAction {
4141
{
4242
when: ContextKeyExpr.and(
4343
NOTEBOOK_EDITOR_FOCUSED,
44-
ContextKeyExpr.has(InputFocusedContextKey),
45-
EditorContextKeys.editorTextFocus,
46-
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('top'),
47-
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('none'),
48-
ContextKeyExpr.equals('config.notebook.navigation.allowNavigateToSurroundingCells', true)
44+
ContextKeyExpr.equals('config.notebook.navigation.allowNavigateToSurroundingCells', true),
45+
ContextKeyExpr.or(
46+
ContextKeyExpr.and(
47+
ContextKeyExpr.has(InputFocusedContextKey),
48+
EditorContextKeys.editorTextFocus,
49+
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('top'),
50+
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('none'),
51+
),
52+
ContextKeyExpr.and(
53+
NOTEBOOK_CELL_TYPE.isEqualTo('markup'),
54+
NOTEBOOK_CELL_MARKDOWN_EDIT_MODE.isEqualTo(false),
55+
NOTEBOOK_CURSOR_NAVIGATION_MODE
56+
)
57+
)
4958
),
5059
primary: KeyCode.DownArrow,
51-
weight: NOTEBOOK_EDITOR_WIDGET_ACTION_WEIGHT
60+
weight: KeybindingWeight.WorkbenchContrib,
5261
},
5362
{
5463
when: ContextKeyExpr.and(NOTEBOOK_EDITOR_FOCUSED, NOTEBOOK_OUTPUT_FOCUSED),
@@ -77,7 +86,6 @@ registerAction2(class FocusNextCellAction extends NotebookCellAction {
7786
const newCell = editor.cellAt(idx + 1);
7887
const newFocusMode = newCell.cellKind === CellKind.Markup && newCell.getEditState() === CellEditState.Preview ? 'container' : 'editor';
7988
await editor.focusNotebookCell(newCell, newFocusMode, { focusEditorLine: 1 });
80-
editor.cursorNavigationMode = true;
8189
}
8290
});
8391

@@ -90,14 +98,23 @@ registerAction2(class FocusPreviousCellAction extends NotebookCellAction {
9098
keybinding: {
9199
when: ContextKeyExpr.and(
92100
NOTEBOOK_EDITOR_FOCUSED,
93-
ContextKeyExpr.has(InputFocusedContextKey),
94-
EditorContextKeys.editorTextFocus,
95-
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('bottom'),
96-
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('none'),
97-
ContextKeyExpr.equals('config.notebook.navigation.allowNavigateToSurroundingCells', true)
101+
ContextKeyExpr.equals('config.notebook.navigation.allowNavigateToSurroundingCells', true),
102+
ContextKeyExpr.or(
103+
ContextKeyExpr.and(
104+
ContextKeyExpr.has(InputFocusedContextKey),
105+
EditorContextKeys.editorTextFocus,
106+
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('bottom'),
107+
NOTEBOOK_EDITOR_CURSOR_BOUNDARY.notEqualsTo('none'),
108+
),
109+
ContextKeyExpr.and(
110+
NOTEBOOK_CELL_TYPE.isEqualTo('markup'),
111+
NOTEBOOK_CELL_MARKDOWN_EDIT_MODE.isEqualTo(false),
112+
NOTEBOOK_CURSOR_NAVIGATION_MODE
113+
)
114+
)
98115
),
99116
primary: KeyCode.UpArrow,
100-
weight: NOTEBOOK_EDITOR_WIDGET_ACTION_WEIGHT
117+
weight: KeybindingWeight.WorkbenchContrib,
101118
},
102119
});
103120
}
@@ -119,7 +136,6 @@ registerAction2(class FocusPreviousCellAction extends NotebookCellAction {
119136
const newCell = editor.cellAt(idx - 1);
120137
const newFocusMode = newCell.cellKind === CellKind.Markup && newCell.getEditState() === CellEditState.Preview ? 'container' : 'editor';
121138
await editor.focusNotebookCell(newCell, newFocusMode, { focusEditorLine: newCell.textBuffer.getLineCount() });
122-
editor.cursorNavigationMode = true;
123139
}
124140
});
125141

‎src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,8 +414,6 @@ export interface INotebookEditor {
414414
setFocus(focus: ICellRange): void;
415415
getId(): string;
416416

417-
cursorNavigationMode: boolean;
418-
419417
_getViewModel(): INotebookViewModel | undefined;
420418
hasModel(): this is IActiveNotebookEditor;
421419
dispose(): void;

‎src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ import { NotebookOverviewRuler } from 'vs/workbench/contrib/notebook/browser/vie
7373
import { ListTopCellToolbar } from 'vs/workbench/contrib/notebook/browser/viewParts/notebookTopCellToolbar';
7474
import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/notebookTextModel';
7575
import { CellKind, INotebookSearchOptions, SelectionStateType } from 'vs/workbench/contrib/notebook/common/notebookCommon';
76-
import { NOTEBOOK_EDITOR_EDITABLE, NOTEBOOK_EDITOR_FOCUSED, NOTEBOOK_OUTPUT_FOCUSED } from 'vs/workbench/contrib/notebook/common/notebookContextKeys';
76+
import { NOTEBOOK_CURSOR_NAVIGATION_MODE, NOTEBOOK_EDITOR_EDITABLE, NOTEBOOK_EDITOR_FOCUSED, NOTEBOOK_OUTPUT_FOCUSED } from 'vs/workbench/contrib/notebook/common/notebookContextKeys';
7777
import { INotebookExecutionService } from 'vs/workbench/contrib/notebook/common/notebookExecutionService';
7878
import { INotebookExecutionStateService } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService';
7979
import { INotebookKernelService } from 'vs/workbench/contrib/notebook/common/notebookKernelService';
@@ -272,6 +272,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
272272
private readonly _editorFocus: IContextKey<boolean>;
273273
private readonly _outputFocus: IContextKey<boolean>;
274274
private readonly _editorEditable: IContextKey<boolean>;
275+
private readonly _cursorNavMode: IContextKey<boolean>;
275276
protected readonly _contributions = new Map<string, INotebookEditorContribution>();
276277
private _scrollBeyondLastLine: boolean;
277278
private readonly _insetModifyQueueByOutputId = new SequencerByKey<string>();
@@ -313,16 +314,6 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
313314
return this._renderedEditors.get(focused);
314315
}
315316

316-
private _cursorNavigationMode: boolean = false;
317-
get cursorNavigationMode(): boolean {
318-
return this._cursorNavigationMode;
319-
}
320-
321-
set cursorNavigationMode(v: boolean) {
322-
this._cursorNavigationMode = v;
323-
}
324-
325-
326317
get visibleRanges() {
327318
return this._list.visibleRanges || [];
328319
}
@@ -443,6 +434,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
443434
this._editorFocus = NOTEBOOK_EDITOR_FOCUSED.bindTo(this.scopedContextKeyService);
444435
this._outputFocus = NOTEBOOK_OUTPUT_FOCUSED.bindTo(this.scopedContextKeyService);
445436
this._editorEditable = NOTEBOOK_EDITOR_EDITABLE.bindTo(this.scopedContextKeyService);
437+
this._cursorNavMode = NOTEBOOK_CURSOR_NAVIGATION_MODE.bindTo(this.scopedContextKeyService);
446438

447439
this._editorEditable.set(!creationOptions.isReadOnly);
448440

@@ -962,10 +954,6 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
962954
return nls.localize('notebookTreeAriaLabel', "Notebook");
963955
}
964956
},
965-
focusNextPreviousDelegate: {
966-
onFocusNext: (applyFocusNext: () => void) => this._updateForCursorNavigationMode(applyFocusNext),
967-
onFocusPrevious: (applyFocusPrevious: () => void) => this._updateForCursorNavigationMode(applyFocusPrevious),
968-
}
969957
},
970958
);
971959
this._dndController.setList(this._list);
@@ -1013,7 +1001,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
10131001
this._register(this._list.onDidChangeFocus(_e => {
10141002
this._onDidChangeActiveEditor.fire(this);
10151003
this._onDidChangeActiveCell.fire();
1016-
this._cursorNavigationMode = false;
1004+
this._cursorNavMode.set(false);
10171005
}));
10181006

10191007
this._register(this._list.onContextMenu(e => {
@@ -1077,23 +1065,6 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
10771065
}));
10781066
}
10791067

1080-
private async _updateForCursorNavigationMode(applyFocusChange: () => void): Promise<void> {
1081-
if (this._cursorNavigationMode) {
1082-
// Will fire onDidChangeFocus, resetting the state to Container
1083-
applyFocusChange();
1084-
1085-
const newFocusedCell = this._list.getFocusedElements()[0];
1086-
if (newFocusedCell.cellKind === CellKind.Code || newFocusedCell.getEditState() === CellEditState.Editing) {
1087-
await this.focusNotebookCell(newFocusedCell, 'editor');
1088-
} else {
1089-
// Reset to "Editor", the state has not been consumed
1090-
this._cursorNavigationMode = true;
1091-
}
1092-
} else {
1093-
applyFocusChange();
1094-
}
1095-
}
1096-
10971068
getDomNode() {
10981069
return this._overlayContainer;
10991070
}
@@ -2353,6 +2324,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
23532324
cell.focusMode = CellFocusMode.Editor;
23542325
if (!options?.skipReveal) {
23552326
if (typeof options?.focusEditorLine === 'number') {
2327+
this._cursorNavMode.set(true);
23562328
await this.revealLineInViewAsync(cell, options.focusEditorLine);
23572329
const editor = this._renderedEditors.get(cell)!;
23582330
const focusEditorLine = options.focusEditorLine!;
@@ -2404,7 +2376,12 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD
24042376
this.focusElement(cell);
24052377
this._cellFocusAria(cell, focusItem);
24062378
if (!options?.skipReveal) {
2407-
this.revealInCenterIfOutsideViewport(cell);
2379+
if (typeof options?.focusEditorLine === 'number') {
2380+
this._cursorNavMode.set(true);
2381+
this.revealInView(cell);
2382+
} else {
2383+
this.revealInCenterIfOutsideViewport(cell);
2384+
}
24082385
}
24092386
this._list.focusView();
24102387
}

‎src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,6 @@ export interface IFocusNextPreviousDelegate {
7373
onFocusPrevious(applyFocusPrevious: () => void): void;
7474
}
7575

76-
export interface INotebookCellListOptions extends IWorkbenchListOptions<CellViewModel> {
77-
focusNextPreviousDelegate: IFocusNextPreviousDelegate;
78-
}
79-
8076
export const NOTEBOOK_WEBVIEW_BOUNDARY = 5000;
8177

8278
function validateWebviewBoundary(element: HTMLElement) {
@@ -141,8 +137,6 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
141137

142138
private _isInLayout: boolean = false;
143139

144-
private readonly _focusNextPreviousDelegate: IFocusNextPreviousDelegate;
145-
146140
private readonly _viewContext: ViewContext;
147141

148142
private _webviewElement: FastDomNode<HTMLElement> | null = null;
@@ -159,7 +153,7 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
159153
delegate: IListVirtualDelegate<CellViewModel>,
160154
renderers: IListRenderer<CellViewModel, BaseCellRenderTemplate>[],
161155
contextKeyService: IContextKeyService,
162-
options: INotebookCellListOptions,
156+
options: IWorkbenchListOptions<CellViewModel>,
163157
@IListService listService: IListService,
164158
@IThemeService themeService: IThemeService,
165159
@IConfigurationService configurationService: IConfigurationService,
@@ -168,7 +162,6 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
168162
super(listUser, container, delegate, renderers, options, contextKeyService, listService, themeService, configurationService, keybindingService);
169163
NOTEBOOK_CELL_LIST_FOCUSED.bindTo(this.contextKeyService).set(true);
170164
this._viewContext = viewContext;
171-
this._focusNextPreviousDelegate = options.focusNextPreviousDelegate;
172165
this._previousFocusedElements = this.getFocusedElements();
173166
this._localDisposableStore.add(this.onDidChangeFocus((e) => {
174167
this._previousFocusedElements.forEach(element => {
@@ -685,18 +678,6 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
685678
this.setSelection(indices);
686679
}
687680

688-
override focusNext(n: number | undefined, loop: boolean | undefined, browserEvent?: UIEvent, filter?: (element: CellViewModel) => boolean): void {
689-
this._focusNextPreviousDelegate.onFocusNext(() => {
690-
super.focusNext(n, loop, browserEvent, filter);
691-
});
692-
}
693-
694-
override focusPrevious(n: number | undefined, loop: boolean | undefined, browserEvent?: UIEvent, filter?: (element: CellViewModel) => boolean): void {
695-
this._focusNextPreviousDelegate.onFocusPrevious(() => {
696-
super.focusPrevious(n, loop, browserEvent, filter);
697-
});
698-
}
699-
700681
override setFocus(indexes: number[], browserEvent?: UIEvent, ignoreTextModelUpdate?: boolean): void {
701682
if (ignoreTextModelUpdate) {
702683
super.setFocus(indexes, browserEvent);

‎src/vs/workbench/contrib/notebook/common/notebookContextKeys.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export const NOTEBOOK_HAS_RUNNING_CELL = new RawContextKey<boolean>('notebookHas
2323
export const NOTEBOOK_USE_CONSOLIDATED_OUTPUT_BUTTON = new RawContextKey<boolean>('notebookUseConsolidatedOutputButton', false);
2424
export const NOTEBOOK_BREAKPOINT_MARGIN_ACTIVE = new RawContextKey<boolean>('notebookBreakpointMargin', false);
2525
export const NOTEBOOK_CELL_TOOLBAR_LOCATION = new RawContextKey<'left' | 'right' | 'hidden'>('notebookCellToolbarLocation', 'left');
26+
export const NOTEBOOK_CURSOR_NAVIGATION_MODE = new RawContextKey<boolean>('notebookCursorNavigationMode', false);
2627

2728
// Cell keys
2829
export const NOTEBOOK_VIEW_TYPE = new RawContextKey<string>('notebookType', undefined);

0 commit comments

Comments
 (0)