-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix leaking comment thread when CellComment is reused. #214589
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove accidental change in test/automation/package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have one question:
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you! I'll just let @rebornix re-review.
@rebornix Are you still happy with the changes I made in response to other reviewers' comments? Can this change be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@rehmsen sorry for the late response, the code looks great! Thank you for the contribution ❤️ |
…ecycle Fix leaking comment thread when CellComment is reused.
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 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.
…ecycle Fix leaking comment thread when CellComment is reused.
…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.
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.