Merge pull request atom#19207 from atom/aw/versioned-application-state

Only store directories in serialized application state

Author: smashwilson

spec/atom-environment-spec.js

@@ -336,57 +336,34 @@ describe('AtomEnvironment', () => {
     })
 
     describe('deserialization failures', () => {
-      it('propagates project state restoration failures', async () => {
+      it('propagates unrecognized project state restoration failures', async () => {
+        let err
         spyOn(atom.project, 'deserialize').andCallFake(() => {
-          const err = new Error('deserialization failure')
-          err.missingProjectPaths = ['/foo']
-          return Promise.reject(err)
-        })
-        spyOn(atom.notifications, 'addError')
-
-        await atom.deserialize({ project: 'should work' })
-        expect(atom.notifications.addError).toHaveBeenCalledWith(
-          'Unable to open project directory',
-          {
-            description: 'Project directory `/foo` is no longer on disk.'
-          }
-        )
-      })
-
-      it('accumulates and reports two errors with one notification', async () => {
-        spyOn(atom.project, 'deserialize').andCallFake(() => {
-          const err = new Error('deserialization failure')
-          err.missingProjectPaths = ['/foo', '/wat']
+          err = new Error('deserialization failure')
           return Promise.reject(err)
         })
         spyOn(atom.notifications, 'addError')
 
         await atom.deserialize({ project: 'should work' })
         expect(atom.notifications.addError).toHaveBeenCalledWith(
-          'Unable to open 2 project directories',
+          'Unable to deserialize project',
           {
-            description:
-              'Project directories `/foo` and `/wat` are no longer on disk.'
+            description: 'deserialization failure',
+            stack: err.stack
           }
         )
       })
 
-      it('accumulates and reports three+ errors with one notification', async () => {
+      it('disregards missing project folder errors', async () => {
         spyOn(atom.project, 'deserialize').andCallFake(() => {
           const err = new Error('deserialization failure')
-          err.missingProjectPaths = ['/foo', '/wat', '/stuff', '/things']
+          err.missingProjectPaths = ['nah']
           return Promise.reject(err)
         })
         spyOn(atom.notifications, 'addError')
 
         await atom.deserialize({ project: 'should work' })
-        expect(atom.notifications.addError).toHaveBeenCalledWith(
-          'Unable to open 4 project directories',
-          {
-            description:
-              'Project directories `/foo`, `/wat`, `/stuff`, and `/things` are no longer on disk.'
-          }
-        )
+        expect(atom.notifications.addError).not.toHaveBeenCalled()
       })
     })
   })

spec/main-process/atom-application.test.js

@@ -100,10 +100,13 @@ describe('AtomApplication', function () {
 
         beforeEach(function () {
           app = scenario.addApplication({
-            applicationJson: [
-              { initialPaths: ['b'] },
-              { initialPaths: ['c'] }
-            ]
+            applicationJson: {
+              version: '1',
+              windows: [
+                { projectRoots: [scenario.convertRootPath('b')] },
+                { projectRoots: [scenario.convertRootPath('c')] }
+              ]
+            }
           })
         })
 
@@ -161,12 +164,12 @@ describe('AtomApplication', function () {
 
           it('restores windows when launched with a project path to open', async function () {
             await scenario.launch({ app, pathsToOpen: ['a'] })
-            await scenario.assert('[a _] [b _] [c _]')
+            await scenario.assert('[b _] [c _] [a _]')
           })
 
           it('restores windows when launched with a file path to open', async function () {
             await scenario.launch({ app, pathsToOpen: ['a/1.md'] })
-            await scenario.assert('[b 1.md] [c _]')
+            await scenario.assert('[b _] [c 1.md]')
           })
 
           it('collapses new paths into restored windows when appropriate', async function () {
@@ -186,6 +189,33 @@ describe('AtomApplication', function () {
           })
         })
       })
+
+      describe('with unversioned application state', function () {
+        it('reads "initialPaths" as project roots', async function () {
+          const app = scenario.addApplication({
+            applicationJson: [
+              {initialPaths: [scenario.convertRootPath('a')]},
+              {initialPaths: [scenario.convertRootPath('b'), scenario.convertRootPath('c')]}
+            ]
+          })
+          app.config.set('core.restorePreviousWindowsOnStart', 'always')
+
+          await scenario.launch({app})
+          await scenario.assert('[a _] [b,c _]')
+        })
+
+        it('filters file paths from project root lists', async function () {
+          const app = scenario.addApplication({
+            applicationJson: [
+              {initialPaths: [scenario.convertRootPath('b'), scenario.convertEditorPath('a/1.md')]}
+            ]
+          })
+          app.config.set('core.restorePreviousWindowsOnStart', 'always')
+
+          await scenario.launch({app})
+          await scenario.assert('[b _]')
+        })
+      })
     })
 
     describe('with one empty window', function () {
@@ -855,10 +885,13 @@ describe('AtomApplication', function () {
 
       assert.isTrue(scenario.getApplication(0).storageFolder.store.calledWith(
         'application.json',
-        [
-          { initialPaths: [scenario.convertRootPath('a')] },
-          { initialPaths: [scenario.convertRootPath('b'), scenario.convertRootPath('c')] }
-        ]
+        {
+          version: '1',
+          windows: [
+            { projectRoots: [scenario.convertRootPath('a')] },
+            { projectRoots: [scenario.convertRootPath('b'), scenario.convertRootPath('c')] }
+          ]
+        }
       ))
     })
 
@@ -872,9 +905,12 @@ describe('AtomApplication', function () {
 
       assert.isTrue(scenario.getApplication(0).storageFolder.store.calledWith(
         'application.json',
-        [
-          { initialPaths: [scenario.convertRootPath('a')] }
-        ]
+        {
+          version: '1',
+          windows: [
+            { projectRoots: [scenario.convertRootPath('a')] }
+          ]
+        }
       ))
     })
 
@@ -1338,9 +1374,7 @@ class LaunchScenario {
       return newWindow
     })
     this.sinon.stub(app.storageFolder, 'load', () => Promise.resolve(
-      (options.applicationJson || []).map(each => ({
-        initialPaths: this.convertPaths(each.initialPaths)
-      }))
+      options.applicationJson || {version: '1', windows: []}
     ))
     this.sinon.stub(app.storageFolder, 'store', () => Promise.resolve())
     this.applications.add(app)

spec/main-process/atom-window.test.js

@@ -190,6 +190,7 @@ describe('AtomWindow', function () {
       ]
 
       const w = new AtomWindow(app, service, { browserWindowConstructor: StubBrowserWindow, locationsToOpen })
+      assert.deepEqual(w.projectRoots, ['/directory'])
 
       const loadPromise = emitterEventPromise(w, 'window:loaded')
       w.browserWindow.emit('window:loaded')
@@ -258,6 +259,24 @@ describe('AtomWindow', function () {
       assert.isTrue(w.hasProjectPaths())
     })
 
+    it('is updated synchronously by openLocations', async function () {
+      const locationsToOpen = [
+        { pathToOpen: 'file.txt', isFile: true },
+        { pathToOpen: 'directory1', isDirectory: true },
+        { pathToOpen: 'directory0', isDirectory: true },
+        { pathToOpen: 'directory0', isDirectory: true },
+        { pathToOpen: 'new-file.txt' }
+      ]
+
+      const w = new AtomWindow(app, service, { browserWindowConstructor: StubBrowserWindow })
+      assert.deepEqual(w.projectRoots, [])
+
+      const promise = w.openLocations(locationsToOpen)
+      assert.deepEqual(w.projectRoots, ['directory0', 'directory1'])
+      w.resolveLoadedPromise()
+      await promise
+    })
+
     it('is updated by setProjectRoots', function () {
       const locationsToOpen = [
         { pathToOpen: 'directory0', exists: true, isDirectory: true }

src/atom-environment.js

@@ -1247,9 +1247,8 @@ or use Pane::saveItemAs for programmatic saving.`)
       try {
         await this.project.deserialize(state.project, this.deserializers)
       } catch (error) {
-        if (error.missingProjectPaths) {
-          missingProjectPaths.push(...error.missingProjectPaths)
-        } else {
+        // We handle the missingProjectPaths case in openLocations().
+        if (!error.missingProjectPaths) {
           this.notifications.addError('Unable to deserialize project', {
             description: error.message,
             stack: error.stack
@@ -1268,7 +1267,7 @@ or use Pane::saveItemAs for programmatic saving.`)
 
     if (missingProjectPaths.length > 0) {
       const count = missingProjectPaths.length === 1 ? '' : missingProjectPaths.length + ' '
-      const noun = missingProjectPaths.length === 1 ? 'directory' : 'directories'
+      const noun = missingProjectPaths.length === 1 ? 'folder' : 'folders'
       const toBe = missingProjectPaths.length === 1 ? 'is' : 'are'
       const escaped = missingProjectPaths.map(projectPath => `\`${projectPath}\``)
       let group

src/main-process/atom-application.js

@@ -23,6 +23,11 @@ const ConfigSchema = require('../config-schema')
 
 const LocationSuffixRegExp = /(:\d+)(:\d+)?$/
 
+// Increment this when changing the serialization format of `${ATOM_HOME}/storage/application.json` used by
+// AtomApplication::saveCurrentWindowOptions() and AtomApplication::loadPreviousWindowOptions() in a backward-
+// incompatible way.
+const APPLICATION_STATE_VERSION = '1'
+
 const getDefaultPath = () => {
   const editor = atom.workspace.getActiveTextEditor()
   if (!editor || !editor.getPath()) {
@@ -247,8 +252,7 @@ class AtomApplication extends EventEmitter {
       this.config.onDidChange('core.colorProfile', () => this.promptForRestart())
     }
 
-    const optionsForWindowsToOpen = []
-
+    let optionsForWindowsToOpen = []
     let shouldReopenPreviousWindows = false
 
     if (options.test || options.benchmark || options.benchmarkTest) {
@@ -264,9 +268,7 @@ class AtomApplication extends EventEmitter {
     }
 
     if (shouldReopenPreviousWindows) {
-      for (const previousOptions of await this.loadPreviousWindowOptions()) {
-        optionsForWindowsToOpen.push(previousOptions)
-      }
+      optionsForWindowsToOpen = [...await this.loadPreviousWindowOptions(), ...optionsForWindowsToOpen]
     }
 
     if (optionsForWindowsToOpen.length === 0) {
@@ -1139,27 +1141,58 @@ class AtomApplication extends EventEmitter {
   async saveCurrentWindowOptions (allowEmpty = false) {
     if (this.quitting) return
 
-    const states = []
-    for (let window of this.getAllWindows()) {
-      if (!window.isSpec) states.push({initialPaths: window.projectRoots})
+    const state = {
+      version: APPLICATION_STATE_VERSION,
+      windows: this.getAllWindows()
+        .filter(window => !window.isSpec)
+        .map(window => ({projectRoots: window.projectRoots}))
     }
-    states.reverse()
+    state.windows.reverse()
 
-    if (states.length > 0 || allowEmpty) {
-      await this.storageFolder.store('application.json', states)
+    if (state.windows.length > 0 || allowEmpty) {
+      await this.storageFolder.store('application.json', state)
       this.emit('application:did-save-state')
     }
   }
 
   async loadPreviousWindowOptions () {
-    const states = await this.storageFolder.load('application.json')
-    if (states) {
-      return states.map(state => ({
-        foldersToOpen: state.initialPaths,
+    const state = await this.storageFolder.load('application.json')
+    if (!state) {
+      return []
+    }
+
+    if (state.version === APPLICATION_STATE_VERSION) {
+      // Atom >=1.36.1
+      // Schema: {version: '1', windows: [{projectRoots: ['<root-dir>', ...]}, ...]}
+      return state.windows.map(each => ({
+        foldersToOpen: each.projectRoots,
         devMode: this.devMode,
-        safeMode: this.safeMode
+        safeMode: this.safeMode,
+        newWindow: true
       }))
+    } else if (state.version === undefined) {
+      // Atom <= 1.36.0
+      // Schema: [{initialPaths: ['<root-dir>', ...]}, ...]
+      return await Promise.all(
+        state.map(async windowState => {
+          // Classify each window's initialPaths as directories or non-directories
+          const classifiedPaths = await Promise.all(
+            windowState.initialPaths.map(initialPath => new Promise(resolve => {
+              fs.isDirectory(initialPath, isDir => resolve({initialPath, isDir}))
+            }))
+          )
+
+          // Only accept initialPaths that are existing directories
+          return {
+            foldersToOpen: classifiedPaths.filter(({isDir}) => isDir).map(({initialPath}) => initialPath),
+            devMode: this.devMode,
+            safeMode: this.safeMode,
+            newWindow: true
+          }
+        })
+      )
     } else {
+      // Unrecognized version (from a newer Atom?)
       return []
     }
   }

src/main-process/atom-window.js

@@ -72,10 +72,7 @@ class AtomWindow extends EventEmitter {
     if (this.loadSettings.safeMode == null) this.loadSettings.safeMode = false
     if (this.loadSettings.clearWindowState == null) this.loadSettings.clearWindowState = false
 
-    this.projectRoots = locationsToOpen
-      .filter(location => location.pathToOpen && location.exists && location.isDirectory)
-      .map(location => location.pathToOpen)
-    this.projectRoots.sort()
+    this.addLocationsToOpen(locationsToOpen)
 
     this.loadSettings.hasOpenFiles = locationsToOpen
       .some(location => location.pathToOpen && !location.isDirectory)
@@ -240,6 +237,7 @@ class AtomWindow extends EventEmitter {
   }
 
   async openLocations (locationsToOpen) {
+    this.addLocationsToOpen(locationsToOpen)
     await this.loadedPromise
     this.sendMessage('open-locations', locationsToOpen)
   }
@@ -252,6 +250,18 @@ class AtomWindow extends EventEmitter {
     this.sendMessage('did-fail-to-read-user-settings', message)
   }
 
+  addLocationsToOpen (locationsToOpen) {
+    const roots = new Set(this.projectRoots || [])
+    for (const {pathToOpen, isDirectory} of locationsToOpen) {
+      if (isDirectory) {
+        roots.add(pathToOpen)
+      }
+    }
+
+    this.projectRoots = Array.from(roots)
+    this.projectRoots.sort()
+  }
+
   replaceEnvironment (env) {
     this.browserWindow.webContents.send('environment', env)
   }