From 9e80a72dd5bb42e3ed28845d7e27850bc9b83e6f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 24 Apr 2019 17:22:10 -0400 Subject: [PATCH] Merge pull request #19207 from atom/aw/versioned-application-state Only store directories in serialized application state --- spec/atom-environment-spec.js | 41 +++----------- spec/main-process/atom-application.test.js | 66 ++++++++++++++++------ spec/main-process/atom-window.test.js | 19 +++++++ src/atom-environment.js | 7 +-- src/main-process/atom-application.js | 65 +++++++++++++++------ src/main-process/atom-window.js | 18 ++++-- 6 files changed, 144 insertions(+), 72 deletions(-) diff --git a/spec/atom-environment-spec.js b/spec/atom-environment-spec.js index ea30eab9990..661af1e967d 100644 --- a/spec/atom-environment-spec.js +++ b/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() }) }) }) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index eb7483ce341..86ca3752250 100644 --- a/spec/main-process/atom-application.test.js +++ b/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) diff --git a/spec/main-process/atom-window.test.js b/spec/main-process/atom-window.test.js index c65b91546b1..72f79f9d4f3 100644 --- a/spec/main-process/atom-window.test.js +++ b/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 } diff --git a/src/atom-environment.js b/src/atom-environment.js index ee27c7ebbba..5add94a28d8 100644 --- a/src/atom-environment.js +++ b/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 diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 61c44f6fd00..4224dba69e1 100644 --- a/src/main-process/atom-application.js +++ b/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: ['', ...]}, ...]} + 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: ['', ...]}, ...] + 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 [] } } diff --git a/src/main-process/atom-window.js b/src/main-process/atom-window.js index a28f5559ab8..367fb77e4e3 100644 --- a/src/main-process/atom-window.js +++ b/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) }