Fix issue with 'undo' not updating model

[rchiodo]

For #4058

Update the model when the user hits the undo or redo key sequence in the old editor
Also fix a problem in the custom editor where delete undos are not sent to the correct location

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
• Title summarizes what is changing.
• Has a news entry file (remember to thank yourself!).
• Appropriate comments and documentation strings in the code.
• Has sufficient logging.
• Has telemetry for enhancements.
• Unit tests & system/integration tests are added/updated.
• Test plan is updated as appropriate.
• package-lock.json has been regenerated by running npm install (if dependencies have changed).

[rchiodo]

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.

[IanMatthewHuff]

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

[rchiodo]

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.

[rchiodo]

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.

[IanMatthewHuff]

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.

[IanMatthewHuff]

So I'll approve.

[rchiodo]

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

[codecov-io]

Codecov Report

Merging #4298 (2be16d1) into main (f8d6ae5) will increase coverage by 4%.
The diff coverage is 92%.

@@           Coverage Diff           @@
##            main   #4298     +/-   ##
=======================================
+ Coverage     70%     75%     +4%     
=======================================
  Files        389     389             
  Lines      25388   25456     +68     
  Branches    3631    3639      +8     
=======================================
+ Hits       17961   19201   +1240     
+ Misses      6027    4774   -1253     
- Partials    1400    1481     +81     

Impacted Files	Coverage Δ
.../datascience/interactive-common/synchronization.ts	84% <ø> (ø)
...cience/interactive-ipynb/nativeEditorOldWebView.ts	85% <85%> (+<1%)	⬆️
.../datascience/interactive-common/interactiveBase.ts	71% <100%> (-1%)	⬇️
...ience/interactive-common/interactiveWindowTypes.ts	100% <100%> (ø)
...lient/datascience/notebookStorage/notebookModel.ts	82% <100%> (+2%)	⬆️
...c/client/datascience/variablesView/variableView.ts	23% <0%> (-10%)	⬇️
...t/common/variables/environmentVariablesProvider.ts	77% <0%> (-4%)	⬇️
...active-common/intellisense/intellisenseDocument.ts	67% <0%> (-1%)	⬇️
src/client/datascience/liveshare/liveshare.ts	66% <0%> (ø)
... and 89 more