Skip to content

Remove root level flags used to track cell selection and focus #9882

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

Merged
DonJayamanne merged 2 commits into from
Feb 4, 2020
Merged

Remove root level flags used to track cell selection and focus #9882

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
ed75f46
Do not track individual cell state at root level
DonJayamanne Feb 3, 2020
d6a9463
Fix tests
DonJayamanne Feb 4, 2020
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
4 changes: 1 addition & 3 deletions src/datascience-ui/history-react/redux/reducers/creation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,7 @@ export namespace Creation {
return {
...arg.prevState,
cellVMs: [],
undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs),
selectedCellId: undefined,
focusedCellId: undefined
undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs)
};
}

Expand Down
10 changes: 2 additions & 8 deletions src/datascience-ui/history-react/redux/reducers/execution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,13 @@ export namespace Execution {
const cells = arg.prevState.undoStack[arg.prevState.undoStack.length - 1];
const undoStack = arg.prevState.undoStack.slice(0, arg.prevState.undoStack.length - 1);
const redoStack = Helpers.pushStack(arg.prevState.redoStack, arg.prevState.cellVMs);
const selected = cells.findIndex(c => c.selected);
arg.queueAction(createPostableAction(InteractiveWindowMessages.Undo));
return {
...arg.prevState,
cellVMs: cells,
undoStack: undoStack,
redoStack: redoStack,
skipNextScroll: true,
selectedCellId: selected >= 0 ? cells[selected].cell.id : undefined,
focusedCellId: selected >= 0 && cells[selected].focused ? cells[selected].cell.id : undefined
skipNextScroll: true
};
}

Expand All @@ -46,16 +43,13 @@ export namespace Execution {
const cells = arg.prevState.redoStack[arg.prevState.redoStack.length - 1];
const redoStack = arg.prevState.redoStack.slice(0, arg.prevState.redoStack.length - 1);
const undoStack = Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs);
const selected = cells.findIndex(c => c.selected);
arg.queueAction(createPostableAction(InteractiveWindowMessages.Redo));
return {
...arg.prevState,
cellVMs: cells,
undoStack: undoStack,
redoStack: redoStack,
skipNextScroll: true,
selectedCellId: selected >= 0 ? cells[selected].cell.id : undefined,
focusedCellId: selected >= 0 && cells[selected].focused ? cells[selected].cell.id : undefined
skipNextScroll: true
};
}

