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

Fire grammar activation hooks after emitting new editors #17299

Merged
merged 2 commits into from May 15, 2018

Conversation

Projects
None yet
4 participants
@hansonw
Copy link
Contributor

hansonw commented May 11, 2018

This fixes a bug where packages with grammar activation hooks may receive duplicate editors from the atom.workspace.observeTextEditors API.

To see why this happens, we can see that in atom.workspace.subscribeToAddedItems a new editor E will be processed with the following sequence of events:

  1. E is added to the TextEditorRegistry.
  2. Activation hooks are fired for the editor's grammar (potentially activating a package, "A")
  3. A "did-add-text-editor" event for E is fired (triggering atom.workspace.observeTextEditors callbacks).

However, this sequence can lead to text editors being received multiple times from a package A's perspective - if package A activates and immediately starts listening to atom.workspace.observeTextEditors in step 2 then it sees editor E twice - once in the initial list of editors and once again when the did-add-text-editor event fires.

As long as we invert steps 2) and 3) above everything should be OK (I think). I modified the grammar-used spec to check for this - hopefully that makes things clear if the description above didn't make sense.

@hansonw hansonw requested review from maxbrunsfeld and matthewwithanm May 11, 2018

// It's important to call handleGrammarUsed after emitting the did-add event:
// if we activate a package between adding the editor to the registry and emitting
// the package may receive the editor twice from `observeTextEditors`.
subscriptions.add(

This comment has been minimized.

@hansonw

hansonw May 11, 2018

Author Contributor

I wonder if this needs to check if item has been disposed (however unlikely that may be?)

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 14, 2018

Contributor

Yeah, I guess it should.

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld left a comment

Thanks for the fix and the great explanation!

@hansonw hansonw force-pushed the hw-grammar-hook branch from 12e1632 to d646f70 May 15, 2018

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented May 15, 2018

@maxbrunsfeld Is this ready to be merged if the tests pass?

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented May 15, 2018

Is this ready to be merged if the tests pass?

Looks good to me.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented May 15, 2018

CI is clear, merging this. Thanks a lot @hansonw!

@daviwil daviwil merged commit 4953766 into master May 15, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the hw-grammar-hook branch May 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.