From 7f76320387e0e4e39a6b856237bc8bf572fa0831 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2018 14:28:27 -0800 Subject: [PATCH 1/4] Backfill a test for existing --wait functionality --- spec/main-process/atom-application.test.js | 36 +++++++++++++++++++--- src/main-process/atom-application.js | 3 +- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index b73a57fab..8d991f52c 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -389,6 +389,34 @@ describe('AtomApplication', function () { assert.deepEqual(app2Window.representedDirectoryPaths, []) }) + describe('when the `pidToKillWhenClosed` flag is passed', () => { + let killedPids, atomApplication + + beforeEach(() => { + killedPids = [] + atomApplication = buildAtomApplication({ + killProcess (pid) { killedPids.push(pid) } + }) + }) + + it('kills the specified pid after a newly-opened window is closed', async () => { + const window1 = atomApplication.launch(parseCommandLine([makeTempDir(), '--wait', '--pid', '101'])) + await focusWindow(window1) + + const [window2] = atomApplication.launch(parseCommandLine(['--wait', '--pid', '102'])) + await focusWindow(window2) + assert.deepEqual(killedPids, []) + + window1.close() + await window1.closedPromise + assert.deepEqual(killedPids, [101]) + + window2.close() + await window2.closedPromise + assert.deepEqual(killedPids, [101, 102]) + }) + }) + describe('when closing the last window', () => { if (process.platform === 'linux' || process.platform === 'win32') { it('quits the application', async () => { @@ -529,11 +557,11 @@ describe('AtomApplication', function () { assert(electron.app.didQuit()) }) - function buildAtomApplication () { - const atomApplication = new AtomApplication({ + function buildAtomApplication (params = {}) { + const atomApplication = new AtomApplication(Object.assign({ resourcePath: ATOM_RESOURCE_PATH, - atomHomeDirPath: process.env.ATOM_HOME - }) + atomHomeDirPath: process.env.ATOM_HOME, + }, params)) atomApplicationsToDestroy.push(atomApplication) return atomApplication } diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 459520722..df5c5e202 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -101,6 +101,7 @@ class AtomApplication extends EventEmitter { this.socketPath = options.socketPath this.logFile = options.logFile this.userDataDir = options.userDataDir + this._killProcess = options.killProcess || process.kill.bind(process) if (options.test || options.benchmark || options.benchmarkTest) this.socketPath = null this.pidsToOpenWindows = {} @@ -880,7 +881,7 @@ class AtomApplication extends EventEmitter { killProcess (pid) { try { const parsedPid = parseInt(pid) - if (isFinite(parsedPid)) process.kill(parsedPid) + if (isFinite(parsedPid)) this._killProcess(parsedPid) } catch (error) { if (error.code !== 'ESRCH') { console.log(`Killing process ${pid} failed: ${error.code != null ? error.code : error.message}`) From 1f4ccf302426c7699122d5e817108456bef3efc4 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2018 16:42:02 -0800 Subject: [PATCH 2/4] Allow existing windows to be reused when running --wait --- spec/main-process/atom-application.test.js | 65 +++++++++++++++++++--- src/application-delegate.js | 4 ++ src/atom-environment.js | 30 ++++++---- src/main-process/atom-application.js | 58 ++++++++++++------- src/main-process/atom-window.js | 4 ++ 5 files changed, 122 insertions(+), 39 deletions(-) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index 8d991f52c..e9775d225 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -1,3 +1,4 @@ +const temp = require('temp').track() const season = require('season') const dedent = require('dedent') const electron = require('electron') @@ -389,32 +390,81 @@ describe('AtomApplication', function () { assert.deepEqual(app2Window.representedDirectoryPaths, []) }) - describe('when the `pidToKillWhenClosed` flag is passed', () => { - let killedPids, atomApplication + describe('when the `--wait` flag is passed', () => { + let killedPids, atomApplication, onDidKillProcess beforeEach(() => { killedPids = [] + onDidKillProcess = null atomApplication = buildAtomApplication({ - killProcess (pid) { killedPids.push(pid) } + killProcess (pid) { + killedPids.push(pid) + if (onDidKillProcess) onDidKillProcess() + } }) }) it('kills the specified pid after a newly-opened window is closed', async () => { - const window1 = atomApplication.launch(parseCommandLine([makeTempDir(), '--wait', '--pid', '101'])) + const window1 = atomApplication.launch(parseCommandLine(['--wait', '--pid', '101'])) await focusWindow(window1) - const [window2] = atomApplication.launch(parseCommandLine(['--wait', '--pid', '102'])) + const [window2] = atomApplication.launch(parseCommandLine(['--new-window', '--wait', '--pid', '102'])) await focusWindow(window2) assert.deepEqual(killedPids, []) + let processKillPromise = new Promise(resolve => { onDidKillProcess = resolve }) window1.close() - await window1.closedPromise + await processKillPromise assert.deepEqual(killedPids, [101]) + processKillPromise = new Promise(resolve => { onDidKillProcess = resolve }) window2.close() - await window2.closedPromise + await processKillPromise assert.deepEqual(killedPids, [101, 102]) }) + + it('kills the specified pid after a newly-opened file in an existing window is closed', async () => { + const window = atomApplication.launch(parseCommandLine(['--wait', '--pid', '101'])) + await focusWindow(window) + + const filePath1 = temp.openSync('test').path + const filePath2 = temp.openSync('test').path + fs.writeFileSync(filePath1, 'File 1') + fs.writeFileSync(filePath2, 'File 2') + + const reusedWindow = atomApplication.launch(parseCommandLine(['--wait', '--pid', '102', filePath1, filePath2])) + assert.equal(reusedWindow, window) + + const activeEditorPath = await evalInWebContents(window.browserWindow.webContents, send => { + const subscription = atom.workspace.onDidChangeActivePaneItem(editor => { + send(editor.getPath()) + subscription.dispose() + }) + }) + + assert([filePath1, filePath2].includes(activeEditorPath)) + assert.deepEqual(killedPids, []) + + await evalInWebContents(window.browserWindow.webContents, send => { + atom.workspace.getActivePaneItem().destroy() + send() + }) + await timeoutPromise(100) + assert.deepEqual(killedPids, []) + + let processKillPromise = new Promise(resolve => { onDidKillProcess = resolve }) + await evalInWebContents(window.browserWindow.webContents, send => { + atom.workspace.getActivePaneItem().destroy() + send() + }) + await processKillPromise + assert.deepEqual(killedPids, [102]) + + processKillPromise = new Promise(resolve => { onDidKillProcess = resolve }) + window.close() + await processKillPromise + assert.deepEqual(killedPids, [102, 101]) + }) }) describe('when closing the last window', () => { @@ -594,7 +644,6 @@ describe('AtomApplication', function () { } function makeTempDir (name) { - const temp = require('temp').track() return fs.realpathSync(temp.mkdirSync(name)) } diff --git a/src/application-delegate.js b/src/application-delegate.js index 87e531b85..6d6d892ca 100644 --- a/src/application-delegate.js +++ b/src/application-delegate.js @@ -139,6 +139,10 @@ class ApplicationDelegate { return ipcRenderer.send('execute-javascript-in-dev-tools', code) } + didCloseInitialPath (path) { + return ipcHelpers.call('window-method', 'didCloseInitialPath', path) + } + setWindowDocumentEdited (edited) { return ipcHelpers.call('window-method', 'setDocumentEdited', edited) } diff --git a/src/atom-environment.js b/src/atom-environment.js index fc3201dfc..b629e96d2 100644 --- a/src/atom-environment.js +++ b/src/atom-environment.js @@ -70,6 +70,7 @@ class AtomEnvironment { this.loadTime = null this.emitter = new Emitter() this.disposables = new CompositeDisposable() + this.pathsToNotifyWhenClosed = new Set() // Public: A {DeserializerManager} instance this.deserializers = new DeserializerManager(this) @@ -359,6 +360,7 @@ class AtomEnvironment { this.grammars.clear() this.textEditors.clear() this.views.clear() + this.pathsToNotifyWhenClosed.clear() } destroy () { @@ -822,7 +824,15 @@ class AtomEnvironment { this.document.body.appendChild(this.workspace.getElement()) if (this.backgroundStylesheet) this.backgroundStylesheet.remove() - this.watchProjectPaths() + this.disposables.add(this.project.onDidChangePaths(() => { + this.applicationDelegate.setRepresentedDirectoryPaths(this.project.getPaths()) + })) + this.disposables.add(this.workspace.onDidDestroyPaneItem(({item}) => { + const path = item.getPath && item.getPath() + if (this.pathsToNotifyWhenClosed.has(path)) { + this.applicationDelegate.didCloseInitialPath(path) + } + })) this.packages.activate() this.keymaps.loadUserKeymap() @@ -1025,13 +1035,6 @@ class AtomEnvironment { return this.themes.load() } - // Notify the browser project of the window's current project path - watchProjectPaths () { - this.disposables.add(this.project.onDidChangePaths(() => { - this.applicationDelegate.setRepresentedDirectoryPaths(this.project.getPaths()) - })) - } - setDocumentEdited (edited) { if (typeof this.applicationDelegate.setWindowDocumentEdited === 'function') { this.applicationDelegate.setWindowDocumentEdited(edited) @@ -1300,8 +1303,9 @@ class AtomEnvironment { } } - for (var {pathToOpen, initialLine, initialColumn, forceAddToWindow} of locations) { - if (pathToOpen && (needsProjectPaths || forceAddToWindow)) { + for (const location of locations) { + const {pathToOpen} = location + if (pathToOpen && (needsProjectPaths || location.forceAddToWindow)) { if (fs.existsSync(pathToOpen)) { pushFolderToOpen(this.project.getDirectoryForProjectPath(pathToOpen).getPath()) } else if (fs.existsSync(path.dirname(pathToOpen))) { @@ -1312,8 +1316,10 @@ class AtomEnvironment { } if (!fs.isDirectorySync(pathToOpen)) { - fileLocationsToOpen.push({pathToOpen, initialLine, initialColumn}) + fileLocationsToOpen.push(location) } + + if (location.notifyWhenClosed) this.pathsToNotifyWhenClosed.add(pathToOpen) } let restoredState = false @@ -1334,7 +1340,7 @@ class AtomEnvironment { if (!restoredState) { const fileOpenPromises = [] - for ({pathToOpen, initialLine, initialColumn} of fileLocationsToOpen) { + for (const {pathToOpen, initialLine, initialColumn} of fileLocationsToOpen) { fileOpenPromises.push(this.workspace && this.workspace.open(pathToOpen, {initialLine, initialColumn})) } await Promise.all(fileOpenPromises) diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index df5c5e202..46a5f8afa 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -104,7 +104,7 @@ class AtomApplication extends EventEmitter { this._killProcess = options.killProcess || process.kill.bind(process) if (options.test || options.benchmark || options.benchmarkTest) this.socketPath = null - this.pidsToOpenWindows = {} + this.waitSessionsByWindow = new Map() this.windowStack = new WindowStack() this.config = new Config({enablePersistence: true}) @@ -789,13 +789,17 @@ class AtomApplication extends EventEmitter { safeMode = Boolean(safeMode) clearWindowState = Boolean(clearWindowState) - const locationsToOpen = pathsToOpen.map(pathToOpen => - this.locationForPathToOpen(pathToOpen, executedFrom, addToLastWindow) - ) - pathsToOpen = locationsToOpen.map(locationToOpen => locationToOpen.pathToOpen) + const locationsToOpen = [] + for (let i = 0; i < pathsToOpen.length; i++) { + const location = this.parsePathToOpen(pathsToOpen[i], executedFrom, addToLastWindow) + location.forceAddToWindow = addToLastWindow + location.notifyWhenClosed = pidToKillWhenClosed != null + locationsToOpen.push(location) + pathsToOpen[i] = location.pathToOpen + } let existingWindow - if (!pidToKillWhenClosed && !newWindow) { + if (!newWindow) { existingWindow = this.windowForPaths(pathsToOpen, devMode) const stats = pathsToOpen.map(pathToOpen => fs.statSyncNoException(pathToOpen)) if (!existingWindow) { @@ -853,26 +857,43 @@ class AtomApplication extends EventEmitter { } if (pidToKillWhenClosed != null) { - this.pidsToOpenWindows[pidToKillWhenClosed] = openedWindow + if (!this.waitSessionsByWindow.has(openedWindow)) { + this.waitSessionsByWindow.set(openedWindow, []) + } + this.waitSessionsByWindow.get(openedWindow).push({ + pid: pidToKillWhenClosed, + remainingPaths: new Set(pathsToOpen) + }) } - openedWindow.browserWindow.once('closed', () => this.killProcessForWindow(openedWindow)) + openedWindow.browserWindow.once('closed', () => this.killProcessesForWindow(openedWindow)) return openedWindow } // Kill all processes associated with opened windows. killAllProcesses () { - for (let pid in this.pidsToOpenWindows) { - this.killProcess(pid) + for (let window of this.waitSessionsByWindow.keys()) { + this.killProcessesForWindow(window) } } - // Kill process associated with the given opened window. - killProcessForWindow (openedWindow) { - for (let pid in this.pidsToOpenWindows) { - const trackedWindow = this.pidsToOpenWindows[pid] - if (trackedWindow === openedWindow) { - this.killProcess(pid) + killProcessesForWindow (window) { + const sessions = this.waitSessionsByWindow.get(window) + if (!sessions) return + for (const session of sessions) { + this.killProcess(session.pid) + } + this.waitSessionsByWindow.delete(window) + } + + windowDidCloseInitialPath (window, initialPath) { + const waitSessions = this.waitSessionsByWindow.get(window) + for (let i = waitSessions.length - 1; i >= 0; i--) { + const session = waitSessions[i] + session.remainingPaths.delete(initialPath) + if (session.remainingPaths.size === 0) { + this.killProcess(session.pid) + waitSessions.splice(i, 1) } } } @@ -887,7 +908,6 @@ class AtomApplication extends EventEmitter { console.log(`Killing process ${pid} failed: ${error.code != null ? error.code : error.message}`) } } - delete this.pidsToOpenWindows[pid] } saveState (allowEmpty = false) { @@ -1193,7 +1213,7 @@ class AtomApplication extends EventEmitter { } } - locationForPathToOpen (pathToOpen, executedFrom = '', forceAddToWindow) { + parsePathToOpen (pathToOpen, executedFrom = '') { let initialColumn, initialLine if (!pathToOpen) { return {pathToOpen} @@ -1218,7 +1238,7 @@ class AtomApplication extends EventEmitter { pathToOpen = path.resolve(executedFrom, fs.normalize(pathToOpen)) } - return {pathToOpen, initialLine, initialColumn, forceAddToWindow} + return {pathToOpen, initialLine, initialColumn} } // Opens a native dialog to prompt the user for a path. diff --git a/src/main-process/atom-window.js b/src/main-process/atom-window.js index 582852ad4..77dd09b31 100644 --- a/src/main-process/atom-window.js +++ b/src/main-process/atom-window.js @@ -411,6 +411,10 @@ class AtomWindow extends EventEmitter { return this.atomApplication.saveState() } + didCloseInitialPath (path) { + this.atomApplication.windowDidCloseInitialPath(this, path) + } + copy () { return this.browserWindow.copy() } From 386b786d93b68d9b72e7a483a39d46ede7bcab0d Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2018 17:43:51 -0800 Subject: [PATCH 3/4] Let 'atom --wait -a folder' exit due to removing the project folder --- spec/main-process/atom-application.test.js | 29 +++++++++++++++++++++- src/atom-environment.js | 11 ++++++-- src/main-process/atom-application.js | 1 + 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index e9775d225..7818314db 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -465,6 +465,33 @@ describe('AtomApplication', function () { await processKillPromise assert.deepEqual(killedPids, [102, 101]) }) + + it('kills the specified pid after a newly-opened directory in an existing window is closed', async () => { + const window = atomApplication.launch(parseCommandLine([])) + await focusWindow(window) + + const dirPath1 = makeTempDir() + const reusedWindow = atomApplication.launch(parseCommandLine(['--wait', '--pid', '101', dirPath1])) + assert.equal(reusedWindow, window) + assert.deepEqual(await getTreeViewRootDirectories(window), [dirPath1]) + assert.deepEqual(killedPids, []) + + const dirPath2 = makeTempDir() + await evalInWebContents(window.browserWindow.webContents, (send, dirPath1, dirPath2) => { + atom.project.setPaths([dirPath1, dirPath2]) + send() + }, dirPath1, dirPath2) + await timeoutPromise(100) + assert.deepEqual(killedPids, []) + + let processKillPromise = new Promise(resolve => { onDidKillProcess = resolve }) + await evalInWebContents(window.browserWindow.webContents, (send, dirPath2) => { + atom.project.setPaths([dirPath2]) + send() + }, dirPath2) + await processKillPromise + assert.deepEqual(killedPids, [101]) + }) }) describe('when closing the last window', () => { @@ -662,7 +689,7 @@ describe('AtomApplication', function () { function sendBackToMainProcess (result) { require('electron').ipcRenderer.send('${channelId}', result) } - (${source})(sendBackToMainProcess) + (${source})(sendBackToMainProcess, ${args.map(JSON.stringify).join(', ')}) `) }) } diff --git a/src/atom-environment.js b/src/atom-environment.js index b629e96d2..6e42f88a0 100644 --- a/src/atom-environment.js +++ b/src/atom-environment.js @@ -824,8 +824,15 @@ class AtomEnvironment { this.document.body.appendChild(this.workspace.getElement()) if (this.backgroundStylesheet) this.backgroundStylesheet.remove() - this.disposables.add(this.project.onDidChangePaths(() => { - this.applicationDelegate.setRepresentedDirectoryPaths(this.project.getPaths()) + let previousProjectPaths = this.project.getPaths() + this.disposables.add(this.project.onDidChangePaths(newPaths => { + for (let path of previousProjectPaths) { + if (this.pathsToNotifyWhenClosed.has(path) && !newPaths.includes(path)) { + this.applicationDelegate.didCloseInitialPath(path) + } + } + previousProjectPaths = newPaths + this.applicationDelegate.setRepresentedDirectoryPaths(newPaths) })) this.disposables.add(this.workspace.onDidDestroyPaneItem(({item}) => { const path = item.getPath && item.getPath() diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 46a5f8afa..52bc1287b 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -888,6 +888,7 @@ class AtomApplication extends EventEmitter { windowDidCloseInitialPath (window, initialPath) { const waitSessions = this.waitSessionsByWindow.get(window) + if (!waitSessions) return for (let i = waitSessions.length - 1; i >= 0; i--) { const session = waitSessions[i] session.remainingPaths.delete(initialPath) From 6f50f32116c21eca0dc47abc6f3ea464127b422f Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2018 17:49:44 -0800 Subject: [PATCH 4/4] Rename pathsToNotifyWhenClosed -> pathsWithWaitSessions --- src/application-delegate.js | 4 ++-- src/atom-environment.js | 14 +++++++------- src/main-process/atom-application.js | 4 ++-- src/main-process/atom-window.js | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/application-delegate.js b/src/application-delegate.js index 6d6d892ca..1b1dd1e9c 100644 --- a/src/application-delegate.js +++ b/src/application-delegate.js @@ -139,8 +139,8 @@ class ApplicationDelegate { return ipcRenderer.send('execute-javascript-in-dev-tools', code) } - didCloseInitialPath (path) { - return ipcHelpers.call('window-method', 'didCloseInitialPath', path) + didClosePathWithWaitSession (path) { + return ipcHelpers.call('window-method', 'didClosePathWithWaitSession', path) } setWindowDocumentEdited (edited) { diff --git a/src/atom-environment.js b/src/atom-environment.js index 6e42f88a0..70fb352e2 100644 --- a/src/atom-environment.js +++ b/src/atom-environment.js @@ -70,7 +70,7 @@ class AtomEnvironment { this.loadTime = null this.emitter = new Emitter() this.disposables = new CompositeDisposable() - this.pathsToNotifyWhenClosed = new Set() + this.pathsWithWaitSessions = new Set() // Public: A {DeserializerManager} instance this.deserializers = new DeserializerManager(this) @@ -360,7 +360,7 @@ class AtomEnvironment { this.grammars.clear() this.textEditors.clear() this.views.clear() - this.pathsToNotifyWhenClosed.clear() + this.pathsWithWaitSessions.clear() } destroy () { @@ -827,8 +827,8 @@ class AtomEnvironment { let previousProjectPaths = this.project.getPaths() this.disposables.add(this.project.onDidChangePaths(newPaths => { for (let path of previousProjectPaths) { - if (this.pathsToNotifyWhenClosed.has(path) && !newPaths.includes(path)) { - this.applicationDelegate.didCloseInitialPath(path) + if (this.pathsWithWaitSessions.has(path) && !newPaths.includes(path)) { + this.applicationDelegate.didClosePathWithWaitSession(path) } } previousProjectPaths = newPaths @@ -836,8 +836,8 @@ class AtomEnvironment { })) this.disposables.add(this.workspace.onDidDestroyPaneItem(({item}) => { const path = item.getPath && item.getPath() - if (this.pathsToNotifyWhenClosed.has(path)) { - this.applicationDelegate.didCloseInitialPath(path) + if (this.pathsWithWaitSessions.has(path)) { + this.applicationDelegate.didClosePathWithWaitSession(path) } })) @@ -1326,7 +1326,7 @@ class AtomEnvironment { fileLocationsToOpen.push(location) } - if (location.notifyWhenClosed) this.pathsToNotifyWhenClosed.add(pathToOpen) + if (location.hasWaitSession) this.pathsWithWaitSessions.add(pathToOpen) } let restoredState = false diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 52bc1287b..372bd537c 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -793,7 +793,7 @@ class AtomApplication extends EventEmitter { for (let i = 0; i < pathsToOpen.length; i++) { const location = this.parsePathToOpen(pathsToOpen[i], executedFrom, addToLastWindow) location.forceAddToWindow = addToLastWindow - location.notifyWhenClosed = pidToKillWhenClosed != null + location.hasWaitSession = pidToKillWhenClosed != null locationsToOpen.push(location) pathsToOpen[i] = location.pathToOpen } @@ -886,7 +886,7 @@ class AtomApplication extends EventEmitter { this.waitSessionsByWindow.delete(window) } - windowDidCloseInitialPath (window, initialPath) { + windowDidClosePathWithWaitSession (window, initialPath) { const waitSessions = this.waitSessionsByWindow.get(window) if (!waitSessions) return for (let i = waitSessions.length - 1; i >= 0; i--) { diff --git a/src/main-process/atom-window.js b/src/main-process/atom-window.js index 77dd09b31..0492b5f8f 100644 --- a/src/main-process/atom-window.js +++ b/src/main-process/atom-window.js @@ -411,8 +411,8 @@ class AtomWindow extends EventEmitter { return this.atomApplication.saveState() } - didCloseInitialPath (path) { - this.atomApplication.windowDidCloseInitialPath(this, path) + didClosePathWithWaitSession (path) { + this.atomApplication.windowDidClosePathWithWaitSession(this, path) } copy () {