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

Use a single package manager when deserializing #903

Merged
merged 5 commits into from Feb 24, 2017

Conversation

50Wliu
Copy link
Contributor

@50Wliu 50Wliu commented Jan 20, 2017

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

Recent changes to the status bar tile necessitated that a single PackageManager instance was used throughout the codebase so that events would be caught correctly. Unfortunately, there was one instance that I forgot to change: when deserializing, two PackageManagers get created (one in main, one in settings-view), leading to the status bar not receiving any events. This PR fixes that.

Alternate Designs

None.

Benefits

The status bar and other parts of the code will work correctly when the Settings View is deserialized.

Possible Drawbacks

None.

Applicable Issues

Fixes #902

Note: I am not sure how to add a spec for this, or if it is necessary.

@50Wliu
Copy link
Contributor Author

50Wliu commented Feb 22, 2017

Hmm. Just noticed the failing tests.

@50Wliu
Copy link
Contributor Author

50Wliu commented Feb 22, 2017

@damieng Can you take a quick 👀 on this and make sure my changes look ok?

Edit: Oh boy, IDBObjectStore errors. Maybe packageManager can't be serialized, in which case that will be a big problem to tackle.

@50Wliu
Copy link
Contributor Author

50Wliu commented Feb 22, 2017

Ok, now it's ready for review :). You aren't supposed to call the SettingsView constructor directly - the actual deserializer is createSettingsView.

@damieng
Copy link
Contributor

damieng commented Feb 23, 2017

Yeah this looks good to me. Create it once in createsettingsview and then always use it again. Nowhere else goes into initialize other than through createSettingsView right?

@50Wliu
Copy link
Contributor Author

50Wliu commented Feb 23, 2017

Yeah. Just removed the last instance of new SettingsView other than createSettingsView.

@50Wliu 50Wliu merged commit 5bc6653 into master Feb 24, 2017
@50Wliu 50Wliu deleted the wl-ensure-consistent-package-manager branch February 24, 2017 18:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status bar tile does not update when Updates panel is open when Atom starts
2 participants