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

When only a file is specified, don't open and index the parent directory #18608

Merged
merged 18 commits into from Jan 15, 2019

Conversation

Projects
None yet
4 participants
@smashwilson
Copy link
Member

smashwilson commented Dec 20, 2018

Requirements for Adding, Changing, or Removing a Feature

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
  • The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/atom/tree/master/CONTRIBUTING.md#pull-requests.

Issue or RFC Endorsed by Atom's Maintainers

This should help with #13253. It also addresses #1847, #10223, #8030, #8934, #18646, and possibly #10110. It's tangentially related to #10517 as well - it helps clarify the launch scenario in a specific situation, namely "I want to open Atom to edit a single file."

Description of the Change

When launching Atom from the command line with a single file as an argument:

atom ~/notes.md

The parent directory of the file (here, your home directory) is implicitly added as a project root. This causes problems when the parent directory is large, because Atom then scans and indexes everything beneath it, launches filesystem watchers, and so on, which can lead to a massive performance hit.

I've modified AtomEnvironment::openLocations() to prevent this from happening. If a file path is specified as an open location, that file will be opened in a TextEditor. If a directory path is specified, then the directory will be added as a project root.

Alternate Designs

I could have made Project::getDirectoryForProjectPath() not have the behavior of returning a Directory for the parent directory when the path is a file. That would have involved changing the contract for directory providers, though.

Possible Drawbacks

Opening a single file by path will load and modify the serialized project state for Atom's "temporary window state". This is different from the current behavior, which is to load the project state for the file's parent directory. I'd argue that this is more intuitive, though, and honestly I'm not sure how many developers understand Atom's state management enough to reason about this anyway.

Verification Process

  • Open Atom on a single file with atom ~/file.txt. Atom should open with no project roots and a single open TextEditor on the requested file.
  • Open Atom on a single directory with atom ~/src/atom/atom. Atom should open with the specified directory as a project root, shown in the tree view.

Release Notes

  • Opening a file path with Atom no longer implicitly adds the file's parent directory as a project root.
  • 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.

New Launch Behavior

Here's the new launch behavior for the scenarios described in #10517. In these cases, the described outcome is the result regardless of whether or not existing Atom windows are present or not.

Scenario Command Outcome
Project (single root) atom ~/projects/one New Atom window opens, project root ~/projects/one
Project (multiple roots) atom ~/projects/one ~/projects/two New Atom window opens, project roots ~/projects/one and ~/projects/two
Specific files atom ~/.bashrc New Atom window opens, no project roots, single editor on ~/.bashrc

Previous session restoration is triggered by lauching Atom with no location arguments (atom) or opening it graphically, from the dock or start menu, while no Atom windows are already open. If at least one Atom window is already open, or if --new-window is passed, a new Atom window is launched with an untitled editor.

Additionally, here are the behaviors enabled with --add from the command line. Assume that two Atom windows are already open, (1) on ~/projects/one and (2) on ~/projects/two, launched in that order.

Scenario Command Outcome
Add a file to the latest opened Atom window atom --add ~/notes.md Editor opened on notes.md in window (2)
Add a file to a previously opened Atom window atom --add ~/projects/one/subdirectory/file.txt Editor opened on file.txt in window (1)
Add a project root to the latest opened Atom window atom --add ~/projects/three Project root ~/projects/tree added to window (2)

Remaining Work

  • Make atom with no arguments behave the way I describe it ☝️
  • Green build
  • Bring atom --help up to date
  • Documentation pass to find references to the old launch behavior and open PRs to update them
    • Flight manual
@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Dec 20, 2018

I've also changed openLocations to do less synchronous filesystem I/O by making the stat() calls async. We do a lot of synchronous I/O in our startup path; hopefully I can whittle it down little by little as I see it 🙈

@smashwilson smashwilson requested review from daviwil and removed request for daviwil Dec 20, 2018

