[geary/mjog/493-undo-send: 26/37] Rework Composer.Widget close management

[Michael Gratton <mjog src gnome org>]

commit bc78622eeb1bcbe68b788d1f4760e7c5158a2870
Author: Michael Gratton <mike vee net>
Date:   Tue Nov 12 12:28:24 2019 +1100

    Rework Composer.Widget close management
    
    Rename Composer.Widget::should_close to ::confirm_close and actually
    start closing the composer in all cases if not cancelled, so that call
    sites don't need to do so manually. Simplify CloseStatus enum and
    Application.Controller's management as a result.

 src/client/application/application-controller.vala |  60 ++++----
 src/client/components/main-window.vala             |  15 +-
 src/client/composer/composer-widget.vala           | 154 ++++++++++++---------
 src/client/composer/composer-window.vala           |   8 +-
 4 files changed, 121 insertions(+), 116 deletions(-)
---
diff --git a/src/client/application/application-controller.vala 
b/src/client/application/application-controller.vala
index 2abb151a..549f83d4 100644
--- a/src/client/application/application-controller.vala
+++ b/src/client/application/application-controller.vala
@@ -427,7 +427,7 @@ public class Application.Controller : Geary.BaseObject {
     /** Adds a new composer to be kept track of. */
     public void add_composer(Composer.Widget widget) {
         debug(@"Added composer of type $(widget.compose_type); $(this.composer_widgets.size) composers 
total");
-        widget.destroy.connect(this.on_composer_widget_destroy);
+        widget.destroy.connect_after(this.on_composer_widget_destroy);
         this.composer_widgets.add(widget);
     }
 
@@ -1478,56 +1478,46 @@ public class Application.Controller : Geary.BaseObject {
         composer.set_focus();
     }
 
-    internal bool close_composition_windows(bool main_window_only = false) {
-        Gee.List<Composer.Widget> composers_to_destroy = new Gee.ArrayList<Composer.Widget>();
+    internal bool close_composition_windows() {
+        // Take a copy of the collection of composers since closing
+        // any will cause the underlying collection to change.
+        var composers = new Gee.LinkedList<Composer.Widget>();
+        composers.add_all(this.composer_widgets);
         bool quit_cancelled = false;
 
-        // If there's composer windows open, give the user a chance to
-        // save or cancel.
-        foreach(Composer.Widget cw in composer_widgets) {
-            if (!main_window_only ||
-                cw.state != Composer.Widget.ComposerState.DETACHED) {
-                // Check if we should close the window immediately, or
-                // if we need to wait.
-                Composer.Widget.CloseStatus status = cw.should_close();
-                if (status == Composer.Widget.CloseStatus.PENDING_CLOSE) {
-                    // Window is currently busy saving.
-                    waiting_to_close.add(cw);
-                } else if (status == Composer.Widget.CloseStatus.CANCEL_CLOSE) {
-                    // User cancelled operation.
+        foreach (var composer in composers) {
+            if (composer.current_mode == NONE) {
+                // Composer currently isn't being presented at all
+                // (it's probably in the undo stack), so just close it
+                this.waiting_to_close.add(composer);
+                composer.close.begin();
+            } else {
+                switch (composer.confirm_close()) {
+                case Composer.Widget.CloseStatus.PENDING:
+                    this.waiting_to_close.add(composer);
+                    break;
+
+                case Composer.Widget.CloseStatus.CANCELLED:
                     quit_cancelled = true;
                     break;
-                } else if (status == Composer.Widget.CloseStatus.DO_CLOSE) {
-                    // Hide any existing composer windows for the
-                    // moment; actually deleting the windows will
-                    // result in their removal from composer_windows,
-                    // which could crash this loop.
-                    composers_to_destroy.add(cw);
-                    cw.set_enabled(false);
                 }
             }
         }
 
-        // Safely destroy windows.
-        foreach (Composer.Widget cw in composers_to_destroy) {
-            cw.close.begin();
-        }
-
         // If we cancelled the quit we can bail here.
         if (quit_cancelled) {
-            waiting_to_close.clear();
-
+            this.waiting_to_close.clear();
             return false;
         }
 
-        // If there's still windows saving, we can't exit just yet.  Hide the main window and wait.
-        if (waiting_to_close.size > 0) {
-            main_window.hide();
-
+        // If there's still windows saving, we can't exit just yet.
+        if (this.waiting_to_close.size > 0) {
+            this.main_window.set_sensitive(false);
             return false;
         }
 
-        // If we deleted all composer windows without the user cancelling, we can exit.
+        // If we deleted all composer windows without the user
+        // cancelling, we can exit.
         return true;
     }
 
diff --git a/src/client/components/main-window.vala b/src/client/components/main-window.vala
index b64cc6e5..7faf45c8 100644
--- a/src/client/components/main-window.vala
+++ b/src/client/components/main-window.vala
@@ -626,16 +626,9 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
     public bool close_composer() {
         bool closed = true;
         Composer.Widget? composer = this.conversation_viewer.current_composer;
-        if (composer != null) {
-            switch (composer.should_close()) {
-            case DO_CLOSE:
-                composer.close.begin();
-                break;
-
-            case CANCEL_CLOSE:
-                closed = false;
-                break;
-            }
+        if (composer != null &&
+            composer.confirm_close() == CANCELLED) {
+            closed = false;
         }
         return closed;
     }
@@ -1605,7 +1598,7 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
     [GtkCallback]
     private bool on_delete_event() {
         if (this.application.config.startup_notifications) {
-            if (this.application.controller.close_composition_windows(true)) {
+            if (close_composer()) {
                 hide();
             }
         } else {
diff --git a/src/client/composer/composer-widget.vala b/src/client/composer/composer-widget.vala
index e3a01999..b85fbcd3 100644
--- a/src/client/composer/composer-widget.vala
+++ b/src/client/composer/composer-widget.vala
@@ -31,10 +31,18 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
         FORWARD
     }
 
+    /**
+     * Determines the result of prompting whether to close the composer.
+     *
+     * @see confirm_close
+     */
     public enum CloseStatus {
-        DO_CLOSE,
-        PENDING_CLOSE,
-        CANCEL_CLOSE
+
+        /** The composer is being closed. */
+        PENDING,
+
+        /** Closing the composer was not confirmed by a human. */
+        CANCELLED;
     }
 
     /** Defines different supported user interface modes. */
@@ -745,8 +753,79 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
         }
     }
 
+    /**
+     * Prompts to close the composer if needed, before closing it.
+     *
+     * The return value specifies whether the composer is being
+     * closed, if it was already being closed, or the prompt was
+     * cancelled by a human.
+     */
+    public CloseStatus confirm_close() {
+        CloseStatus status = PENDING;
+
+        if (this.is_blank) {
+            this.close.begin();
+        }
+
+        if (!this.is_closing && !this.is_blank) {
+            present();
+
+            if (this.can_save) {
+                var dialog = new TernaryConfirmationDialog(
+                    this.container.top_window,
+                    // Translators: This dialog text is displayed to the
+                    // user when closing a composer where the options are
+                    // Keep, Discard or Cancel.
+                    _("Do you want to keep or discard this draft message?"),
+                    null,
+                    Stock._KEEP,
+                    Stock._DISCARD, Gtk.ResponseType.CLOSE,
+                    "",
+                    "destructive-action",
+                    Gtk.ResponseType.OK // Default == Keep
+                );
+                Gtk.ResponseType response = dialog.run();
+                if (response == CANCEL ||
+                    response == DELETE_EVENT) {
+                    // Cancel
+                    status = CANCELLED;
+                } else if (response == OK) {
+                    // Keep
+                    if (!this.is_draft_saved) {
+                        this.save_and_exit_async.begin();
+                    } else {
+                        this.close.begin();
+                    }
+                } else {
+                    // Discard
+                    this.discard_and_exit_async.begin();
+                }
+            } else {
+                AlertDialog dialog = new ConfirmationDialog(
+                    container.top_window,
+                    // Translators: This dialog text is displayed to the
+                    // user when closing a composer where the options are
+                    // only Discard or Cancel.
+                    _("Do you want to discard this draft message?"),
+                    null,
+                    Stock._DISCARD,
+                    "destructive-action"
+                );
+                Gtk.ResponseType response = dialog.run();
+                if (response == OK) {
+                    this.discard_and_exit_async.begin();
+                } else {
+                    status = CANCELLED;
+                }
+            }
+        }
+
+        return status;
+    }
+
     /** Closes the composer unconditionally. */
     public async void close() {
+        this.is_closing = true;
         set_enabled(false);
 
         if (this.draft_manager_opening != null) {
@@ -788,7 +867,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
      */
     public void set_enabled(bool enabled) {
         this.current_mode = NONE;
-        this.is_closing = !enabled;
         this.set_sensitive(enabled);
 
         // Need to update this separately since it may be detached
@@ -796,6 +874,7 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
         this.header.set_sensitive(enabled);
 
         if (enabled) {
+            this.is_closing = false;
             this.open_draft_manager_async.begin(null, null);
         } else {
             if (this.container != null) {
@@ -1278,67 +1357,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
         this.referred_ids.add(referred.id);
     }
 
-    public CloseStatus should_close() {
-        if (this.is_closing)
-            return CloseStatus.PENDING_CLOSE;
-        if (this.is_blank)
-            return CloseStatus.DO_CLOSE;
-
-        present();
-
-        CloseStatus status = CloseStatus.PENDING_CLOSE;
-        if (this.can_save) {
-            AlertDialog dialog = new TernaryConfirmationDialog(
-                container.top_window,
-                // Translators: This dialog text is displayed to the
-                // user when closing a composer where the options are
-                // Keep, Discard or Cancel.
-                _("Do you want to keep or discard this draft message?"),
-                null,
-                Stock._KEEP,
-                Stock._DISCARD, Gtk.ResponseType.CLOSE,
-                "",
-                "destructive-action",
-                Gtk.ResponseType.OK // Default == Keep
-            );
-            Gtk.ResponseType response = dialog.run();
-            if (response == Gtk.ResponseType.CANCEL ||
-                response == Gtk.ResponseType.DELETE_EVENT) {
-                // Cancel
-                status = CloseStatus.CANCEL_CLOSE;
-            } else if (response == Gtk.ResponseType.OK) {
-                // Keep
-                if (!this.is_draft_saved) {
-                    save_and_exit_async.begin();
-                } else {
-                    status = CloseStatus.DO_CLOSE;
-                }
-            } else {
-                // Discard
-                discard_and_exit_async.begin();
-            }
-        } else {
-            AlertDialog dialog = new ConfirmationDialog(
-                container.top_window,
-                // Translators: This dialog text is displayed to the
-                // user when closing a composer where the options are
-                // only Discard or Cancel.
-                _("Do you want to discard this draft message?"),
-                null,
-                Stock._DISCARD,
-                "destructive-action"
-            );
-            Gtk.ResponseType response = dialog.run();
-            if (response == Gtk.ResponseType.OK) {
-                discard_and_exit_async.begin();
-            } else {
-                status = CloseStatus.CANCEL_CLOSE;
-            }
-        }
-
-        return status;
-    }
-
     public override bool key_press_event(Gdk.EventKey event) {
         // Override the method since key-press-event is run last, and
         // we want this behaviour to take precedence over the default
@@ -1618,12 +1636,14 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
     }
 
     private async void save_and_exit_async() {
+        this.is_closing = true;
         set_enabled(false);
         yield save_draft();
         yield close();
     }
 
     private async void discard_and_exit_async() {
+        this.is_closing = true;
         set_enabled(false);
         if (this.draft_manager != null) {
             discard_draft();
@@ -2662,9 +2682,7 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
     }
 
     private void on_close(SimpleAction action, Variant? param) {
-        if (should_close() == CloseStatus.DO_CLOSE) {
-            this.close.begin();
-        }
+        confirm_close();
     }
 
     private void on_close_and_save(SimpleAction action, Variant? param) {
diff --git a/src/client/composer/composer-window.vala b/src/client/composer/composer-window.vala
index cd4deee8..da2fd72c 100644
--- a/src/client/composer/composer-window.vala
+++ b/src/client/composer/composer-window.vala
@@ -116,8 +116,12 @@ public class Composer.Window : Gtk.ApplicationWindow, Container {
 
     public override bool delete_event(Gdk.EventAny event) {
         bool ret = Gdk.EVENT_PROPAGATE;
-        Widget? composer = get_child() as Widget;
-        if (composer != null && composer.should_close() == CANCEL_CLOSE) {
+        // Use the child instead of the `composer` property so we don't check
+        // with the composer if it has already been removed from the
+        // container.
+        Widget? child = get_child() as Widget;
+        if (child != null &&
+            child.confirm_close() == CANCELLED) {
             ret = Gdk.EVENT_STOP;
         }
         return ret;