Merge pull request #605 from github/beforeunload

Use beforeunload handler to control whether window should close

spec/app/pane-container-spec.coffee

@@ -151,41 +151,27 @@ describe "PaneContainer", ->
           expect(item.saved).toBeTruthy()
 
   describe ".confirmClose()", ->
-    it "resolves the returned promise after modified files are saved", ->
+    it "returns true after modified files are saved", ->
       pane1.itemAtIndex(0).isModified = -> true
       pane2.itemAtIndex(0).isModified = -> true
-      spyOn(atom, "confirm").andCallFake (a, b, c, d, e, f, g, noSaveFn) -> noSaveFn()
+      spyOn(atom, "confirmSync").andReturn(0)
 
-      promiseHandler = jasmine.createSpy("promiseHandler")
-      failedPromiseHandler = jasmine.createSpy("failedPromiseHandler")
-      promise = container.confirmClose()
-      promise.done promiseHandler
-      promise.fail failedPromiseHandler
-
-      waitsFor ->
-        promiseHandler.wasCalled
+      saved = container.confirmClose()
 
       runs ->
-        expect(failedPromiseHandler).not.toHaveBeenCalled()
-        expect(atom.confirm).toHaveBeenCalled()
+        expect(saved).toBeTruthy()
+        expect(atom.confirmSync).toHaveBeenCalled()
 
-    it "rejects the returned promise if the user cancels saving", ->
+    it "returns false if the user cancels saving", ->
       pane1.itemAtIndex(0).isModified = -> true
       pane2.itemAtIndex(0).isModified = -> true
-      spyOn(atom, "confirm").andCallFake (a, b, c, d, e, cancelFn, f, g) -> cancelFn()
+      spyOn(atom, "confirmSync").andReturn(1)
 
-      promiseHandler = jasmine.createSpy("promiseHandler")
-      failedPromiseHandler = jasmine.createSpy("failedPromiseHandler")
-      promise = container.confirmClose()
-      promise.done promiseHandler
-      promise.fail failedPromiseHandler
-
-      waitsFor ->
-        failedPromiseHandler.wasCalled
+      saved = container.confirmClose()
 
       runs ->
-        expect(promiseHandler).not.toHaveBeenCalled()
-        expect(atom.confirm).toHaveBeenCalled()
+        expect(saved).toBeFalsy()
+        expect(atom.confirmSync).toHaveBeenCalled()
 
   describe "serialization", ->
     it "can be serialized and deserialized, and correctly adjusts dimensions of deserialized panes after attach", ->

spec/app/pane-spec.coffee

@@ -106,28 +106,17 @@ describe "Pane", ->
 
     describe "if the item is modified", ->
       beforeEach ->
-        spyOn(atom, 'confirm')
-        spyOn(atom, 'showSaveDialog')
         spyOn(editSession2, 'save')
         spyOn(editSession2, 'saveAs')
 
-        atom.confirm.selectOption = (buttonText) ->
-          for arg, i in @argsForCall[0] when arg is buttonText
-            @argsForCall[0][i + 1]?()
-
         editSession2.insertText('a')
         expect(editSession2.isModified()).toBeTruthy()
-        pane.destroyItem(editSession2)
-
-      it "presents a dialog with the option to save the item first", ->
-        expect(atom.confirm).toHaveBeenCalled()
-        expect(pane.getItems().indexOf(editSession2)).not.toBe -1
-        expect(editSession2.destroyed).toBeFalsy()
 
       describe "if the [Save] option is selected", ->
         describe "when the item has a uri", ->
           it "saves the item before removing and destroying it", ->
-            atom.confirm.selectOption('Save')
+            spyOn(atom, 'confirmSync').andReturn(0)
+            pane.destroyItem(editSession2)
 
             expect(editSession2.save).toHaveBeenCalled()
             expect(pane.getItems().indexOf(editSession2)).toBe -1
@@ -137,11 +126,11 @@ describe "Pane", ->
           it "presents a save-as dialog, then saves the item with the given uri before removing and destroying it", ->
             editSession2.buffer.setPath(undefined)
 
-            atom.confirm.selectOption('Save')
+            spyOn(atom, 'showSaveDialogSync').andReturn("/selected/path")
+            spyOn(atom, 'confirmSync').andReturn(0)
+            pane.destroyItem(editSession2)
 