smashwilson added some commits Jan 3, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 4, 2019

@lee-dohm, @rsese: Along with the original goal of improving Atom's launch behavior when opening a single file, I've taken the liberty of making what I think is an incremental improvement to our other launch stories, based on @lee-dohm's write-up in #10517. This doesn't do anything to change the way that project state is determined - it only attempts to make the behavior of the atom command more explicit and predictable.

I wanted to check in with you two before I go further to see if the changes that I'm making make sense and more or less match what you've seen most users expect to see over the years. A description of what I'm aiming for is summarized in the PR description ☝️ in the "New Launch Behavior" section.

(I know that no matter what we change our launch behavior to, we'll be breaking someone's workflow and we'll get issues complaining about it. Mostly I want to give you a chance to weigh in to make sure that we're breaking expectations in the "right" direction 😇 ).

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 4, 2019

Put another way: this is making --new-window the default and requiring an explicit --add argument to modify an existing window. I'm getting rid of the weird "guess what you mean" middle ground when you specified neither:

One or more paths to files or folders may be specified. If there is an existing Atom window that contains all of the given folders, the paths will be opened in that window. Otherwise, they will be opened in a new window.

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Jan 4, 2019

(I know that no matter what we change our launch behavior to, we'll be breaking someone's workflow and we'll get issues complaining about it. Mostly I want to give you a chance to weigh in to make sure that we're breaking expectations in the "right" direction 😇 ).

👍 😄

I wanted to check in with you two before I go further to see if the changes that I'm making make sense and more or less match what you've seen most users expect to see over the years.

