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

Improve launch behavior #19169

Merged
merged 87 commits into from Apr 23, 2019

Conversation

Projects
None yet
3 participants
@smashwilson
Copy link
Member

commented Apr 16, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Atom's launch scenarios are complex and full of edge cases and dark, dusty corners. Recent launch scenario changes have introduced some fairly major regressions. Let's bring the launch story back in line with what we expect it to be, and leave some more comprehensive test coverage behind us so that we can make launch scenario changes in the future with more confidence.

Related to #19147, although some of the work to improve the zero-project-folder situation being encountered there is likely out of scope of this PR (a better empty-state for the tree view, for example).

#19166 contains a map of the way that we want launching to work, including a comparison of each situation to the way it worked in Atom <=1.35.1 and 1.36.0. The rendered document is likely easier to read.

Description of the Change

See the release notes for a detailed description of the exact launch behaviors that have been changed from 1.35.1 and 1.36.0.

Progress

  • Construct a test harness system that can be used to tersely assert launch behavior in specific situations.
  • Convert the table created in #19166 to test cases.
  • Get the new test suite green.
  • Stub the createWindow method to prevent AtomApplication from actually creating windows within the test suite.
  • Create additional tests for AtomWindow to make up for the loss of coverage from the mocking in the AtomApplication tests.
  • Port over additional test cases from the original AtomApplication suite to ensure we aren't losing any coverage.
  • Correct anything remaining that's red in CI.
  • Document the way that the LaunchScenario stuff works.
  • Manually verify against the desired behavior tables.

Alternate Designs

We could have reverted #18608 and #19028 entirely to restore opening behavior to exactly what it was <= 1.35.1. The intentional change in those PRs, though, fixes one of the oldest and longest-standing pain points in our launch story, so I really wanted to try to preserve it if I could. Also: the main process code that's responsible for these sorts of things is fragile and largely untested (hence the regressions when we touch it). I wanted to get us into a better place to fix further issues here with confidence by building up an efficient, comprehensive test suite that covers all of the different launch behaviors that we exhibit, so that we can be sure that changes we make in the future don't have unintentional side effects like this one did.

Possible Drawbacks

See ☝️ for this area of the codebase being fragile and, until now, largely untested. It's possible that there are still other launch behaviors that are currently working by accident that are not covered by the tests that I've introduced. I did my best to establish a basis set to cover the launch space here, but my hope is that, moving forward, we'll at least have an expedient way to cover new scenarios as they're reported.

The new AtomApplication tests mock AtomWindow creation and make assertions about its load settings to ensure correctness. I've also added independent AtomWindow tests that mock actual BrowserWindow launching (except for one), and ensured that the AtomEnvironment tests have reasonable coverage over the interpretation of load settings on the renderer process side. Still, I'm essentially replacing a small set of very slow, integration-style tests with several larger sets of fast, more comprehensive, unit-style tests, and there is a risk that bugs have fallen through the cracks between them.

Even with the changes that I'm making here, our launch story remains a hidden magic understood fully by few (if any? I was certainly surprised by some of the corners I turned up here). It would be nice to have launch behavior that was intuitive to new users that we could fully describe in a few succinct sentences. However, our userbase is broad with varying needs and habits, and almost any change we make is going to break someone. Getting the launch story to that point would require substantial user experience research.

Verification Process

In addition to the test suite, I also ran through the full set of launch scenarios and outcomes @jasonrudolph and I populated in #19166 and verified that my dev build did indeed exhibit the desired behavior for each case. I'd love it if someone other than me could repeat this experiment to make sure that I didn't slip a row anywhere.

Release Notes

  • If any existing Atom window contains no project roots and --new-window is not specified, opened files and directories will be added to the most recently focused empty window.
  • When core.restorePreviousWindowsOnStart is set to "always", --new-window is specified, and one or more paths are opened, previous windows are no longer restored; --new-window always takes precedence.
  • When previous windows are restored at application launch, paths and arguments given on the command-line are no longer duplicated within each opened window.
  • When locating an existing window to open a new path within, existing windows are considered in the order in which they were most recently focused, not the order in which they were created. This is consistent with the way that --add finds its window and URLs are routed to windows.
  • When locating an existing window to open a new path within, --safe mode is respected - if --safe is provided, only a safe-mode window will be considered.
  • If --add is provided and the most recently focused window is a dev mode window, a previously focused non-dev mode window will be used if available instead of opening a new window.
  • If --add and --safe are both provided, the paths will only be added to an existing, most recently focused safe-mode window. If --safe is not provided, the paths will only be added to a non-safe-mode window.

smashwilson and others added some commits Apr 16, 2019

