Skip to content
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

Remove dispose from IContextKeyService #176673

Merged
merged 3 commits into from Mar 11, 2023
Merged

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Mar 9, 2023

From #176659 (comment)

It's unclear whether context key services created via createOverlay need to be disposed without reading the impl and seeing that dispose is a noop, this PR attempts to clarify that by updating ContextKeyServices to not require dispose. Since ScopedContextKeyServices are often a drop in replacement but do need to be disposed by the caller, this also introduces a new interface IScopedContextKeyService. Finally, I updated a few places where overlay context key services were being unnecessarily disposed.

@joyceerhl joyceerhl self-assigned this Mar 9, 2023
@VSCodeTriageBot VSCodeTriageBot added this to the March 2023 milestone Mar 9, 2023
connor4312
connor4312 previously approved these changes Mar 9, 2023
@joyceerhl joyceerhl changed the title Overlay context service need not implement dispose Remove dispose from IContextKeyService Mar 10, 2023
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyceerhl joyceerhl merged commit d799808 into main Mar 11, 2023
6 checks passed
@joyceerhl joyceerhl deleted the dev/joyceerhl/shiny-dove branch March 11, 2023 20:11
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants