Fix leaking comment thread when CellComment is reused.#214589
Merged
rebornix merged 5 commits intomicrosoft:mainfrom Jun 24, 2024
Merged
Fix leaking comment thread when CellComment is reused.#214589rebornix merged 5 commits intomicrosoft:mainfrom
rebornix merged 5 commits intomicrosoft:mainfrom
Conversation
rehmsen
commented
Jun 7, 2024
Contributor
Author
rehmsen
left a comment
rehmsen
left a comment
There was a problem hiding this comment.
Remove accidental change in test/automation/package.json.
Contributor
Author
|
Hi @alexr00, did you have a chance to look at this? Do you have any questions? It would be cool to get this fix in. Thanks! |
alexr00
reviewed
Jun 17, 2024
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
Tyriar
previously approved these changes
Jun 17, 2024
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
alexr00
reviewed
Jun 18, 2024
Member
alexr00
left a comment
alexr00
left a comment
There was a problem hiding this comment.
Other than the change to test/automation/package.json I'm happy with the change!
I wonder if VSCode or some extension I have keeps updating this? I am not doing it handishly. Anyways, gone again.
Contributor
Author
|
@rebornix Are you still happy with the changes I made in response to other reviewers' comments? Can this change be merged? |
Member
|
@rehmsen sorry for the late response, the code looks great! Thank you for the contribution ❤️ |
bricefriha
pushed a commit
to bricefriha/vscode
that referenced
this pull request
Jun 26, 2024
…ecycle Fix leaking comment thread when CellComment is reused.
rehmsen
added a commit
to rehmsen/vscode
that referenced
this pull request
Jun 26, 2024
1. We must not `dispose()` the `MutableDisposable` `this._commentThreadWidget` - as that disposes the MutableDisposable itself, and it cannot then later be reassigned to a new value. Instead, we need to assign `value=undefined`, which disposes the previous `value`, but keeps the `MutableDisposable` available to be reused later. 2. `initialize()` is a no-op if `this.currentElement` is already identical to the passed `element`, so we must not do that assignment before calling initialize - instead `initialize()` does the assignment after checking.
alexr00
pushed a commit
that referenced
this pull request
Jun 26, 2024
* Fix two bugs in #214589. 1. We must not `dispose()` the `MutableDisposable` `this._commentThreadWidget` - as that disposes the MutableDisposable itself, and it cannot then later be reassigned to a new value. Instead, we need to assign `value=undefined`, which disposes the previous `value`, but keeps the `MutableDisposable` available to be reused later. 2. `initialize()` is a no-op if `this.currentElement` is already identical to the passed `element`, so we must not do that assignment before calling initialize - instead `initialize()` does the assignment after checking. * Fix blank line. * Register the _commentThreadWidget MutableDisposable so that it gets disposed when CellComments is disposed.
aaronchucarroll
pushed a commit
to aaronchucarroll/vscode
that referenced
this pull request
Jul 10, 2024
…ecycle Fix leaking comment thread when CellComment is reused.
aaronchucarroll
pushed a commit
to aaronchucarroll/vscode
that referenced
this pull request
Jul 10, 2024
…218357) * Fix two bugs in microsoft#214589. 1. We must not `dispose()` the `MutableDisposable` `this._commentThreadWidget` - as that disposes the MutableDisposable itself, and it cannot then later be reassigned to a new value. Instead, we need to assign `value=undefined`, which disposes the previous `value`, but keeps the `MutableDisposable` available to be reused later. 2. `initialize()` is a no-op if `this.currentElement` is already identical to the passed `element`, so we must not do that assignment before calling initialize - instead `initialize()` does the assignment after checking. * Fix blank line. * Register the _commentThreadWidget MutableDisposable so that it gets disposed when CellComments is disposed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #214585.
As none of the involved classes are covered by unit tests, to test this one needs to create a notebook with a code cell, then a page long markup cell, then a code cell, so that the two code cells are not visible at the same time, and the renderer reuses the same CellComments for them as you scroll up and down. Then you need to put a notebook comment onto the first cell. Before this change, that comment also is shown incorrectly on the second code cell when scrolling down as it is not correctly cleaned up. with this change, this does not happen, the CommentThreadWidget and associated DOM elements are removed, and then added back again cleanly when scrolling back up to the first code cell.