[geary/mjog/493-undo-send] Clean up composer handling on application quit
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/mjog/493-undo-send] Clean up composer handling on application quit
- Date: Sun, 17 Nov 2019 00:51:07 +0000 (UTC)
commit 0773e516033f8bca6f68bf63f8d84c34816c13b3
Author: Michael Gratton <mike vee net>
Date: Sun Nov 17 11:46:31 2019 +1100
Clean up composer handling on application quit
Rather than a convoluted systems of signals and callbacks to be able to
prompt if unsaved composers should be closed on application quit, just
do so directly from GearyApplication::quit, and close any still-open
composers on controller shutdown.
This requires some more complicated state handling in the composer, but
allows greatly simplifiying the processes to both quit the application
and close th controller.
src/client/application/application-controller.vala | 126 +++++++++------------
src/client/application/geary-application.vala | 115 +++++++------------
src/client/components/main-window.vala | 2 +-
src/client/composer/composer-widget.vala | 88 +++++++++-----
4 files changed, 153 insertions(+), 178 deletions(-)
---
diff --git a/src/client/application/application-controller.vala
b/src/client/application/application-controller.vala
index 4c30b06a..c483fb31 100644
--- a/src/client/application/application-controller.vala
+++ b/src/client/application/application-controller.vala
@@ -164,14 +164,10 @@ public class Application.Controller : Geary.BaseObject {
private Cancellable cancellable_open_account = new Cancellable();
- // Currently open composers
+ // List composers that have not yet been closed
private Gee.Collection<Composer.Widget> composer_widgets =
new Gee.LinkedList<Composer.Widget>();
- // Composers that are in the process of closing
- private Gee.Collection<Composer.Widget> waiting_to_close =
- new Gee.LinkedList<Composer.Widget>();
-
// Requested mailto composers not yet fullfulled
private Gee.List<string?> pending_mailtos = new Gee.ArrayList<string>();
@@ -191,9 +187,6 @@ public class Application.Controller : Geary.BaseObject {
// custom icons)
IconFactory.instance.init();
- // Listen for attempts to close the application.
- this.application.exiting.connect(on_application_exiting);
-
// Create DB upgrade dialog.
this.upgrade_dialog = new UpgradeDialog();
this.upgrade_dialog.notify[UpgradeDialog.PROP_VISIBLE_NAME].connect(
@@ -318,13 +311,49 @@ public class Application.Controller : Geary.BaseObject {
return this.accounts.get(account);
}
- /** Closes all accounts and windows, releasing held resources. */
- public async void close_async() {
- // Cancel internal processes early so they don't block
- // shutdown
- this.open_cancellable.cancel();
+ /** Closes all windows and accounts, releasing held resources. */
+ public async void close() {
+ this.application.engine.account_available.disconnect(
+ on_account_available
+ );
+
+ this.main_window.set_sensitive(false);
- this.application.engine.account_available.disconnect(on_account_available);
+ // Close any open composers up-front before anything else is
+ // shut down so any pending operations have a chance to
+ // complete.
+ //
+ // Close them in parallel to minimise time taken for this to
+ // complete, but use a barrier to wait for them all to
+ // actually finish closing.
+ var composer_barrier = new Geary.Nonblocking.CountingSemaphore(null);
+
+ // 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);
+ foreach (var composer in composers) {
+ if (composer.current_mode != CLOSED) {
+ composer_barrier.acquire();
+ composer.close.begin(
+ (obj, res) => {
+ composer.close.end(res);
+ composer_barrier.blind_notify();
+ }
+ );
+ }
+ }
+
+ try {
+ yield composer_barrier.wait_async();
+ } catch (GLib.Error err) {
+ debug("Error waiting at composer barrier: %s", err.message);
+ }
+
+ // Now that all composers are closed, we can shut down the
+ // rest of the client and engine. Cancel internal processes
+ // first so they don't block shutdown.
+ this.open_cancellable.cancel();
// Release folder and conversations in the main window
yield this.main_window.select_folder(null, false);
@@ -394,7 +423,6 @@ public class Application.Controller : Geary.BaseObject {
this.pending_mailtos.clear();
this.composer_widgets.clear();
- this.waiting_to_close.clear();
this.avatars.close();
@@ -1481,15 +1509,6 @@ public class Application.Controller : Geary.BaseObject {
}
}
- // We need to include the second parameter, or valac doesn't recognize the function as matching
- // GearyApplication.exiting's signature.
- private bool on_application_exiting(GearyApplication sender, bool panicked) {
- if (close_composition_windows())
- return true;
-
- return sender.cancel_exit();
- }
-
private bool should_notify_new_messages(Geary.Folder folder) {
// A monitored folder must be selected to squelch notifications;
// if conversation list is at top of display, don't display
@@ -1536,47 +1555,15 @@ public class Application.Controller : Geary.BaseObject {
composer.set_focus();
}
- 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;
-
- 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.conditional_close(true, true)) {
- case Composer.Widget.CloseStatus.PENDING:
- this.waiting_to_close.add(composer);
- break;
-
- case Composer.Widget.CloseStatus.CANCELLED:
- quit_cancelled = true;
- break;
- }
+ internal bool check_open_composers() {
+ var do_quit = true;
+ foreach (var composer in this.composer_widgets) {
+ if (composer.conditional_close(true, true) == CANCELLED) {
+ do_quit = false;
+ break;
}
}
-
- // If we cancelled the quit we can bail here.
- if (quit_cancelled) {
- this.waiting_to_close.clear();
- return false;
- }
-
- // 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.
- return true;
+ return do_quit;
}
/**
@@ -1700,14 +1687,11 @@ public class Application.Controller : Geary.BaseObject {
}
private void on_composer_widget_destroy(Gtk.Widget sender) {
- composer_widgets.remove((Composer.Widget) sender);
- debug(@"Destroying composer of type $(((Composer.Widget) sender).compose_type); "
- + @"$(composer_widgets.size) composers remaining");
-
- if (waiting_to_close.remove((Composer.Widget) sender)) {
- // If we just removed the last window in the waiting to close list, it's time to exit!
- if (waiting_to_close.size == 0)
- this.application.exit();
+ Composer.Widget? composer = sender as Composer.Widget;
+ if (composer != null) {
+ composer_widgets.remove((Composer.Widget) sender);
+ debug(@"Composer type $(composer.compose_type) destroyed; " +
+ @"$(this.composer_widgets.size) composers remaining");
}
}
diff --git a/src/client/application/geary-application.vala b/src/client/application/geary-application.vala
index 0baae273..53db7dc5 100644
--- a/src/client/application/geary-application.vala
+++ b/src/client/application/geary-application.vala
@@ -245,9 +245,6 @@ public class GearyApplication : Gtk.Application {
private File exec_dir;
private string binary;
private bool start_hidden = false;
- private bool exiting_fired = false;
- private int exitcode = 0;
- private bool is_destroyed = false;
private GLib.Cancellable controller_cancellable = new GLib.Cancellable();
private Components.Inspector? inspector = null;
private Geary.Nonblocking.Mutex controller_mutex = new Geary.Nonblocking.Mutex();
@@ -340,16 +337,6 @@ public class GearyApplication : Gtk.Application {
return info;
}
- /**
- * Signal that is activated when 'exit' is called, but before the application actually exits.
- *
- * To cancel an exit, a callback should return GearyApplication.cancel_exit(). To proceed with
- * an exit, a callback should return true.
- */
- public virtual signal bool exiting(bool panicked) {
- return true;
- }
-
public GearyApplication() {
Object(
@@ -455,6 +442,12 @@ public class GearyApplication : Gtk.Application {
return -1;
}
+ public override void shutdown() {
+ Util.Date.terminate();
+ Geary.Logging.clear();
+ base.shutdown();
+ }
+
public override void activate() {
base.activate();
this.present.begin();
@@ -659,67 +652,40 @@ public class GearyApplication : Gtk.Application {
}
}
- // This call will fire "exiting" only if it's not already been fired.
- public void exit(int exitcode = 0) {
- if (this.exiting_fired)
- return;
-
- this.exitcode = exitcode;
-
- exiting_fired = true;
- if (!exiting(false)) {
- exiting_fired = false;
- this.exitcode = 0;
-
- return;
- }
-
- this.controller_cancellable.cancel();
-
- // Give asynchronous destroy_controller() a chance to
- // complete, but to avoid bug(s) where Geary hangs at exit,
- // shut the whole thing down if destroy_controller() takes too
- // long to complete
- int64 start_usec = get_monotonic_time();
- destroy_controller.begin();
- while (!is_destroyed || Gtk.events_pending()) {
- Gtk.main_iteration();
-
- int64 delta_usec = get_monotonic_time() - start_usec;
- if (delta_usec >= FORCE_SHUTDOWN_USEC) {
- debug("Forcing shutdown of Geary, %ss passed...", (delta_usec / USEC_PER_SEC).to_string());
- Posix.exit(2);
- }
- }
-
- quit();
-
- Geary.Logging.clear();
- Util.Date.terminate();
- }
-
- /**
- * A callback for GearyApplication.exiting should return
- * cancel_exit() to prevent the application from exiting.
- */
- public bool cancel_exit() {
- Signal.stop_emission_by_name(this, "exiting");
- return false;
- }
-
/**
- * Causes the application to exit immediately.
+ * Closes the controller and all open windows, exiting if possible.
*
- * This call will fire "exiting" only if it's not already been
- * fired and halt the application in its tracks
+ * Any open composers with unsaved or un-savable changes will be
+ * prompted about and if cancelled, will cancel shut-down here.
*/
- public void panic() {
- if (!exiting_fired) {
- exiting_fired = true;
- exiting(true);
- }
+ public new void quit() {
+ if (this.controller == null ||
+ this.controller.check_open_composers()) {
+
+ bool controller_closed = false;
+ this.destroy_controller.begin((obj, res) => {
+ this.destroy_controller.end(res);
+ controller_closed = true;
+ });
+
+ // Give asynchronous destroy_controller() a chance to
+ // complete, but to avoid bug(s) where Geary hangs at exit,
+ // shut the whole thing down if destroy_controller() takes too
+ // long to complete
+ int64 start_usec = get_monotonic_time();
+ while (!controller_closed) {
+ Gtk.main_iteration();
+
+ int64 delta_usec = get_monotonic_time() - start_usec;
+ if (delta_usec >= FORCE_SHUTDOWN_USEC) {
+ debug("Forcing shutdown of Geary, %ss passed...",
+ (delta_usec / USEC_PER_SEC).to_string());
+ Posix.exit(2);
+ }
+ }
- Posix.exit(1);
+ base.quit();
+ }
}
/**
@@ -804,22 +770,21 @@ public class GearyApplication : Gtk.Application {
try {
int mutex_token = yield this.controller_mutex.claim_async();
if (this.controller != null) {
- yield this.controller.close_async();
+ yield this.controller.close();
this.controller = null;
}
this.controller_mutex.release(ref mutex_token);
- } catch (Error err) {
+ } catch (GLib.Error err) {
debug("Error destroying controller: %s", err.message);
}
release();
- this.is_destroyed = true;
}
private int handle_general_options(GLib.ApplicationCommandLine command_line) {
GLib.VariantDict options = command_line.get_options_dict();
if (options.contains(OPTION_QUIT)) {
- exit();
+ quit();
return 0;
}
@@ -966,7 +931,7 @@ public class GearyApplication : Gtk.Application {
}
private void on_activate_quit() {
- exit();
+ quit();
}
private void on_activate_show_email(GLib.SimpleAction action,
diff --git a/src/client/components/main-window.vala b/src/client/components/main-window.vala
index 50bbfb70..02e5936d 100644
--- a/src/client/components/main-window.vala
+++ b/src/client/components/main-window.vala
@@ -1603,7 +1603,7 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
}
} else {
if (close_composer(true, false)) {
- this.application.exit();
+ this.application.quit();
}
}
return Gdk.EVENT_STOP;
diff --git a/src/client/composer/composer-widget.vala b/src/client/composer/composer-widget.vala
index ebb137d0..ba4e9cd4 100644
--- a/src/client/composer/composer-widget.vala
+++ b/src/client/composer/composer-widget.vala
@@ -34,19 +34,27 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
/**
* Determines the result of prompting whether to close the composer.
*
- * @see confirm_close
+ * @see conditional_close
*/
public enum CloseStatus {
- /** The composer is being closed. */
- PENDING,
+ /** The composer is already closed. */
+ CLOSED,
+
+ /** The composer is ready to be closed, but is not yet. */
+ READY,
/** Closing the composer was not confirmed by a human. */
CANCELLED;
+
}
/** Defines different supported user interface modes. */
public enum PresentationMode {
+
+ /** Composer has been closed. */
+ CLOSED,
+
/** Composer is not currently visible. */
NONE,
@@ -76,7 +84,8 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
*
* @see Embed
*/
- INLINE_COMPACT
+ INLINE_COMPACT;
+
}
private enum AttachPending { ALL, INLINE_ONLY }
@@ -447,9 +456,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
}
private bool _can_delete_quote = false;
- // Is the composer closing (e.g. saving a draft or sending)?
- private bool is_closing = false;
-
private Container? container {
get { return this.parent as Container; }
}
@@ -740,17 +746,32 @@ 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.
+ * If the composer is already closed no action is taken. If the
+ * composer is blank then this method will call {@link exit},
+ * destroying the composer, else the composer will either be saved
+ * or discarded as needed then closed.
+ *
+ * The return value specifies whether the composer is being closed
+ * or if the prompt was cancelled by a human.
*/
public CloseStatus conditional_close(bool should_prompt,
bool is_shutdown = false) {
- CloseStatus status = PENDING;
+ CloseStatus status = CLOSED;
+ switch (this.current_mode) {
+ case PresentationMode.CLOSED:
+ // no-op
+ break;
- if (!this.is_closing) {
+ case PresentationMode.NONE:
+ status = READY;
+ break;
+
+ default:
if (this.is_blank) {
this.close.begin();
+ // This may be a bit of a lie but will very soon
+ // become true.
+ status = CLOSED;
} else if (should_prompt) {
present();
if (this.can_save) {
@@ -802,30 +823,39 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
} else {
this.discard_and_close.begin();
}
+ break;
}
return status;
}
- /** Closes the composer unconditionally. */
+ /**
+ * Closes the composer and any drafts unconditionally.
+ *
+ * This method disables the composer, closes the draft manager,
+ * then destroys the composer itself.
+ */
public async void close() {
- this.is_closing = true;
- set_enabled(false);
-
- if (this.draft_manager_opening != null) {
- this.draft_manager_opening.cancel();
- this.draft_manager_opening = null;
- }
+ if (this.current_mode != CLOSED) {
+ // this will set current_mode to NONE first
+ set_enabled(false);
+ this.current_mode = CLOSED;
+
+ if (this.draft_manager_opening != null) {
+ this.draft_manager_opening.cancel();
+ this.draft_manager_opening = null;
+ }
- if (this.draft_manager != null) {
- try {
- yield close_draft_manager(null);
- } catch (Error err) {
- debug("Error closing draft manager on composer close");
+ if (this.draft_manager != null) {
+ try {
+ yield close_draft_manager(null);
+ } catch (Error err) {
+ debug("Error closing draft manager on composer close");
+ }
}
- }
- destroy();
+ destroy();
+ }
}
public override void destroy() {
@@ -858,7 +888,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
this.header.set_sensitive(enabled);
if (enabled) {
- this.is_closing = false;
this.open_draft_manager.begin(this.current_draft_id, null);
} else {
if (this.container != null) {
@@ -1436,7 +1465,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
// Used internally by on_send()
private async void on_send_async() {
- this.is_closing = true;
set_enabled(false);
try {
@@ -1604,7 +1632,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
}
private async void save_and_close() {
- this.is_closing = true;
set_enabled(false);
if (this.should_save) {
@@ -1628,7 +1655,6 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface {
}
private async void discard_and_close() {
- this.is_closing = true;
set_enabled(false);
if (this.draft_manager != null) {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