About opening a single file - I haven't come across tooo many new issues about this over the last year or 2, but I have read through the existing issues. I think #1847 for example is another good issue in favor of this change where there's some expectation based on how other editors work and what people want to do when they explicitly open a single file (though I wonder if there was no performance problem, how big of an issue it would be for people when Atom opens the file's project and tree view for single files 🤔).

Put another way: this is making --new-window the default and requiring an explicit --add argument to modify an existing window. I'm getting rid of the weird "guess what you mean" middle ground when you specified neither:

Using your example above, in this scenario (so without the explicit --add):

atom ~/projects/one/subdirectory/file.txt

Would Atom open a new window or open the file in window (1)?

Small side question

This should help with #1874

Is that the right issue you wanted to link to?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 4, 2019

Is that the right issue you wanted to link to?

Haha, not even a little. I've changed the reference list to be more accurate. (It looks like I was going for #1847 but transposed the last two digits?)

(though I wonder if there was no performance problem, how big of an issue it would be for people when Atom opens the file's project and tree view for single files 🤔)

Performance aside, I think there are other problems with loading and indexing large project folders (${HOME}, /etc), like that macOS Mojave bug where we were suddenly requesting access to everyone's calendar, or Linux with the experimental filesystem watchers which will eat up watch descriptors. We also save ourselves some potential pain from third-party packages doing something costly within atom.project.getPaths().

Using your example above, in this scenario (so without the explicit --add):

atom ~/projects/one/subdirectory/file.txt

Would Atom open a new window or open the file in window (1)?

Atom would open a new window with no project folders and a single editor open on ~/file.txt. The rule I went for is "say --add if you want to add".

On the other hand if you said:

atom --add ~/projects/one/subdirectory/file.txt

Then file.txt would be opened in window (1), even though window (2) was opened more recently. The rule with --add is "the existing window with a project folder containing the path, or the most recently opened window if there are none".

Is that reasonable... ?

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Jan 4, 2019

Atom would open a new window with no project folders and a single editor open on ~/file.txt. The rule I went for is "say --add if you want to add".

On the other hand if you said:

atom --add ~/projects/one/subdirectory/file.txt

Then file.txt would be opened in window (1), even though window (2) was opened more recently. The rule with --add is "the existing window with a project folder containing the path, or the most recently opened window if there are none".

Gotcha - at first I found it unexpected but then I re-read your comments and the behavior totally makes sense with these changes.

It does kinda seem like that particular case feels a bit fuzzy though - if I have a project open and I'm on the command line in that project directory, would I expect that opening a single file would by default open in an existing window with that project open? Personally though, I like the behavior here that chooses an explicit default and if you want to open in an existing window, you have to use --add.

So with the caveat you mentioned where some people might be super used to the existing behaviors, I like the changes. The --new-window by default behavior change feels a tiny bit more sticky than the single file behavior change though, so I'm curious what Lee thinks since he's seen years more feedback about these startup issues than I have 😄 👴

Sorry one more question too:

Opening a single file by path will load and modify the serialized project state for Atom's "temporary window state". This is different from the current behavior, which is to load the project state for the file's parent directory.

I definitely might be misunderstanding, but would it be possible to step on saved state in this case? E.g. if I:

  1. atom ~/projects/one
  2. Modify ~/projects/one/file.txt
  3. Don't save the changes and quit Atom
  4. atom ~/projects/one/file.txt

Will I see my unsaved changed when I get to step 4?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 6, 2019

Will I see my unsaved changed when I get to step 4?

Nope 😒 I will note that you can do this today by running atom --new-window ~/projects/one/file.txt in step 4. Of course this makes it more likely...

I didn't touch any of the logic used to manage serialized state - it's still a key derived from the set of project folders. There are a few other ways you can stomp on previously saved state with no warning today, I believe.

I'd also love to revisit the way Atom sessions are managed to also be more explicit and comprehensible. Maybe even with some UI to be able to see what sessions are available, unsaved buffers in each, etc. I'm even less certain what the ideal solution looks like there, though, so I haven't touched anything on that side of things yet 😄

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 7, 2019

Note to self: I think this will address #16645. Need to verify.

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 7, 2019

Nope 😒 I will note that you can do this today by running atom --new-window ~/projects/one/file.txt in step 4. Of course this makes it more likely...

Yep, this change will make it much more likely that people will run into #13318. We may want to hold off on making this change until we decide what to do about #13318 🤔

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 8, 2019

@lee-dohm I just checked, and we do show a save dialog if you have no open project folders:

screen shot 2019-01-08 at 10 41 35 am

Is that good enough to prevent data loss until we can rethink the way that project state is managed? Or do you think it's still too risky - are you worried about people automatically clicking "don't save" without thinking?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 8, 2019

@lee-dohm: Also - independently of that, what do you think about the changes I've proposed to the way that atom <path> <path>... works when neither --new-window nor --add are specified? Or is that not part of the problem with our launch scenarios and should I take it back to the way it was? (I'm honestly completely undecided about this - I tried to take it in a "more explicit/unambiguous" direction in the spirit of #10517, but I don't feel like I have strong insight into what users expect to result from any given command invocation...)

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 9, 2019

we do show a save dialog if you have no open project folders:

🤔 Hmmmmmmm ... We don't get that dialog when closing if there is at least one project folder, so yeah, I guess that's sufficient for now 👍

what do you think about the changes I've proposed to the way that atom ... works when neither --new-window nor --add are specified?

I think this is great, actually. The main concerns are that no matter what state Atom is currently in (not started, started with no windows (macOS only), started with one or more windows) that someone can launch the environment they want (empty environment, single-file, one or more project roots) as consistently as possible. I think you've achieved that or at the very least, taken us a good chunk of the way there 😀

The one question I have is can I launch an empty environment from the command line when there is no window open already? I assume that using atom --new-window with no arguments would achieve that?

smashwilson added some commits Jan 14, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 14, 2019

The one question I have is can I launch an empty environment from the command line when there is no window open already? I assume that using atom --new-window with no arguments would achieve that?

I just made sure that was the case 👍

Here's the new help text. Totally open to workshopping it, it's a little wordy:

$ atom-dev --help
Atom Editor v1.36.0-dev-f20aa038b

Usage:
  atom
  atom [options] [path ...]
  atom file[:line[:column]]

If no arguments are given and no Atom windows are already open, restore all windows
from the previous editing session. Use "atom --new-window" to open a single empty
Atom window instead.

If no arguments are given and at least one Atom window is open, open a new, empty
Atom window.

One or more paths to files or folders may be specified. All paths will be opened
in a new Atom window. Each file may be opened at the desired line (and optionally
column) by appending the numbers after the file name, e.g. `atom file:5:8`.

Paths that start with `atom://` will be interpreted as URLs.

Environment Variables:

  ATOM_DEV_RESOURCE_PATH  The path from which Atom loads source code in dev mode.
                          Defaults to `~/github/atom`.

  ATOM_HOME               The root path for all configuration files and folders.
                          Defaults to `~/.atom`.

Options:
  -1, --one                  This option is no longer supported.  [boolean]
  --include-deprecated-apis  This option is not currently supported.  [boolean]
  -d, --dev                  Run in development mode.  [boolean]
  -f, --foreground           Keep the main process in the foreground.  [boolean]
  -h, --help                 Print this usage message.  [boolean]
  -l, --log-file             Log all output to file.  [string]
  -n, --new-window           Launch an empty Atom window instead of restoring previous session.  [boolean]
  --profile-startup          Create a profile of the startup execution time.  [boolean]
  -r, --resource-path        Set the path to the Atom source directory and enable dev-mode.  [string]
  --safe                     Do not load packages from ~/.atom/packages or ~/.atom/dev/packages.  [boolean]
  --benchmark                Open a new window that runs the specified benchmarks.  [boolean]
  --benchmark-test           Run a faster version of the benchmarks in headless mode.  [boolean]
  -t, --test                 Run the specified specs and exit with error code on failures.  [boolean]
  -m, --main-process         Run the specified specs in the main process.  [boolean]
  --timeout                  When in test mode, waits until the specified time (in minutes) and kills the process (exit code: 130).  [string]
  -v, --version              Print the version information.  [boolean]
  -w, --wait                 Wait for window to be closed before returning.  [boolean]
  --clear-window-state       Delete all Atom environment state.  [boolean]
  --enable-electron-logging  Enable low-level logging messages from Electron.  [boolean]
  -a, --add                  Open path as a new project in last used window.  [boolean]
@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 14, 2019

For help text, I think this is good enough for now. If I feel inspired, I'll workshop it later 😀

@smashwilson smashwilson requested a review from daviwil Jan 14, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 14, 2019

Note: this does not address #16645. I'll revamp the way we track and serialize representedDirectoryPaths next in its own PR.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jan 14, 2019

I really like the simplicity and explicitness of this approach, should make it easier for users to know what to expect when they launch Atom from the command line. I suppose the biggest danger right now related to #13318 is when Atom gets forcibly closed either through a crash, process kill, or forced system restart (ahem Windows). Would it be worth trying to solve that problem too before the next Beta release (in a separate PR!) so that users don't get bitten by this change?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 14, 2019

Would it be worth trying to solve that problem too before the next Beta release (in a separate PR!) so that users don't get bitten by this change?

🤔 I think addressing that is going to require some more invasive redesign of the way we manage session state.... which I'd love to tackle, but I'd want to write up in an RFC and get buy-in before I started hacking on, so I'm not sure I'd be able to get it shipped by the next beta. Unless there was a quicker band-aid fix you had in mind that I could tackle in the next day or so?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 15, 2019

Okay - I'll merge this, then poke at writing an RFC for some ideas we could pursue for session management. Thanks for the feedback 😄

@smashwilson smashwilson merged commit cbe3080 into master Jan 15, 2019

3 checks passed

Atom Pull Requests #20190114.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw/single-file branch Jan 15, 2019

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.