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

Only store directories in serialized application state #19207

Merged
merged 6 commits into from Apr 24, 2019

Conversation

Projects
None yet
2 participants
@smashwilson
Copy link
Member

commented Apr 24, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Fixes #19146 without regressing #16645.

Atom's serialized application state has been erroneously written with file paths given on the command line as well as project root directories. This is triggering the behavior introduced in #18742 to fix #16645, which is intended to detect the situation when a project root directory has been deleted from disk (and possibly replaced by a file) and rescue any window session state that was keyed by that directory.

Description of the Change

I've modified the schema used to write ${ATOM_HOME}/storage/application.json to be version-tagged and only include project root directories, with some renaming for clarity:

{
  "version": "1",
  "windows": [
    {"projectRoots": ["<root-dir>", "<root-dir>"]},
    {"projectRoots": ["<root-dir>"]}
  ]
}

Application state may be loaded from either this schema or the previous, untagged one. When loading serialized state from the previous schema, non-directory paths are filtered out from each initialPaths array.

Alternate Designs

One solution to the immediate problem would be to leave the schema unchanged and always filter non-directories from initialPaths on load. The problem with this is that we use the set of project root directories to key a window's serialized session state. By removing paths that used to be legitimate project root directories, we would be changing the session key, and possibly orphaning the session. For example:

# Assume core.restorePreviousWindowsOnStart !== "no"
$ atom a/ b/
# Open a file within b/, make changes, and do not save them
# Quit the application
$ rm -r a/
$ atom
# Atom launches one window containing b/; unsaved file changes are lost!

Possible Drawbacks

There is a small chance that a path in initialPaths within unversioned application state used to be a legitimate project root, and therefore should be used to derive the window's state key. Unfortunately, we have no way to tell the difference between this case and the much more common case of initialPaths containing a file path.

Verification Process

For all cases, set the core.restorePreviousWindowsOnStart configuration setting to "always".

Assume the following directory structure:

temp/
  a/
    1.md
  b/
    2.md
  c/

Reading unversioned application state

Populate ~/.atom/storage/application.json with the following:

[
  {"initialPaths": ["/temp/a", "/temp/b/2.md"]},
  {"initialPaths": ["/temp/b"]},
]

Open Atom with no arguments.

  • No error notifications should appear.
  • Two windows should open: one containing only the project root /temp/a, and one containing only the project root /temp/b.

Writing and reading version 1 application state

Open Atom and create two windows. Adjust project root directories such that:

  • One window contains /temp/a.
  • One window contains /temp/b and /temp/c. Focus this window (so that it's the most recently focused).

Quit Atom.

Verify that ~/.atom/storage/application.json contains the following (formatted less nicely):

{
  "version": "1",
  "windows": [
    {"projectRoots": ["/temp/a"]},
    {"projectRoots": ["/temp/b", "/temp/c"]}
  ]
}

Re-open Atom with no arguments. Verify that:

  • The original windows are restored, and
  • Focus is on the window containing /temp/b and /temp/c.

Make changes to the file /temp/b/2.md and don't save them. Quit Atom.

Delete the /temp/c directory:

rm -r /temp/c

Open Atom with no arguments. Verify:

  • One window is opened containing /temp/a.
  • Another window is opened containing /temp/b.
  • A notification appears in the window containing /temp/b indicating that /temp/c no longer exists.
  • A buffer is open in the window containing /temp/b on the file /temp/b/2.md and it includes your unsaved changes.

Release Notes

  • After opening Atom with a file path and quitting, re-opening Atom no longer displays an incorrect error stating that the file could not be found.

smashwilson added some commits Apr 24, 2019

Save application state with a version-tagged schema
Load application state from the current or previous application.json 
schema. Filter out file paths when loading old state.
@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Hah. Looks like we're showing two notifications when a project directory is no longer present:

image

Let's see where those are coming from.

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Alright, got the duplicate notification fixed. This one's ready for review 🏁

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

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

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

👋 Hi @smashwilson: Thanks for taking this on. I tried this out locally, and there's one oddity that I'm seeing.

I had the following serialized state created with Atom 1.36.0, containing a total of 5 windows:

$ cat ~/.atom/storage/application.json | jq

[
  {
    "initialPaths": []
  },
  {
    "initialPaths": [
      "/Users/j/github/atom"
    ]
  },
  {
    "initialPaths": [
      "/Users/j/github/apm"
    ]
  },
  {
    "initialPaths": [
      "/Users/j/github/toggle-quotes/README.md"
    ]
  },
  {
    "initialPaths": [
      "/Users/j/github/settings-view"
    ]
  }
]

When I then opened Atom Dev (built from this branch), it only opened 2 windows, and it serialized the state as follows:

$ cat ~/.atom/storage/application.json | jq

{
  "version": "1",
  "windows": [
    {
      "projectRoots": [
        "/Users/j/github/apm"
      ]
    },
    {
      "projectRoots": [
        "/Users/j/github/settings-view"
      ]
    }
  ]
}

I understand why no Atom window was opened for "initialPaths": ["/Users/j/github/toggle-quotes/README.md"] (since that's a file, it's not a directory). And I could see why Atom might not open a window for "initialPaths": []. But, I'm surprised that no window was opened for "initialPaths": ["/Users/j/github/atom"]. Does that seem like a bug, or am I perhaps misunderstanding how things should function?

smashwilson added some commits Apr 24, 2019

@smashwilson

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@jasonrudolph: Finally tracked it down. The issue is that AtomWindow::openLocations() calls only update the projectRoots property asynchronously, while we were making decisions about where to open recovered paths in a synchronous loop.

I've updated this to:

  • Update projectRoots synchronously as soon as openLocations() is called;
  • Add the {newWindow: true} argument to options for recovered window options; then,
  • Open recovered window options before command-line-argument-provided options are opened, so that they collapse into existing windows as expected.
@jasonrudolph
Copy link
Member

left a comment

Nice work, @smashwilson. :shipit:

@smashwilson smashwilson merged commit 286a110 into master Apr 24, 2019

2 checks passed

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

@smashwilson smashwilson deleted the aw/versioned-application-state branch Apr 24, 2019

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

Merge pull request #19207 from atom/aw/versioned-application-state
Only store directories in serialized application state

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

Merge pull request #19207 from atom/aw/versioned-application-state
Only store directories in serialized application state

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