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

"Open file" in existing window #19136

Merged
merged 4 commits into from Apr 10, 2019

Conversation

Projects
None yet
2 participants
@smashwilson
Copy link
Member

commented Apr 10, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Fixes #19133.

Description of the Change

In AtomApplication:

  • Made the getLoadSettings() helper optionally include the last focused window in its output.
  • Modified openPaths() to more strongly respect an explicitly provided {window} argument. (Previously, {window} would be used only if {addToLastWindow} was set to true.)
  • Changed promptForPathsToOpen() to pass a {window} argument on to openPaths() if and only if all of the chosen paths are non-directories. (This is to be consistent with the behavior in 1.35.1; "open folder" opens a new window, "open file" opens in the existing window.)
  • Used getLoadSettings(true) in all of the codepaths that an "open" or "open file" dialog can hit.

Alternate Designs

We could refactor the opening logic completely I guess to be clearer and less error-prone. Now isn't the time to do that, though, and it went poorly the last time I tried it.

Possible Drawbacks

application:open-file and application:open-folder have inconsistent and confusing behavior.

Verification Process

I did a manual Atom build and verified that the File -> Open... menu item and the cmd-O keybinding opened files in the current window and folders in a new window.

Release Notes

This is fixing a regression, so no release notes.

smashwilson added some commits Apr 10, 2019

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

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Possible Drawbacks

application:open-file and application:open-folder have inconsistent and confusing behavior.

@smashwilson: Am I correct in thinking that this inconsistent/confusing behavior predates this pull request (i.e., that this has been Atom's behavior for a while now)?

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@jasonrudolph: Yup - basically, I downloaded Atom 1.35.1 and did my best to match its behavior.

Oh, also, our test coverage for AtomApplication is not great. I'd take the time to add tests for this, but ugh.

@jasonrudolph
Copy link
Member

left a comment

@smashwilson: Thanks for jumping on this. 🙇💟

Do you anticipate this pull request resulting in any changes on other platforms (i.e., Linux and macOS)? Would it make sense try out a build on those other platforms to ensure that it's behaving as expected?

const areDirectories = await Promise.all(
pathsToOpen.map(pathToOpen => new Promise(resolve => fs.isDirectory(pathToOpen, resolve)))
)
if (!areDirectories.some(Boolean)) {

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Apr 10, 2019

Member

Current status: Wishing JavaScript Array provided a none method.

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 10, 2019

Author Member

Yeah... I spent a little time trying to juggle this in a way that was more readable, but I didn't have much luck. Notably I couldn't avoid the negative by looking for files instead because paths that don't exist yet are also not "files". And negating it to produce notDirectories just made it worse 🙃

}

this.on('application:quit', () => app.quit())
this.on('application:new-window', () => this.openPath(getLoadSettings()))
this.on('application:new-window', () => this.openPath(getLoadSettings(false)))

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Apr 10, 2019

Member

This isn't something that should hold up this pull request, but I think calls to getLoadSettings could be more intention-revealing/self-documenting we used a named argument.

getLoadSettings({includeWindow: false})

What do you think?

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 10, 2019

Author Member

Yah, that's reasonable. Honestly this whole class could use an overhaul to its readability. I tried to at least leave comments where I was changing things.

@smashwilson
Copy link
Member Author

left a comment

Thanks @jasonrudolph 🙇

const areDirectories = await Promise.all(
pathsToOpen.map(pathToOpen => new Promise(resolve => fs.isDirectory(pathToOpen, resolve)))
)
if (!areDirectories.some(Boolean)) {

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 10, 2019

Author Member

Yeah... I spent a little time trying to juggle this in a way that was more readable, but I didn't have much luck. Notably I couldn't avoid the negative by looking for files instead because paths that don't exist yet are also not "files". And negating it to produce notDirectories just made it worse 🙃

}

this.on('application:quit', () => app.quit())
this.on('application:new-window', () => this.openPath(getLoadSettings()))
this.on('application:new-window', () => this.openPath(getLoadSettings(false)))

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 10, 2019

Author Member

Yah, that's reasonable. Honestly this whole class could use an overhaul to its readability. I tried to at least leave comments where I was changing things.

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Do you anticipate this pull request resulting in any changes on other platforms (i.e., Linux and macOS)? Would it make sense try out a build on those other platforms to ensure that it's behaving as expected?

This will actually have the same behavior on all three, although the menu items present in the "file" menu differ from platform to platform. I tested builds on macOS by running commands from the command palette. But I'll be sure to give it a final check on the others.

@smashwilson smashwilson merged commit b89fa55 into master Apr 10, 2019

2 checks passed

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

@smashwilson smashwilson deleted the aw/open-file-regression branch Apr 10, 2019

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

Merge pull request #19136 from atom/aw/open-file-regression
"Open file" in existing window

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

Merge pull request #19136 from atom/aw/open-file-regression
"Open file" in existing window

@UziTech UziTech referenced this pull request Apr 11, 2019

Merged

Fix open in new window #392

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