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

Make an option to always restore the last session #13947

Merged
merged 5 commits into from May 8, 2017

Conversation

Projects
None yet
@anatoli26
Contributor

anatoli26 commented Mar 7, 2017

Make an option to always restore the last session, no matter how Atom is invoked (#9643).

Description of the Change

src/config-schema.js: the change adds the option to the Settings window.

src/main-process/atom-application.coffee:

  • line 114: load previous session if the option is turned on;
  • line 563: IMO it's a bug, here the newly created window is activated, but @lastFocusedWindow is not updated, so on the second invocation at the same moment in time there'll be no lastFocusedWindow and a second (new) window would be created (at line 542/544 -> 550/552).
  • line 604: if any of the options is checked, restore the last session (applicable when starting atom without arguments)
  • line 817: a function to duplicate an object, taken from: https://coffeescript-cookbook.github.io/chapters/classes_and_objects/cloning

Alternate Designs

TL;DR: none, see the issue #9643 for an extended discussion.

Why Should This Be In Core?

This is a standard behavior for most of modern multi-tab apps today, i.e. the ability to restore the last session after crash or normal exit, no matter how the app is invoked for the first time since the last session (i.e. with or without path parameters).

Benefits

See the previous point.

Possible Drawbacks

None as this is an option turned off by default, so it won't affect anyone if not explicitly terned on by the user.

Applicable Issues

So far no issues were discovered.

Other

script/test: 23 passing (2m), i.e. no errors when the option is turned off (the default behavior)
Jasmine specs in the ./spec folder are not provided

atom_settings

Show outdated Hide outdated src/config-schema.js
@@ -262,6 +262,11 @@ const configSchema = {
type: 'boolean',
default: true
},
restorePreviousWindowsOnStartAlways: {
description: 'When checked *ALWAYS* restores the last state of all Atom windows.',

This comment has been minimized.

@BrunoBernardino

BrunoBernardino Mar 7, 2017

I'd only add , even if opening a single file with Atom right before the full stop.

@BrunoBernardino

BrunoBernardino Mar 7, 2017

I'd only add , even if opening a single file with Atom right before the full stop.

This comment has been minimized.

@anatoli26

anatoli26 Mar 7, 2017

Contributor

@BrunoBernardino, actually, it could be more than one file, like: atom fileX fileY fileZ and it will restore the last session + there will open the 3 files, so if the short "just always" version is not quite clear, maybe add at the end ", no matter how Atom is started".

@anatoli26

anatoli26 Mar 7, 2017

Contributor

@BrunoBernardino, actually, it could be more than one file, like: atom fileX fileY fileZ and it will restore the last session + there will open the 3 files, so if the short "just always" version is not quite clear, maybe add at the end ", no matter how Atom is started".

This comment has been minimized.

@BrunoBernardino

BrunoBernardino Mar 7, 2017

I see your point. I think your version is better then.

@BrunoBernardino

BrunoBernardino Mar 7, 2017

I see your point. I think your version is better then.

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Mar 7, 2017

Member

This shouldn't be a separate option from core.restorePreviousWindowsOnStart. core.restorePreviousWindowsOnStart should become a three-state option: false, true and always, and the description text should be updated to reflect what each means.

Member

lee-dohm commented Mar 7, 2017

This shouldn't be a separate option from core.restorePreviousWindowsOnStart. core.restorePreviousWindowsOnStart should become a three-state option: false, true and always, and the description text should be updated to reflect what each means.

Make an option to *always* restore the last session, no matter how At…
…om is invoked (#9643), part2: new option in Settings is merged with the old one, the result is a 3-value combobox [no, yes, always]
@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 8, 2017

Contributor

Done. Also updated the test to reflect the new set of values.

atom

Contributor

anatoli26 commented Mar 8, 2017

Done. Also updated the test to reflect the new set of values.

atom

Show outdated Hide outdated src/main-process/atom-application.coffee
@@ -812,3 +814,10 @@ class AtomApplication
args.push("--resource-path=#{@resourcePath}")
app.relaunch({args})
app.quit()
clone = (obj) ->

This comment has been minimized.

@steelbrain

steelbrain Mar 8, 2017

Contributor

Maybe use Object.assign or _.clone instead of reinventing clone? :)

@steelbrain

steelbrain Mar 8, 2017

Contributor

Maybe use Object.assign or _.clone instead of reinventing clone? :)

This comment has been minimized.

@anatoli26

anatoli26 Mar 8, 2017

Contributor

Here a recursive clone is needed, so neither of these functions will work, but maybe _.cloneDeep could be used.

@anatoli26

anatoli26 Mar 8, 2017

Contributor

Here a recursive clone is needed, so neither of these functions will work, but maybe _.cloneDeep could be used.

Make an option to *always* restore the last session, no matter how At…
…om is invoked (#9643), part3: use _.cloneDeep from lodash instead of own clone function
@jsolisu

This comment has been minimized.

Show comment
Hide comment
@jsolisu

jsolisu Mar 10, 2017

Please check always option, it does not work well, Atom hangs on Windows 10.

jsolisu commented Mar 10, 2017

Please check always option, it does not work well, Atom hangs on Windows 10.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

How do you start it? What do you get with 'yes' option? I don't have windows 10 to test, any other OS that manifests the same behavior?

Contributor

anatoli26 commented Mar 10, 2017

How do you start it? What do you get with 'yes' option? I don't have windows 10 to test, any other OS that manifests the same behavior?

@jsolisu

This comment has been minimized.

Show comment
Hide comment
@jsolisu

jsolisu Mar 10, 2017

@anatoli26 yes option is ok. You can test using "Open with Atom" (as folder) from Windows Explorer. I only use Windows, sorry.

jsolisu commented Mar 10, 2017

@anatoli26 yes option is ok. You can test using "Open with Atom" (as folder) from Windows Explorer. I only use Windows, sorry.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

If you "Open with Atom" a single file what happens? If you run it from the command line like atom <path_to_file>? Do you have access to Win7/8 to test it?

What do you mean by hangs? Complete freeze, atom process has no activity in task manager? Or does it process something (e.g. high CPU usage, disk reads, etc.)?

Contributor

anatoli26 commented Mar 10, 2017

If you "Open with Atom" a single file what happens? If you run it from the command line like atom <path_to_file>? Do you have access to Win7/8 to test it?

What do you mean by hangs? Complete freeze, atom process has no activity in task manager? Or does it process something (e.g. high CPU usage, disk reads, etc.)?

@jsolisu

This comment has been minimized.

Show comment
Hide comment
@jsolisu

jsolisu Mar 10, 2017

  • Using "Open with Atom" with a folder, atom process has no activity in task manager.
  • Using "Open with Atom" with a single, fails too.
  • Using command line atom <path_to_file>, fails too.

In all cases, if you try again (Without killing the frozen process) it works.

Calling Atom directly (using Icon) works well.

Sorry. I don´t have Win7/8 to test.

jsolisu commented Mar 10, 2017

  • Using "Open with Atom" with a folder, atom process has no activity in task manager.
  • Using "Open with Atom" with a single, fails too.
  • Using command line atom <path_to_file>, fails too.

In all cases, if you try again (Without killing the frozen process) it works.

Calling Atom directly (using Icon) works well.

Sorry. I don´t have Win7/8 to test.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

Try removing node_modules folder in the root of the repo and rebuild the package again.

If that doesn't help, please try compiling it without the latest commit (6cc90c4) that uses cloneDeep instead of own clone function.

Contributor

anatoli26 commented Mar 10, 2017

Try removing node_modules folder in the root of the repo and rebuild the package again.

If that doesn't help, please try compiling it without the latest commit (6cc90c4) that uses cloneDeep instead of own clone function.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

The problem is actually with cloneDeep from lodash not available as _ is defined as underscore-plus package, not lodash, so cloneDeep is searched in underscore-plus and logically is not found. At the same time, when Atom is not running in foreground (with -f flag) it doesn't show errors, but silently stops (hangs) in the middle of initialization, that's why it was appearing as hanged to @jsolisu.

In this case I'm not sure if it makes sense to include lodash (looks like it's not included otherwise) or to define own deepclone function as it was before the latest commit. @steelbrain, any suggestions?

Can anyone please explain the rationale behind underscore-plus, i.e. why atom has its own underscore package with some enhancements instead of lodash, which is a much better option, than underscore (frequent updates, extended functionality, etc.)?

Contributor

anatoli26 commented Mar 10, 2017

The problem is actually with cloneDeep from lodash not available as _ is defined as underscore-plus package, not lodash, so cloneDeep is searched in underscore-plus and logically is not found. At the same time, when Atom is not running in foreground (with -f flag) it doesn't show errors, but silently stops (hangs) in the middle of initialization, that's why it was appearing as hanged to @jsolisu.

In this case I'm not sure if it makes sense to include lodash (looks like it's not included otherwise) or to define own deepclone function as it was before the latest commit. @steelbrain, any suggestions?