Expand Down
25 changes: 23 additions & 2 deletions src/datascience-ui/interactive-common/mainState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ export type IMainState = {
currentExecutionCount: number;
debugging: boolean;
dirty?: boolean;
selectedCellId?: string;
focusedCellId?: string;
isAtBottom: boolean;
newCellId?: string;
loadTotal?: number;
Expand All @@ -75,6 +73,29 @@ export type IMainState = {
kernel: IServerState;
};

/**
* Returns the cell id and index of selected and focused cells.
*/
export function getSelectedAndFocusedInfo(state: IMainState) {
const info: { selectedCellId?: string; selectedCellIndex?: number; focusedCellId?: string; focusedCellIndex?: number } = {};
for (let index = 0; index < state.cellVMs.length; index += 1) {
const cell = state.cellVMs[index];
if (cell.selected) {
Copy link
Member

@IanMatthewHuff IanMatthewHuff Feb 3, 2020

Choose a reason for hiding this comment

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

Focus turns on selected as well. And they should never be different so you could simplify this a bit (no need to check for focuses after finding selected, just check if that cell is also focused). NBD if you don't want to change since this doesn't matter much.

Copy link
Author

@DonJayamanne DonJayamanne Feb 4, 2020

Choose a reason for hiding this comment

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

I agree, however I'd prefer if we had a single bitwise or enum flag instead.
Right now having two flags indicates they are mutually exclusive. This could lead to funky code.
I'd prefer to do that as a separate PR.

Focus turns on selected as well

Though the styles are different in the cell, e.g. when a code cell has focus, the style is not that of a selected cell (not blue) but the striped green.

info.selectedCellId = cell.cell.id;
info.selectedCellIndex = index;
}
if (cell.focused) {
info.focusedCellId = cell.cell.id;
info.focusedCellIndex = index;
}
if (info.selectedCellId && info.focusedCellId) {
break;
}
}

return info;
}

export interface IFont {
size: number;
family: string;
Expand Down
5 changes: 3 additions & 2 deletions src/datascience-ui/interactive-common/redux/reducers/transfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
'use strict';
import { InteractiveWindowMessages } from '../../../../client/datascience/interactive-common/interactiveWindowTypes';
import { CssMessages } from '../../../../client/datascience/messages';
import { extractInputText, IMainState } from '../../mainState';
import { extractInputText, getSelectedAndFocusedInfo, IMainState } from '../../mainState';
import { createPostableAction } from '../postOffice';
import { Helpers } from './helpers';
import { CommonActionType, CommonReducerArg, ICellAction, IEditCellAction, ILinkClickAction, ISendCommandAction, IShowDataViewerAction } from './types';
Expand Down Expand Up @@ -99,7 +99,8 @@ export namespace Transfer {
// We keep this saved here so we don't re-render and we put this code into the input / code data
// when focus is lost
const index = arg.prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.data.cellId);
if (index >= 0 && arg.prevState.focusedCellId === arg.payload.data.cellId) {
const selectionInfo = getSelectedAndFocusedInfo(arg.prevState);
if (index >= 0 && selectionInfo.focusedCellId === arg.payload.data.cellId) {
const newVMs = [...arg.prevState.cellVMs];
const current = arg.prevState.cellVMs[index];
const newCell = {
Expand Down
13 changes: 8 additions & 5 deletions src/datascience-ui/interactive-common/redux/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { InteractiveWindowMessages } from '../../../client/datascience/interacti
import { BaseReduxActionPayload } from '../../../client/datascience/interactive-common/types';
import { CssMessages } from '../../../client/datascience/messages';
import { CellState } from '../../../client/datascience/types';
import { IMainState, ServerStatus } from '../../interactive-common/mainState';
import { getSelectedAndFocusedInfo, IMainState, ServerStatus } from '../../interactive-common/mainState';
import { getLocString } from '../../react-common/locReactSide';
import { PostOffice } from '../../react-common/postOffice';
import { combineReducers, createQueueableActionMiddleware, QueuableAction } from '../../react-common/reduxUtils';
Expand Down Expand Up @@ -73,9 +73,10 @@ function createSendInfoMiddleware(): Redux.Middleware<{}, IStore> {
const afterState = store.getState();

// If cell vm count changed or selected cell changed, send the message
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
if (
prevState.main.cellVMs.length !== afterState.main.cellVMs.length ||
prevState.main.selectedCellId !== afterState.main.selectedCellId ||
getSelectedAndFocusedInfo(prevState.main).selectedCellId !== currentSelection.selectedCellId ||
prevState.main.undoStack.length !== afterState.main.undoStack.length ||
prevState.main.redoStack.length !== afterState.main.redoStack.length
) {
Expand All @@ -84,7 +85,7 @@ function createSendInfoMiddleware(): Redux.Middleware<{}, IStore> {
cellCount: afterState.main.cellVMs.length,
undoCount: afterState.main.undoStack.length,
redoCount: afterState.main.redoStack.length,
selectedCell: afterState.main.selectedCellId
selectedCell: currentSelection.selectedCellId
})
);
}
Expand Down Expand Up @@ -112,12 +113,14 @@ function createTestMiddleware(): Redux.Middleware<{}, IStore> {
};

// Special case for focusing a cell
if (prevState.main.focusedCellId !== afterState.main.focusedCellId && afterState.main.focusedCellId) {
const previousSelection = getSelectedAndFocusedInfo(prevState.main);
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && currentSelection.focusedCellId) {
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
sendMessage(InteractiveWindowMessages.FocusedCellEditor, { cellId: action.payload.cellId });
}
// Special case for unfocusing a cell
if (prevState.main.focusedCellId !== afterState.main.focusedCellId && !afterState.main.focusedCellId) {
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && !currentSelection.focusedCellId) {
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
sendMessage(InteractiveWindowMessages.UnfocusedCellEditor);
}
Expand Down
18 changes: 9 additions & 9 deletions src/datascience-ui/native-editor/nativeEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { concatMultilineStringInput } from '../common';
import { ContentPanel, IContentPanelProps } from '../interactive-common/contentPanel';
import { handleLinkClick } from '../interactive-common/handlers';
import { KernelSelection } from '../interactive-common/kernelSelection';
import { ICellViewModel, IMainState } from '../interactive-common/mainState';
import { getSelectedAndFocusedInfo, ICellViewModel, IMainState } from '../interactive-common/mainState';
import { IMainWithVariables, IStore } from '../interactive-common/redux/store';
import { IVariablePanelProps, VariablePanel } from '../interactive-common/variablePanel';
import { getOSType } from '../react-common/constants';
Expand Down Expand Up @@ -109,7 +109,7 @@ export class NativeEditor extends React.Component<INativeEditorProps> {
// tslint:disable: react-this-binding-issue
// tslint:disable-next-line: max-func-body-length
private renderToolbarPanel() {
const selectedIndex = this.props.cellVMs.findIndex(c => c.cell.id === this.props.selectedCellId);
const selectedInfo = getSelectedAndFocusedInfo(this.props);

const addCell = () => {
this.props.addCell();
Expand All @@ -132,16 +132,16 @@ export class NativeEditor extends React.Component<INativeEditorProps> {
? getLocString('DataScience.collapseVariableExplorerTooltip', 'Hide variables active in jupyter kernel')
: getLocString('DataScience.expandVariableExplorerTooltip', 'Show variables active in jupyter kernel');
const runAbove = () => {
if (this.props.selectedCellId) {
this.props.executeAbove(this.props.selectedCellId);
if (selectedInfo.selectedCellId) {
this.props.executeAbove(selectedInfo.selectedCellId);
this.props.sendCommand(NativeCommandType.RunAbove, 'mouse');
}
};
const runBelow = () => {
if (this.props.selectedCellId) {
if (selectedInfo.selectedCellId && typeof selectedInfo.selectedCellIndex === 'number') {
// tslint:disable-next-line: no-suspicious-comment
// TODO: Is the source going to be up to date during run below?
this.props.executeCellAndBelow(this.props.selectedCellId, concatMultilineStringInput(this.props.cellVMs[selectedIndex].cell.data.source));
this.props.executeCellAndBelow(selectedInfo.selectedCellId, concatMultilineStringInput(this.props.cellVMs[selectedInfo.selectedCellIndex].cell.data.source));
this.props.sendCommand(NativeCommandType.RunBelow, 'mouse');
}
};
Expand All @@ -153,8 +153,8 @@ export class NativeEditor extends React.Component<INativeEditorProps> {
this.props.selectServer();
this.props.sendCommand(NativeCommandType.SelectServer, 'mouse');
};
const canRunAbove = selectedIndex > 0;
const canRunBelow = selectedIndex < this.props.cellVMs.length - 1 && this.props.selectedCellId;
const canRunAbove = (selectedInfo.selectedCellIndex ?? -1) > 0;
const canRunBelow = (selectedInfo.selectedCellIndex ?? -1) < this.props.cellVMs.length - 1 && selectedInfo.selectedCellId;

return (
<div id="toolbar-panel">
Expand Down Expand Up @@ -315,7 +315,7 @@ export class NativeEditor extends React.Component<INativeEditorProps> {
}
case 'z':
case 'Z':
if (this.props.focusedCellId === undefined) {
if (!getSelectedAndFocusedInfo(this.props).focusedCellId) {
if (event.shiftKey && !event.ctrlKey && !event.altKey) {
event.stopPropagation();
this.props.redo();
Expand Down
22 changes: 9 additions & 13 deletions src/datascience-ui/native-editor/redux/reducers/creation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import { ILoadAllCells, InteractiveWindowMessages } from '../../../../client/datascience/interactive-common/interactiveWindowTypes';
import { ICell, IDataScienceExtraSettings } from '../../../../client/datascience/types';
import { createCellVM, createEmptyCell, CursorPos, extractInputText, ICellViewModel, IMainState } from '../../../interactive-common/mainState';
import { createCellVM, createEmptyCell, CursorPos, extractInputText, getSelectedAndFocusedInfo, ICellViewModel, IMainState } from '../../../interactive-common/mainState';
import { createPostableAction } from '../../../interactive-common/redux/postOffice';
import { Helpers } from '../../../interactive-common/redux/reducers/helpers';
import { IAddCellAction, ICellAction } from '../../../interactive-common/redux/reducers/types';
Expand Down Expand Up @@ -113,7 +113,10 @@ export namespace Creation {

export function addNewCell(arg: NativeEditorReducerArg<IAddCellAction>): IMainState {
// Do the same thing that an insertBelow does using the currently selected cell.
return insertBelow({ ...arg, payload: { ...arg.payload, data: { cellId: arg.prevState.selectedCellId, newCellId: arg.payload.data.newCellId } } });
return insertBelow({
...arg,
payload: { ...arg.payload, data: { cellId: getSelectedAndFocusedInfo(arg.prevState).selectedCellId, newCellId: arg.payload.data.newCellId } }
});
}

export function startCell(arg: NativeEditorReducerArg<ICell>): IMainState {
Expand Down Expand Up @@ -152,9 +155,7 @@ export namespace Creation {
return {
...arg.prevState,
cellVMs: [newVM],
undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs),
selectedCellId: undefined,
focusedCellId: undefined
undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs)
};
}

Expand Down Expand Up @@ -194,23 +195,18 @@ export namespace Creation {
arg.queueAction(createPostableAction(InteractiveWindowMessages.RemoveCell, { id: arg.payload.data.cellId }));

// Recompute select/focus if this item has either
let newSelection = arg.prevState.selectedCellId;
let newFocused = arg.prevState.focusedCellId;
const previousSelection = getSelectedAndFocusedInfo(arg.prevState);
const newVMs = [...arg.prevState.cellVMs.filter(c => c.cell.id !== arg.payload.data.cellId)];
const nextOrPrev = index === arg.prevState.cellVMs.length - 1 ? index - 1 : index;
if (arg.prevState.selectedCellId === arg.payload.data.cellId || arg.prevState.focusedCellId === arg.payload.data.cellId) {
if (previousSelection.selectedCellId === arg.payload.data.cellId || previousSelection.focusedCellId === arg.payload.data.cellId) {
if (nextOrPrev >= 0) {
newVMs[nextOrPrev] = { ...newVMs[nextOrPrev], selected: true, focused: arg.prevState.focusedCellId === arg.payload.data.cellId };
newSelection = newVMs[nextOrPrev].cell.id;
newFocused = newVMs[nextOrPrev].focused ? newVMs[nextOrPrev].cell.id : undefined;
newVMs[nextOrPrev] = { ...newVMs[nextOrPrev], selected: true, focused: previousSelection.focusedCellId === arg.payload.data.cellId };
}
}

return {
...arg.prevState,
cellVMs: newVMs,
selectedCellId: newSelection,
focusedCellId: newFocused,
undoStack: Helpers.pushStack(arg.prevState.undoStack, arg.prevState.cellVMs),
skipNextScroll: true
};
Expand Down
Loading