Merge pull request #219657 from rehmsen/markup_cell_comments

Support comments on notebook markup cells
2026-05-25 09:50:51 +01:00 · 2024-07-03 10:01:48 -07:00
parent 7a580b79e5 facf5e0650
commit baafc8eaaa
5 changed files with 101 additions and 112 deletions
@@ -175,47 +175,47 @@ export enum CellLayoutState {
 	Measured
 }

-export interface CodeCellLayoutInfo {
+/** LayoutInfo of the parts that are shared between all cell types. */
+export interface CellLayoutInfo {
+	readonly layoutState: CellLayoutState;
 	readonly fontInfo: FontInfo | null;
 	readonly chatHeight: number;
-	readonly editorHeight: number;
 	readonly editorWidth: number;
-	readonly estimatedHasHorizontalScrolling: boolean;
+	readonly editorHeight: number;
 	readonly statusBarHeight: number;
+	readonly commentOffset: number;
 	readonly commentHeight: number;
+	readonly bottomToolbarOffset: number;
 	readonly totalHeight: number;
+}
+
+export interface CellLayoutChangeEvent {
+	readonly font?: FontInfo;
+	readonly outerWidth?: number;
+	readonly commentHeight?: boolean;
+}
+
+export interface CodeCellLayoutInfo extends CellLayoutInfo {
+	readonly estimatedHasHorizontalScrolling: boolean;
 	readonly outputContainerOffset: number;
 	readonly outputTotalHeight: number;
 	readonly outputShowMoreContainerHeight: number;
 	readonly outputShowMoreContainerOffset: number;
-	readonly bottomToolbarOffset: number;
-	readonly layoutState: CellLayoutState;
 	readonly codeIndicatorHeight: number;
 	readonly outputIndicatorHeight: number;
 }

-export interface CodeCellLayoutChangeEvent {
+export interface CodeCellLayoutChangeEvent extends CellLayoutChangeEvent {
 	readonly source?: string;
 	readonly chatHeight?: boolean;
 	readonly editorHeight?: boolean;
-	readonly commentHeight?: boolean;
 	readonly outputHeight?: boolean;
 	readonly outputShowMoreContainerHeight?: number;
 	readonly totalHeight?: boolean;
-	readonly outerWidth?: number;
-	readonly font?: FontInfo;
 }

-export interface MarkupCellLayoutInfo {
-	readonly fontInfo: FontInfo | null;
-	readonly chatHeight: number;
-	readonly editorWidth: number;
-	readonly editorHeight: number;
-	readonly statusBarHeight: number;
+export interface MarkupCellLayoutInfo extends CellLayoutInfo {
 	readonly previewHeight: number;
-	readonly bottomToolbarOffset: number;
-	readonly totalHeight: number;
-	readonly layoutState: CellLayoutState;
 	readonly foldHintHeight: number;
 }

@@ -223,9 +223,7 @@ export enum CellLayoutContext {
 	Fold
 }

-export interface MarkupCellLayoutChangeEvent {
-	readonly font?: FontInfo;
-	readonly outerWidth?: number;
+export interface MarkupCellLayoutChangeEvent extends CellLayoutChangeEvent {
 	readonly editorHeight?: number;
 	readonly previewHeight?: number;
 	totalHeight?: number;
@@ -241,7 +239,7 @@ export interface ICellViewModel extends IGenericCellViewModel {
 	readonly model: NotebookCellTextModel;
 	readonly id: string;
 	readonly textBuffer: IReadonlyTextBuffer;
-	readonly layoutInfo: { totalHeight: number; bottomToolbarOffset: number; editorWidth: number; editorHeight: number; statusBarHeight: number; chatHeight: number };
+	readonly layoutInfo: CellLayoutInfo;
 	readonly onDidChangeLayout: Event<ICommonCellViewModelLayoutChangeInfo>;
 	readonly onDidChangeCellStatusBarItems: Event<void>;
 	readonly onCellDecorationsChanged: Event<{ added: INotebookCellDecorationOptions[]; removed: INotebookCellDecorationOptions[] }>;
@@ -259,6 +257,7 @@ export interface ICellViewModel extends IGenericCellViewModel {
 	cellKind: CellKind;
 	lineNumbers: 'on' | 'off' | 'inherit';
 	chatHeight: number;
+	commentHeight: number;
 	focusMode: CellFocusMode;
 	focusedOutputId?: string | undefined;
 	outputIsHovered: boolean;
@@ -15,13 +15,11 @@ import { ICommentService } from 'vs/workbench/contrib/comments/browser/commentSe
 import { CommentThreadWidget } from 'vs/workbench/contrib/comments/browser/commentThreadWidget';
 import { ICellViewModel, INotebookEditorDelegate } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
 import { CellContentPart } from 'vs/workbench/contrib/notebook/browser/view/cellPart';
-import { CodeCellViewModel } from 'vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel';
-import { CellKind } from 'vs/workbench/contrib/notebook/common/notebookCommon';
 import { ICellRange } from 'vs/workbench/contrib/notebook/common/notebookRange';

 export class CellComments extends CellContentPart {
 	private readonly _commentThreadWidget: MutableDisposable<CommentThreadWidget<ICellRange>>;
-	private currentElement: CodeCellViewModel | undefined;
+	private currentElement: ICellViewModel | undefined;
 	private readonly _commentThreadDisposables = this._register(new DisposableStore());

 	constructor(
@@ -49,7 +47,7 @@ export class CellComments extends CellContentPart {
 			return;
 		}

-		this.currentElement = element as CodeCellViewModel;
+		this.currentElement = element;
 		await this._updateThread();
 	}

@@ -83,7 +81,7 @@ export class CellComments extends CellContentPart {
 		this._applyTheme();

 		this._commentThreadDisposables.add(this._commentThreadWidget.value.onDidResize(() => {
-			if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget.value) {
+			if (this.currentElement && this._commentThreadWidget.value) {
 				this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height);
 			}
 		}));
@@ -100,8 +98,7 @@ export class CellComments extends CellContentPart {
 		const info = await this._getCommentThreadForCell(this.currentElement);
 		if (!this._commentThreadWidget.value && info) {
 			await this._createCommentTheadWidget(info.owner, info.thread);
-			const layoutInfo = (this.currentElement as CodeCellViewModel).layoutInfo;
-			this.container.style.top = `${layoutInfo.outputContainerOffset + layoutInfo.outputTotalHeight}px`;
+			this.container.style.top = `${this.currentElement.layoutInfo.commentOffset}px`;
 			this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value!.getDimensions().height);
 			return;
 		}
@@ -154,23 +151,19 @@ export class CellComments extends CellContentPart {
 	}

 	override didRenderCell(element: ICellViewModel): void {
-		if (element.cellKind === CellKind.Code) {
-			this.initialize(element);
-			this._bindListeners();
-		}
-
+		this.initialize(element);
+		this._bindListeners();
 	}

 	override prepareLayout(): void {
-		if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget.value) {
+		if (this.currentElement && this._commentThreadWidget.value) {
 			this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height);
 		}
 	}

 	override updateInternalLayoutNow(element: ICellViewModel): void {
-		if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget.value) {
-			const layoutInfo = (element as CodeCellViewModel).layoutInfo;
-			this.container.style.top = `${layoutInfo.outputContainerOffset + layoutInfo.outputTotalHeight}px`;
+		if (this.currentElement && this._commentThreadWidget.value) {
+			this.container.style.top = `${element.layoutInfo.commentOffset}px`;
 		}
 	}
 }
@@ -19,7 +19,7 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur
 import { IUndoRedoService } from 'vs/platform/undoRedo/common/undoRedo';
 import { IWordWrapTransientState, readTransientState, writeTransientState } from 'vs/workbench/contrib/codeEditor/browser/toggleWordWrap';
 import { InlineChatController } from 'vs/workbench/contrib/inlineChat/browser/inlineChatController';
-import { CellEditState, CellFocusMode, CursorAtBoundary, CursorAtLineBoundary, IEditableCellViewModel, INotebookCellDecorationOptions } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
+import { CellEditState, CellFocusMode, CellLayoutChangeEvent, CursorAtBoundary, CursorAtLineBoundary, IEditableCellViewModel, INotebookCellDecorationOptions } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
 import { NotebookOptionsChangeEvent } from 'vs/workbench/contrib/notebook/browser/notebookOptions';
 import { CellViewModelStateChangeEvent } from 'vs/workbench/contrib/notebook/browser/notebookViewEvents';
 import { ViewContext } from 'vs/workbench/contrib/notebook/browser/viewModel/viewContext';
@@ -158,6 +158,16 @@ export abstract class BaseCellViewModel extends Disposable {
 		this._onDidChangeState.fire({ outputCollapsedChanged: true });
 	}

+	protected _commentHeight = 0;
+
+	set commentHeight(height: number) {
+		if (this._commentHeight === height) {
+			return;
+		}
+		this._commentHeight = height;
+		this.layoutChange({ commentHeight: true }, 'BaseCellViewModel#commentHeight');
+	}
+
 	private _isDisposed = false;

 	constructor(
@@ -204,7 +214,7 @@ export abstract class BaseCellViewModel extends Disposable {
 	abstract updateOptions(e: NotebookOptionsChangeEvent): void;
 	abstract getHeight(lineHeight: number): number;
 	abstract onDeselect(): void;
-	abstract layoutChange(change: any): void;
+	abstract layoutChange(change: CellLayoutChangeEvent, source?: string): void;

 	assertTextModelAttached(): boolean {
 		if (this.textModel && this._textEditor && this._textEditor.getModel() === this.textModel) {
@@ -80,16 +80,6 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
 		return this._chatHeight;
 	}

-	private _commentHeight = 0;
-
-	set commentHeight(height: number) {
-		if (this._commentHeight === height) {
-			return;
-		}
-		this._commentHeight = height;
-		this.layoutChange({ commentHeight: true }, 'CodeCellViewModel#commentHeight');
-	}
-
 	private _hoveringOutput: boolean = false;
 	public get outputIsHovered(): boolean {
 		return this._hoveringOutput;
@@ -193,6 +183,7 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
 				: 0,
 			chatHeight: 0,
 			statusBarHeight: 0,
+			commentOffset: 0,
 			commentHeight: 0,
 			outputContainerOffset: 0,
 			outputTotalHeight: 0,
@@ -289,11 +280,12 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
 				editorHeight,
 				editorWidth,
 				statusBarHeight,
-				commentHeight,
 				outputContainerOffset,
 				outputTotalHeight,
 				outputShowMoreContainerHeight,
 				outputShowMoreContainerOffset,
+				commentOffset: outputContainerOffset + outputTotalHeight,
+				commentHeight,
 				totalHeight,
 				codeIndicatorHeight,
 				outputIndicatorHeight,
@@ -330,11 +322,12 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
 				editorWidth,
 				chatHeight: chatHeight,
 				statusBarHeight: 0,
-				commentHeight,
 				outputContainerOffset,
 				outputTotalHeight,
 				outputShowMoreContainerHeight,
 				outputShowMoreContainerOffset,
+				commentOffset: outputContainerOffset + outputTotalHeight,
+				commentHeight,
 				totalHeight,
 				codeIndicatorHeight,
 				outputIndicatorHeight,
@@ -359,22 +352,9 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
 		super.restoreEditorViewState(editorViewStates);
 		if (totalHeight !== undefined && this._layoutInfo.layoutState !== CellLayoutState.Measured) {
 			this._layoutInfo = {
-				fontInfo: this._layoutInfo.fontInfo,
-				chatHeight: this._layoutInfo.chatHeight,
-				editorHeight: this._layoutInfo.editorHeight,
-				editorWidth: this._layoutInfo.editorWidth,
-				statusBarHeight: this.layoutInfo.statusBarHeight,
-				commentHeight: this.layoutInfo.commentHeight,
-				outputContainerOffset: this._layoutInfo.outputContainerOffset,
-				outputTotalHeight: this._layoutInfo.outputTotalHeight,
-				outputShowMoreContainerHeight: this._layoutInfo.outputShowMoreContainerHeight,
-				outputShowMoreContainerOffset: this._layoutInfo.outputShowMoreContainerOffset,
+				...this._layoutInfo,
 				totalHeight: totalHeight,
-				codeIndicatorHeight: this._layoutInfo.codeIndicatorHeight,
-				outputIndicatorHeight: this._layoutInfo.outputIndicatorHeight,
-				bottomToolbarOffset: this._layoutInfo.bottomToolbarOffset,
 				layoutState: CellLayoutState.FromCache,
-				estimatedHasHorizontalScrolling: this._layoutInfo.estimatedHasHorizontalScrolling
 			};
 		}
 	}
@@ -134,6 +134,8 @@ export class MarkupCellViewModel extends BaseCellViewModel implements ICellViewM
 			editorWidth: initialNotebookLayoutInfo?.width
 				? this.viewContext.notebookOptions.computeMarkdownCellEditorWidth(initialNotebookLayoutInfo.width)
 				: 0,
+			commentOffset: 0,
+			commentHeight: 0,
 			bottomToolbarOffset: bottomToolbarGap,
 			totalHeight: 100,
 			layoutState: CellLayoutState.Uninitialized,
@@ -160,13 +162,14 @@ export class MarkupCellViewModel extends BaseCellViewModel implements ICellViewM
 				+ layoutConfiguration.markdownCellTopMargin
 				+ layoutConfiguration.markdownCellBottomMargin
 				+ bottomToolbarGap
-				+ this._statusBarHeight;
+				+ this._statusBarHeight
+				+ this._commentHeight;
 		} else {
 			// @rebornix
 			// On file open, the previewHeight + bottomToolbarGap for a cell out of viewport can be 0
 			// When it's 0, the list view will never try to render it anymore even if we scroll the cell into view.
 			// Thus we make sure it's greater than 0
-			return Math.max(1, this._previewHeight + bottomToolbarGap + foldHintHeight);
+			return Math.max(1, this._previewHeight + bottomToolbarGap + foldHintHeight + this._commentHeight);
 		}
 	}

@@ -204,50 +207,58 @@ export class MarkupCellViewModel extends BaseCellViewModel implements ICellViewM
 	}

 	layoutChange(state: MarkupCellLayoutChangeEvent) {
-		// recompute
-		const foldHintHeight = this._computeFoldHintHeight();
+		let totalHeight: number;
+		let foldHintHeight: number;
 		if (!this.isInputCollapsed) {
-			const editorWidth = state.outerWidth !== undefined
-				? this.viewContext.notebookOptions.computeMarkdownCellEditorWidth(state.outerWidth)
-				: this._layoutInfo.editorWidth;
-			const totalHeight = state.totalHeight === undefined
-				? (this._layoutInfo.layoutState === CellLayoutState.Uninitialized ? 100 : this._layoutInfo.totalHeight)
-				: state.totalHeight;
-			const previewHeight = this._previewHeight;
-
-			this._layoutInfo = {
-				fontInfo: state.font || this._layoutInfo.fontInfo,
-				editorWidth,
-				previewHeight,
-				chatHeight: this._chatHeight,
-				editorHeight: this._editorHeight,
-				statusBarHeight: this._statusBarHeight,
-				bottomToolbarOffset: this.viewContext.notebookOptions.computeBottomToolbarOffset(totalHeight, this.viewType),
-				totalHeight,
-				layoutState: CellLayoutState.Measured,
-				foldHintHeight
-			};
+			totalHeight = state.totalHeight === undefined ?
+				(this._layoutInfo.layoutState ===
+					CellLayoutState.Uninitialized ?
+					100 :
+					this._layoutInfo.totalHeight) :
+				state.totalHeight;
+			// recompute
+			foldHintHeight = this._computeFoldHintHeight();
 		} else {
-			const editorWidth = state.outerWidth !== undefined
-				? this.viewContext.notebookOptions.computeMarkdownCellEditorWidth(state.outerWidth)
-				: this._layoutInfo.editorWidth;
-			const totalHeight = this.viewContext.notebookOptions.computeCollapsedMarkdownCellHeight(this.viewType);
-
+			totalHeight =
+				this.viewContext.notebookOptions
+					.computeCollapsedMarkdownCellHeight(this.viewType);
 			state.totalHeight = totalHeight;

-			this._layoutInfo = {
-				fontInfo: state.font || this._layoutInfo.fontInfo,
-				editorWidth,
-				chatHeight: this._chatHeight,
-				editorHeight: this._editorHeight,
-				statusBarHeight: this._statusBarHeight,
-				previewHeight: this._previewHeight,
-				bottomToolbarOffset: this.viewContext.notebookOptions.computeBottomToolbarOffset(totalHeight, this.viewType),
-				totalHeight,
-				layoutState: CellLayoutState.Measured,
-				foldHintHeight: 0
-			};
+			foldHintHeight = 0;
 		}
+		let commentOffset: number;
+		if (this.getEditState() === CellEditState.Editing) {
+			const notebookLayoutConfiguration = this.viewContext.notebookOptions.getLayoutConfiguration();
+			commentOffset = notebookLayoutConfiguration.editorToolbarHeight
+				+ notebookLayoutConfiguration.cellTopMargin // CELL_TOP_MARGIN
+				+ this._chatHeight
+				+ this._editorHeight
+				+ this._statusBarHeight;
+		} else {
+			commentOffset = this._previewHeight;
+		}
+
+		this._layoutInfo = {
+			fontInfo: state.font || this._layoutInfo.fontInfo,
+			editorWidth: state.outerWidth !== undefined ?
+				this.viewContext.notebookOptions
+					.computeMarkdownCellEditorWidth(state.outerWidth) :
+				this._layoutInfo.editorWidth,
+			chatHeight: this._chatHeight,
+			editorHeight: this._editorHeight,
+			statusBarHeight: this._statusBarHeight,
+			previewHeight: this._previewHeight,
+			bottomToolbarOffset: this.viewContext.notebookOptions
+				.computeBottomToolbarOffset(
+					totalHeight, this.viewType),
+			totalHeight,
+			layoutState: CellLayoutState.Measured,
+			foldHintHeight,
+			commentOffset,
+			commentHeight: state.commentHeight ?
+				this._commentHeight :
+				this._layoutInfo.commentHeight,
+		};

 		this._onDidChangeLayout.fire(state);
 	}
@@ -257,16 +268,12 @@ export class MarkupCellViewModel extends BaseCellViewModel implements ICellViewM
 		// we might already warmup the viewport so the cell has a total height computed
 		if (totalHeight !== undefined && this.layoutInfo.layoutState === CellLayoutState.Uninitialized) {
 			this._layoutInfo = {
-				fontInfo: this._layoutInfo.fontInfo,
-				editorWidth: this._layoutInfo.editorWidth,
-				previewHeight: this._layoutInfo.previewHeight,
-				bottomToolbarOffset: this._layoutInfo.bottomToolbarOffset,
+				...this.layoutInfo,
 				totalHeight: totalHeight,
 				chatHeight: this._chatHeight,
 				editorHeight: this._editorHeight,
 				statusBarHeight: this._statusBarHeight,
 				layoutState: CellLayoutState.FromCache,
-				foldHintHeight: this._layoutInfo.foldHintHeight
 			};
 			this.layoutChange({});
 		}