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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Reopen projects" in a new window #19203

Merged
merged 5 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@smashwilson
Copy link
Member

commented Apr 23, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Fixes #19149.

Description of the Change

I'm reworking the IPC messages that AtomEnvironment and AtomApplication use to communicate messages related to file opening to clarify which are used for what, then making sure that the application:reopen-projects, application:open, application:open-file, and application:open-folder use them consistently. I'm also backfilling tests for IPC message handling.

Alternate Designs

I could have opted for a more surgical fix within the confines of the existing IPC message structure. The way they were structured was fairly confusing though - we had listeners for "open", "open-command" with a subcommand switch for application:open, application:open-file, or application:open-folder, and "open-file", all of which behaved in slightly different ways.

I also considered making application:reopen-projects pass a newWindow parameter to always open a new window for the reopened projects, but opted to preserve the old behavior as closely as I could.

Possible Drawbacks

Because the "open" command handler used by application:reopen-projects uses the standard command-line rules for reusing existing windows, there is still a situation which may exhibit undesired behavior:

Given [a,b _], reopening the project b will only refocus the window because the existing window already contains the named path, but that window won't have the same state as the project with b alone.

Verification Process

In addition to unit tests, here's my behavior verification matrix.

Given this directory tree:

a/
  1.md
b/
  2.md
  • 1.36.0 outcome cells marked with 馃啎 have changed from 1.35.1 to 1.36.0 (intentionally or otherwise).
  • 1.36.1 outcome cells marked with 馃啎 have intentionally changed from 1.35.1 to 1.36.1.
  • 1.36.1 outcome cells marked with 馃悰 were unintentionally changed from 1.35.1 to 1.36.0 and reverted to their original behavior for 1.36.1.
  • 1.36.1 outcome cells marked with 馃悰 馃啎 unintentionally changed from 1.35.1 to 1.36.0, but changed to different behavior for 1.36.1.

No open windows

Action Outcome (<=1.35.1) Outcome (1.36.0) Outcome (1.36.1)
File -> Reopen Project -> a (nothing) (nothing) [a _] 馃悰 馃啎
File -> Open... -> a/1.md [a 1.md] [_ 1.md] 馃啎 [_ 1.md] 馃啎

One empty window

[_ _]

Action Outcome (<=1.35.1) Outcome (1.36.0) Outcome (1.36.1)
File -> Reopen Project -> b [b _] [_ _] 馃啎 [b _] 馃悰
File -> Open... -> a/1.md [a 1.md] [_ 1.md] 馃啎 [_ 1.md] 馃啎
File -> Open... -> a [a _] [_ _] [a _] 馃啎 [a _] 馃悰
application:reopen-project, choose a [a _] [a _] [a _]
application:open, choose a/1.md [a 1.md] [_ 1.md] 馃啎 [_ 1.md] 馃啎
application:open, choose b [b _] [_ _] [b _] [b _] 馃悰
application:open-file, choose a/1.md [a 1.md] [_ 1.md] 馃啎 [_ 1.md] 馃悰 馃啎
application:open-folder, choose b [b _] [_ _] [b _] 馃啎 [b _] 馃悰 馃啎

One window with a project root

[a _]

Action Outcome (<=1.35.1) Outcome (1.36.0) Outcome (1.36.1)
File -> Reopen Project -> b [a _] [b _] [a _] 馃啎 [a _] [b _] 馃悰
File -> Open... -> a/1.md [a 1.md] [a 1.md] [a 1.md]
File -> Open... -> b/2.md [a 2.md] [a 2.md] [a 2.md]
File -> Open... -> b [a _] [b _] [a _] [b _] [a _] [b _]
application:reopen-project, choose b [a _] [b _] [a,b _] 馃啎 [a _] [b _] 馃悰
application:open, choose b/2.md [a 2.md] [a 2.md] [a 2.md]
application:open, choose b [a _] [b _] [a _] [b _] [a _] [b _]
application:open-file, choose a/1.md [a 1.md] [a 1.md] [a 1.md]
application:open-file, choose b/2.md [a 2.md] [a 2.md] [a 2.md]
application:open-folder, choose b [a _] [b _] [a _] [b _] [a _] [b _]

That weird edge case

[a,b _]

b has project state of 2.md

Action Outcome (<=1.35.1) Outcome (1.36.0) Outcome (1.36.1)
File -> Reopen Project -> b [a _] [b _] [a _] [b _] [a _] [b _]

Release Notes

  • "File -> Reopen Project" works correctly on macOS when no windows are open.
  • "File -> Reopen Project" from the application menu opens the chosen project folders.
  • Choosing application:reopen-project from the command palette opens chosen project folders in a new window when appropriate.

The other 馃啎 and 馃悰 馃啎 rows correspond to changes introduced by #19169.

smashwilson added some commits Apr 23, 2019

@smashwilson smashwilson marked this pull request as ready for review Apr 23, 2019

@smashwilson smashwilson requested a review from jasonrudolph Apr 23, 2019

@jasonrudolph
Copy link
Member

left a comment

In addition to unit tests, here's my behavior verification matrix.

Thanks for writing this up! 馃槏 As a reviewer, I found this matrix super helpful in getting up to speed on the behavioral changes provided by this pull request.

I reviewed the code, and it matches my expectations for how we want things to behave.

I also built this branch locally and manually tested a handful of scenarios, and all of them behaved as advertised.

馃憤馃殌

Show resolved Hide resolved src/main-process/atom-application.js Outdated
Update src/main-process/atom-application.js
Co-Authored-By: smashwilson <smashwilson@github.com>

@smashwilson smashwilson merged commit a0e9bb7 into master Apr 23, 2019

2 checks passed

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

@smashwilson smashwilson deleted the aw/reopen-projects branch Apr 23, 2019

smashwilson added a commit that referenced this pull request Apr 23, 2019

Merge pull request #19203 from atom/aw/reopen-projects
"Reopen projects" in a new window

smashwilson added a commit that referenced this pull request Apr 23, 2019

Merge pull request #19203 from atom/aw/reopen-projects
"Reopen projects" in a new window

@bittin bittin referenced this pull request May 6, 2019

Closed

1.37 releases #19256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.