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

Activate package when deserializing #16100

Merged
merged 23 commits into from Jun 3, 2019

Conversation

Projects
None yet
5 participants
@50Wliu
Copy link
Member

commented Nov 4, 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

This PR unconditionally activates packages when a deserializer for that package is called. It also adds a new way of deferring package activation through the workspaceOpeners key. Supply it an array of workspace opener filepaths/URIs that your package watches for and it will remain deactivated until one of those openers is called. This is useful when used in conjunction with deserializers, which often are created using workspace openers.

To demonstrate how a package would benefit from this PR, consider a package that listens to the opener atom://example to create a new ExampleView that has a serializer. atom://example is listed as a workspace opener in package.json. The example:open-example-view command calls atom.workspace.open('atom://example') and is listed as an activation command in package.json. This package structure is in use by packages such as Timecop and Markdown Preview.
Previously, there was no way to safely defer activation for a package that implemented serialization because it would not be activated when deserialization occurred.

  • Defining the activation command would break deserialization, as the view would get added but the package was technically not activated and as such had no registered styles, commands, menus, etc.
    • With this PR, the deserializer forces the package to be activated.
  • Opening the view, closing it, reloading Atom, then reopening the view through methods such as pane:reopen-closed-item would throw an EINVAL: open atom:\example error on Windows due to the workspace opener not being registered.
    • With this PR, workspaceOpeners registers a dummy opener for the opener that then activates the package and calls the real opener.

Alternate Designs

There could be another key added to deserializers stating whether or not that deserializer should activate the package.

Why Should This Be In Core?

Deserializing happens in core.

Benefits

Deserializing, openers, and deferred activation techniques can be used together.

Possible Drawbacks

Perhaps there are some deserializers that don't need the package to be activated? In which case this will slow down Atom's load time unnecessarily.

Applicable Issues

Closes #16099

@50Wliu 50Wliu added the needs-review label Nov 4, 2017

@50Wliu 50Wliu force-pushed the wl-deserialize-and-activate branch from 4300008 to 629a0a5 Nov 8, 2017

50Wliu added some commits Nov 4, 2017

@50Wliu 50Wliu force-pushed the wl-deserialize-and-activate branch from 7b38b09 to 9a40239 Nov 13, 2017

Allow activation to be deferred if `workspaceOpeners` is present
Fixes the case where `pane:reopen-closed-item` is called and the item
happens to be a URI for a package whose activation is deferred

50Wliu added some commits Nov 14, 2017

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

@BinaryMuse I think this is ready for a review. Interested in hearing if you have any alternative approaches in mind :).

@50Wliu 50Wliu referenced this pull request Nov 16, 2017

Merged

Convert Package to JS #16199

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

I think this is a great change. The workspaceOpeners key is a great idea as well.

I'm wondering if this will break any assumptions that packages would previously have made regarding the timing of the call to activate.

Previously, packages' activate methods were never called until atom.workspace.getElement() was attached to the DOM, so packages might assume that any items they add to the workspace will immediately be on the DOM as well.

/cc @atom/maintainers Thoughts? Is it ok to change the timing in this way? If not, should we just attach the workspace element to the DOM before we do any deserialization?

@lee-dohm

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

@maxbrunsfeld If it isn't too much trouble, I would prefer we maintain the assumption since many packages immediately start working with the DOM on activation. It could be quite disconcerting to try and figure out why 1 out of 15 times it doesn't work.

maxbrunsfeld and others added some commits Nov 29, 2017

@maxbrunsfeld maxbrunsfeld force-pushed the wl-deserialize-and-activate branch from 8920450 to eb4cd11 Nov 29, 2017

50Wliu added some commits Dec 4, 2017

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

@maxbrunsfeld I've delayed package activation until initial package activation in order to preserve the activation timing. However if initial packages have already been activated by the time the deserializer is called, activate the package immediately.

50Wliu added some commits Jan 3, 2018

Use atom.workspace.createItemForURI
This avoids nested atom.workspace.open calls, which don't work well, and has the same effect as createItemForURI will call the newly-added opener.

50Wliu and others added some commits May 15, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Hey! 👋

We have recently started using prettier and this has caused major style changes on the Atom JS codebase (you can check the related PR).

This is good news: now the Atom code is more consistent and it's much easier to re-format the code to follow the codestyle (now you only need to run script/lint --fix).

This change caused conflicts on your PR that we have automatically solved, hope you don't mind 😄

With ❤️, the Atom team.

@nathansobo nathansobo self-assigned this Jun 3, 2019

@nathansobo nathansobo merged commit 2d3e332 into master Jun 3, 2019

2 checks passed

Atom Pull Requests #20190531.17 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the wl-deserialize-and-activate branch Jun 3, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

❤️ Thanks @50Wliu. This is an important improvement, especially fixing the holes in the serialization scheme.

nathansobo added a commit to atom/flight-manual.atom.io that referenced this pull request Jun 3, 2019

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.