-            expect(atom.showSaveDialog).toHaveBeenCalled()
-
-            atom.showSaveDialog.argsForCall[0][0]("/selected/path")
+            expect(atom.showSaveDialogSync).toHaveBeenCalled()
 
             expect(editSession2.saveAs).toHaveBeenCalledWith("/selected/path")
             expect(pane.getItems().indexOf(editSession2)).toBe -1
@@ -149,7 +138,8 @@ describe "Pane", ->
 
       describe "if the [Don't Save] option is selected", ->
         it "removes and destroys the item without saving it", ->
-          atom.confirm.selectOption("Don't Save")
+          spyOn(atom, 'confirmSync').andReturn(2)
+          pane.destroyItem(editSession2)
 
           expect(editSession2.save).not.toHaveBeenCalled()
           expect(pane.getItems().indexOf(editSession2)).toBe -1
@@ -157,7 +147,8 @@ describe "Pane", ->
 
       describe "if the [Cancel] option is selected", ->
         it "does not save, remove, or destroy the item", ->
-          atom.confirm.selectOption("Cancel")
+          spyOn(atom, 'confirmSync').andReturn(1)
+          pane.destroyItem(editSession2)
 
           expect(editSession2.save).not.toHaveBeenCalled()
           expect(pane.getItems().indexOf(editSession2)).not.toBe -1
@@ -309,7 +300,7 @@ describe "Pane", ->
 
     describe "when the current item has no uri", ->
       beforeEach ->
-        spyOn(atom, 'showSaveDialog')
+        spyOn(atom, 'showSaveDialogSync').andReturn('/selected/path')
 
       describe "when the current item has a saveAs method", ->
         it "opens a save dialog and saves the current item as the selected path", ->
@@ -319,19 +310,18 @@ describe "Pane", ->
 
           pane.trigger 'core:save'
 
-          expect(atom.showSaveDialog).toHaveBeenCalled()
-          atom.showSaveDialog.argsForCall[0][0]('/selected/path')
+          expect(atom.showSaveDialogSync).toHaveBeenCalled()
           expect(editSession2.saveAs).toHaveBeenCalledWith('/selected/path')
 
       describe "when the current item has no saveAs method", ->
         it "does nothing", ->
           expect(pane.activeItem.saveAs).toBeUndefined()
           pane.trigger 'core:save'
-          expect(atom.showSaveDialog).not.toHaveBeenCalled()
+          expect(atom.showSaveDialogSync).not.toHaveBeenCalled()
 
   describe "core:save-as", ->
     beforeEach ->
-      spyOn(atom, 'showSaveDialog')
+      spyOn(atom, 'showSaveDialogSync').andReturn('/selected/path')
 
     describe "when the current item has a saveAs method", ->
       it "opens the save dialog and calls saveAs on the item with the selected path", ->
@@ -340,15 +330,14 @@ describe "Pane", ->
 
         pane.trigger 'core:save-as'
 
-        expect(atom.showSaveDialog).toHaveBeenCalled()
-        atom.showSaveDialog.argsForCall[0][0]('/selected/path')
+        expect(atom.showSaveDialogSync).toHaveBeenCalled()
         expect(editSession2.saveAs).toHaveBeenCalledWith('/selected/path')
 
     describe "when the current item does not have a saveAs method", ->
       it "does nothing", ->
         expect(pane.activeItem.saveAs).toBeUndefined()
         pane.trigger 'core:save-as'
-        expect(atom.showSaveDialog).not.toHaveBeenCalled()
+        expect(atom.showSaveDialogSync).not.toHaveBeenCalled()
 
   describe "pane:show-next-item and pane:show-previous-item", ->
     it "advances forward/backward through the pane's items, looping around at either end", ->

spec/app/window-spec.coffee

@@ -38,30 +38,43 @@ describe "Window", ->
         expect($("body")).not.toHaveClass("is-blurred")
 
   describe "window:close event", ->
-    describe "when no pane items are modified", ->
-      it "calls window.closeWithoutConfirm", ->
-        spyOn window, 'closeWithoutConfirm'
-        $(window).trigger 'window:close'
-        expect(window.closeWithoutConfirm).toHaveBeenCalled()
+    it "closes the window", ->
+      spyOn(window, 'close')
+      $(window).trigger 'window:close'
+      expect(window.close).toHaveBeenCalled()
 
+    it "emits the beforeunload event", ->
+      $(window).off 'beforeunload'
+      beforeunload = jasmine.createSpy('beforeunload').andReturn(false)
+      $(window).on 'beforeunload', beforeunload
+
+      $(window).trigger 'window:close'
+      expect(beforeunload).toHaveBeenCalled()
+
+  describe "beforeunload event", ->
     describe "when pane items are are modified", ->
