Merge pull request #16497 from atom/mb-reuse-windows-with-wait-flag

Allow existing windows to be used when running atom --wait

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,6 +390,110 @@ describe('AtomApplication', function () {
       assert.deepEqual(app2Window.representedDirectoryPaths, [])
     })
 
+    describe('when the `--wait` flag is passed', () => {
+      let killedPids, atomApplication, onDidKillProcess
+
+      beforeEach(() => {
+        killedPids = []
+        onDidKillProcess = null
+        atomApplication = buildAtomApplication({
+          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(['--wait', '--pid', '101']))
+        await focusWindow(window1)
+
+        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 processKillPromise
+        assert.deepEqual(killedPids, [101])
+
+        processKillPromise = new Promise(resolve => { onDidKillProcess = resolve })
+        window2.close()
+        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])
+      })
+
+      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', () => {
       if (process.platform === 'linux' || process.platform === 'win32') {
         it('quits the application', async () => {
@@ -529,11 +634,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
   }
@@ -566,7 +671,6 @@ describe('AtomApplication', function () {
   }
 
   function makeTempDir (name) {
-    const temp = require('temp').track()
     return fs.realpathSync(temp.mkdirSync(name))
   }
 
@@ -585,7 +689,7 @@ describe('AtomApplication', function () {
         function sendBackToMainProcess (result) {
           require('electron').ipcRenderer.send('${channelId}', result)
         }
-        (${source})(sendBackToMainProcess)
+        (${source})(sendBackToMainProcess, ${args.map(JSON.stringify).join(', ')})
       `)
     })
   }

src/application-delegate.js

@@ -139,6 +139,10 @@ class ApplicationDelegate {
     return ipcRenderer.send('execute-javascript-in-dev-tools', code)
   }
 
+  didClosePathWithWaitSession (path) {
+    return ipcHelpers.call('window-method', 'didClosePathWithWaitSession', path)
+  }
+
   setWindowDocumentEdited (edited) {
     return ipcHelpers.call('window-method', 'setDocumentEdited', edited)
   }

src/atom-environment.js

@@ -70,6 +70,7 @@ class AtomEnvironment {
     this.loadTime = null
     this.emitter = new Emitter()
     this.disposables = new CompositeDisposable()
+    this.pathsWithWaitSessions = 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.pathsWithWaitSessions.clear()
   }
 
   destroy () {
@@ -822,7 +824,22 @@ class AtomEnvironment {
       this.document.body.appendChild(this.workspace.getElement())
       if (this.backgroundStylesheet) this.backgroundStylesheet.remove()
 
-      this.watchProjectPaths()
+      let previousProjectPaths = this.project.getPaths()
+      this.disposables.add(this.project.onDidChangePaths(newPaths => {
+        for (let path of previousProjectPaths) {
+          if (this.pathsWithWaitSessions.has(path) && !newPaths.includes(path)) {
+            this.applicationDelegate.didClosePathWithWaitSession(path)
+          }
+        }
+        previousProjectPaths = newPaths
+        this.applicationDelegate.setRepresentedDirectoryPaths(newPaths)
+      }))
+      this.disposables.add(this.workspace.onDidDestroyPaneItem(({item}) => {
+        const path = item.getPath && item.getPath()
+        if (this.pathsWithWaitSessions.has(path)) {
+          this.applicationDelegate.didClosePathWithWaitSession(path)
+        }
+      }))
 
       this.packages.activate()
       this.keymaps.loadUserKeymap()
@@ -1025,13 +1042,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 +1310,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 +1323,10 @@ class AtomEnvironment {
       }
 
       if (!fs.isDirectorySync(pathToOpen)) {
-        fileLocationsToOpen.push({pathToOpen, initialLine, initialColumn})
+        fileLocationsToOpen.push(location)
       }
+
+      if (location.hasWaitSession) this.pathsWithWaitSessions.add(pathToOpen)
     }
 
     let restoredState = false
@@ -1334,7 +1347,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)

src/main-process/atom-application.js

@@ -101,9 +101,10 @@ 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 = {}
+    this.waitSessionsByWindow = new Map()
     this.windowStack = new WindowStack()
 
     this.config = new Config({enablePersistence: true})
@@ -788,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.hasWaitSession = 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) {
@@ -852,26 +857,44 @@ 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)
+  }
+
+  windowDidClosePathWithWaitSession (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)
+      if (session.remainingPaths.size === 0) {
+        this.killProcess(session.pid)
+        waitSessions.splice(i, 1)
       }
     }
   }
@@ -880,13 +903,12 @@ 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}`)
       }
     }
-    delete this.pidsToOpenWindows[pid]
   }
 
   saveState (allowEmpty = false) {
@@ -1192,7 +1214,7 @@ class AtomApplication extends EventEmitter {
     }
   }
 
-  locationForPathToOpen (pathToOpen, executedFrom = '', forceAddToWindow) {
+  parsePathToOpen (pathToOpen, executedFrom = '') {
     let initialColumn, initialLine
     if (!pathToOpen) {
       return {pathToOpen}
@@ -1217,7 +1239,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.

src/main-process/atom-window.js

@@ -411,6 +411,10 @@ class AtomWindow extends EventEmitter {
     return this.atomApplication.saveState()
   }
 
+  didClosePathWithWaitSession (path) {
+    this.atomApplication.windowDidClosePathWithWaitSession(this, path)
+  }
+
   copy () {
     return this.browserWindow.copy()
   }