Implement undo support for the custom editor #9946

rchiodo · 2020-02-06T17:54:21Z

For #9821

Implement undo support by creating a new NotebookModelChange type that we send to both VS code and to ourselves to keep track of changes to the underlying model of a notebook editor.

Edits that come from the UI are tagged with 'user' and are forwarded onto VS code.
Edits that come from VS code (either undo or redo) are tagged with 'redo' and 'undo' and are forwarded onto the UI.

Also eliminated undo/redo support in the monaco editor so we could control the undo stack. This didn't turn out perfect though for a number of reasons:

Cursor position isn't tracked very faithfully. Perhaps we can fix this better later. Root cause is that the cursor position is not part of the update when editing and it has to be computed.
Monaco collapses edits into a single undo. Like if you type 'print' and hit ctrl+z, monaco will undo the entire word. Since we can't collapse the undo stack that VS code is using (they don't have a way to do so), we have to instead track each and every edit.

…ustom_editor_undo

rchiodo · 2020-02-06T18:07:02Z

.vscode/launch.json

@@ -187,7 +187,7 @@
                "--ui=tdd",
                "--recursive",
                "--colors",
-                //"--grep", "<suite name>",
+                "--grep", "Edit/delete cells",


cells [](start = 39, length = 5)

Oops #Resolved

rchiodo · 2020-02-06T18:09:42Z

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

@@ -374,19 +331,37 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
        }
    }