Use _ for an empty project root or editor path group
Co-Authored-By: Jason Rudolph <jason@jasonrudolph.com>
Increase beforeEach, afterEach, and emitterEventPromise timeouts
This is to temporarily work around spikes in the window launch time.
Replace containsPath(s) with containsLocation(s)
This lets us use existing stat data.
Rename initialPaths and representedDirectoryPaths
They're now called initialProjectRoots and projectRoots, which is closer 
to what they actually are.

smashwilson added some commits Apr 19, 2019

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

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

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

When I made the "finding an existing window" cases respect --safe as well, I thought I was addressing an inconsistency within AtomApplication::openPaths() (meaning, in some cases it would use --safe to find a window and in others it would not), but looking back at the source from v1.35.1, it looks like we were uniformly checking --dev:

let existingWindow
if (!newWindow) {
existingWindow = this.windowForPaths(pathsToOpen, devMode)
if (!existingWindow) {
let lastWindow = window || this.getLastFocusedWindow()
if (lastWindow && lastWindow.devMode === devMode) {
if (addToLastWindow || (
locationsToOpen.every(({stat}) => stat && stat.isFile()) ||
(locationsToOpen.some(({stat}) => stat && stat.isDirectory()) && !lastWindow.hasProjectPath()))) {
existingWindow = lastWindow
}
}
}
}

Should I restore the original behavior and ignore --safe to find existing windows to minimize user-observable changes? It is kind of odd that you can run atom --add --safe file.txt and open file.txt in a non-safe-mode window 🤔

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

I also ran through the full set of launch scenarios and outcomes @jasonrudolph and I populated in #19166 and verified that my dev build did indeed exhibit the desired behavior for each case. I'd love it if someone other than me could repeat this experiment to make sure that I didn't slip a row anywhere.

I ran through all of the scenarios from #19166 on macOS, and everything checks out! I've included the full list below for posterity.


All examples use the following directory structure:

/a/1.md
/a/2.md
/b/3.md

CLI actions

No open windows

When no Atom windows are open, CLI actions have the following outcomes:

Action Outcome (1.36.1)
atom ~/Desktop/a/1.md [ 1.md] 🆕
atom ~/Desktop/a [/a]
atom --add ~/Desktop/a/1.md [ 1.md] 🆕
atom --add ~/Desktop/a [/a]
atom --new-window ~/Desktop/a/1.md [ 1.md] 🆕
atom --new-window ~/Desktop/a [/a]

Open window, no project roots

With the following starting state:

[]

Action Outcome (1.36.1)
atom ~/Desktop/a/1.md [ 1.md] 🆕
atom ~/Desktop/a [/a]
atom --add ~/Desktop/a/1.md [ 1.md] 🆕
atom --add ~/Desktop/a [/a]
atom --new-window ~/Desktop/a/1.md [] [ 1.md] 🆕
atom --new-window ~/Desktop/a [] [/a]

Open window, project root

With the following starting state:

[/a]

Action Outcome (1.36.1)
atom ~/Desktop/a/1.md [/a 1.md]
atom ~/Desktop/a [/a]
atom ~/Desktop/b/3.md [/a 3.md]
atom ~/Desktop/b [/a] [/b]
atom --add ~/Desktop/a/1.md [/a 1.md]
atom --add ~/Desktop/a [/a]
atom --add ~/Desktop/b/3.md [/a 3.md] 🆕
atom --add ~/Desktop/b [/a,/b]
atom --new-window ~/Desktop/a/1.md [/a] [ 1.md] 🆕
atom --new-window ~/Desktop/a [/a] [/a]
atom --new-window ~/Desktop/b/3.md [/a] [ 3.md] 🆕
atom --new-window ~/Desktop/b [/a] [/b]

Open windows, one with a project root and one without

[/a] []

Action Outcome (1.36.1)
atom ~/Desktop/a/1.md [/a 1.md] []
atom ~/Desktop/a [/a] []
atom ~/Desktop/b/3.md [/a] [ 3.md] 🆕
atom ~/Desktop/b [/a] [/b]
atom --add ~/Desktop/a/1.md [/a 1.md] []
atom --add ~/Desktop/a [/a] []
atom --add ~/Desktop/b/3.md [/a] [ 3.md] 🆕
atom --add ~/Desktop/b [/a] [/b]
atom --new-window ~/Desktop/a/1.md [/a] [] [ 1.md] 🆕
atom --new-window ~/Desktop/a [/a] [] [/a]
atom --new-window ~/Desktop/b/3.md [/a] [] [ 3.md] 🆕
atom --new-window ~/Desktop/b [/a] [] [/b]

[] [/a]

