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

Revert launch scenario changes #19028

Merged
merged 6 commits into from Mar 27, 2019

Conversation

Projects
None yet
2 participants
@smashwilson
Copy link
Member

commented Mar 21, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Fixes #18967 by selectively reverting functionality introduced in #18608.

Description of the Change

  • Modify AtomApplication to interpret the newWindow and addToLastWindow options as it did prior to #18608. Specifically:
    • If newWindow is true, always open the given paths in a new Atom window.
    • If addToLastWindow is true, open the given paths in an existing Atom window that "contains" all of the current paths. If none are found, open them in in the last-opened Atom window.
    • If both newWindow and addToLastWindow are false, open the given paths in an existing Atom window that "contains" all of the current paths. If none are found, open them in a new Atom window.
  • Re-propagate the newWindow option throughout AtomApplication.
  • Revert the changes to --help text.
  • Ensure that --new-window and --add are mutually exclusive - exit with an error when both are provided.
  • Close atom/flight-manual.atom.io#510 to keep our docs accurate.

Alternate Designs

See #18967 (comment) for some discussions of alternatives.

Possible Drawbacks

Messing with people's muscle memory again?

Verification Process

Existing Atom windows are launched in the order of their numbering. Windows in the "outcome" column marked in bold are focused. Paths in parenthesis are the project root folders in each window.

Precondition Command Outcome Verified?
No Atom windows atom Window 0 (empty)
No Atom windows atom ~/zero Window 0 (~/zero)
No Atom windows atom --add ~/zero Window 0 (~/zero)
No Atom windows atom --new-window ~/zero Window 0 (~/zero)
Window 0 (~/zero) atom Window 0 (~/zero)
Window 1 (empty)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/zero/file Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom --add ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom --new-window ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/zero)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/two Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/two)
Window 0 (~/zero)
Window 1 (~/one)
atom --add ~/two Window 0 (~/zero)
Window 1 (~/one, ~/two)
Window 0 (~/zero)
Window 1 (~/one)
atom --new-window ~/two Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/two)

Behavior on Atom 1.35.1

This is the behavior for the same scenarios with the current stable Atom release, before #18608 was merged.

Precondition Command Outcome Consistent?
No Atom windows atom Window 0 (empty)
No Atom windows atom ~/zero Window 0 (~/zero)
No Atom windows atom --add ~/zero Window 0 (~/zero)
No Atom windows atom --new-window ~/zero Window 0 (~/zero)
Window 0 (~/zero) atom Window 0 (~/zero)
Window 1 (empty)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/zero/file Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom --add ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 0 (~/zero)
Window 1 (~/one)
atom --new-window ~/zero Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/zero)
Window 0 (~/zero)
Window 1 (~/one)
atom ~/two Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/two)
Window 0 (~/zero)
Window 1 (~/one)
atom --add ~/two Window 0 (~/zero)
Window 1 (~/one, ~/two)
Window 0 (~/zero)
Window 1 (~/one)
atom --new-window ~/two Window 0 (~/zero)
Window 1 (~/one)
Window 2 (~/two)

Release Notes

N/A (kind of). Specifically, we should remove this bullet point:

When launching Atom from the command line, a new window is always opened with the provided paths. To open the requested locations in an existing window, --add must be specified.

smashwilson added some commits Mar 21, 2019

@smashwilson smashwilson marked this pull request as ready for review Mar 22, 2019

@smashwilson smashwilson requested a review from jasonrudolph Mar 22, 2019

@jasonrudolph
Copy link
Member

left a comment

Thanks for this, @smashwilson! ⚡️ 🙇

I love the detailed verification process. That’s a huge help in understanding how things will function following this change! 😻

Just a couple questions:

  • Can you identify which of the rows in the verification process differ from the behavior present in the current stable Atom release (1.35.1)?
  • I see that the changes in atom.sh only apply to macOS and Linux. Are similar changes needed for Windows?

atom/atom.sh

Lines 3 to 10 in 2526e69

if [ "$(uname)" == 'Darwin' ]; then
OS='Mac'
elif [ "$(expr substr $(uname -s) 1 5)" == 'Linux' ]; then
OS='Linux'
else
echo "Your platform ($(uname -a)) is not supported."
exit 1
fi

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

Can you identify which of the rows in the verification process differ from the behavior present in the current stable Atom release (1.35.1)?

There shouldn't be any changes between this and Atom stable. The PR that modified launch behavior was first introduced in 1.36.0-beta0. But, I should verify that. I'll do another chart and edit it in to the PR description before I re-request review.

I see that the changes in atom.sh only apply to macOS and Linux. Are similar changes needed for Windows?

Oh, right. Possibly? I'll verify when I'm downstairs 😄

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Okay:

  • I've ported the launch script changes to atom.cmd and verified that atom --add --new-window generates a visible error message on Windows as well.
  • I ran through the checklist on Atom 1.35.1 and verified that Atom's launch behavior is consistent with the way it used to work.

I'll merge on green and backport to 1.36-releases 👌

@smashwilson smashwilson merged commit 53c0202 into master Mar 27, 2019

2 checks passed

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

@smashwilson smashwilson deleted the aw/revert-launch-scenario-change branch Mar 27, 2019

smashwilson added a commit that referenced this pull request Mar 27, 2019

@smashwilson smashwilson referenced this pull request Apr 19, 2019

Merged

Improve launch behavior #19169

9 of 9 tasks complete
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.