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

Fix issue with 'undo' not updating model #4298

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Fix issue with 'undo' not updating model #4298

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9531720
Fix model when undoing
rchiodo Jan 5, 2021
5791110
Fix test to use correct editor
rchiodo Jan 5, 2021
abc4978
Fix linter
rchiodo Jan 5, 2021
2be16d1
Review feedback and fix undo/redo for interactive command
rchiodo Jan 5, 2021
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
1 change: 1 addition & 0 deletions news/2 Fixes/4058.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix Z (and CTRL+Z when using custom editor support) to update data model so that save works.
4 changes: 2 additions & 2 deletions src/client/datascience/interactive-common/interactiveWindowTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,8 +609,8 @@ export class IInteractiveWindowMapping {
public [InteractiveWindowMessages.GetAllCellCode]: IResponse;
public [InteractiveWindowMessages.ReturnAllCellCode]: IReturnAllCodeResponse;
public [InteractiveWindowMessages.DeleteAllCells]: IAddCellAction;
public [InteractiveWindowMessages.Undo]: never | undefined;
public [InteractiveWindowMessages.Redo]: never | undefined;
public [InteractiveWindowMessages.Undo]: ICell[];
public [InteractiveWindowMessages.Redo]: ICell[];
public [InteractiveWindowMessages.ExpandAll]: never | undefined;
public [InteractiveWindowMessages.CollapseAll]: never | undefined;
public [InteractiveWindowMessages.StartProgress]: never | undefined;
Expand Down
2 changes: 2 additions & 0 deletions src/client/datascience/interactive-common/synchronization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ const messageWithMessageTypes: MessageMapping<IInteractiveWindowMapping> & Messa
[CommonActionType.MOVE_CELL_DOWN]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare,
[CommonActionType.MOVE_CELL_UP]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare,
[CommonActionType.OPEN_SETTINGS]: MessageType.other,
[CommonActionType.REDO]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare,
[CommonActionType.RESTART_KERNEL]: MessageType.other,
[CommonActionType.RUN_BY_LINE]: MessageType.other,
[CommonActionType.SAVE]: MessageType.other,
Expand All @@ -95,6 +96,7 @@ const messageWithMessageTypes: MessageMapping<IInteractiveWindowMapping> & Messa
[CommonActionType.TOGGLE_OUTPUT]: MessageType.syncWithLiveShare,
[CommonActionType.TOGGLE_VARIABLE_EXPLORER]: MessageType.syncWithLiveShare,
[CommonActionType.SET_VARIABLE_EXPLORER_HEIGHT]: MessageType.other,
[CommonActionType.UNDO]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare,
[CommonActionType.UNFOCUS_CELL]: MessageType.syncWithLiveShare,
[CommonActionType.UNMOUNT]: MessageType.other,
[CommonActionType.PostOutgoingMessage]: MessageType.other,
Expand Down
16 changes: 16 additions & 0 deletions src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { KernelSelector } from '../jupyter/kernels/kernelSelector';
import { NativeEditorNotebookModel } from '../notebookStorage/notebookModel';
import { INotebookStorageProvider } from '../notebookStorage/notebookStorageProvider';
import {
ICell,
ICodeCssGenerator,
IDataScienceErrorHandler,
IInteractiveWindowListener,
Expand Down Expand Up @@ -157,6 +158,21 @@ export class NativeEditorOldWebView extends NativeEditor {
});
}

// tslint:disable-next-line: no-any
public onMessage(message: string, payload: any) {
super.onMessage(message, payload);
switch (message) {
case InteractiveWindowMessages.Undo:
case InteractiveWindowMessages.Redo:
// Payload should be the new cells. Just replace all cells
this.model?.replaceCells(payload as ICell[], true);
this.setDirty().ignoreErrors();
break;
default:
break;
}
}

protected async close(): Promise<void> {
// Ask user if they want to save. It seems hotExit has no bearing on
// whether or not we should ask
Expand Down
5 changes: 4 additions & 1 deletion src/client/datascience/notebookStorage/notebookModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,11 @@ export class NativeEditorNotebookModel extends BaseNotebookModel {
public update(change: NotebookModelChange): void {
this.handleModelChange(change);
}
public replaceCells(cells: ICell[]) {
public replaceCells(cells: ICell[], markDirty?: boolean) {
this._cells = cells;
if (markDirty) {
this.changeCount += 1;
}
}

public async applyEdits(edits: readonly NotebookModelChange[]): Promise<void> {
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/history-react/redux/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ export const actionCreators = {
deleteAllCells: (): CommonAction => createIncomingAction(InteractiveWindowMessages.DeleteAllCells),
deleteCell: (cellId: string): CommonAction<ICellAction> =>
createIncomingActionWithPayload(CommonActionType.DELETE_CELL, { cellId }),
undo: (): CommonAction => createIncomingAction(InteractiveWindowMessages.Undo),
redo: (): CommonAction => createIncomingAction(InteractiveWindowMessages.Redo),
undo: (): CommonAction => createIncomingAction(CommonActionType.UNDO),
redo: (): CommonAction => createIncomingAction(CommonActionType.REDO),
linkClick: (href: string): CommonAction<ILinkClickAction> =>
createIncomingActionWithPayload(CommonActionType.LINK_CLICK, { href }),
showPlot: (imageHtml: string) => createIncomingActionWithPayload(InteractiveWindowMessages.ShowPlot, imageHtml),
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/history-react/redux/reducers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ export const reducerMap: Partial<IInteractiveActionMapping> = {
[CommonActionType.LOAD_IPYWIDGET_CLASS_FAILURE]: CommonEffects.handleLoadIPyWidgetClassFailure,
[CommonActionType.IPYWIDGET_WIDGET_VERSION_NOT_SUPPORTED]: CommonEffects.notifyAboutUnsupportedWidgetVersions,
[CommonActionType.IPYWIDGET_RENDER_FAILURE]: CommonEffects.handleIPyWidgetRenderFailure,
[CommonActionType.UNDO]: Execution.undo,
[CommonActionType.REDO]: Execution.redo,

// Messages from the webview (some are ignored)
[InteractiveWindowMessages.Undo]: Execution.undo,
[InteractiveWindowMessages.Redo]: Execution.redo,
[InteractiveWindowMessages.StartCell]: Creation.startCell,
[InteractiveWindowMessages.FinishCell]: Creation.finishCell,
[InteractiveWindowMessages.UpdateCellWithExecutionResults]: Creation.updateCell,
Expand Down
4 changes: 4 additions & 0 deletions src/datascience-ui/interactive-common/redux/reducers/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export enum CommonActionType {
MOVE_CELL_UP = 'action.move_cell_up',
OPEN_SETTINGS = 'action.open_settings',
PostOutgoingMessage = 'action.postOutgoingMessage',
REDO = 'action.redo',
REFRESH_VARIABLES = 'action.refresh_variables',
RESTART_KERNEL = 'action.restart_kernel_action',
RUN_BY_LINE = 'action.run_by_line',
Expand All @@ -82,6 +83,7 @@ export enum CommonActionType {
TOGGLE_LINE_NUMBERS = 'action.toggle_line_numbers',
TOGGLE_OUTPUT = 'action.toggle_output',
TOGGLE_VARIABLE_EXPLORER = 'action.toggle_variable_explorer',
UNDO = 'action.undo',
UNFOCUS_CELL = 'action.unfocus_cell',
UNMOUNT = 'action.unmount',
VARIABLE_VIEW_LOADED = 'action.variable_view_loaded'
Expand Down Expand Up @@ -148,6 +150,8 @@ export type CommonActionTypeMapping = {
[CommonActionType.CONTINUE]: ICellAction;
[CommonActionType.RUN_BY_LINE]: ICellAction;
[CommonActionType.VARIABLE_VIEW_LOADED]: never | undefined;
[CommonActionType.UNDO]: never | undefined;
[CommonActionType.REDO]: never | undefined;
};

export interface IShowDataViewerAction extends IShowDataViewer {}
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/native-editor/redux/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ export const actionCreators = {
createIncomingActionWithPayload(CommonActionType.TOGGLE_OUTPUT, { cellId }),
deleteCell: (cellId: string): CommonAction<ICellAction> =>
createIncomingActionWithPayload(CommonActionType.DELETE_CELL, { cellId }),
undo: (): CommonAction => createIncomingAction(InteractiveWindowMessages.Undo),
redo: (): CommonAction => createIncomingAction(InteractiveWindowMessages.Redo),
undo: (): CommonAction => createIncomingAction(CommonActionType.UNDO),
redo: (): CommonAction => createIncomingAction(CommonActionType.REDO),
arrowUp: (cellId: string, code: string): CommonAction<ICodeAction> =>
createIncomingActionWithPayload(CommonActionType.ARROW_UP, { cellId, code }),
arrowDown: (cellId: string, code: string): CommonAction<ICodeAction> =>
Expand Down
6 changes: 3 additions & 3 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 @@ -100,8 +100,8 @@ export namespace Creation {
if (position >= 0) {
newList.splice(position, 0, newVM);
} else {
newList.splice(0, 0, newVM);
position = 0;
newList.splice(newList.length, 0, newVM);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the problem in the custom editor with undo. When the 'above' cell isn't found it was inserting at the top instead of the bottom.

Copy link
Member

Choose a reason for hiding this comment

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

@rchiodo Sorry wasn't fully getting this. insertAbove requires the vm value, right? Wouldn't the case where we can't find that vm be an error? Or if it's not an error I'm not sure why the default for insertAbove would be the end and not the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't require the VM value, but it did assume if it wasn't found it should insert at the top. The way the function is called though is it passes an empty value when it expects it to be inserted at the end.

I agree it's confusing but it was that or special case the delete to have a new insert. Maybe that would be better. It'd certainly make more sense given the name and the common assumption that it would insert above everything if it can't find the item to insert above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I can also see the argument that if I can't find what to insert above, why would I assume above everything? If there's an item in the list I should definitely be able to find it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I get it a bit more now. The API is just a bit wonky for that function. I think that special casing the delete might be better if we are getting super strict. But at this point, especially with us moving away from the webviews, I'm perfectly fine with trusting your judgement on if that's worth the time.

Copy link
Member

Choose a reason for hiding this comment

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

So I'll approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added a separate code path for delete outside the index.

position = newList.length;
}

const result = {
Expand Down Expand Up @@ -296,7 +296,7 @@ export namespace Creation {
// Otherwise just a straight delete
const index = arg.prevState.cellVMs.findIndex((c) => c.cell.id === arg.payload.data.cellId);
if (index >= 0) {
Transfer.postModelRemove(arg, 0, cells[index].cell);
Transfer.postModelRemove(arg, index, cells[index].cell);

// Recompute select/focus if this item has either
const previousSelection = getSelectedAndFocusedInfo(arg.prevState);
Expand Down
18 changes: 14 additions & 4 deletions src/datascience-ui/native-editor/redux/reducers/execution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,18 @@ 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);
postActionToExtension(arg, InteractiveWindowMessages.Undo);
postActionToExtension(
arg,
InteractiveWindowMessages.Undo,
cells.map((c) => c.cell)
);
return {
...arg.prevState,
cellVMs: cells,
undoStack: undoStack,
redoStack: redoStack,
skipNextScroll: true
skipNextScroll: true,
dirty: true
};
}

Expand All @@ -259,13 +264,18 @@ 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);
postActionToExtension(arg, InteractiveWindowMessages.Redo);
postActionToExtension(
arg,
InteractiveWindowMessages.Redo,
cells.map((c) => c.cell)
);
return {
...arg.prevState,
cellVMs: cells,
undoStack: undoStack,
redoStack: redoStack,
skipNextScroll: true
skipNextScroll: true,
dirty: true
};
}

Expand Down
8 changes: 4 additions & 4 deletions src/datascience-ui/native-editor/redux/reducers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ export const reducerMap: Partial<INativeEditorActionMapping> = {
[CommonActionType.TOGGLE_LINE_NUMBERS]: Effects.toggleLineNumbers,
[CommonActionType.TOGGLE_OUTPUT]: Effects.toggleOutput,
[CommonActionType.CHANGE_CELL_TYPE]: Execution.changeCellType,
[InteractiveWindowMessages.Undo]: Execution.undo,
[InteractiveWindowMessages.Redo]: Execution.redo,
[CommonActionType.UNDO]: Execution.undo,
[CommonActionType.REDO]: Execution.redo,
[CommonActionType.ARROW_UP]: Movement.arrowUp,
[CommonActionType.ARROW_DOWN]: Movement.arrowDown,
[CommonActionType.EDIT_CELL]: Transfer.editCell,
Expand All @@ -66,6 +66,8 @@ export const reducerMap: Partial<INativeEditorActionMapping> = {
[CommonActionType.STEP]: Execution.step,
[CommonActionType.RUN_BY_LINE]: Execution.runByLine,
[CommonActionType.OPEN_SETTINGS]: CommonEffects.openSettings,
[CommonActionType.UNDO]: Execution.undo,
[CommonActionType.REDO]: Execution.redo,

// Messages from the webview (some are ignored)
[InteractiveWindowMessages.StartCell]: Creation.startCell,
Expand All @@ -83,8 +85,6 @@ export const reducerMap: Partial<INativeEditorActionMapping> = {
[InteractiveWindowMessages.GetAllCells]: Transfer.getAllCells,
[InteractiveWindowMessages.GetCellCode]: Transfer.getCellCode,
[InteractiveWindowMessages.GetAllCellCode]: Transfer.getAllCellCode,
[InteractiveWindowMessages.Undo]: Execution.undo,
[InteractiveWindowMessages.Redo]: Execution.redo,
[InteractiveWindowMessages.StartProgress]: CommonEffects.startProgress,
[InteractiveWindowMessages.StopProgress]: CommonEffects.stopProgress,
[SharedMessages.UpdateSettings]: Effects.updateSettings,
Expand Down
11 changes: 11 additions & 0 deletions src/test/datascience/nativeEditor.functional.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,7 @@ df.head()`;
suite('Editor tests', () => {
let wrapper: ReactWrapper<any, Readonly<{}>, React.Component>;
let mount: IMountedWebView;
let notebookEditor: INotebookEditor;
const disposables: Disposable[] = [];
let ioc: DataScienceIocContainer;
const baseFile = `
Expand Down Expand Up @@ -1289,6 +1290,7 @@ df.head()`;
const ne = await openEditor(ioc, fileContents ? fileContents : baseFile, notebookFile.filePath);
wrapper = ne.mount.wrapper;
mount = ne.mount;
notebookEditor = ne.editor;
}

teardown(async () => {
Expand Down Expand Up @@ -2344,6 +2346,8 @@ df.head()`;
assert.equal(isCellFocused(wrapper, 'NativeCell', 0), false);
assert.equal(isCellFocused(wrapper, 'NativeCell', 1), false);
assert.equal(wrapper.find('NativeCell').length, 4);
// Verify our model is correct
assert.equal(notebookEditor?.model.cellCount, 4, 'Undo is not changing cell count in model')

// Press 'meta+z'. This should do nothing
simulateKeyPressOnCell(0, { code: 'z', metaKey: true });
Expand All @@ -2366,6 +2370,8 @@ df.head()`;
assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true);
assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false);
assert.equal(wrapper.find('NativeCell').length, 3);
// Verify our model is correct
assert.equal(notebookEditor?.model.cellCount, 3, 'Undo is not changing cell count in model')

// Press 'shift+z' to redo
update = waitForMessage(ioc, InteractiveWindowMessages.ExecutionRendered);
Expand All @@ -2378,6 +2384,8 @@ df.head()`;
assert.equal(isCellFocused(wrapper, 'NativeCell', 0), false);
assert.equal(isCellFocused(wrapper, 'NativeCell', 1), false);
assert.equal(wrapper.find('NativeCell').length, 4);
// Verify our model is correct
assert.equal(notebookEditor?.model.cellCount, 4, 'Redo is not changing cell count in model')

// Press 'z' to undo.
update = waitForMessage(ioc, InteractiveWindowMessages.ExecutionRendered);
Expand All @@ -2388,6 +2396,9 @@ df.head()`;
assert.equal(isCellSelected(wrapper, 'NativeCell', 0), true);
assert.equal(isCellSelected(wrapper, 'NativeCell', 1), false);
assert.equal(wrapper.find('NativeCell').length, 3);

// Verify our model is correct
assert.equal(notebookEditor?.model.cellCount, 3, 'Undo is not changing cell count in model')
}
});

Expand Down