Review feedback and fix undo/redo for interactive command

microsoft · Jan 5, 2021 · 2be16d1 · 2be16d1
1 parent abc4978
commit 2be16d1
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 17 deletions.
diff --git a/src/client/datascience/interactive-common/interactiveBase.ts b/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)

diff --git a/src/client/datascience/interactive-common/interactiveWindowTypes.ts b/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',
@@ -611,6 +613,8 @@ export class IInteractiveWindowMapping {
     public [InteractiveWindowMessages.DeleteAllCells]: IAddCellAction;
     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;

diff --git a/src/client/datascience/interactive-common/synchronization.ts b/src/client/datascience/interactive-common/synchronization.ts
@@ -208,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,

diff --git a/src/datascience-ui/history-react/redux/reducers/index.ts b/src/datascience-ui/history-react/redux/reducers/index.ts
@@ -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
 };
diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts
@@ -100,8 +100,8 @@ export namespace Creation {
         if (position >= 0) {
             newList.splice(position, 0, newVM);
         } else {
-            newList.splice(newList.length, 0, newVM);
-            position = newList.length;
+            newList.splice(0, 0, newVM);
+            position = 0;
         }
 
         const result = {
@@ -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
@@ -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,