From 63173cdabcbb9f199e520d2632a19ea9bc4fccdc Mon Sep 17 00:00:00 2001 From: Aaron Munger <2019016+amunger@users.noreply.github.com> Date: Fri, 10 Oct 2025 11:35:41 -0700 Subject: [PATCH] add logging service to notebook text model to trace edit activity (#270786) * add logging service to text model and small cleanup * log output edits * less redundant category * Update src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../services/notebookLoggingServiceImpl.ts | 4 ++ .../common/model/notebookCellTextModel.ts | 36 ++++++++++------ .../common/model/notebookTextModel.ts | 41 +++++++++++++------ .../notebook/common/notebookLoggingService.ts | 1 + .../test/browser/notebookViewModel.test.ts | 5 ++- .../test/browser/testNotebookEditor.ts | 32 ++++++++++++++- 6 files changed, 92 insertions(+), 27 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/services/notebookLoggingServiceImpl.ts b/src/vs/workbench/contrib/notebook/browser/services/notebookLoggingServiceImpl.ts index 93e248e3c74..40081064446 100644 --- a/src/vs/workbench/contrib/notebook/browser/services/notebookLoggingServiceImpl.ts +++ b/src/vs/workbench/contrib/notebook/browser/services/notebookLoggingServiceImpl.ts @@ -24,6 +24,10 @@ export class NotebookLoggingService extends Disposable implements INotebookLoggi this._logger = this._register(loggerService.createLogger(logChannelId, { name: nls.localize('renderChannelName', "Notebook"), group: windowLogGroup })); } + trace(category: string, output: string): void { + this._logger.trace(`[${category}] ${output}`); + } + debug(category: string, output: string): void { this._logger.debug(`[${category}] ${output}`); } diff --git a/src/vs/workbench/contrib/notebook/common/model/notebookCellTextModel.ts b/src/vs/workbench/contrib/notebook/common/model/notebookCellTextModel.ts index 21e5e9a987f..e712afb30a5 100644 --- a/src/vs/workbench/contrib/notebook/common/model/notebookCellTextModel.ts +++ b/src/vs/workbench/contrib/notebook/common/model/notebookCellTextModel.ts @@ -15,12 +15,13 @@ import { createTextBuffer, TextModel } from '../../../../../editor/common/model/ import { PLAINTEXT_LANGUAGE_ID } from '../../../../../editor/common/languages/modesRegistry.js'; import { ILanguageService } from '../../../../../editor/common/languages/language.js'; import { NotebookCellOutputTextModel } from './notebookCellOutputTextModel.js'; -import { CellInternalMetadataChangedEvent, CellKind, ICell, ICellDto2, ICellOutput, IOutputDto, IOutputItemDto, NotebookCellCollapseState, NotebookCellInternalMetadata, NotebookCellMetadata, NotebookCellOutputsSplice, TransientCellMetadata, TransientOptions } from '../notebookCommon.js'; +import { CellInternalMetadataChangedEvent, CellKind, ICell, ICellDto2, ICellOutput, IOutputItemDto, NotebookCellCollapseState, NotebookCellDefaultCollapseConfig, NotebookCellInternalMetadata, NotebookCellMetadata, NotebookCellOutputsSplice, TransientCellMetadata, TransientOptions } from '../notebookCommon.js'; import { ThrottledDelayer } from '../../../../../base/common/async.js'; import { ILanguageDetectionService } from '../../../../services/languageDetection/common/languageDetectionWorkerService.js'; import { toFormattedString } from '../../../../../base/common/jsonFormatter.js'; import { IModelContentChangedEvent } from '../../../../../editor/common/textModelEvents.js'; import { splitLines } from '../../../../../base/common/strings.js'; +import { INotebookLoggingService } from '../notebookLoggingService.js'; export class NotebookCellTextModel extends Disposable implements ICell { private readonly _onDidChangeTextModel = this._register(new Emitter()); @@ -190,26 +191,34 @@ export class NotebookCellTextModel extends Disposable implements ICell { private _hasLanguageSetExplicitly: boolean = false; get hasLanguageSetExplicitly(): boolean { return this._hasLanguageSetExplicitly; } + private _source: string; + private _language: string; + private _mime: string | undefined; + public readonly cellKind: CellKind; + public readonly collapseState: NotebookCellCollapseState | undefined; + constructor( readonly uri: URI, public readonly handle: number, - private readonly _source: string, - private _language: string, - private _mime: string | undefined, - public readonly cellKind: CellKind, - outputs: IOutputDto[], - metadata: NotebookCellMetadata | undefined, - internalMetadata: NotebookCellInternalMetadata | undefined, - public readonly collapseState: NotebookCellCollapseState | undefined, + cell: ICellDto2, public readonly transientOptions: TransientOptions, private readonly _languageService: ILanguageService, private readonly _defaultEOL: model.DefaultEndOfLine, + defaultCollapseConfig: NotebookCellDefaultCollapseConfig | undefined, private readonly _languageDetectionService: ILanguageDetectionService | undefined = undefined, + private readonly _notebookLoggingService: INotebookLoggingService ) { super(); - this._outputs = outputs.map(op => new NotebookCellOutputTextModel(op)); - this._metadata = metadata ?? {}; - this._internalMetadata = internalMetadata ?? {}; + this._source = cell.source; + this._language = cell.language; + this._mime = cell.mime; + this.cellKind = cell.cellKind; + // Compute collapse state: use cell's state if provided, otherwise use default config for this cell kind + const defaultConfig = cell.cellKind === CellKind.Code ? defaultCollapseConfig?.codeCell : defaultCollapseConfig?.markupCell; + this.collapseState = cell.collapseState ?? (defaultConfig ?? undefined); + this._outputs = cell.outputs.map(op => new NotebookCellOutputTextModel(op)); + this._metadata = cell.metadata ?? {}; + this._internalMetadata = cell.internalMetadata ?? {}; } enableAutoLanguageDetection() { @@ -322,6 +331,7 @@ export class NotebookCellTextModel extends Disposable implements ICell { } spliceNotebookCellOutputs(splice: NotebookCellOutputsSplice): void { + this._notebookLoggingService.trace('textModelEdits', `splicing outputs at ${splice.start} length: ${splice.deleteCount} with ${splice.newOutputs.length} new outputs`); if (splice.deleteCount > 0 && splice.newOutputs.length > 0) { const commonLen = Math.min(splice.deleteCount, splice.newOutputs.length); // update @@ -349,6 +359,7 @@ export class NotebookCellTextModel extends Disposable implements ICell { return false; } + this._notebookLoggingService.trace('textModelEdits', `replacing an output item at index ${outputIndex}`); const output = this.outputs[outputIndex]; // convert to dto and dispose the cell output model output.replaceData({ @@ -369,6 +380,7 @@ export class NotebookCellTextModel extends Disposable implements ICell { } const output = this.outputs[outputIndex]; + this._notebookLoggingService.trace('textModelEdits', `${append ? 'appending' : 'replacing'} ${items.length} output items to for output index ${outputIndex}`); if (append) { output.appendData(items); } else { diff --git a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts index e2b18cb470d..9b553ef1de8 100644 --- a/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts +++ b/src/vs/workbench/contrib/notebook/common/model/notebookTextModel.ts @@ -23,8 +23,9 @@ import { IModelContentChangedEvent } from '../../../../../editor/common/textMode import { IResourceUndoRedoElement, IUndoRedoElement, IUndoRedoService, IWorkspaceUndoRedoElement, UndoRedoElementType, UndoRedoGroup } from '../../../../../platform/undoRedo/common/undoRedo.js'; import { ILanguageDetectionService } from '../../../../services/languageDetection/common/languageDetectionWorkerService.js'; import { SnapshotContext } from '../../../../services/workingCopy/common/fileWorkingCopy.js'; -import { CellEditType, CellKind, CellUri, diff, ICell, ICellDto2, ICellEditOperation, ICellOutput, INotebookSnapshotOptions, INotebookTextModel, IOutputDto, IOutputItemDto, ISelectionState, NotebookCellCollapseState, NotebookCellDefaultCollapseConfig, NotebookCellExecutionState, NotebookCellInternalMetadata, NotebookCellMetadata, NotebookCellOutputsSplice, NotebookCellsChangeType, NotebookCellTextModelSplice, NotebookData, NotebookDocumentMetadata, NotebookTextModelChangedEvent, NotebookTextModelWillAddRemoveEvent, NullablePartialNotebookCellInternalMetadata, NullablePartialNotebookCellMetadata, TransientOptions } from '../notebookCommon.js'; +import { CellEditType, CellUri, diff, ICell, ICellDto2, ICellEditOperation, ICellOutput, INotebookSnapshotOptions, INotebookTextModel, IOutputDto, IOutputItemDto, ISelectionState, NotebookCellDefaultCollapseConfig, NotebookCellExecutionState, NotebookCellInternalMetadata, NotebookCellMetadata, NotebookCellOutputsSplice, NotebookCellsChangeType, NotebookCellTextModelSplice, NotebookData, NotebookDocumentMetadata, NotebookTextModelChangedEvent, NotebookTextModelWillAddRemoveEvent, NullablePartialNotebookCellInternalMetadata, NullablePartialNotebookCellMetadata, TransientOptions } from '../notebookCommon.js'; import { INotebookExecutionStateService } from '../notebookExecutionStateService.js'; +import { INotebookLoggingService } from '../notebookLoggingService.js'; import { CellMetadataEdit, MoveCellEdit, SpliceCellsEdit } from './cellEdit.js'; import { NotebookCellOutputTextModel } from './notebookCellOutputTextModel.js'; import { NotebookCellTextModel } from './notebookCellTextModel.js'; @@ -252,6 +253,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel @ILanguageService private readonly _languageService: ILanguageService, @ILanguageDetectionService private readonly _languageDetectionService: ILanguageDetectionService, @INotebookExecutionStateService private readonly _notebookExecutionStateService: INotebookExecutionStateService, + @INotebookLoggingService private readonly _notebookLoggingService: INotebookLoggingService ) { super(); this.transientOptions = options; @@ -309,6 +311,8 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel this._overwriteAlternativeVersionId(alternativeVersionId); } ); + + this._notebookLoggingService.trace('notebookTextModel', `Initialized notebook text model for ${uri.toString()}`); } setCellCollapseDefault(collapseConfig: NotebookCellDefaultCollapseConfig | undefined) { @@ -323,9 +327,17 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel const mainCells = cells.map(cell => { const cellHandle = this._cellhandlePool++; const cellUri = CellUri.generate(this.uri, cellHandle); - const collapseState = this._getDefaultCollapseState(cell); - return new NotebookCellTextModel(cellUri, cellHandle, cell.source, cell.language, cell.mime, cell.cellKind, cell.outputs, cell.metadata, cell.internalMetadata, collapseState, this.transientOptions, this._languageService, - this._modelService.getCreationOptions(cell.language, cellUri, false).defaultEOL, this._languageDetectionService); + return new NotebookCellTextModel( + cellUri, + cellHandle, + cell, + this.transientOptions, + this._languageService, + this._modelService.getCreationOptions(cell.language, cellUri, false).defaultEOL, + this._defaultCollapseConfig, + this._languageDetectionService, + this._notebookLoggingService + ); }); for (let i = 0; i < mainCells.length; i++) { @@ -621,6 +633,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel } applyEdits(rawEdits: ICellEditOperation[], synchronous: boolean, beginSelectionState: ISelectionState | undefined, endSelectionsComputer: () => ISelectionState | undefined, undoRedoGroup: UndoRedoGroup | undefined, computeUndoRedo: boolean): boolean { + this._notebookLoggingService.trace('textModelEdits', `Begin applying ${rawEdits.length} raw edits`); this._pauseableEmitter.pause(); try { this._operationManager.pushStackElement(this._alternativeVersionId, undefined); @@ -648,6 +661,7 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel // Broadcast changes this._pauseableEmitter.fire({ rawEvents: [], versionId: this.versionId, synchronous: synchronous, endSelectionState: endSelections }); + this._notebookLoggingService.trace('textModelEdits', `End applying ${rawEdits.length} raw edits`); } } } finally { @@ -821,11 +835,6 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel return mergedEdits; } - private _getDefaultCollapseState(cellDto: ICellDto2): NotebookCellCollapseState | undefined { - const defaultConfig = cellDto.cellKind === CellKind.Code ? this._defaultCollapseConfig?.codeCell : this._defaultCollapseConfig?.markupCell; - return cellDto.collapseState ?? (defaultConfig ?? undefined); - } - private _replaceCells(index: number, count: number, cellDtos: ICellDto2[], synchronous: boolean, computeUndoRedo: boolean, beginSelectionState: ISelectionState | undefined, undoRedoGroup: UndoRedoGroup | undefined): void { if (count === 0 && cellDtos.length === 0) { @@ -849,13 +858,19 @@ export class NotebookTextModel extends Disposable implements INotebookTextModel const cells = cellDtos.map(cellDto => { const cellHandle = this._cellhandlePool++; const cellUri = CellUri.generate(this.uri, cellHandle); - const collapseState = this._getDefaultCollapseState(cellDto); + if (!cellDto.outputs) { + cellDto.outputs = []; + } const cell = new NotebookCellTextModel( - cellUri, cellHandle, - cellDto.source, cellDto.language, cellDto.mime, cellDto.cellKind, cellDto.outputs || [], cellDto.metadata, cellDto.internalMetadata, collapseState, this.transientOptions, + cellUri, + cellHandle, + cellDto, + this.transientOptions, this._languageService, this._modelService.getCreationOptions(cellDto.language, cellUri, false).defaultEOL, - this._languageDetectionService + this._defaultCollapseConfig, + this._languageDetectionService, + this._notebookLoggingService ); const textModel = this._modelService.getModel(cellUri); if (textModel && textModel instanceof TextModel) { diff --git a/src/vs/workbench/contrib/notebook/common/notebookLoggingService.ts b/src/vs/workbench/contrib/notebook/common/notebookLoggingService.ts index 148f31569ec..f851c332755 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookLoggingService.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookLoggingService.ts @@ -13,4 +13,5 @@ export interface INotebookLoggingService { warn(category: string, output: string): void; error(category: string, output: string): void; debug(category: string, output: string): void; + trace(category: string, output: string): void; } diff --git a/src/vs/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts b/src/vs/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts index d12be746e53..44c613b094f 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/notebookViewModel.test.ts @@ -32,6 +32,7 @@ import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/tes import { mainWindow } from '../../../../../base/browser/window.js'; import { ICodeEditorService } from '../../../../../editor/browser/services/codeEditorService.js'; import { ILanguageDetectionService } from '../../../../services/languageDetection/common/languageDetectionWorkerService.js'; +import { INotebookLoggingService } from '../../common/notebookLoggingService.js'; suite('NotebookViewModel', () => { ensureNoDisposablesAreLeakedInTestSuite(); @@ -45,6 +46,7 @@ suite('NotebookViewModel', () => { let languageService: ILanguageService; let languageDetectionService: ILanguageDetectionService; let notebookExecutionStateService: INotebookExecutionStateService; + let notebookLogger: INotebookLoggingService; suiteSetup(() => { disposables = new DisposableStore(); @@ -56,6 +58,7 @@ suite('NotebookViewModel', () => { languageService = instantiationService.get(ILanguageService); languageDetectionService = instantiationService.get(ILanguageDetectionService); notebookExecutionStateService = instantiationService.get(INotebookExecutionStateService); + notebookLogger = instantiationService.get(INotebookLoggingService); instantiationService.stub(IConfigurationService, new TestConfigurationService()); instantiationService.stub(IThemeService, new TestThemeService()); @@ -64,7 +67,7 @@ suite('NotebookViewModel', () => { suiteTeardown(() => disposables.dispose()); test('ctor', function () { - const notebook = new NotebookTextModel('notebook', URI.parse('test'), [], {}, { transientCellMetadata: {}, transientDocumentMetadata: {}, transientOutputs: false, cellContentMetadata: {} }, undoRedoService, modelService, languageService, languageDetectionService, notebookExecutionStateService); + const notebook = new NotebookTextModel('notebook', URI.parse('test'), [], {}, { transientCellMetadata: {}, transientDocumentMetadata: {}, transientOutputs: false, cellContentMetadata: {} }, undoRedoService, modelService, languageService, languageDetectionService, notebookExecutionStateService, notebookLogger); const model = new NotebookEditorTestModel(notebook); const options = new NotebookOptions(mainWindow, false, undefined, instantiationService.get(IConfigurationService), instantiationService.get(INotebookExecutionStateService), instantiationService.get(ICodeEditorService)); const eventDispatcher = new NotebookEventDispatcher(); diff --git a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts index 37eb0b35ad1..8731f5e2c3f 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts @@ -71,6 +71,16 @@ import { INotebookOutlineEntryFactory, NotebookOutlineEntryFactory } from '../.. import { IOutlineService } from '../../../../services/outline/browser/outline.js'; import { DefaultEndOfLine } from '../../../../../editor/common/model.js'; import { ITextResourcePropertiesService } from '../../../../../editor/common/services/textResourceConfiguration.js'; +import { INotebookLoggingService } from '../../common/notebookLoggingService.js'; + +class NullNotebookLoggingService implements INotebookLoggingService { + _serviceBrand: undefined; + info(category: string, output: string): void { } + warn(category: string, output: string): void { } + error(category: string, output: string): void { } + debug(category: string, output: string): void { } + trace(category: string, message: string): void { } +} export class TestCell extends NotebookCellTextModel { constructor( @@ -82,7 +92,26 @@ export class TestCell extends NotebookCellTextModel { outputs: IOutputDto[], languageService: ILanguageService, ) { - super(CellUri.generate(URI.parse('test:///fake/notebook'), handle), handle, source, language, Mimes.text, cellKind, outputs, undefined, undefined, undefined, { transientCellMetadata: {}, transientDocumentMetadata: {}, transientOutputs: false, cellContentMetadata: {} }, languageService, DefaultEndOfLine.LF); + super( + CellUri.generate(URI.parse('test:///fake/notebook'), handle), + handle, + { + source, + language, + mime: Mimes.text, + cellKind, + outputs, + metadata: undefined, + internalMetadata: undefined, + collapseState: undefined + }, + { transientCellMetadata: {}, transientDocumentMetadata: {}, transientOutputs: false, cellContentMetadata: {} }, + languageService, + DefaultEndOfLine.LF, + undefined, // defaultCollapseConfig + undefined, // languageDetectionService + new NullNotebookLoggingService() + ); } } @@ -207,6 +236,7 @@ export function setupInstantiationService(disposables: Pick() { override registerOutlineCreator() { return { dispose() { } }; } }); instantiationService.stub(INotebookCellOutlineDataSourceFactory, instantiationService.createInstance(NotebookCellOutlineDataSourceFactory)); instantiationService.stub(INotebookOutlineEntryFactory, instantiationService.createInstance(NotebookOutlineEntryFactory)); + instantiationService.stub(INotebookLoggingService, new NullNotebookLoggingService()); instantiationService.stub(ILanguageDetectionService, new class MockLanguageDetectionService implements ILanguageDetectionService { _serviceBrand: undefined;