From dedd8bb62d1704bab8d63082ddb3b21e8901bfe1 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 21 Sep 2022 13:16:38 -0700 Subject: [PATCH] Mark more fields in notebook types as `readonly` (#161428) Ran into these while debugging through notebook code. Adding readonly makes it more clear the objects are not mutable In one case, we were also reversing an array passed to an event. This is not safe as it reverses in place so all event listeners will now see the reversed array --- .../browser/contrib/troubleshoot/layout.ts | 2 +- .../notebook/browser/notebookBrowser.ts | 80 +++++++++---------- .../notebook/browser/notebookEditorWidget.ts | 2 +- .../browser/viewModel/baseCellViewModel.ts | 4 +- .../browser/viewModel/codeCellViewModel.ts | 11 ++- .../viewModel/notebookViewModelImpl.ts | 2 +- .../notebookEditorWidgetContextKeys.ts | 2 +- 7 files changed, 53 insertions(+), 50 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/contrib/troubleshoot/layout.ts b/src/vs/workbench/contrib/notebook/browser/contrib/troubleshoot/layout.ts index ab714f85b51..3644400d74b 100644 --- a/src/vs/workbench/contrib/notebook/browser/contrib/troubleshoot/layout.ts +++ b/src/vs/workbench/contrib/notebook/browser/contrib/troubleshoot/layout.ts @@ -70,7 +70,7 @@ export class TroubleshootController extends Disposable implements INotebookEdito } this._localStore.add(this._notebookEditor.onDidChangeViewCells(e => { - e.splices.reverse().forEach(splice => { + [...e.splices].reverse().forEach(splice => { const [start, deleted, newCells] = splice; const deletedCells = this._cellStateListeners.splice(start, deleted, ...newCells.map(cell => { return cell.onDidChangeLayout((e: ICommonCellViewModelLayoutChangeInfo) => { diff --git a/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts b/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts index 5a3028c7400..b95a790cc6d 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts @@ -58,8 +58,8 @@ KERNEL_RECOMMENDATIONS.get(IPYNB_VIEW_TYPE)?.set('python', { }); export interface INotebookExtensionRecommendation { - extensionId: string; - displayName?: string; + readonly extensionId: string; + readonly displayName?: string; } //#endregion @@ -77,16 +77,16 @@ export const enum RenderOutputType { } export interface IRenderPlainHtmlOutput { - type: RenderOutputType.Html; - source: IDisplayOutputViewModel; - htmlContent: string; + readonly type: RenderOutputType.Html; + readonly source: IDisplayOutputViewModel; + readonly htmlContent: string; } export interface IRenderOutputViaExtension { - type: RenderOutputType.Extension; - source: IDisplayOutputViewModel; - mimeType: string; - renderer: INotebookRendererInfo; + readonly type: RenderOutputType.Extension; + readonly source: IDisplayOutputViewModel; + readonly mimeType: string; + readonly renderer: INotebookRendererInfo; } export type IInsetRenderOutput = IRenderPlainHtmlOutput | IRenderOutputViaExtension; @@ -135,9 +135,9 @@ export interface IDisplayOutputLayoutUpdateRequest { } export interface ICommonCellInfo { - cellId: string; - cellHandle: number; - cellUri: URI; + readonly cellId: string; + readonly cellHandle: number; + readonly cellUri: URI; } export interface IFocusNotebookCellOptions { @@ -173,14 +173,14 @@ export interface CodeCellLayoutInfo { } export interface CodeCellLayoutChangeEvent { - source?: string; - editorHeight?: boolean; - commentHeight?: boolean; - outputHeight?: boolean; - outputShowMoreContainerHeight?: number; - totalHeight?: boolean; - outerWidth?: number; - font?: FontInfo; + readonly source?: string; + readonly editorHeight?: boolean; + readonly commentHeight?: boolean; + readonly outputHeight?: boolean; + readonly outputShowMoreContainerHeight?: number; + readonly totalHeight?: boolean; + readonly outerWidth?: number; + readonly font?: FontInfo; } export interface MarkupCellLayoutInfo { @@ -200,18 +200,18 @@ export enum CellLayoutContext { } export interface MarkupCellLayoutChangeEvent { - font?: FontInfo; - outerWidth?: number; - editorHeight?: number; - previewHeight?: number; + readonly font?: FontInfo; + readonly outerWidth?: number; + readonly editorHeight?: number; + readonly previewHeight?: number; totalHeight?: number; - context?: CellLayoutContext; + readonly context?: CellLayoutContext; } export interface ICommonCellViewModelLayoutChangeInfo { - totalHeight?: boolean | number; - outerWidth?: number; - context?: CellLayoutContext; + readonly totalHeight?: boolean | number; + readonly outerWidth?: number; + readonly context?: CellLayoutContext; } export interface ICellViewModel extends IGenericCellViewModel { readonly model: NotebookCellTextModel; @@ -248,7 +248,7 @@ export interface ICellViewModel extends IGenericCellViewModel { getCellStatusBarItems(): INotebookCellStatusBarItem[]; getEditState(): CellEditState; updateEditState(state: CellEditState, source: string): void; - deltaModelDecorations(oldDecorations: string[], newDecorations: IModelDeltaDecoration[]): string[]; + deltaModelDecorations(oldDecorations: readonly string[], newDecorations: readonly IModelDeltaDecoration[]): string[]; getCellDecorationRange(id: string): Range | null; } @@ -289,13 +289,13 @@ export interface INotebookCellDecorationOptions { } export interface INotebookDeltaDecoration { - handle: number; - options: INotebookCellDecorationOptions; + readonly handle: number; + readonly options: INotebookCellDecorationOptions; } export interface INotebookDeltaCellStatusBarItems { - handle: number; - items: INotebookCellStatusBarItem[]; + readonly handle: number; + readonly items: readonly INotebookCellStatusBarItem[]; } @@ -338,7 +338,7 @@ export interface INotebookEditorCreationOptions { } export interface INotebookWebviewMessage { - message: unknown; + readonly message: unknown; } //#region Notebook View Model @@ -357,13 +357,13 @@ export interface INotebookEditorViewState { } export interface ICellModelDecorations { - ownerId: number; - decorations: string[]; + readonly ownerId: number; + readonly decorations: readonly string[]; } export interface ICellModelDeltaDecorations { - ownerId: number; - decorations: IModelDeltaDecoration[]; + readonly ownerId: number; + readonly decorations: readonly IModelDeltaDecoration[]; } export interface IModelDecorationsChangeAccessor { @@ -378,8 +378,8 @@ export type NotebookViewCellsSplice = [ ]; export interface INotebookViewCellsUpdateEvent { - synchronous: boolean; - splices: NotebookViewCellsSplice[]; + readonly synchronous: boolean; + readonly splices: readonly NotebookViewCellsSplice[]; } export interface INotebookViewModel { diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 81de7c57db3..9a3c8d38140 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1408,7 +1408,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD } // update cell listener - e.splices.reverse().forEach(splice => { + [...e.splices].reverse().forEach(splice => { const [start, deleted, newCells] = splice; const deletedCells = this._localCellStateListeners.splice(start, deleted, ...newCells.map(cell => this._bindCellListener(cell))); diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.ts index 9f5be84ed97..dc2265e5f26 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.ts @@ -373,7 +373,7 @@ export abstract class BaseCellViewModel extends Disposable { this._resolvedDecorations.delete(decorationId); } - deltaModelDecorations(oldDecorations: string[], newDecorations: model.IModelDeltaDecoration[]): string[] { + deltaModelDecorations(oldDecorations: readonly string[], newDecorations: readonly model.IModelDeltaDecoration[]): string[] { oldDecorations.forEach(id => { this.removeModelDecoration(id); }); @@ -427,7 +427,7 @@ export abstract class BaseCellViewModel extends Disposable { return ret; } - deltaCellStatusBarItems(oldItems: string[], newItems: INotebookCellStatusBarItem[]): string[] { + deltaCellStatusBarItems(oldItems: readonly string[], newItems: readonly INotebookCellStatusBarItem[]): string[] { oldItems.forEach(id => { const item = this._cellStatusBarItems.get(id); if (item) { diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel.ts index a8ef03bcdcb..099e660c7cb 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel.ts @@ -24,8 +24,10 @@ import { NotebookLayoutInfo } from 'vs/workbench/contrib/notebook/browser/notebo export class CodeCellViewModel extends BaseCellViewModel implements ICellViewModel { readonly cellKind = CellKind.Code; + protected readonly _onLayoutInfoRead = this._register(new Emitter()); readonly onLayoutInfoRead = this._onLayoutInfoRead.event; + protected readonly _onDidChangeOutputs = this._register(new Emitter()); readonly onDidChangeOutputs = this._onDidChangeOutputs.event; @@ -288,10 +290,11 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod }; } - state.totalHeight = this.layoutInfo.totalHeight !== originalLayout.totalHeight; - state.source = source; - - this._fireOnDidChangeLayout(state); + this._fireOnDidChangeLayout({ + ...state, + totalHeight: this.layoutInfo.totalHeight !== originalLayout.totalHeight, + source, + }); } private _fireOnDidChangeLayout(state: CodeCellLayoutChangeEvent) { diff --git a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl.ts b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl.ts index 4229a1a96d9..8dcc07dddf2 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewModel/notebookViewModelImpl.ts @@ -839,7 +839,7 @@ export class NotebookViewModel extends Disposable implements EditorFoldingStateD private _deltaModelDecorationsImpl(oldDecorations: ICellModelDecorations[], newDecorations: ICellModelDeltaDecorations[]): ICellModelDecorations[] { - const mapping = new Map(); + const mapping = new Map(); oldDecorations.forEach(oldDecoration => { const ownerId = oldDecoration.ownerId; diff --git a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts index 35506ece47f..f8e26930397 100644 --- a/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts +++ b/src/vs/workbench/contrib/notebook/browser/viewParts/notebookEditorWidgetContextKeys.ts @@ -125,7 +125,7 @@ export class NotebookEditorContextKeys { this._updateForInstalledExtension(); this._viewModelDisposables.add(this._editor.onDidChangeViewCells(e => { - e.splices.reverse().forEach(splice => { + [...e.splices].reverse().forEach(splice => { const [start, deleted, newCells] = splice; const deletedCellOutputStates = this._cellOutputsListeners.splice(start, deleted, ...newCells.map(addCellOutputsListener)); dispose(deletedCellOutputStates);