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

Restore atom environment when adding project folders to a fresh window #13963

Merged
merged 25 commits into from Mar 27, 2017

Conversation

Projects
None yet
4 participants
@BinaryMuse
Member

BinaryMuse commented Mar 9, 2017

This PR addresses #13873, where the core issue is that re-opening a project when you have a window open with no project folders simply adds those project folders to the existing window, but adding project folders does not restore state for that set of project folders

The primary bad UX that results from this is that re-opening a project from the "Recent Projects" menu when you have a fresh Atom window doesn't really restore the previous project state; it just adds those folders to the project and any unsaved work that's stored in the project state is lost, making the user a sad panda.

This addresses the problem by simply restoring the project state for a folder or set of folders when:

  1. The folders are added via "Add Project Folders," or the folders are added automatically via the "Open" menu or via the atom command,
  2. The folders are being added to an Atom window in which there are zero project folders

This results in happier pandas.

Edge Cases

The Atom window has no project folders but already contains pane items

In this case, we serialize all the current pane items, restore the saved state from the snapshot, and then re-add the serialized pane items to the new workspace. We skip unchanged, empty editors (ones with no changes and with no content, like the one you get when "Open Empty Editor On Start" is enabled), so in the case that the user opens a new window and immediately re-opens a previously open project, the right thing should Just Happen™. It's automagical! 🌈

restore-project-state-with-open-items

TODO:

  • Don't ask the user to save or discard the changes
  • Make the currently active pane item the active pane item after the state is restored (unless the user specifically opened a file, in which case that file will be active post-restore)

The Atom window has pane items that "conflict" with ones in the state snapshot

If the user already has a file open with unsaved changes, and that same file is open in the saved state snapshot, then the result of the operation is that there will be two copies of the file open, each backed by a different buffer.

This is super edge-casey as it's kinda hard to get into this state (since opening a file adds its parent folder to the project automatically), so I'm not sure if it's one we need to address. 💭 s appreciated.

TODO:

  • ???

Using project.setPaths or project.addPath does not trigger this behavior

Right now, the code paths for "Application: Add Project Folder" and for opening a file with the "Open" command or with atom go through code path that project.setPaths and project.addPath do not. We opted not to change that behavior since it would be a breaking change.

Design Considerations

It turns out that serializing a text editor and then deserializing it into a new AtomEnvironment is not very straightforward. The sorta hacky solution implemented here so far is to:

  1. Serialize all the current pane items (text editors)
  2. Also serialize all the buffers in @project.buffers
  3. Restore the serialized AtomEnvironment state (minus window size and fullscreen setting)
  4. Push the serialized buffers from step 2 into the new @project.buffers
  5. Deserialize the serialized pane items from step 1

This feels 🙅 pretty gross 🙅 but we're not quite sure of a better way to handle it. 💭 s from @atom/core highly appreciated!


Fixes #13873
Fixes #13861
Fixes #11293
Fixes #10044

kuychaco added some commits Mar 8, 2017

Add AtomEnvironment::restoreStateIntoEnvironment
This takes an existing Atom environment and restores a saved state from
the IndexDB state storage into it by:

1. Serializing the existing pane items
2. Restoring the saved state
3. Deserializing the saved pane items into the newly restored
environment
@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Mar 9, 2017

Member

We caught more interesting edge cases.

  1. Have a project open with one folder, and some pane items open.
  2. Remove the project folder.
  3. Re-add the project folder. The tabs you currently have open will be duplicated (because we add them to the deserialized saved state).

TODO:

  • Don't restore unmodified initial pane items after restoration if those pane items already exist in the saved state snapshot

If you repeat steps 2 and 3 again, you'll get an exception: Uncaught (in promise) Error: Assertion failed: defaultMarkerLayer destroyed at an unexpected time(…)

Member

BinaryMuse commented Mar 9, 2017

We caught more interesting edge cases.

  1. Have a project open with one folder, and some pane items open.
  2. Remove the project folder.
  3. Re-add the project folder. The tabs you currently have open will be duplicated (because we add them to the deserialized saved state).

TODO:

  • Don't restore unmodified initial pane items after restoration if those pane items already exist in the saved state snapshot

If you repeat steps 2 and 3 again, you'll get an exception: Uncaught (in promise) Error: Assertion failed: defaultMarkerLayer destroyed at an unexpected time(…)

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Mar 9, 2017

Member

/cc @nathansobo for 💭 s as we chatted briefly about copying TextEditors across AtomEnvironments.

Member

BinaryMuse commented Mar 9, 2017

/cc @nathansobo for 💭 s as we chatted briefly about copying TextEditors across AtomEnvironments.

Merge remote-tracking branch 'origin/master' into ku-mkt-restore-atom…
…-env-when-adding-folder-to-fresh-window
@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Mar 9, 2017

Member

Another option is to do away with the whole idea of trying to "merge" the current session with the saved session.

There are four ways in which the user might end up adding a project folder to a window that doesn't have any:

  1. atom . or atom /path/to/folder
  2. File -> Open
  3. File -> Reopen Project
  4. Add Project Folder

In the case of 1, 2, and 3, it seems like we could detect that the current window is "dirty" (has pane items) and then simply open a new Atom window instead of reusing the current one.

In the case of 4, we could notify the user that there is a saved project state, and give them the option of opening that state in a new Atom window, or adding the folder to the current window (which would cause the user to lose that saved state, including any unsaved buffers in that state snapshot).

Member

BinaryMuse commented Mar 9, 2017

Another option is to do away with the whole idea of trying to "merge" the current session with the saved session.

