From 8c82ca149cd36c6a269ed3d765b7791bded87334 Mon Sep 17 00:00:00 2001 From: Ole Date: Fri, 7 Jun 2024 13:50:01 +0200 Subject: [PATCH 1/4] Fix leaking comment thread when CellComment is reused. Fixes #214585. --- .../comments/browser/commentThreadHeader.ts | 3 +- .../comments/browser/commentThreadWidget.ts | 3 +- .../browser/view/cellParts/cellComments.ts | 70 +++++++++---------- test/automation/package.json | 4 +- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/src/vs/workbench/contrib/comments/browser/commentThreadHeader.ts b/src/vs/workbench/contrib/comments/browser/commentThreadHeader.ts index 2849c9afd76..8333654958e 100644 --- a/src/vs/workbench/contrib/comments/browser/commentThreadHeader.ts +++ b/src/vs/workbench/contrib/comments/browser/commentThreadHeader.ts @@ -7,7 +7,7 @@ import * as dom from 'vs/base/browser/dom'; import { ActionBar } from 'vs/base/browser/ui/actionbar/actionbar'; import { Action, ActionRunner } from 'vs/base/common/actions'; import { Codicon } from 'vs/base/common/codicons'; -import { Disposable } from 'vs/base/common/lifecycle'; +import { Disposable, toDisposable } from 'vs/base/common/lifecycle'; import * as strings from 'vs/base/common/strings'; import * as languages from 'vs/editor/common/languages'; import { IRange } from 'vs/editor/common/core/range'; @@ -46,6 +46,7 @@ export class CommentThreadHeader extends Disposable { super(); this._headElement = dom.$('.head'); container.appendChild(this._headElement); + this._register(toDisposable(() => this._headElement.remove())); this._fillHead(); } diff --git a/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts b/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts index a919fe262a9..521455bb77a 100644 --- a/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts +++ b/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts @@ -6,7 +6,7 @@ import 'vs/css!./media/review'; import * as dom from 'vs/base/browser/dom'; import { Emitter } from 'vs/base/common/event'; -import { Disposable, dispose, IDisposable } from 'vs/base/common/lifecycle'; +import { Disposable, dispose, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import * as languages from 'vs/editor/common/languages'; import { IMarkdownRendererOptions } from 'vs/editor/browser/widget/markdownRenderer/browser/markdownRenderer'; @@ -104,6 +104,7 @@ export class CommentThreadWidget extends const bodyElement = dom.$('.body'); container.appendChild(bodyElement); + this._register(toDisposable(() => bodyElement.remove())); const tracker = this._register(dom.trackFocus(bodyElement)); this._register(registerNavigableContainer({ diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts index b8ccbf07abf..7e4ad29842b 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts @@ -20,10 +20,9 @@ import { CellKind } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { ICellRange } from 'vs/workbench/contrib/notebook/common/notebookRange'; export class CellComments extends CellContentPart { - private _initialized: boolean = false; private _commentThreadWidget: CommentThreadWidget | null = null; private currentElement: CodeCellViewModel | undefined; - private readonly commentTheadDisposables = this._register(new DisposableStore()); + private readonly _commentThreadDisposables = this._register(new DisposableStore()); constructor( private readonly notebookEditor: INotebookEditorDelegate, @@ -45,21 +44,17 @@ export class CellComments extends CellContentPart { } private async initialize(element: ICellViewModel) { - if (this._initialized) { + if (this.currentElement === element) { return; } - this._initialized = true; - const info = await this._getCommentThreadForCell(element); - - if (info) { - await this._createCommentTheadWidget(info.owner, info.thread); - } + this.currentElement = element as CodeCellViewModel; + this._updateThread(); } private async _createCommentTheadWidget(owner: string, commentThread: languages.CommentThread) { this._commentThreadWidget?.dispose(); - this.commentTheadDisposables.clear(); + this._commentThreadDisposables.clear(); this._commentThreadWidget = this.instantiationService.createInstance( CommentThreadWidget, this.container, @@ -87,7 +82,7 @@ export class CellComments extends CellContentPart { await this._commentThreadWidget.display(layoutInfo.fontInfo.lineHeight, true); this._applyTheme(); - this.commentTheadDisposables.add(this._commentThreadWidget.onDidResize(() => { + this._commentThreadDisposables.add(this._commentThreadWidget.onDidResize(() => { if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget) { this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); } @@ -95,33 +90,38 @@ export class CellComments extends CellContentPart { } private _bindListeners() { - this.cellDisposables.add(this.commentService.onDidUpdateCommentThreads(async () => { - if (this.currentElement) { - const info = await this._getCommentThreadForCell(this.currentElement); - if (!this._commentThreadWidget && 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.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget!.getDimensions().height); - return; - } + this.cellDisposables.add(this.commentService.onDidUpdateCommentThreads(async () => this._updateThread())); + } - if (this._commentThreadWidget) { - if (!info) { - this._commentThreadWidget.dispose(); - this.currentElement.commentHeight = 0; - return; - } - if (this._commentThreadWidget.commentThread === info.thread) { - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); - return; - } + private async _updateThread() { + if (!this.currentElement) { + return; + } + const info = await this._getCommentThreadForCell(this.currentElement); + if (!this._commentThreadWidget && 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.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget!.getDimensions().height); + return; + } - await this._commentThreadWidget.updateCommentThread(info.thread); - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); - } + if (this._commentThreadWidget) { + if (!info) { + this._commentThreadDisposables.clear(); + this._commentThreadWidget.dispose(); + this._commentThreadWidget = null; + this.currentElement.commentHeight = 0; + return; } - })); + if (this._commentThreadWidget.commentThread === info.thread) { + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + return; + } + + await this._commentThreadWidget.updateCommentThread(info.thread); + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + } } private _calculateCommentThreadHeight(bodyHeight: number) { diff --git a/test/automation/package.json b/test/automation/package.json index b9dbbec4bb8..8172250a9a3 100644 --- a/test/automation/package.json +++ b/test/automation/package.json @@ -1,6 +1,6 @@ { "name": "vscode-automation", - "version": "1.71.0", + "version": "1.91.0", "description": "VS Code UI automation driver", "author": { "name": "Microsoft Corporation" @@ -33,4 +33,4 @@ "npm-run-all": "^4.1.5", "watch": "^1.0.2" } -} +} \ No newline at end of file From f68fa7e09f252a6121d110bdd0664a7f4e64e1ad Mon Sep 17 00:00:00 2001 From: Ole Date: Fri, 7 Jun 2024 14:01:44 +0200 Subject: [PATCH 2/4] Undo accidental change in test/automation/package.json. --- test/automation/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/automation/package.json b/test/automation/package.json index 8172250a9a3..b9dbbec4bb8 100644 --- a/test/automation/package.json +++ b/test/automation/package.json @@ -1,6 +1,6 @@ { "name": "vscode-automation", - "version": "1.91.0", + "version": "1.71.0", "description": "VS Code UI automation driver", "author": { "name": "Microsoft Corporation" @@ -33,4 +33,4 @@ "npm-run-all": "^4.1.5", "watch": "^1.0.2" } -} \ No newline at end of file +} From 825680cf03c8fae03fba14152475c0e5e2d541b5 Mon Sep 17 00:00:00 2001 From: Ole Date: Mon, 17 Jun 2024 17:04:53 +0200 Subject: [PATCH 3/4] Use MutableDisposable and await _updateThread. --- .../browser/view/cellParts/cellComments.ts | 40 +++++++++---------- test/automation/package.json | 4 +- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts index 7e4ad29842b..cbf886228ef 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { coalesce } from 'vs/base/common/arrays'; -import { DisposableStore } from 'vs/base/common/lifecycle'; +import { DisposableStore, MutableDisposable } from 'vs/base/common/lifecycle'; import { EDITOR_FONT_DEFAULTS, IEditorOptions } from 'vs/editor/common/config/editorOptions'; import * as languages from 'vs/editor/common/languages'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; @@ -20,7 +20,7 @@ import { CellKind } from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { ICellRange } from 'vs/workbench/contrib/notebook/common/notebookRange'; export class CellComments extends CellContentPart { - private _commentThreadWidget: CommentThreadWidget | null = null; + private readonly _commentThreadWidget = new MutableDisposable>; private currentElement: CodeCellViewModel | undefined; private readonly _commentThreadDisposables = this._register(new DisposableStore()); @@ -49,13 +49,12 @@ export class CellComments extends CellContentPart { } this.currentElement = element as CodeCellViewModel; - this._updateThread(); + await this._updateThread(); } private async _createCommentTheadWidget(owner: string, commentThread: languages.CommentThread) { - this._commentThreadWidget?.dispose(); this._commentThreadDisposables.clear(); - this._commentThreadWidget = this.instantiationService.createInstance( + this._commentThreadWidget.value = this.instantiationService.createInstance( CommentThreadWidget, this.container, this.notebookEditor, @@ -79,12 +78,12 @@ export class CellComments extends CellContentPart { const layoutInfo = this.notebookEditor.getLayoutInfo(); - await this._commentThreadWidget.display(layoutInfo.fontInfo.lineHeight, true); + await this._commentThreadWidget.value.display(layoutInfo.fontInfo.lineHeight, true); this._applyTheme(); - this._commentThreadDisposables.add(this._commentThreadWidget.onDidResize(() => { - if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget) { - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + this._commentThreadDisposables.add(this._commentThreadWidget.value.onDidResize(() => { + if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget.value) { + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height); } })); } @@ -98,29 +97,28 @@ export class CellComments extends CellContentPart { return; } const info = await this._getCommentThreadForCell(this.currentElement); - if (!this._commentThreadWidget && info) { + 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.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget!.getDimensions().height); + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value!.getDimensions().height); return; } - if (this._commentThreadWidget) { + if (this._commentThreadWidget.value) { if (!info) { this._commentThreadDisposables.clear(); this._commentThreadWidget.dispose(); - this._commentThreadWidget = null; this.currentElement.commentHeight = 0; return; } - if (this._commentThreadWidget.commentThread === info.thread) { - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + if (this._commentThreadWidget.value.commentThread === info.thread) { + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height); return; } - await this._commentThreadWidget.updateCommentThread(info.thread); - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + await this._commentThreadWidget.value.updateCommentThread(info.thread); + this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height); } } @@ -151,7 +149,7 @@ export class CellComments extends CellContentPart { private _applyTheme() { const theme = this.themeService.getColorTheme(); const fontInfo = this.notebookEditor.getLayoutInfo().fontInfo; - this._commentThreadWidget?.applyTheme(theme, fontInfo); + this._commentThreadWidget.value?.applyTheme(theme, fontInfo); } override didRenderCell(element: ICellViewModel): void { @@ -164,13 +162,13 @@ export class CellComments extends CellContentPart { } override prepareLayout(): void { - if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget) { - this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height); + if (this.currentElement?.cellKind === CellKind.Code && 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) { + if (this.currentElement?.cellKind === CellKind.Code && this._commentThreadWidget.value) { const layoutInfo = (element as CodeCellViewModel).layoutInfo; this.container.style.top = `${layoutInfo.outputContainerOffset + layoutInfo.outputTotalHeight}px`; } diff --git a/test/automation/package.json b/test/automation/package.json index b9dbbec4bb8..8172250a9a3 100644 --- a/test/automation/package.json +++ b/test/automation/package.json @@ -1,6 +1,6 @@ { "name": "vscode-automation", - "version": "1.71.0", + "version": "1.91.0", "description": "VS Code UI automation driver", "author": { "name": "Microsoft Corporation" @@ -33,4 +33,4 @@ "npm-run-all": "^4.1.5", "watch": "^1.0.2" } -} +} \ No newline at end of file From 52f062f709290eec516dbf5db8562ef33d5ef5b9 Mon Sep 17 00:00:00 2001 From: Ole Date: Tue, 18 Jun 2024 11:11:37 +0200 Subject: [PATCH 4/4] Revert again the unintentional change to test/automation/package.json. I wonder if VSCode or some extension I have keeps updating this? I am not doing it handishly. Anyways, gone again. --- test/automation/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/automation/package.json b/test/automation/package.json index 8172250a9a3..b9dbbec4bb8 100644 --- a/test/automation/package.json +++ b/test/automation/package.json @@ -1,6 +1,6 @@ { "name": "vscode-automation", - "version": "1.91.0", + "version": "1.71.0", "description": "VS Code UI automation driver", "author": { "name": "Microsoft Corporation" @@ -33,4 +33,4 @@ "npm-run-all": "^4.1.5", "watch": "^1.0.2" } -} \ No newline at end of file +}