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

Remove 'project' command line flag #17212

Merged
merged 1 commit into from Apr 25, 2018

Conversation

Projects
None yet
3 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Apr 25, 2018

Fixes #17178

This (temporarily) removes the command line interface to the feature added in #16845. As far as I can tell, that interface never quite worked correctly. In my testing, the paths specified in the given project file were not opened. I think there are actually multiple causes of this:

  1. Because the project specification was not integrated into the state serialization system, once the state was loaded from indexedDB, the paths would immediately be replaced by different paths.
  2. Project paths are also updated after the call to project.replace when the asynchronous open-paths message is received.
  3. As someone reported on the initial PR, some logic in the command line parsing caused the directory containing the project path itself to be opened.

The actual reason that I discovered this is while investigating #17178. macOS users are getting this error when trying to open files in Atom by double clicking them. I added some code to log out the path when that error occurs, and I'm seeing that sometimes when double clicking a file, Atom is getting invoked with a project flag set to some weird non-existent path like /sn_1_01235345. I have no idea what causes that flag to get set.

/cc @matthewwithanm @philipfweiss It seemed like the command line project flag was not an important feature on your end; you mainly needed the Project.replace API. Is that right?

I think we can put back the project flag at some point, but for now I just need to get a hotfix out for #17178 and don't have time to fully backfill tests for this feature, think through how to integrate the project flag with the rest of the system, and avoid the bug described above.

Remove 'project' command line flag
From what I can tell, this flag never worked correctly. Instead of
opening the paths specified in the project file, the directory
containing the project file itself would always be opened.

@maxbrunsfeld maxbrunsfeld merged commit d233e20 into master Apr 25, 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

@maxbrunsfeld maxbrunsfeld deleted the mb-remove-project-flags branch Apr 25, 2018

maxbrunsfeld added a commit that referenced this pull request Apr 25, 2018

maxbrunsfeld added a commit that referenced this pull request Apr 25, 2018

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Apr 25, 2018

Wow, this is a bummer. But yeah, atom.project.replace() is the most important thing for us.

@kopischke

This comment has been minimized.

kopischke commented Apr 26, 2018

Forgive me for jumping in, but I think I can shed some light on what happens here, as I was bitten by the underlying macOS quick in an unrelated project recently. As succinctly put in this SO answer:

Mac OS X assigns a unique process serial number ("PSN") to all apps launched via GUI. It's used for identifying various processes and instances of executables.

which, and this is the kicker, is passed in the form of a -psn_* argument, which in turn Atom interprets as the -p short form of the --project switch and makes it process a spurious path.

Note that, unlike stated in the comments to the answer linked above, this behaviour persists up to at least macOS 10.12 Sierra, which is where I experienced it.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Apr 26, 2018

Ah, thanks so much for the insight @kopischke! I'm so happy to have that question answered.

So yeah, I think that we should add back the --project / -p flag at some point; it makes sense as a feature to expose, but there are enough issues with it that I think for the current patch release, we can just take it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment