Save initial paths immediately on change #13564

Merged
merged 11 commits into from Jan 9, 2017

Projects

None yet

4 participants

@vovkasm
Contributor
vovkasm commented Jan 7, 2017

This is #12545 (send event to save state after 'Add Project Folder' or 'Remove Projec') + tests. Does it appropriate for merge now?

Copy of the description from #12545:

Fixes #12495

When we add or remove project folder, the window's current project path will be changed, then "initialPaths" will be reloaded, it is the best time to save state

/cc @maxbrunsfeld @lee-dohm

yuanwhy and others added some commits Aug 29, 2016
@yuanwhy yuanwhy send event to save state after 'Add Project Folder' or 'Remove Projec…
…t Folder
4e0da56
@yuanwhy yuanwhy rename to did-change-paths
1c1640c
@yuanwhy yuanwhy delete empty line
1320ff2
@vovkasm vovkasm Merge commit '1320ff28c39afd563820b30b43389ea38552e74c' into fix-12495 a21841e
@vovkasm vovkasm Add test to ensure the state saved when project folders changed.
bc77a79
@vovkasm
Contributor
vovkasm commented Jan 7, 2017

Okey, windows fails... most probably because I try to read ATOM_HOME/storage/application.json too fast after creation.

There is some variants (and I need help from atom devs to select most appropriate one):

  • We can simple test that AtomApplication#saveState method called (aka test spy).
  • We can add short sleep to ensure application.json saved (something like in this test )
  • We can test this functional in unit like - split to two tests:
    • did-change-paths event generated on right places
    • did-change-paths event handled, i.e. call saveState
@vovkasm vovkasm Try fix tests on Windows.
Only count AtomApplication#saveState calls, not saved content.
4ba6919
@vovkasm
Contributor
vovkasm commented Jan 7, 2017

Bad, seems like on Windows AtomApplication#saveState does not called at all, will investigate...

@vovkasm vovkasm Another try to fix tests on Windows.
Wait for ipc.
94f28a3
@vovkasm
Contributor
vovkasm commented Jan 7, 2017

Hmm... I will try to setup dev environment on windows to fix that, but it will need some time...
If anyone with windows can fix it - welcome!

@lee-dohm lee-dohm changed the title from Fix 12495 to Save initial paths immediately on change Jan 7, 2017
vovkasm added some commits Jan 7, 2017
@vovkasm vovkasm Really fix tests on windows
Path names with backslashes was not quoted.
1fb066a
@vovkasm vovkasm Restore test behaviour to check content of storage/application.json 4d5312f
@50Wliu 50Wliu added needs-review and removed requires-changes labels Jan 7, 2017
vovkasm added some commits Jan 7, 2017
@vovkasm vovkasm Additional check that storage/application.json exists be57e4c
@vovkasm vovkasm Refactor test code
a7dda0e
@vovkasm
Contributor
vovkasm commented Jan 7, 2017

Done. Now it pass tests.
After fixing test code for windows, I reverted tests to initial behavior to check actual ATOM_HOME/storage/application.json content.
I can if needed squash my commits into one and rebase this branch.

@maxbrunsfeld maxbrunsfeld self-assigned this Jan 8, 2017
@maxbrunsfeld
Contributor

@vovkasm No need to squash or rebase these commits; they're fine as they are. Really nice job figuring out how to test this! I'll take a look on Monday.

@maxbrunsfeld
Contributor

👏 Great work @yuanwhy, @vovkasm.

@maxbrunsfeld maxbrunsfeld merged commit 6dd35ec into atom:master Jan 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxbrunsfeld maxbrunsfeld added a commit that referenced this pull request Jan 9, 2017
@maxbrunsfeld maxbrunsfeld Avoid assertions about application.json in main process test
We can test the user-facing behavior by launching a second instance
of AtomApplication.

Refs #13564
d6bddb4
@maxbrunsfeld
Contributor

This will go out in Atom 1.14 Beta.

@vovkasm vovkasm deleted the vovkasm:fix-12495 branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment