Fix issue with 'undo' not updating model

news/2 Fixes/4058.md

@@ -0,0 +1 @@
+Fix Z (and CTRL+Z when using custom editor support) to update data model so that save works.

src/client/datascience/interactive-common/interactiveBase.ts

@@ -406,12 +406,12 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
 
     @captureTelemetry(Telemetry.Undo)
     public undoCells() {
-        this.postMessage(InteractiveWindowMessages.Undo).ignoreErrors();
+        this.postMessage(InteractiveWindowMessages.UndoCommand).ignoreErrors();
     }
 
     @captureTelemetry(Telemetry.Redo)
     public redoCells() {
-        this.postMessage(InteractiveWindowMessages.Redo).ignoreErrors();
+        this.postMessage(InteractiveWindowMessages.RedoCommand).ignoreErrors();
     }
 
     @captureTelemetry(Telemetry.DeleteAllCells)

src/client/datascience/interactive-common/interactiveWindowTypes.ts

@@ -51,6 +51,8 @@ export enum InteractiveWindowMessages {
     DeleteAllCells = 'delete_all_cells',
     Undo = 'undo',
     Redo = 'redo',
+    UndoCommand = 'undo.command', // Only used by the interactive window
+    RedoCommand = 'redo.command',
     ExpandAll = 'expand_all',
     CollapseAll = 'collapse_all',
     StartProgress = 'start_progress',
@@ -609,8 +611,10 @@ 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.UndoCommand]: never | undefined;
+    public [InteractiveWindowMessages.RedoCommand]: never | undefined;
     public [InteractiveWindowMessages.ExpandAll]: never | undefined;
     public [InteractiveWindowMessages.CollapseAll]: never | undefined;
     public [InteractiveWindowMessages.StartProgress]: never | undefined;

src/client/datascience/interactive-common/synchronization.ts

@@ -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,
@@ -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,
@@ -206,6 +208,8 @@ const messageWithMessageTypes: MessageMapping<IInteractiveWindowMapping> & Messa
     [InteractiveWindowMessages.SubmitNewCell]: MessageType.other,
     [InteractiveWindowMessages.Sync]: MessageType.other,
     [InteractiveWindowMessages.Undo]: MessageType.other,
+    [InteractiveWindowMessages.UndoCommand]: MessageType.other,
+    [InteractiveWindowMessages.RedoCommand]: MessageType.other,
     [InteractiveWindowMessages.UnfocusedCellEditor]: MessageType.syncWithLiveShare,
     [InteractiveWindowMessages.UpdateCellWithExecutionResults]:
         MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare,

src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts

@@ -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,
@@ -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

src/client/datascience/notebookStorage/notebookModel.ts

@@ -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> {

src/datascience-ui/history-react/redux/actions.ts

@@ -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),

src/datascience-ui/history-react/redux/reducers/index.ts

@@ -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,
@@ -70,5 +70,7 @@ export const reducerMap: Partial<IInteractiveActionMapping> = {
     [InteractiveWindowMessages.UpdateDisplayData]: CommonEffects.handleUpdateDisplayData,
     [InteractiveWindowMessages.HasCell]: Transfer.hasCell,
     [InteractiveWindowMessages.UpdateExternalCellButtons]: CommonEffects.handleWebviewButtons,
-    [InteractiveWindowMessages.ExecuteExternalCommand]: Transfer.executeExternalCommand
+    [InteractiveWindowMessages.ExecuteExternalCommand]: Transfer.executeExternalCommand,
+    [InteractiveWindowMessages.UndoCommand]: Execution.undo,
+    [InteractiveWindowMessages.RedoCommand]: Execution.redo
 };

src/datascience-ui/interactive-common/redux/reducers/types.ts

@@ -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',
@@ -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'
@@ -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 {}

src/datascience-ui/native-editor/redux/actions.ts

@@ -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> =>

src/datascience-ui/native-editor/redux/reducers/creation.ts

@@ -145,7 +145,19 @@ export namespace Creation {
     }
 
     export function insertBelow(arg: NativeEditorReducerArg<ICellAction & IAddCellAction>): IMainState {
-        const newVM = prepareCellVM(createEmptyCell(arg.payload.data.newCellId, null), false, arg.prevState.settings);
+        return insertExistingBelow({
+            ...arg,
+            payload: {
+                ...arg.payload,
+                data: { ...arg.payload.data, cell: createEmptyCell(arg.payload.data.newCellId, null) }
+            }
+        });
+    }
+
+    export function insertExistingBelow(
+        arg: NativeEditorReducerArg<ICellAction & IAddCellAction & { cell: ICell }>
+    ): IMainState {
+        const newVM = prepareCellVM(arg.payload.data.cell, false, arg.prevState.settings);
         const newList = [...arg.prevState.cellVMs];
 
         // Find the position where we want to insert
@@ -296,7 +308,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);
@@ -370,17 +382,29 @@ export namespace Creation {
                     payload: { ...arg.payload, data: { cellId: arg.payload.data.cell.id } }
                 });
             case 'remove':
-                const cellBelow =
-                    arg.prevState.cellVMs.length > arg.payload.data.index
-                        ? arg.prevState.cellVMs[arg.payload.data.index].cell
-                        : undefined;
-                return insertExistingAbove({
-                    ...disabledQueueArg,
-                    payload: {
-                        ...arg.payload,
-                        data: { cell: arg.payload.data.cell, cellId: cellBelow ? cellBelow.id : undefined }
-                    }
-                });
+                if (arg.prevState.cellVMs.length > arg.payload.data.index) {
+                    const cellBelow = arg.prevState.cellVMs[arg.payload.data.index].cell;
+                    return insertExistingAbove({
+                        ...disabledQueueArg,
+                        payload: {
+                            ...arg.payload,
+                            data: { cell: arg.payload.data.cell, cellId: cellBelow ? cellBelow.id : undefined }
+                        }
+                    });
+                } else {
+                    // Delete is outside current range. Insert at the bottom
+                    return insertExistingBelow({
+                        ...disabledQueueArg,
+                        payload: {
+                            ...arg.payload,
+                            data: {
+                                cell: arg.payload.data.cell,
+                                cellId: undefined,
+                                newCellId: arg.payload.data.cell.id
+                            }
+                        }
+                    });
+                }
             case 'remove_all':
                 return loadAllCells({
                     ...disabledQueueArg,

src/datascience-ui/native-editor/redux/reducers/execution.ts

@@ -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
             };
         }
 
@@ -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
             };
         }
 

src/datascience-ui/native-editor/redux/reducers/index.ts

@@ -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,
@@ -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,
@@ -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,