Skip to content

Commit

Permalink
Merge pull request #214589 from rehmsen/notebook_comment_lifecycle
Browse files Browse the repository at this point in the history 
Fix leaking comment thread when CellComment is reused.
  • Loading branch information
@rebornix
rebornix authored Jun 24, 2024
2 parents 36dc195 + 52f062f commit a5fea0c
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 50 deletions.
3 changes: 2 additions & 1 deletion src/vs/workbench/contrib/comments/browser/commentThreadHeader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -46,6 +46,7 @@ export class CommentThreadHeader<T = IRange> extends Disposable {
super();
this._headElement = <HTMLDivElement>dom.$('.head');
container.appendChild(this._headElement);
this._register(toDisposable(() => this._headElement.remove()));
this._fillHead();
}

Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -104,6 +104,7 @@ export class CommentThreadWidget<T extends IRange | ICellRange = IRange> extends

const bodyElement = <HTMLDivElement>dom.$('.body');
container.appendChild(bodyElement);
this._register(toDisposable(() => bodyElement.remove()));

const tracker = this._register(dom.trackFocus(bodyElement));
this._register(registerNavigableContainer({
Expand Down
94 changes: 46 additions & 48 deletions src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<ICellRange> | null = null;
private readonly _commentThreadWidget = new MutableDisposable<CommentThreadWidget<ICellRange>>;
private currentElement: CodeCellViewModel | undefined;
private readonly commentTheadDisposables = this._register(new DisposableStore());
private readonly _commentThreadDisposables = this._register(new DisposableStore());

constructor(
private readonly notebookEditor: INotebookEditorDelegate,
Expand All @@ -45,22 +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;
await this._updateThread();
}

private async _createCommentTheadWidget(owner: string, commentThread: languages.CommentThread<ICellRange>) {
this._commentThreadWidget?.dispose();
this.commentTheadDisposables.clear();
this._commentThreadWidget = this.instantiationService.createInstance(
this._commentThreadDisposables.clear();
this._commentThreadWidget.value = this.instantiationService.createInstance(
CommentThreadWidget,
this.container,
this.notebookEditor,
Expand All @@ -84,44 +78,48 @@ 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.commentTheadDisposables.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);
}
}));
}

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;
}

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;
}

await this._commentThreadWidget.updateCommentThread(info.thread);
this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.getDimensions().height);
}
this.cellDisposables.add(this.commentService.onDidUpdateCommentThreads(async () => this._updateThread()));
}

private async _updateThread() {
if (!this.currentElement) {
return;
}
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.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value!.getDimensions().height);
return;
}

if (this._commentThreadWidget.value) {
if (!info) {
this._commentThreadDisposables.clear();
this._commentThreadWidget.dispose();
this.currentElement.commentHeight = 0;
return;
}
}));
if (this._commentThreadWidget.value.commentThread === info.thread) {
this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height);
return;
}

await this._commentThreadWidget.value.updateCommentThread(info.thread);
this.currentElement.commentHeight = this._calculateCommentThreadHeight(this._commentThreadWidget.value.getDimensions().height);
}
}

private _calculateCommentThreadHeight(bodyHeight: number) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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`;
}
Expand Down

0 comments on commit a5fea0c

Please sign in to comment.