Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Conversation

@winstliu
Copy link
Contributor

@winstliu winstliu 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

@winstliu winstliu force-pushed the wl-deserialize-and-activate branch from 4300008 to 629a0a5 Compare November 8, 2017 10:59
@winstliu winstliu force-pushed the wl-deserialize-and-activate branch from 7b38b09 to 9a40239 Compare November 13, 2017 23:14
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
@winstliu
Copy link
Contributor Author

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

@winstliu winstliu mentioned this pull request Nov 16, 2017
@maxbrunsfeld
Copy link
Contributor

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
Copy link
Contributor

@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 maxbrunsfeld force-pushed the wl-deserialize-and-activate branch from 8920450 to eb4cd11 Compare November 29, 2017 20:28
@winstliu
Copy link
Contributor Author

winstliu 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.

Wliu added 4 commits January 2, 2018 21:16
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.
@rafeca
Copy link
Contributor

rafeca 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
@nathansobo nathansobo deleted the wl-deserialize-and-activate branch June 3, 2019 20:45
@nathansobo
Copy link
Contributor

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

nathansobo pushed a commit to atom/flight-manual.atom.io that referenced this pull request Jun 3, 2019
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.

Deserializing a package should activate it

5 participants