Can anyone please explain the rationale behind underscore-plus, i.e. why atom has its own underscore package with some enhancements instead of lodash, which is a much better option, than underscore (frequent updates, extended functionality, etc.)?

@steelbrain

This comment has been minimized.

Show comment
Hide comment
@steelbrain

steelbrain Mar 10, 2017

Contributor

@anatoli26 Whenever I need a lightweight lodash plugin that I don't wanna install the whole thing, I do npm install lodash.clonedeep and yes, these are official packages by the lodash team

Contributor

steelbrain commented Mar 10, 2017

@anatoli26 Whenever I need a lightweight lodash plugin that I don't wanna install the whole thing, I do npm install lodash.clonedeep and yes, these are official packages by the lodash team

Make an option to *always* restore the last session, no matter how At…
…om is invoked (#9643), part4: import cloneDeep directly from lodash
@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

OK, just defined the cloneDeep function as cloneDeep = require 'lodash.clonedeep' and added a dependency for it in package.json.

@jsolisu, please test it again with the latest commit.

Contributor

anatoli26 commented Mar 10, 2017

OK, just defined the cloneDeep function as cloneDeep = require 'lodash.clonedeep' and added a dependency for it in package.json.

@jsolisu, please test it again with the latest commit.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 10, 2017

Member

I think we'd prefer not adding the lodash dependency and just going with what you had previously (/cc @atom/maintainers).

@steelbrain my guess would be that lodash didn't exist or wasn't mature enough when Atom was being built. Underscore was released in 2009; lodash in 2012.

Member

50Wliu commented Mar 10, 2017

I think we'd prefer not adding the lodash dependency and just going with what you had previously (/cc @atom/maintainers).

@steelbrain my guess would be that lodash didn't exist or wasn't mature enough when Atom was being built. Underscore was released in 2009; lodash in 2012.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 10, 2017

Contributor

If we need a single function from lodash and it's not trivially implemented in another way or already available in our existing dependencies, I'm comfortable pulling in just that function. In general I want to minimize additional dependencies.

Contributor

nathansobo commented Mar 10, 2017

If we need a single function from lodash and it's not trivially implemented in another way or already available in our existing dependencies, I'm comfortable pulling in just that function. In general I want to minimize additional dependencies.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 10, 2017

Member

Also, I just checked and underscore-plus does offer a deep clone, _.deepClone. https://github.com/atom/underscore-plus/blob/4a022cf721c2561d20890c5b7faf5e9ba4832a7b/src/underscore-plus.coffee#L98

Member

50Wliu commented Mar 10, 2017

Also, I just checked and underscore-plus does offer a deep clone, _.deepClone. https://github.com/atom/underscore-plus/blob/4a022cf721c2561d20890c5b7faf5e9ba4832a7b/src/underscore-plus.coffee#L98

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 10, 2017

Contributor

Then we definitely shouldn't add a new dependency for something we already have.

Contributor

nathansobo commented Mar 10, 2017

Then we definitely shouldn't add a new dependency for something we already have.

Make an option to *always* restore the last session, no matter how At…
…om is invoked (#9643), part5: use _.deepClone from underscore-plus
@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

underscore-plus does offer a deep clone

Haven't seen it due to its reverse-name. Sure now it's much simpler, just had to change the name. No dependency on lodash now.

src/config.coffee should probably also use _.deepClone from underscore-plus (it implements its own function now (line 999)).

Contributor

anatoli26 commented Mar 10, 2017

underscore-plus does offer a deep clone

Haven't seen it due to its reverse-name. Sure now it's much simpler, just had to change the name. No dependency on lodash now.

src/config.coffee should probably also use _.deepClone from underscore-plus (it implements its own function now (line 999)).

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 10, 2017

Member

src/config.coffee should probably also use _.deepClone from underscore-plus

Can you create a new issue for that so we don't forget about it?

Member

50Wliu commented Mar 10, 2017

src/config.coffee should probably also use _.deepClone from underscore-plus

Can you create a new issue for that so we don't forget about it?

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Mar 10, 2017

Contributor

I don't see how this is going to solve the problem in the issue linked.

The underlying problem there isn't that Atom doesn't restore your last project (it does) - it's that opening a random file effectively creates a new project that is now your last project.

How does this address that - I can't tell from the diff.

Contributor

damieng commented Mar 10, 2017

I don't see how this is going to solve the problem in the issue linked.

The underlying problem there isn't that Atom doesn't restore your last project (it does) - it's that opening a random file effectively creates a new project that is now your last project.

How does this address that - I can't tell from the diff.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 10, 2017

Contributor

Can you create a new issue...

#13980

@damieng, there's a mix of use cases in #9643, most of which are solved with this PR.

The original post said:

If I have any added project folders they are successfully restored every time I fire up Atom. However, when I choose to open a single file or a directory using the right click menu, all added project folders and open file tabs are removed.

This is the actual behavior with the option set to 'yes', i.e. when Atom is started without arguments, it restores the previous session with all folders and tabs. When it's started with arguments, it doesn't restore the previous session. So the user then said:

I believe adding a folder/file would be much more intuitive

This is the behavior with the option set to 'always', i.e. when Atom is started with arguments, it restores the previous session and adds the files (requested via arguments) to the restored session.

Then other users said:

Yes this is very annoying. I'm forever re-adding all my project folders.

This can only be a blatant bug. This makes no sense. Now I have to go add the project folder back---and I'll have to do it every time I use Atom to quickly edit a file on the fly. Which means I won't use Atom to edit files on the fly, and I'll stick with EmEditor or something else.

Very unhappy about this behaviour as well. I am constantly re-adding my project folders which is very time consuming.

What most users were reporting is that opening Atom with arguments was forgetting the previously opened folders and tabs, so the users had to re-add them manually again and again. What the 'always' option does is that the project folders and tabs are never lost, so no need to re-add them if you start Atom with arguments or use the context menu "Open with Atom" when Atom is not running.

If you just seen the code, I suggest you try to build Atom with this PR and compare the behavior with 'yes' and 'always'. You'll see that with 'always' Atom never forgets the previously added folders/tabs.

Contributor

anatoli26 commented Mar 10, 2017

Can you create a new issue...

#13980

@damieng, there's a mix of use cases in #9643, most of which are solved with this PR.

The original post said:

If I have any added project folders they are successfully restored every time I fire up Atom. However, when I choose to open a single file or a directory using the right click menu, all added project folders and open file tabs are removed.

This is the actual behavior with the option set to 'yes', i.e. when Atom is started without arguments, it restores the previous session with all folders and tabs. When it's started with arguments, it doesn't restore the previous session. So the user then said:

I believe adding a folder/file would be much more intuitive

This is the behavior with the option set to 'always', i.e. when Atom is started with arguments, it restores the previous session and adds the files (requested via arguments) to the restored session.

Then other users said:

Yes this is very annoying. I'm forever re-adding all my project folders.

This can only be a blatant bug. This makes no sense. Now I have to go add the project folder back---and I'll have to do it every time I use Atom to quickly edit a file on the fly. Which means I won't use Atom to edit files on the fly, and I'll stick with EmEditor or something else.

Very unhappy about this behaviour as well. I am constantly re-adding my project folders which is very time consuming.

What most users were reporting is that opening Atom with arguments was forgetting the previously opened folders and tabs, so the users had to re-add them manually again and again. What the 'always' option does is that the project folders and tabs are never lost, so no need to re-add them if you start Atom with arguments or use the context menu "Open with Atom" when Atom is not running.

If you just seen the code, I suggest you try to build Atom with this PR and compare the behavior with 'yes' and 'always'. You'll see that with 'always' Atom never forgets the previously added folders/tabs.

@jsolisu

This comment has been minimized.

Show comment
Hide comment
@jsolisu

jsolisu Mar 11, 2017

@anatoli26 With _.deepClone Atom does not freeze, but now it creates two instances.

jsolisu commented Mar 11, 2017

@anatoli26 With _.deepClone Atom does not freeze, but now it creates two instances.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 11, 2017

Contributor

@jsolisu, if you mean that it creates 2 windows when you open a folder, it's by design. I.e. if you want to open a folder that would be added to the restored session in the same window, you should pass -a (--add) param (e.g. atom -a <path_to_a_folder>). Otherwise, it'll restore the previous session in one window and open a new window for the requested folder. It's the same behavior as if Atom is opened and you open a folder: the folder will be opened in a new window.

If you open file(s), it(they) will be added to the same window where the previous session is restored. It's possible to make some changes to open a new folder always in the same window, but I guess it's not what others would expect.

Please let me know if my explanation is not clear.

We could create a new option that would indicate to always open a new folder in the same window, but I believe it would be better to start a new issue and a new PR for this.

Contributor

anatoli26 commented Mar 11, 2017

@jsolisu, if you mean that it creates 2 windows when you open a folder, it's by design. I.e. if you want to open a folder that would be added to the restored session in the same window, you should pass -a (--add) param (e.g. atom -a <path_to_a_folder>). Otherwise, it'll restore the previous session in one window and open a new window for the requested folder. It's the same behavior as if Atom is opened and you open a folder: the folder will be opened in a new window.

If you open file(s), it(they) will be added to the same window where the previous session is restored. It's possible to make some changes to open a new folder always in the same window, but I guess it's not what others would expect.

Please let me know if my explanation is not clear.

We could create a new option that would indicate to always open a new folder in the same window, but I believe it would be better to start a new issue and a new PR for this.

@jsolisu

This comment has been minimized.

Show comment
Hide comment
@jsolisu

jsolisu Mar 11, 2017

@anatoli26 everything is clear. Thanks.

jsolisu commented Mar 11, 2017

@anatoli26 everything is clear. Thanks.

@lee-dohm lee-dohm changed the title from Make an option to *always* restore the last session, no matter how At… to Make an option to always restore the last session Mar 13, 2017

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Mar 18, 2017

Contributor

Should anything else be done to integrate the PR to the master?

Contributor

anatoli26 commented Mar 18, 2017

Should anything else be done to integrate the PR to the master?

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Mar 20, 2017

Contributor

We have a parallel PR here #13963 that should get finished this week. At that time we'll figure out how to handle this one.

/cc @BinaryMuse @kuychaco

Contributor

iolsen commented Mar 20, 2017

We have a parallel PR here #13963 that should get finished this week. At that time we'll figure out how to handle this one.

/cc @BinaryMuse @kuychaco

@BrunoBernardino

This comment has been minimized.

Show comment
Hide comment
@BrunoBernardino

BrunoBernardino Apr 6, 2017

Any updates on this? I just got hit hard by this one (I try to not use Atom for single files, but opened one by mistake and "lost" 15 projects on the tree view).

BrunoBernardino commented Apr 6, 2017

Any updates on this? I just got hit hard by this one (I try to not use Atom for single files, but opened one by mistake and "lost" 15 projects on the tree view).

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Apr 6, 2017

Contributor

@BrunoBernardino They're not lost - you can get them back just by using Reopen Project from the File menu - it will have all 15 already in there.

Contributor

damieng commented Apr 6, 2017

@BrunoBernardino They're not lost - you can get them back just by using Reopen Project from the File menu - it will have all 15 already in there.

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 Apr 28, 2017

Contributor

@iolsen, I see that the #13963 was merged, any progress on this PR?

Contributor

anatoli26 commented Apr 28, 2017

@iolsen, I see that the #13963 was merged, any progress on this PR?

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Apr 29, 2017

Contributor

@anatoli26, I'll spend some time today testing this and get back to you!

I kicked off the builds again since they seem to of failed.

Contributor

ungb commented Apr 29, 2017

@anatoli26, I'll spend some time today testing this and get back to you!

I kicked off the builds again since they seem to of failed.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb May 1, 2017

Contributor

Hey @anatoli26,

Sorry took so long to get back to you, I wanted to understand the issues and try to better understand this fix. I think that the main issue was a combination of a few issues that have been resolved since.
#13873 - Resolved by pr #13963
Reopen project Feature: #13046

From testing, always and yes behave the same when opening atom by itself(without opening a file or a folder with atom)

On Windows

Opening file from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom
  4. Open a file with atom from file explorer.

Result: Atom opens the previous open project and opens the file on a new tab in the same Atom window.

Opening folder from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom
  4. Open Powershell and open a folder with atom .

Result: Atom opens the previous open project(from step 1) and opens a new atom window with the folder you opened in step 4

On Mac

Opening file from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom via red x(notice atom application is still running)
  4. Open a file with atom from finder

Result: Atom opens a window with the project of the file open on step 4, the previous project doesn't open.
Note: The behavior is the same as windows when you completely exit out of Atom, but closing atom via the red x on mac doesn't completely exit out of Atom.

Opening folder from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom via red x(notice atom application is still running)
  4. Open terminal and open a folder with atom .

Result: Atom opens a window with the project from step 4, the previous project isn't open.
Note: The behavior is the same as windows when you completely exit out of Atom, but closing atom via the red x on mac doesn't completely exit out of Atom.

On Ubuntu, I saw the same behavior as Windows.

Overall, I think this is a improvement and gives people the flexibility to open single files on their current session. The only weird behavior I saw was on Mac when you close all the atom windows but the Atom application is still running. Now that there's a Reopen Project item in the file context menu, and #13873 is fixed. The user has a way to get back their old project.

/cc @iolsen @nathansobo for code review.

Contributor

ungb commented May 1, 2017

Hey @anatoli26,

Sorry took so long to get back to you, I wanted to understand the issues and try to better understand this fix. I think that the main issue was a combination of a few issues that have been resolved since.
#13873 - Resolved by pr #13963
Reopen project Feature: #13046

From testing, always and yes behave the same when opening atom by itself(without opening a file or a folder with atom)

On Windows

Opening file from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom
  4. Open a file with atom from file explorer.

Result: Atom opens the previous open project and opens the file on a new tab in the same Atom window.

Opening folder from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom
  4. Open Powershell and open a folder with atom .

Result: Atom opens the previous open project(from step 1) and opens a new atom window with the folder you opened in step 4

On Mac

Opening file from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom via red x(notice atom application is still running)
  4. Open a file with atom from finder

Result: Atom opens a window with the project of the file open on step 4, the previous project doesn't open.
Note: The behavior is the same as windows when you completely exit out of Atom, but closing atom via the red x on mac doesn't completely exit out of Atom.

Opening folder from file explorer

  1. Open Project on Atom and open a few files
  2. Go to settings and set Restore Previous Windows on Start to Always
  3. Close Atom via red x(notice atom application is still running)
  4. Open terminal and open a folder with atom .

Result: Atom opens a window with the project from step 4, the previous project isn't open.
Note: The behavior is the same as windows when you completely exit out of Atom, but closing atom via the red x on mac doesn't completely exit out of Atom.

On Ubuntu, I saw the same behavior as Windows.

Overall, I think this is a improvement and gives people the flexibility to open single files on their current session. The only weird behavior I saw was on Mac when you close all the atom windows but the Atom application is still running. Now that there's a Reopen Project item in the file context menu, and #13873 is fixed. The user has a way to get back their old project.

/cc @iolsen @nathansobo for code review.

@ungb ungb removed the needs-testing label May 1, 2017

@anatoli26

This comment has been minimized.

Show comment
Hide comment
@anatoli26

anatoli26 May 3, 2017

Contributor

Bryant, thanks for this thorough review.

The issues and PRs you mentioned, if I understand them correctly, are actually for the behavior when the session is NOT restored, i.e. when the current option is set to NO. Aren't they a compliment to this PR?

What this PR is trying to solve is to implement a behavior typical in most multi-tab programs, like browsers, editors, etc.: when you configure the previous session to be restored, it is restored no matter how the program is started, i.e. alone or opening additional files. E.g. if Firefox is instructed to restore previous session, when we start it (say after a reboot) by clicking on a link in a Thunderbird mail, Firefox opens the previous session and creates a new tab for the requested link. The current behavior of Atom is to ignore a previous session if a link is clicked (i.e. a new file is opened), but it restores the previous session if it's started without opening anything in addition.

With respect to Mac, I'm not familiar with Atom on Mac, so I can't say much. From you description it looks like the main process is still running, and the X just makes the windows disappear, so when new windows are created, the initialization logic is not executed. One could say that this particular case doesn't strictly apply to this option as it says "on start", but if Atom is still running, you're actually not starting it.

In any case, if the users will ask to extend this feature to that particular case too, we could investigate the state in which Atom is after closing all windows on Mac and find a solution to restore the previous session for this case too. Does it sound reasonable?

Contributor

anatoli26 commented May 3, 2017

Bryant, thanks for this thorough review.

The issues and PRs you mentioned, if I understand them correctly, are actually for the behavior when the session is NOT restored, i.e. when the current option is set to NO. Aren't they a compliment to this PR?

What this PR is trying to solve is to implement a behavior typical in most multi-tab programs, like browsers, editors, etc.: when you configure the previous session to be restored, it is restored no matter how the program is started, i.e. alone or opening additional files. E.g. if Firefox is instructed to restore previous session, when we start it (say after a reboot) by clicking on a link in a Thunderbird mail, Firefox opens the previous session and creates a new tab for the requested link. The current behavior of Atom is to ignore a previous session if a link is clicked (i.e. a new file is opened), but it restores the previous session if it's started without opening anything in addition.

With respect to Mac, I'm not familiar with Atom on Mac, so I can't say much. From you description it looks like the main process is still running, and the X just makes the windows disappear, so when new windows are created, the initialization logic is not executed. One could say that this particular case doesn't strictly apply to this option as it says "on start", but if Atom is still running, you're actually not starting it.

In any case, if the users will ask to extend this feature to that particular case too, we could investigate the state in which Atom is after closing all windows on Mac and find a solution to restore the previous session for this case too. Does it sound reasonable?

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb May 3, 2017

Contributor

yes, the issues that are fixed, make it possible to open old sessions, and not lose data while doing it. In that sense they are compliments to this PR, and yes the data loss issue occurs when you have a atom window without a project.

The Reopen project Feature: #13046, makes it possible to easily open recent projects, but I do agree this change is a improvement.

I think the current behavior on mac where Atom application is still open is the expected behavior in this case. I've compared this behavior with editors like sublime text behave the same way. If you close all your sessions and click on the dock to open Atom/Sublime, a empty window will open and not your previous session. So I think on mac, the moment you close all your windows, your current sessions is now empty so the expected behavior is to open the file project, or just the folder project if opened with atom .

I'll follow up on this PR to see if someone can take a 👀

Contributor

ungb commented May 3, 2017

yes, the issues that are fixed, make it possible to open old sessions, and not lose data while doing it. In that sense they are compliments to this PR, and yes the data loss issue occurs when you have a atom window without a project.

The Reopen project Feature: #13046, makes it possible to easily open recent projects, but I do agree this change is a improvement.

I think the current behavior on mac where Atom application is still open is the expected behavior in this case. I've compared this behavior with editors like sublime text behave the same way. If you close all your sessions and click on the dock to open Atom/Sublime, a empty window will open and not your previous session. So I think on mac, the moment you close all your windows, your current sessions is now empty so the expected behavior is to open the file project, or just the folder project if opened with atom .

I'll follow up on this PR to see if someone can take a 👀

@BrunoBernardino

This comment has been minimized.

Show comment
Hide comment
@BrunoBernardino

BrunoBernardino May 3, 2017

@ungb totally correct. The big difference between something like Sublime and Atom there is that after you've opened a single file and closed the app, when you open the app again, you have the projects right there. With Atom we currently have to do the "Reopen project and re-collapse all projects on the sidebar one-by-one" dance, which isn't very fun 😢 . While this isn't the most clean solution, I feel like it's an improvement, even if not the same as how other editors behave.

BrunoBernardino commented May 3, 2017

@ungb totally correct. The big difference between something like Sublime and Atom there is that after you've opened a single file and closed the app, when you open the app again, you have the projects right there. With Atom we currently have to do the "Reopen project and re-collapse all projects on the sidebar one-by-one" dance, which isn't very fun 😢 . While this isn't the most clean solution, I feel like it's an improvement, even if not the same as how other editors behave.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo May 4, 2017

Contributor

@ungb If you can confirm this functions as expected, the code looks good to me.

Contributor

nathansobo commented May 4, 2017

@ungb If you can confirm this functions as expected, the code looks good to me.

@ungb ungb merged commit f464bb3 into atom:master May 8, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment