Ability to clear cell output in NativeNotebook #12307
Conversation
| // tslint:disable-next-line: no-suspicious-comment | ||
| //TODO: VSC doesn't handle this well. | ||
| // What if user doesn't save it. | ||
| if (model.isUntitled) { |
There was a problem hiding this comment.
VSC Have fixed upstream issues.
| const cell = findMappedNotebookCellModel(change.document, vscCell, model.cells); | ||
| cell.data.outputs = []; | ||
| updateVSCNotebookCellMetadata(vscCell.metadata, cell); | ||
| model.update({ |
There was a problem hiding this comment.
We want to trigger an event.
| data: newCellData | ||
| }; | ||
|
|
||
| model.update({ |
There was a problem hiding this comment.
We will mutate the cells in place, we don't want to change the cells as the cell mapping stops working
| @@ -135,42 +152,22 @@ function handleCellMove(change: NotebookCellsChangeEvent, model: INotebookModel) | |||
| const cellToSwap = findMappedNotebookCellModel(change.document, insertChange.items[0]!, model.cells); | |||
| const cellToSwapWith = model.cells[insertChange.start]; | |||
| assert.notEqual(cellToSwap, cellToSwapWith, 'Cannot swap cell with the same cell'); | |||
| model.update({ | |||
There was a problem hiding this comment.
We will mutate the cells in place, we don't want to change the cells as the cell mapping stops working
| } | ||
| function handleCellInsertion(change: NotebookCellsChangeEvent, model: INotebookModel) { | ||
| assert.equal(change.changes.length, 1, 'When inserting cells we must have only 1 change'); | ||
| assert.equal(change.changes[0].items.length, 1, 'Insertion of more than 1 cell is not supported'); | ||
| const insertChange = change.changes[0]; | ||
| const cell = change.changes[0].items[0]; | ||
| const newCell = createCellFromVSCNotebookCell(change.document, cell, model); | ||
|
|
||
| model.update({ |
There was a problem hiding this comment.
We will mutate the cells in place, we don't want to change the cells as the cell mapping stops working
| } | ||
| }; | ||
| const updateCell: INotebookModelModifyChange = { |
There was a problem hiding this comment.
We will mutate the cells in place, we don't want to change the cells as the cell mapping stops working
DonJayamanne
requested review from
rchiodo,
DavidKutu,
greazer,
IanMatthewHuff,
joyceerhl and
techwithtim
| * However we are interested in cell output being cleared (when user clears output). | ||
| */ | ||
| function clearCellOutput(change: NotebookCellOutputsChangeEvent, model: INotebookModel) { | ||
| if (!change.cells.every((cell) => cell.outputs.length === 0)) { |
There was a problem hiding this comment.
Just a quick question on this check. If an output change comes in, and it had two cells in the output change and one cell cleared the output and another cell still had output, it looks like this check would skip the output clear for the cell. But I'm not sure if that is a situation that could happen. Instead of this check do we want to just perform the operation below on any cells in the change that do have output length === 0?
There was a problem hiding this comment.
We only clear the cells that were cleared by the user. change.cells didn't contain all cells of notebook, but only the cleared cells.
Ie if you clear one cell then this array would contain just one item in the list change.cells
There was a problem hiding this comment.
Yup, I can tell that it's just the cells involved with the change. It just looks to me like this NotebookCellOutputsChangeEvent can be more general than just the user clearing output for a cell. Just making sure that VSCode would not do something like batch up calls to it (like update output in cell A and clear output in cell B) then just call OutputsChanged once. If that wouldn't happen then the check seems fine to me, and you would have the best judgement on if that's the case or not.
There was a problem hiding this comment.
Yup got it, at this stage that cannot be handled not the way i have written this.
I'd hope VS Code doesn't bundle such events, then it means events are not fired as they happen.
I would suggest we leave this as is for now.
| @@ -213,11 +216,19 @@ export class NativeEditorNotebookModel implements INotebookModel { | |||
| break; | |||
| case 'save': | |||
| this._state.saveChangeCount = this._state.changeCount; | |||
| // Trigger event. | |||
| if (this.useNativeEditorApi) { | |||
There was a problem hiding this comment.
This feels rather crappy. Logic seems like it could be handled by a sub class?
There was a problem hiding this comment.
I don't think we need to complicate this by adding a sub class, most of this code will get much simpler when we use native editor. Else using sub classes we need to look at child class and parent class what's going on.
There was a problem hiding this comment.
No I think its simpler this way, now i need to create another class and make private methods protected. I think this is simpler there's barely much going on here.
Again, when we use NativeNotebook majority of the code will go away (no undo/redo, etc).
There was a problem hiding this comment.
There was a problem hiding this comment.
Sounds okay to me. Just wanted to mention it.
Oh it is a valid suggestion, if there was a bit more code I'd do it, i did want to do it here. Started it out & realized it wasn't worth it.
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| import { nbformat } from '@jupyterlab/coreutils'; | |||
| import { inject, injectable } from 'inversify'; | |||
| import { IDisposable } from 'monaco-editor'; | |||
There was a problem hiding this comment.
o [](start = 14, length = 1)
I don't believe you want this one.
|
Kudos, SonarCloud Quality Gate passed!
|
For #10496
For #12274
Fixing https://github.com/microsoft/vscode-python/issues/12274 only for native notebooks here. Was an easy fix.