Add ability to sync INotebookModel with cell text edits #12092
Conversation
|
SonarCloud Quality Gate failed.
|
| return; | ||
| } | ||
|
|
||
| cell.data.source = splitMultilineString(e.document.getText()); |
There was a problem hiding this comment.
@rchiodo @IanMatthewHuff I'm not using the INotebookModel.update to update the model. Editing cell could end up causing this code to get executed for every keystroke, also don't see any value in triggering model updates for cell source changes (unnecessary slicing of arrays, unnecessary CPU...).
There was a problem hiding this comment.
This is the crux of the PR, when user edits a cell, we take the contents of the cell and update our cell.
FYI - not performing diff updates or the like. This is more efficient.
| test('Cancelling using VSC Command for cell (slow)', async () => { | ||
| test('Cancelling using VSC Command for cell (slow)', async function () { | ||
| // Fails due to VSC bugs. | ||
| return this.skip(); |
There was a problem hiding this comment.
VSC Insiders is now broken.
DonJayamanne
requested review from
DavidKutu,
greazer,
IanMatthewHuff,
joyceerhl,
rchiodo and
techwithtim
| return; | ||
| } | ||
| if (!editor.model) { | ||
| traceError( |
There was a problem hiding this comment.
Should we add telemetry for these?
Not sure what needs to be done here. Basically these conditions should not be met.
I could throw an error, but it wouldn't get handled by anything as this is all happening within the context of an event handler (i.e. error throw will not be displayed to user).
I feel we need to atleast add some logging
| if (!this.experiment.inExperiment(NativeNotebook.experiment)) { | ||
| return; | ||
| } | ||
| this.documentManager.onDidChangeTextDocument(this.onDidChangeTextDocument, this, this.disposables); |
There was a problem hiding this comment.
That's interesting. Didn't realize you could pass in the disposeable[] I always just used the return value.
For #10496