Action Outcome (1.36.1)
atom ~/Desktop/a/1.md [] [/a 1.md]
atom ~/Desktop/a [] [/a]
atom ~/Desktop/b/3.md [ 3.md] [/a] 🆕
atom ~/Desktop/b [/b] [/a] 🆕
atom --add ~/Desktop/a/1.md [] [/a 1.md]
atom --add ~/Desktop/a [] [/a]
atom --add ~/Desktop/b/3.md [] [/a 3.md] 🆕
atom --add ~/Desktop/b [] [/a,/b]
atom --new-window ~/Desktop/a/1.md [] [/a] [ 1.md] 🆕
atom --new-window ~/Desktop/a [] [/a] [/a]
atom --new-window ~/Desktop/b/3.md [] [/a] [ 3.md] 🆕
atom --new-window ~/Desktop/b [] [/a] [/b]

File manager integration actions

No open windows

When no Atom windows are open, file manager context operations have the following outcomes:

Action Outcome (1.36.1)
Click on /a/1.md [ 1.md] 🆕
Click on /a [/a]

Open window, no project roots

With the following starting state:

[]

Action Outcome (1.36.1)
Click on /a/1.md [ 1.md] 🆕
Click on /a [/a] 🆕

Open window, project root

With the following starting state:

[/a]

Action Outcome (1.36.1)
Click on /a/1.md [/a 1.md]
Click on /a [/a]
Click on /b/3.md [/a 3.md]
Click on /b [/a] [/b]

Open windows, one with a project root and one without

[/a] []

Action Outcome (1.36.1)
Click on /a/1.md [/a 1.md] []
Click on /a [/a] []
Click on /b/3.md [/a] [ 3.md] 🆕
Click on /b [/a] [/b]

[] [/a]

Action Outcome (1.36.1)
Click on /a/1.md
Click on /a
Click on /b/3.md
Click on /b
@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

It is kind of odd that you can run atom --add --safe file.txt and open file.txt in a non-safe-mode window 🤔

@smashwilson That does seem odd. Can you help me understand how/whether this differs from the pre-1.35.0 behavior?

@jasonrudolph
Copy link
Member

left a comment

I looked over the diff, and I manually verified the behavior on macOS.

Screen Shot 2019-04-22 at 10 26 50 AM

This pull request is a doozy, so I won't claim to have scrutinized every line of code. 😇 But, having skimmed the entire diff, and then having studied a few specific areas of the diff (e.g., the new unit testing strategy and all the conditional logic for determining which window to use when opening a given path), and then having manually verified the behavior, I feel comfortable recommending that we merge these changes.

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

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@smashwilson That does seem odd. Can you help me understand how/whether this differs from the pre-1.35.0 behavior?

Oh, sure. This is based purely on tracing through the old source, but it appears that only the --dev flag is used to filter existing windows when processing an open request. For example, say we have these windows, opened (/last focused) in this order:

w0 = [a _](dev); w1 = [a _](safe); w2 = [a _]

We'd handle the following open actions like so:

Action <=1.35.1 >=1.36.1
atom a/1.md 1.md opens in w2 1.md opens in w2
atom --dev a/1.md 1.md opens in w0 1.md opens in w0
atom --safe a/1.md 1.md opens in w2 1.md opens in w1 🆕 🐛

My guess is that we introduced the --dev and --safe flags at different points, so they were added to the opening logic slightly differently. It doesn't feel intentional to me, anyway.

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@jasonrudolph Thanks for the review 🙇 I will say that I'm not weeping, though. Maybe next time 😉

I'll :shipit: so we can get it on nightly and start using it in anger tomorrow morning 😄

@smashwilson smashwilson merged commit 758fd9d into master Apr 23, 2019

2 checks passed

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

@smashwilson smashwilson deleted the aw/launch-it branch Apr 23, 2019

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

Merge pull request #19169 from atom/aw/launch-it
Improve launch behavior

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

Merge pull request #19169 from atom/aw/launch-it
Improve launch behavior

UziTech added a commit to UziTech/atom-open that referenced this pull request Apr 26, 2019

fix(atom): Fix Atom v1.36.1
atom/atom#19169
moved getLoadSettings().initialPaths to getLoadSettings().initialProjectRoots
@UziTech

This comment has been minimized.

Copy link
Contributor

commented on 952c42c Apr 26, 2019

This should be a breaking change since getLoadSettings() is a public method.

related UziTech/atom-open#5

This comment has been minimized.

Copy link
Member Author

replied Apr 26, 2019

D'oh. Sorry about that 🤦‍♂️

Would it be worth it to add the old name as an alias for the next release, or has the damage been done?

this.loadSettings.initialProjectRoots = this.projectRoots
this.loadSettings.initialPaths = this.projectRoots

This comment has been minimized.

Copy link
Contributor

replied Apr 26, 2019

I think it might be worth it. I fixed it for my package but other packages might not figure it out right away.

This comment has been minimized.

Copy link
Contributor

replied Apr 26, 2019

maybe add a deprecation notice to the console

This comment has been minimized.

Copy link
Contributor

replied Apr 27, 2019

It would probably be better to have the aliases since any package that changes it wouldn't work for older versions of atom that don't have this change.

@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’t perform that action at this time.