-    protected sendCellsToWebView(cells: ICell[]) {
+    protected async sendCellsToWebView(cells: ICell[]) {


async [](start = 14, length = 5)

Actually this doesn't need to be async. #Resolved

codecov-io · 2020-02-06T18:10:04Z

Codecov Report

❗ No coverage uploaded for pull request base (ds/custom_editor@21d0ee9). Click here to learn what that means.
The diff coverage is 43.75%.

@@                 Coverage Diff                 @@
##             ds/custom_editor    #9946   +/-   ##
===================================================
  Coverage                    ?   60.52%           
===================================================
  Files                       ?      556           
  Lines                       ?    29788           
  Branches                    ?     4487           
===================================================
  Hits                        ?    18029           
  Misses                      ?    10769           
  Partials                    ?      990

Impacted Files	Coverage Δ
...ient/datascience/interactive-ipynb/nativeEditor.ts	`12.68% <0%> (ø)`
src/client/datascience/data-viewing/types.ts	`100% <100%> (ø)`
...ience/interactive-common/interactiveWindowTypes.ts	`100% <100%> (ø)`
src/client/datascience/messages.ts	`100% <100%> (ø)`
...ractive-common/interactiveWindowMessageListener.ts	`17.02% <14.28%> (ø)`
.../datascience/interactive-common/interactiveBase.ts	`5.83% <25%> (ø)`
...tascience/interactive-ipynb/nativeEditorStorage.ts	`55.35% <46.15%> (ø)`
src/datascience-ui/interactive-common/mainState.ts	`53.57% <7.69%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21d0ee9...146c76b. Read the comment docs.

rchiodo · 2020-02-06T18:15:26Z

src/datascience-ui/native-editor/nativeCell.tsx

@@ -280,6 +281,7 @@ export class NativeCell extends React.Component<INativeCellProps> {
                    this.props.sendCommand(NativeCommandType.ToggleOutput, 'keyboard');
                }
                break;
+            case 'NumpadEnter':


Thought I'd fix this while I was changing it :) #Resolved

DonJayamanne

All good, except for some state stuff..

DonJayamanne · 2020-02-07T17:39:17Z

src/datascience-ui/interactive-common/editor.tsx

-        if (this.state.model) {
-            return this.state.model.getValue().replace(/\r/g, '');
+        if (this.monacoRef.current) {
+            return this.monacoRef.current.getContents();


Oh good, I never understood why we removed /r/ #Resolved

DonJayamanne · 2020-02-07T17:41:47Z

src/datascience-ui/interactive-common/editor.tsx

-        if (this.state.editor && this.state.model && this.monacoRef && this.monacoRef.current) {
-            const cursor = this.state.editor.getPosition();
+        if (this.monacoRef.current) {
+            const cursor = this.monacoRef.current.getPosition();


If we're not using state.model or state.editor, do we still need to add them into the state object.
I'm glad we're getting rid of them, much cleaner (clean = not confusing why it was in state).

I.e. please review what's in state and remove, I think we can remove them.

They are removed? Not sure what you're asking. IEditorState doesn't exist anymore.

In reply to: 376519201 [](ancestors = 376519201)

this.setState({ editor, model: editor.getModel() });
Do we need these states anymore? I don't think model is being used.

DonJayamanne · 2020-02-07T17:49:17Z

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

@@ -28,7 +28,7 @@ export namespace Effects {

                if (typeof removeFocusIndex === 'number') {
                    const oldFocusCell = prevState.cellVMs[removeFocusIndex];
-                    const oldCode = oldFocusCell.uncomittedText || oldFocusCell.inputBlockText;
+                    const oldCode = oldFocusCell.uncommittedText || oldFocusCell.inputBlockText;


With the new changes, do we still need uncommittedText?
Feels hacky to me

Yes. For now. Until we make it really update the text on every type.

In reply to: 376522239 [](ancestors = 376522239)

DonJayamanne · 2020-02-07T17:51:51Z

src/datascience-ui/react-common/monacoEditor.tsx

+
+    public getContents(): string {
+        if (this.state.model) {
+            return this.state.model.getValue().replace(/\r/g, '');


Please could you add some comments around the need for this. (the need to remove /r

Sure. It's so it's consistent with what jupyter expects. It doesn't put in /r ever.

In reply to: 376523346 [](ancestors = 376523346)

IanMatthewHuff · 2020-02-07T18:04:31Z

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

-            // Spec exists, just update name and display_name
-            s._state.notebookJson.metadata.kernelspec.name = kernelSpec.name || kernelSpec.display_name || '';
-            s._state.notebookJson.metadata.kernelspec.display_name = kernelSpec.display_name || kernelSpec.name || '';
+        if (changed) {


I'm a bit confused as to why this doesn't use the newDirty from the change as Undo does. I get this is handling user and redo, but in the redo case I would assume that it would be using that.

Can you redo a change and have it not be dirty? I don't think so? Not 100% sure. That's why its like this. The newDirty state from the redo may be off.

In reply to: 376528654 [](ancestors = 376528654)

Ok, I think I get it better now. So the new / old Dirty are always reflecting just reality of the change at the time that it was made. Right?

In reply to: 376529516 [](ancestors = 376529516,376528654)

Yes.

In reply to: 376532221 [](ancestors = 376532221,376529516,376528654)

IanMatthewHuff · 2020-02-07T18:06:46Z

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

+    private swapCells(firstCellId: string, secondCellId: string) {
+        const first = this.cells.findIndex(v => v.id === firstCellId);
+        const second = this.cells.findIndex(v => v.id === secondCellId);
+        if (first >= 0 && second >= 0) {


first !== second check as well? Would keep us from returning changed on a no op. #Resolved

Good idea

In reply to: 376529606 [](ancestors = 376529606)

Code flow isn't showing the change here for some reason, but I did what you suggested.

In reply to: 376530953 [](ancestors = 376530953,376529606)

IanMatthewHuff · 2020-02-07T18:19:51Z

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

-        if (storage) {
-            return handler(storage, ...args);
+    private clearOutputs(): boolean {
+        this._state.cells = this.cells.map(c => this.asCell({ ...c, data: { ...c.data, execution_count: null, outputs: [] } }));


It does make it a bit more complex, but I'm thinking that this could also use a possible false state. If you have a cell or two that is not run, this is going to return changed when nothing has actually happened. If you wanted to be super safe modify and remove could also have false states (no cells) but since we never expect that UI state I don't think it's needed as much for them.

Yeah I was thinking it wasn't worth it for the clear cells (cause of the cost of the compare), but maybe. It would be weird if you cleared, saved, cleared again and it was dirty

In reply to: 376535431 [](ancestors = 376535431)

It's not a bad bug, but this did seem like the only situation where this would be wrong given a valid input state. I get that it does add a bit more. Since you are clearing everything it's not like you need to check each cell. Just if any cell has an execution count or an output this will do something for changed.

In reply to: 376538081 [](ancestors = 376538081,376535431)

I think I need to rethink the dirty state. Right now this doesn't work:

Open notebook
Clear all outputs - editor tab and react ui think dirty
Save - editor tab and react ui think clean
Undo - editor tab says dirty, ui says otherwise.

I think the dirty state should just be a diff between the saved state and the current state. That's what makes it dirty. And I don't want to compare the entire thing on every update. So have to do smart comparisons.

IanMatthewHuff

rchiodo · 2020-02-07T21:32:46Z

@DonJayamanne did you still have concerns? (Other than the save problem I'm working on)

Was going to submit this and work on the save problem separately.

sonarqubecloud · 2020-02-07T21:49:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
2 Code Smells

No Coverage information
0.2% Duplication

rchiodo added 18 commits January 30, 2020 16:37

Partially working undo/redo

d87e6ef

Concept created using update message/command

edc1f36

Put the storage object in the global map

1d57e73

Look for more data during undo/redo

4658b15

New idea with reverse edit and disable undo/redo in monaco

433c4a9

Potentially working with tracking edits

aeb79f9

Fix completions for interactive

1fcd271

Fix unit tests

4398133

First working functional test of undo

6a3366f

More testing of undo

7403fc2

More tests

1714b7f

Close suggestions on undo

3a9b885

Fix other functional tests

9a80c70

Fix undo misses

679f3a4

Fix focus changes on undo

21d0ee9

Merge remote-tracking branch 'origin/ds/custom_editor' into rchiodo/c…

8997bad

…ustom_editor_undo

Fixup after merge

58e6f67

Add news entry

d53f44c

rchiodo self-assigned this Feb 6, 2020

rchiodo requested review from DavidKutu, DonJayamanne and IanMatthewHuff February 6, 2020 17:55

rchiodo commented Feb 6, 2020

View reviewed changes

Put back launch.json

9d62143

rchiodo commented Feb 6, 2020

View reviewed changes

Get rid of asyncness of sendCellsToWebView

5b87c06

rchiodo commented Feb 6, 2020

View reviewed changes

DonJayamanne requested changes Feb 7, 2020

View reviewed changes

IanMatthewHuff reviewed Feb 7, 2020

View reviewed changes

rchiodo added 2 commits February 7, 2020 10:09

Add a comment about why we remove \r

367e476

Prevent swapCells from doing anything if same cell

ee5ab38

IanMatthewHuff reviewed Feb 7, 2020

View reviewed changes

Fix changed event for clear

902a167

IanMatthewHuff approved these changes Feb 7, 2020

View reviewed changes

Actually remove all state from editor class. Try to fix undo on save

146c76b

DonJayamanne approved these changes Feb 7, 2020

View reviewed changes

rchiodo merged commit 9ed71a5 into ds/custom_editor Feb 7, 2020

rchiodo deleted the rchiodo/custom_editor_undo branch February 7, 2020 22:14

lock bot locked as resolved and limited conversation to collaborators Feb 15, 2020

Implement undo support for the custom editor #9946

Implement undo support for the custom editor #9946

Uh oh!

Conversation

rchiodo commented Feb 6, 2020

Uh oh!

rchiodo Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rchiodo Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Feb 7, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Feb 7, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Feb 7, 2020

Uh oh!

sonarqubecloud bot commented Feb 7, 2020

Uh oh!

Uh oh!

rchiodo Feb 6, 2020 •

edited

Loading

rchiodo Feb 6, 2020 •

edited

Loading

codecov-io commented Feb 6, 2020 •

edited

Loading

rchiodo Feb 6, 2020 •

edited

Loading

DonJayamanne Feb 7, 2020 •

edited by rchiodo

Loading

IanMatthewHuff Feb 7, 2020 •

edited by rchiodo

Loading