Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Deactivate cleanly #384

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Deactivate cleanly #384

wants to merge 8 commits into from

Conversation

winstliu
Copy link
Contributor

@winstliu winstliu commented Mar 5, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

bracket-matcher does not deactivate cleanly - that is, it continues to match brackets and provide bracket autocompletion even after it is deactivated. This is because none of its subscriptions are disposed upon deactivation.
We now keep track of the observeTextEditors and editor.onDidDestroy subscriptions and dispose of them so that bracket-matcher does not continue matching brackets whenever a new editor is opened. Furthermore, upon deactivation/editor destruction, we now dispose of all the subscriptions that we create per-editor (this additionally means that each class no longer has to individually subscribe to the editor.onDidDestroy).

Alternate Designs

Coming later

Benefits

Deactivation will work.

Possible Drawbacks

None.

Applicable Issues

Fixes #156

Todo:

  • Is the watchedEditors.has check in observeTextEditors necessary? Seems to me like we're disposing of everything in editor.onDidDestroy, so we shouldn't need a has check when a new editor is added.
  • _.adviseBefore continues to function after deactivation, meaning brackets are still auto-inserted.
  • Specs

@winstliu
Copy link
Contributor Author

winstliu commented Mar 5, 2019

@Aerijo I don't think the WeakSet was necessary before, but we might need a WeakMap now so that we can associate editors to subscriptions.

@Arcanemagus
Copy link

Is the watchedEditors.has check in observeTextEditors necessary? Seems to me like we're disposing of everything in editor.onDidDestroy, so we shouldn't need a has check when a new editor is added.

It doesn't really hurt having it in there, but I think you're right that it's not actually necessary since on activate it should be empty and an editor should be given to us twice.

@winstliu
Copy link
Contributor Author

winstliu commented Mar 6, 2019

Well, this is complicated by the use of _.adviseBefore to hook into insertText and the like.

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.

Package doesn't deactivate correctly
2 participants