-      it "prompts user to save and and calls window.closeWithoutConfirm", ->
-        spyOn(window, 'closeWithoutConfirm')
-        spyOn(atom, "confirm").andCallFake (a, b, c, d, e, f, g, noSave) -> noSave()
+      it "prompts user to save and and calls rootView.confirmClose", ->
+        spyOn(rootView, 'confirmClose').andCallThrough()
+        spyOn(atom, "confirmSync").andReturn(2)
         editSession = rootView.open("sample.js")
         editSession.insertText("I look different, I feel different.")
-        $(window).trigger 'window:close'
-        expect(window.closeWithoutConfirm).toHaveBeenCalled()
-        expect(atom.confirm).toHaveBeenCalled()
+        $(window).trigger 'beforeunload'
+        expect(rootView.confirmClose).toHaveBeenCalled()
+        expect(atom.confirmSync).toHaveBeenCalled()
 
-      it "prompts user to save and aborts if dialog is canceled", ->
-        spyOn(window, 'closeWithoutConfirm')
-        spyOn(atom, "confirm").andCallFake (a, b, c, d, e, cancel) -> cancel()
+      it "prompts user to save and handler returns true if don't save", ->
+        spyOn(atom, "confirmSync").andReturn(2)
         editSession = rootView.open("sample.js")
         editSession.insertText("I look different, I feel different.")
-        $(window).trigger 'window:close'
-        expect(window.closeWithoutConfirm).not.toHaveBeenCalled()
-        expect(atom.confirm).toHaveBeenCalled()
+        expect(window.onbeforeunload(new Event('beforeunload'))).toBeTruthy()
+        expect(atom.confirmSync).toHaveBeenCalled()
+
+      it "prompts user to save and handler returns false if dialog is canceled", ->
+        spyOn(atom, "confirmSync").andReturn(1)
+        editSession = rootView.open("sample.js")
+        editSession.insertText("I look different, I feel different.")
+        expect(window.onbeforeunload(new Event('beforeunload'))).toBeFalsy()
+        expect(atom.confirmSync).toHaveBeenCalled()
 
   describe "requireStylesheet(path)", ->
     it "synchronously loads css at the given path and installs a style tag for it in the head", ->

src/app/atom.coffee

@@ -7,6 +7,7 @@ ipc = require 'ipc'
 remote = require 'remote'
 crypto = require 'crypto'
 path = require 'path'
+dialog = remote.require 'dialog'
 
 window.atom =
   loadedThemes: []
@@ -181,18 +182,22 @@ window.atom =
         buttons.push buttonLabelsAndCallbacks.shift()
         callbacks.push buttonLabelsAndCallbacks.shift()
 
-    chosen = remote.require('dialog').showMessageBox
+    chosen = confirmSync(message, detailedMessage, buttons)
+    callbacks[chosen]?()
+
+  confirmSync: (message, detailedMessage, buttons, browserWindow = null) ->
+    chosen = dialog.showMessageBox browserWindow,
       type: 'info'
       message: message
       detail: detailedMessage
       buttons: buttons
 
-    callbacks[chosen]?()
-
   showSaveDialog: (callback) ->
+    callback(showSaveDialogSync())
+
+  showSaveDialogSync: ->
     currentWindow = remote.getCurrentWindow()
-    result = remote.require('dialog').showSaveDialog currentWindow, title: 'Save File'
-    callback(result)
+    dialog.showSaveDialog currentWindow, title: 'Save File'
 
   openDevTools: ->
     remote.getCurrentWindow().openDevTools()

src/app/pane-container.coffee

@@ -80,21 +80,13 @@ class PaneContainer extends View
     pane.saveItems() for pane in @getPanes()
 
   confirmClose: ->
-    deferred = $.Deferred()
-    modifiedItems = []
+    saved = true
     for pane in @getPanes()
-      modifiedItems.push(item) for item in pane.getItems() when item.isModified?()
-
-    cancel = => deferred.reject()
-    saveNextModifiedItem = =>
-      if modifiedItems.length == 0
-        deferred.resolve()
-      else
-        item = modifiedItems.pop()
-        @paneAtIndex(0).promptToSaveItem item, saveNextModifiedItem, cancel
-
-    saveNextModifiedItem()
-    deferred.promise()
+      for item in pane.getItems() when item.isModified?()
+        if not @paneAtIndex(0).promptToSaveItem item
+          saved = false
+          break
+    saved
 
   getPanes: ->
     @find('.pane').views()

src/app/pane.coffee

@@ -154,7 +154,7 @@ class Pane extends View
     @autosaveItem(item)
 
     if item.shouldPromptToSave?()
-      @promptToSaveItem(item, reallyDestroyItem)
+      reallyDestroyItem() if @promptToSaveItem(item)
     else
       reallyDestroyItem()
 
@@ -164,15 +164,19 @@ class Pane extends View
   destroyInactiveItems: ->
     @destroyItem(item) for item in @getItems() when item isnt @activeItem
 
-  promptToSaveItem: (item, nextAction, cancelAction) ->
+  promptToSaveItem: (item) ->
     uri = item.getUri()
-    atom.confirm(
+    currentWindow = require('remote').getCurrentWindow()
+    chosen = atom.confirmSync(
       "'#{item.getTitle?() ? item.getUri()}' has changes, do you want to save them?"
       "Your changes will be lost if you close this item without saving."
-      "Save", => @saveItem(item, nextAction)
-      "Cancel", cancelAction
-      "Don't Save", nextAction
+      ["Save", "Cancel", "Don't Save"]
+      currentWindow
     )
+    switch chosen
+      when 0 then @saveItem(item, -> true)
+      when 1 then false
+      when 2 then true
 
   saveActiveItem: =>
     @saveItem(@activeItem)
@@ -189,10 +193,10 @@ class Pane extends View
 
   saveItemAs: (item, nextAction) ->
     return unless item.saveAs?
-    atom.showSaveDialog (path) =>
-      if path
-        item.saveAs(path)
-        nextAction?()
+    path = atom.showSaveDialogSync()
+    if path
+      item.saveAs(path)
+      nextAction?()
 
   saveItems: =>
     @saveItem(item) for item in @getItems()

src/app/window-event-handler.coffee

@@ -14,13 +14,10 @@ class WindowEventHandler
     @subscribe $(window), 'blur',  -> $("body").addClass('is-blurred')
     @subscribe $(window), 'window:open-path', (event, pathToOpen) ->
       rootView?.open(pathToOpen) unless fsUtils.isDirectorySync(pathToOpen)
+    @subscribe $(window), 'beforeunload', -> rootView?.confirmClose()
 
     @subscribeToCommand $(window), 'window:toggle-full-screen', => atom.toggleFullScreen()
-    @subscribeToCommand $(window), 'window:close', =>
-      if rootView?
-        rootView.confirmClose().done -> closeWithoutConfirm()
-      else
-        closeWithoutConfirm()
+    @subscribeToCommand $(window), 'window:close', => window.close()
     @subscribeToCommand $(window), 'window:reload', => atom.reload()
 
     @subscribeToCommand $(document), 'core:focus-next', @focusNext

src/app/window.coffee

@@ -210,10 +210,6 @@ window.restoreDimensions = ->
   window.setDimensions(dimensions)
   $(window).on 'unload', -> atom.setWindowState('dimensions', window.getDimensions())
 
-window.closeWithoutConfirm = ->
-  atom.hide()
-  ipc.sendChannel 'close-without-confirm'
-
 window.onerror = ->
   atom.openDevTools()
 

src/atom-application.coffee

@@ -175,11 +175,6 @@ class AtomApplication
       @installUpdate = quitAndUpdate
       @buildApplicationMenu version, quitAndUpdate
 
-    ipc.on 'close-without-confirm', (processId, routingId) ->
-      window = BrowserWindow.fromProcessIdAndRoutingId processId, routingId
-      window.removeAllListeners 'close'
-      window.close()
-
     ipc.on 'open-config', =>
       @openConfig()
 

src/atom-window.coffee

@@ -89,11 +89,6 @@ class AtomWindow
       # Spec window's web view should always have focus
       @browserWindow.on 'blur', =>
         @browserWindow.focusOnWebView()
-    else
-      @browserWindow.on 'close', (event) =>
-        unless @browserWindow.isCrashed()
-          event.preventDefault()
-          @sendCommand 'window:close'
 
   openPath: (pathToOpen) ->
     if @loaded

static/index.html

@@ -13,6 +13,7 @@
         currentWindow.emit('window:loaded');
       }
       catch (error) {
+        currentWindow.show();
         currentWindow.openDevTools();
         console.error(error.stack || error);
       }