Simplify INotebookModel when using VSCode Notebooks #12604
Conversation
| // tslint:disable-next-line: unified-signatures | ||
| load(file: Uri, contents?: string, skipDirtyContents?: boolean): Promise<INotebookModel>; | ||
| load(file: Uri, contents?: string, backupId?: string, forVSCodeNotebook?: boolean): Promise<INotebookModel>; | ||
| load( |
There was a problem hiding this comment.
Added an overloaded argument to decide whether to use the new INotebookModel class or old.
Proposal is to create two classes that implement INotebookModel, and based on this class we'd use the appropriate class.
I'll also move the code that keeps the INotebookModel in sync with VSC Notebook changes.
I.e. today we have code that handles VSC events & the like and updates INotebookModel, all of that will go into this new class so that its obvious that INOtebookModel is merely a readonly view of the VSC Notebook document.
@rchiodo /cc (in case u had any thoughts on this design)
| @@ -1013,7 +1013,7 @@ export interface INotebookModel { | |||
| readonly isDirty: boolean; | |||
| readonly isUntitled: boolean; | |||
| readonly changed: Event<NotebookModelChange>; | |||
| readonly cells: Readonly<ICell>[]; | |||
| readonly cells: readonly Readonly<ICell>[]; | |||
There was a problem hiding this comment.
Absolutely no changes to cells directly.
There was a problem hiding this comment.
Ensure INotebookModel is purely readonly.
| @@ -176,8 +177,10 @@ function handleCellMove(change: NotebookCellsChangeEvent, model: INotebookModel) | |||
| assert.notEqual(cellToSwap, cellToSwapWith, 'Cannot swap cell with the same cell'); | |||
|
|
|||
| const indexOfCellToSwap = model.cells.indexOf(cellToSwap); | |||
| model.cells[insertChange.start] = cellToSwap; | |||
| model.cells[indexOfCellToSwap] = cellToSwapWith; | |||
| // tslint:disable-next-line: no-any | |||
There was a problem hiding this comment.
Hacks (cast readonly array to make changes), to keep PR small.
I'd rather submit a separate PR for this work.
Basically all of this code goes into the VSCNotebookModel class (it will update the cells internally).
There was a problem hiding this comment.
Separate PR, will just move these into the new class.
DonJayamanne
requested review from
techwithtim,
rchiodo,
IanMatthewHuff,
DavidKutu,
greazer and
joyceerhl
| @@ -259,7 +259,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { | |||
| case InteractiveWindowMessages.Started: | |||
| if (this.model) { | |||
| // Load our cells, but don't wait for this to finish, otherwise the window won't load. | |||
| this.sendInitialCellsToWebView(this.model.cells, this.model.isTrusted) | |||
| this.sendInitialCellsToWebView([...this.model.cells], this.model.isTrusted) | |||
There was a problem hiding this comment.
Because we Array !== ReadOnlyArray.
|
Kudos, SonarCloud Quality Gate passed!
|
For #10496 Part 2 of #12604 Move code related to updates to INotebookModel into the corresponding INotebookModel class.
For #10496
This PR just refactors stuff (moving stuff to accommodate the two new INotebook classes).
Will submit separate PR that cleans up updating INotebookModel for VS Code Notebooks (wanted to keep PR and easy for review, else it looks like there's a lot going on, when in fact there isn't much new).