Add repo-list action to open repositories in new windows

Add an "Open Repository in New Window" action to the repository list context menu and the minimum multi-window plumbing needed to support it safely.\n\nAlso updates native window titles with the selected repository name and reveals hidden/minimized windows when notification clicks focus them.

Author: hewigovens

app/src/lib/cli-action.ts

@@ -2,6 +2,7 @@ export type CLIAction =
   | {
       readonly kind: 'open-repository'
       readonly path: string
+      readonly persistSelection?: boolean
     }
   | {
       readonly kind: 'clone-url'

app/src/lib/ipc-shared.ts

@@ -69,6 +69,8 @@ export type RequestChannels = {
   'update-accounts': (accounts: ReadonlyArray<EndpointToken>) => void
   'quit-and-install-updates': () => void
   'quit-app': () => void
+  'open-repository-in-new-window': (path: string) => void
+  'set-window-title': (title: string) => void
   'minimize-window': () => void
   'maximize-window': () => void
   'unmaximize-window': () => void

app/src/lib/stores/app-store.ts

@@ -1897,7 +1897,8 @@ export class AppStore extends TypedBaseStore<IAppState> {
 
   /** This shouldn't be called directly. See `Dispatcher`. */
   public async _selectRepository(
-    repository: Repository | CloningRepository | null
+    repository: Repository | CloningRepository | null,
+    persistSelection: boolean = true
   ): Promise<Repository | null> {
     const previouslySelectedRepository = this.selectedRepository
 
@@ -1929,7 +1930,9 @@ export class AppStore extends TypedBaseStore<IAppState> {
       return Promise.resolve(null)
     }
 
-    setNumber(LastSelectedRepositoryIDKey, repository.id)
+    if (persistSelection) {
+      setNumber(LastSelectedRepositoryIDKey, repository.id)
+    }
 
     const previousRepositoryId = previouslySelectedRepository
       ? previouslySelectedRepository.id
@@ -6388,6 +6391,7 @@ export class AppStore extends TypedBaseStore<IAppState> {
     }
 
     if (this.appIsFocused) {
+      this.updateMenuLabelsForSelectedRepository()
       this.repositoryIndicatorUpdater.resume()
       if (this.selectedRepository instanceof Repository) {
         this.startPullRequestUpdater(this.selectedRepository)

app/src/main-process/app-window.ts

@@ -5,7 +5,9 @@ import {
   BrowserWindow,
   autoUpdater,
   nativeTheme,
+  ipcMain as electronIpcMain,
 } from 'electron'
+import { IpcMainEvent } from 'electron/main'
 import { shell } from '../lib/app-shell'
 import { Emitter, Disposable } from 'event-kit'
 import { encodePathAsUrl } from '../lib/path'
@@ -20,19 +22,16 @@ import { menuFromElectronMenu } from '../models/app-menu'
 import { now } from './now'
 import * as path from 'path'
 import windowStateKeeper from 'electron-window-state'
-import * as ipcMain from './ipc-main'
 import * as ipcWebContents from './ipc-webcontents'
-import {
-  installNotificationCallback,
-  terminateDesktopNotifications,
-} from './notifications'
+import { installNotificationCallback } from './notifications'
 import { addTrustedIPCSender } from './trusted-ipc-sender'
 import { getUpdaterGUID } from '../lib/get-updater-guid'
 import { CLIAction } from '../lib/cli-action'
 
 export class AppWindow {
   private window: Electron.BrowserWindow
   private emitter = new Emitter()
+  private readonly cleanupTasks = new Array<() => void>()
 
   private _loadTime: number | null = null
   private _rendererReadyTime: number | null = null
@@ -43,6 +42,8 @@ export class AppWindow {
 
   // See https://github.com/desktop/desktop/pull/11162
   private shouldMaximizeOnShow = false
+  private quitting = false
+  private quittingEvenIfUpdating = false
 
   public constructor() {
     const savedWindowState = windowStateKeeper({
@@ -84,41 +85,24 @@ export class AppWindow {
     this.window = new BrowserWindow(windowOptions)
     addTrustedIPCSender(this.window.webContents)
 
-    installNotificationCallback(this.window)
+    this.addCleanupTask(installNotificationCallback(this.window))
 
     savedWindowState.manage(this.window)
     this.shouldMaximizeOnShow = savedWindowState.isMaximized
 
-    let quitting = false
-    let quittingEvenIfUpdating = false
-    app.on('before-quit', () => {
-      quitting = true
-    })
-
-    ipcMain.on('will-quit', event => {
-      quitting = true
-      event.returnValue = true
-    })
-
-    ipcMain.on('will-quit-even-if-updating', event => {
-      quitting = true
-      quittingEvenIfUpdating = true
-      event.returnValue = true
-    })
-
-    ipcMain.on('cancel-quitting', event => {
-      quitting = false
-      quittingEvenIfUpdating = false
-      event.returnValue = true
-    })
+    const onBeforeQuit = () => {
+      this.quitting = true
+    }
+    app.on('before-quit', onBeforeQuit)
+    this.addCleanupTask(() => app.removeListener('before-quit', onBeforeQuit))
 
     this.window.on('close', e => {
       // On macOS, closing the window doesn't mean the app is quitting. If the
       // app is updating, we will prevent the window from closing only when the
       // app is also quitting.
       if (
-        (!__DARWIN__ || quitting) &&
-        !quittingEvenIfUpdating &&
+        (!__DARWIN__ || this.quitting) &&
+        !this.quittingEvenIfUpdating &&
         this.isDownloadingUpdate
       ) {
         e.preventDefault()
@@ -136,7 +120,7 @@ export class AppWindow {
       // on macOS, when the user closes the window we really just hide it. This
       // lets us activate quickly and keep all our interesting logic in the
       // renderer.
-      if (__DARWIN__ && !quitting) {
+      if (__DARWIN__ && !this.quitting) {
         e.preventDefault()
         // https://github.com/desktop/desktop/issues/12838
         if (this.window.isFullScreen()) {
@@ -147,10 +131,9 @@ export class AppWindow {
         }
         return
       }
-      nativeTheme.removeAllListeners()
-      autoUpdater.removeAllListeners()
-      terminateDesktopNotifications()
     })
+
+    this.window.on('closed', () => this.cleanup())
   }
 
   public load() {
@@ -187,11 +170,37 @@ export class AppWindow {
       this.window.show()
     })
 
-    // TODO: This should be scoped by the window.
-    ipcMain.once('renderer-ready', (_, readyTime) => {
+    const onRendererReady = (event: IpcMainEvent, readyTime: number) => {
+      if (event.sender !== this.window.webContents) {
+        return
+      }
+
       this._rendererReadyTime = readyTime
       this.maybeEmitDidLoad()
-    })
+      electronIpcMain.removeListener('renderer-ready', onRendererReady)
+    }
+    electronIpcMain.on('renderer-ready', onRendererReady)
+    this.addCleanupTask(() =>
+      electronIpcMain.removeListener('renderer-ready', onRendererReady)
+    )
+
+    const onBackgroundColorUpdated = (event: IpcMainEvent, color: string) => {
+      if (event.sender !== this.window.webContents) {
+        return
+      }
+
+      this.window.setBackgroundColor(color)
+    }
+    electronIpcMain.on(
+      'update-window-background-color',
+      onBackgroundColorUpdated
+    )
+    this.addCleanupTask(() =>
+      electronIpcMain.removeListener(
+        'update-window-background-color',
+        onBackgroundColorUpdated
+      )
+    )
 
     this.window.on('focus', () =>
       ipcWebContents.send(this.window.webContents, 'focus')
@@ -203,13 +212,13 @@ export class AppWindow {
     registerWindowStateChangedEvents(this.window)
     this.window.loadURL(encodePathAsUrl(__dirname, 'index.html'))
 
-    nativeTheme.addListener('updated', () => {
+    const onNativeThemeUpdated = () => {
       ipcWebContents.send(this.window.webContents, 'native-theme-updated')
-    })
-
-    ipcMain.on('update-window-background-color', (_, color) => {
-      this.window.setBackgroundColor(color)
-    })
+    }
+    nativeTheme.addListener('updated', onNativeThemeUpdated)
+    this.addCleanupTask(() =>
+      nativeTheme.removeListener('updated', onNativeThemeUpdated)
+    )
 
     this.setupAutoUpdater()
   }
@@ -231,6 +240,10 @@ export class AppWindow {
     return !!this.loadTime && !!this.rendererReadyTime
   }
 
+  public get isLoaded(): boolean {
+    return this.rendererLoaded
+  }
+
   public onClosed(fn: () => void) {
     this.window.on('closed', fn)
   }
@@ -260,10 +273,31 @@ export class AppWindow {
     return this.window.isFocused()
   }
 
+  public get id() {
+    return this.window.id
+  }
+
   public focus() {
     this.window.focus()
   }
 
+  public revealAndFocus() {
+    if (this.window.isMinimized()) {
+      this.window.restore()
+    }
+
+    if (!this.window.isVisible()) {
+      this.show()
+      return
+    }
+
+    this.window.focus()
+  }
+
+  public setTitle(title: string) {
+    this.window.setTitle(title)
+  }
+
   /** Selects all the windows web contents */
   public selectAllWindowContents() {
     this.window.webContents.selectAll()
@@ -394,42 +428,60 @@ export class AppWindow {
   }
 
   public setupAutoUpdater() {
-    autoUpdater.on('error', (error: Error) => {
+    const onAutoUpdaterError = (error: Error) => {
       this.isDownloadingUpdate = false
       ipcWebContents.send(this.window.webContents, 'auto-updater-error', error)
-    })
+    }
+    autoUpdater.on('error', onAutoUpdaterError)
+    this.addCleanupTask(() => autoUpdater.removeListener('error', onAutoUpdaterError))
 
-    autoUpdater.on('checking-for-update', () => {
+    const onCheckingForUpdate = () => {
       this.isDownloadingUpdate = false
       ipcWebContents.send(
         this.window.webContents,
         'auto-updater-checking-for-update'
       )
-    })
+    }
+    autoUpdater.on('checking-for-update', onCheckingForUpdate)
+    this.addCleanupTask(() =>
+      autoUpdater.removeListener('checking-for-update', onCheckingForUpdate)
+    )
 
-    autoUpdater.on('update-available', () => {
+    const onUpdateAvailable = () => {
       this.isDownloadingUpdate = true
       ipcWebContents.send(
         this.window.webContents,
         'auto-updater-update-available'
       )
-    })
+    }
+    autoUpdater.on('update-available', onUpdateAvailable)
+    this.addCleanupTask(() =>
+      autoUpdater.removeListener('update-available', onUpdateAvailable)
+    )
 
-    autoUpdater.on('update-not-available', () => {
+    const onUpdateNotAvailable = () => {
       this.isDownloadingUpdate = false
       ipcWebContents.send(
         this.window.webContents,
         'auto-updater-update-not-available'
       )
-    })
+    }
+    autoUpdater.on('update-not-available', onUpdateNotAvailable)
+    this.addCleanupTask(() =>
+      autoUpdater.removeListener('update-not-available', onUpdateNotAvailable)
+    )
 
-    autoUpdater.on('update-downloaded', () => {
+    const onUpdateDownloaded = () => {
       this.isDownloadingUpdate = false
       ipcWebContents.send(
         this.window.webContents,
         'auto-updater-update-downloaded'
       )
-    })
+    }
+    autoUpdater.on('update-downloaded', onUpdateDownloaded)
+    this.addCleanupTask(() =>
+      autoUpdater.removeListener('update-downloaded', onUpdateDownloaded)
+    )
   }
 
   public async checkForUpdates(url: string) {
@@ -496,6 +548,30 @@ export class AppWindow {
     const { filePaths } = await dialog.showOpenDialog(this.window, options)
     return filePaths.length > 0 ? filePaths[0] : null
   }
+
+  public markWillQuit() {
+    this.quitting = true
+  }
+
+  public markWillQuitEvenIfUpdating() {
+    this.quitting = true
+    this.quittingEvenIfUpdating = true
+  }
+
+  public cancelQuitting() {
+    this.quitting = false
+    this.quittingEvenIfUpdating = false
+  }
+
+  private addCleanupTask(task: () => void) {
+    this.cleanupTasks.push(task)
+  }
+
+  private cleanup() {
+    for (const task of this.cleanupTasks.splice(0).reverse()) {
+      task()
+    }
+  }
 }
 
 const trySetUpdaterGuid = async (url: string) => {