There are four ways in which the user might end up adding a project folder to a window that doesn't have any:

  1. atom . or atom /path/to/folder
  2. File -> Open
  3. File -> Reopen Project
  4. Add Project Folder

In the case of 1, 2, and 3, it seems like we could detect that the current window is "dirty" (has pane items) and then simply open a new Atom window instead of reusing the current one.

In the case of 4, we could notify the user that there is a saved project state, and give them the option of opening that state in a new Atom window, or adding the folder to the current window (which would cause the user to lose that saved state, including any unsaved buffers in that state snapshot).

BinaryMuse added some commits Mar 23, 2017

Restore state when adding folders to applicable windows
Note: "clean window" is defined as 1) having an empty project and 2)
having no pane items or only empty unnamed buffers

Adding folder(s)
* If we have a clean window, restore project state in window
* If window is dirty, prompt user to
  * add folder to the existing window LOSING state
  * OR open project folder in a new window
Restore state when opening folders to applicable windows
Note: "clean window" is defined as 1) having an empty project and 2)
having no pane items or only empty unnamed buffers

When project is empty and there is saved state associated with the
opened/added folders...
* Open a file or folder (from command line or Open menu)
  * If we have a clean window, restore project state in window
  * If window is dirty, restore saved state in new window
@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Mar 23, 2017

Member

So we did away with merging the current session as mentioned in the previous comment.

@ungb would you mind doing some testing to help ensure we covered all the edge cases? 🙏
Below is a decision tree of the expected behavior after the changes we made:

Note: "clean window" is defined as 1) having an empty project and 2) having no pane items or only empty unnamed buffers

When project is empty and there is saved state associated with the opened/added folders...

  • Open a file or folder (from command line or Open menu)

    • If we have a clean window, restore project state in window
    • If window is dirty, restore saved state in new window
  • Add folder(s)

    • If we have a clean window, restore project state in window
    • If window is dirty, prompt user to
      • add folder to the existing window LOSING state
      • OR open project folder in a new window
Member

BinaryMuse commented Mar 23, 2017

So we did away with merging the current session as mentioned in the previous comment.

@ungb would you mind doing some testing to help ensure we covered all the edge cases? 🙏
Below is a decision tree of the expected behavior after the changes we made:

Note: "clean window" is defined as 1) having an empty project and 2) having no pane items or only empty unnamed buffers

When project is empty and there is saved state associated with the opened/added folders...

  • Open a file or folder (from command line or Open menu)

    • If we have a clean window, restore project state in window
    • If window is dirty, restore saved state in new window
  • Add folder(s)

    • If we have a clean window, restore project state in window
    • If window is dirty, prompt user to
      • add folder to the existing window LOSING state
      • OR open project folder in a new window
@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Mar 27, 2017

Contributor

LGTM! I spend some time testing this on Friday. Only edge case I saw was on mac when there's no windows open and you go to context menu nothing works but that already exist previously. This is a huge improvement for the data loss scenarios.

Contributor

ungb commented Mar 27, 2017

LGTM! I spend some time testing this on Friday. Only edge case I saw was on mac when there's no windows open and you go to context menu nothing works but that already exist previously. This is a huge improvement for the data loss scenarios.

@BinaryMuse BinaryMuse merged commit 1ff5c9e into master Mar 27, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@BinaryMuse BinaryMuse deleted the ku-mkt-restore-atom-env-when-adding-folder-to-fresh-window branch Mar 27, 2017

@50Wliu 50Wliu referenced this pull request Apr 2, 2017

Open

Data loss when adding and removing project folders #14113

1 of 1 task complete
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Apr 4, 2017

Contributor

We've been seeing some intermittent main process test failures on CI that may be caused by this PR. Here's one:

  1) AtomApplication launch reuses existing windows when opening paths, but not directories:
     Error: async(5000ms): timed out waiting until window:locations-opened is emitted
      at Promise.resolve.catch.then.result (node_modules/test-until/index.js:56:25)

https://circleci.com/gh/atom/atom/3256?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Contributor

maxbrunsfeld commented Apr 4, 2017

We've been seeing some intermittent main process test failures on CI that may be caused by this PR. Here's one:

  1) AtomApplication launch reuses existing windows when opening paths, but not directories:
     Error: async(5000ms): timed out waiting until window:locations-opened is emitted
      at Promise.resolve.catch.then.result (node_modules/test-until/index.js:56:25)

https://circleci.com/gh/atom/atom/3256?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

let emitted = false
emitter.once(event, () => { emitted = true })
return until(`${event} is emitted`, () => emitted, timeout)
}

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Apr 4, 2017

Contributor

I think we could just resolve this promise explicitly when the event is emitted. The current code works but the polling seems unnecessary.

export function emitterEventPromise (emitter, event, timeout = 5000) {
  return new Promise((resolve, reject) => {
    const timeoutHandle = setTimeout(reject, timeout)
    emitter.once(event, () => {
      clearTimeout(timeoutHandle)
      resolve()
    })
  })
}
@maxbrunsfeld

maxbrunsfeld Apr 4, 2017

Contributor

I think we could just resolve this promise explicitly when the event is emitted. The current code works but the polling seems unnecessary.

export function emitterEventPromise (emitter, event, timeout = 5000) {
  return new Promise((resolve, reject) => {
    const timeoutHandle = setTimeout(reject, timeout)
    emitter.once(event, () => {
      clearTimeout(timeoutHandle)
      resolve()
    })
  })
}

@50Wliu 50Wliu referenced this pull request Dec 7, 2017

Closed

Tree View creation races with deserialization #1210

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment