From bbdd4f34b626a7489129bb1bf26dcfa7fccb2393 Mon Sep 17 00:00:00 2001 From: jean <27791933+jeanlaroche@users.noreply.github.com> Date: Sat, 22 Aug 2020 16:11:17 -0700 Subject: [PATCH 1/2] Bug 797900 - Crash caused by Quitting while Check and Repair All is running The account tree page didn't have a "finish" function normally used to verify a page can close. I added one, along with two flags that indicate whether a scrubbing operation is currently ongoing and whether we should quit when the scrubbing is done. The result is: If a user attempts to quit while scrubbing isn't done, an alert pops up asking whether the user wants to abort the scrub. If so, the scrub is aborted (safely) and GC quits. If not the app does not quit. I have to say, I'm not sure this is the right way to do this. In my view, the right way would be: - Disable the "quit" menu when scrubbing is happening (for some reason gnc_suspend_gui_refresh() does not cause the quit menu to be grayed) so there's no chance of quitting while scrubbing is ongoing - If needed, add an abort scrubbing button to the main window. Not sure whether that's desirable or not. Let me know what you think: is what I have what we need, or would the above be better. --- gnucash/gnome-utils/gnc-main-window.c | 5 +++ gnucash/gnome/gnc-plugin-page-account-tree.c | 32 ++++++++++++-- gnucash/gnome/gnc-plugin-page-register.c | 26 ++++++++++++ libgnucash/engine/Scrub.c | 44 +++++++++++++++++--- libgnucash/engine/Scrub.h | 10 +++++ 5 files changed, 109 insertions(+), 8 deletions(-) diff --git a/gnucash/gnome-utils/gnc-main-window.c b/gnucash/gnome-utils/gnc-main-window.c index 312d121318..f5cd1ab32d 100644 --- a/gnucash/gnome-utils/gnc-main-window.c +++ b/gnucash/gnome-utils/gnc-main-window.c @@ -1146,6 +1146,11 @@ gnc_main_window_all_finish_pending (void) return FALSE; } } + if (gnc_gui_refresh_suspended ()) + { + gnc_warning_dialog (NULL, "%s", "An operation is still pending, wait for it to complete before quitting "); + return FALSE; + } return TRUE; } diff --git a/gnucash/gnome/gnc-plugin-page-account-tree.c b/gnucash/gnome/gnc-plugin-page-account-tree.c index f664066025..1c7554bf22 100644 --- a/gnucash/gnome/gnc-plugin-page-account-tree.c +++ b/gnucash/gnome/gnc-plugin-page-account-tree.c @@ -413,6 +413,30 @@ gnc_plugin_page_account_tree_new (void) G_DEFINE_TYPE_WITH_PRIVATE(GncPluginPageAccountTree, gnc_plugin_page_account_tree, GNC_TYPE_PLUGIN_PAGE) +static void +prepare_scrubbing () +{ + gnc_suspend_gui_refresh (); + gnc_set_abort_scrub (FALSE); +} + +static gboolean +finish (GncPluginPage* page) +{ + if (gnc_get_ongoing_scrub ()) + { + gboolean ret = gnc_verify_dialog (NULL, FALSE, "%s", "A scrubbing operation is currently pending, do you want to abort it?"); + if (ret) + { + gnc_set_abort_scrub (TRUE); + gnc_resume_gui_refresh (); // This is so quit does not complain about an ongoing operation. + return TRUE; + } + return FALSE; + } + return TRUE; +} + static void gnc_plugin_page_account_tree_class_init (GncPluginPageAccountTreeClass *klass) { @@ -430,6 +454,8 @@ gnc_plugin_page_account_tree_class_init (GncPluginPageAccountTreeClass *klass) gnc_plugin_class->save_page = gnc_plugin_page_account_tree_save_page; gnc_plugin_class->recreate_page = gnc_plugin_page_account_tree_recreate_page; gnc_plugin_class->focus_page_function = gnc_plugin_page_account_tree_focus_widget; + gnc_plugin_class->finish_pending = finish; + plugin_page_signals[ACCOUNT_SELECTED] = g_signal_new ("account_selected", @@ -1913,7 +1939,7 @@ gnc_plugin_page_account_tree_cmd_scrub (GtkAction *action, GncPluginPageAccountT g_return_if_fail (account != NULL); - gnc_suspend_gui_refresh (); + prepare_scrubbing (); window = GNC_WINDOW(GNC_PLUGIN_PAGE (page)->window); gnc_window_set_progressbar_window (window); @@ -1939,7 +1965,7 @@ gnc_plugin_page_account_tree_cmd_scrub_sub (GtkAction *action, GncPluginPageAcco g_return_if_fail (account != NULL); - gnc_suspend_gui_refresh (); + prepare_scrubbing (); window = GNC_WINDOW(GNC_PLUGIN_PAGE (page)->window); gnc_window_set_progressbar_window (window); @@ -1962,7 +1988,7 @@ gnc_plugin_page_account_tree_cmd_scrub_all (GtkAction *action, GncPluginPageAcco Account *root = gnc_get_current_root_account (); GncWindow *window; - gnc_suspend_gui_refresh (); + prepare_scrubbing (); window = GNC_WINDOW(GNC_PLUGIN_PAGE (page)->window); gnc_window_set_progressbar_window (window); diff --git a/gnucash/gnome/gnc-plugin-page-register.c b/gnucash/gnome/gnc-plugin-page-register.c index f8d4034b1e..8720b6ce5b 100644 --- a/gnucash/gnome/gnc-plugin-page-register.c +++ b/gnucash/gnome/gnc-plugin-page-register.c @@ -1905,6 +1905,25 @@ gnc_plugin_page_register_update_edit_menu (GncPluginPage* page, gboolean hide) gtk_action_set_visible (action, !hide || can_paste); } +static gboolean abort_scrub = FALSE; +static gboolean is_scrubbing = FALSE; + +static gboolean +finish_scrub (GncPluginPage* page) +{ + if (is_scrubbing) + { + gboolean ret = gnc_verify_dialog (NULL, FALSE, "%s", "A scrubbing operation is currently pending, do you want to abort it?"); + if (ret) + { + abort_scrub = TRUE; + gnc_resume_gui_refresh (); // This is so quit does not complain about an ongoing operation. + return TRUE; + } + return FALSE; + } + return TRUE; +} static gboolean gnc_plugin_page_register_finish_pending (GncPluginPage* page) @@ -1916,6 +1935,9 @@ gnc_plugin_page_register_finish_pending (GncPluginPage* page) const gchar* name; gint response; + if (!finish_scrub (page)) + return FALSE; + reg_page = GNC_PLUGIN_PAGE_REGISTER (page); priv = GNC_PLUGIN_PAGE_REGISTER_GET_PRIVATE (reg_page); reg = gnc_ledger_display_get_split_register (priv->ledger); @@ -4933,6 +4955,7 @@ gnc_plugin_page_register_cmd_scrub_all (GtkAction* action, } gnc_suspend_gui_refresh(); + is_scrubbing = TRUE; window = GNC_WINDOW (GNC_PLUGIN_PAGE (plugin_page)->window); gnc_window_set_progressbar_window (window); @@ -4956,6 +4979,8 @@ gnc_plugin_page_register_cmd_scrub_all (GtkAction* action, char* progress_msg = g_strdup_printf (message, curr_split_no, split_count); gnc_window_show_progress (progress_msg, (100 * curr_split_no) / split_count); g_free (progress_msg); + if (abort_scrub) + break; } xaccTransScrubOrphans (trans); @@ -4976,6 +5001,7 @@ gnc_plugin_page_register_cmd_scrub_all (GtkAction* action, gnc_window_show_progress (NULL, -1.0); gnc_resume_gui_refresh(); + is_scrubbing = FALSE; LEAVE (" "); } diff --git a/libgnucash/engine/Scrub.c b/libgnucash/engine/Scrub.c index 63ab6d90ab..9efb9e93c7 100644 --- a/libgnucash/engine/Scrub.c +++ b/libgnucash/engine/Scrub.c @@ -57,6 +57,20 @@ #define G_LOG_DOMAIN "gnc.engine.scrub" static QofLogModule log_module = G_LOG_DOMAIN; +static gboolean abort_now = FALSE; +static gint scrub_depth = 0; + +void +gnc_set_abort_scrub (gboolean abort) +{ + abort_now = abort; +} + +gboolean +gnc_get_ongoing_scrub (void) +{ + return scrub_depth > 0; +} /* ================================================================ */ @@ -64,10 +78,11 @@ void xaccAccountTreeScrubOrphans (Account *acc, QofPercentageFunc percentagefunc) { if (!acc) return; - + scrub_depth ++; xaccAccountScrubOrphans (acc, percentagefunc); gnc_account_foreach_descendant(acc, (AccountCb)xaccAccountScrubOrphans, percentagefunc); + scrub_depth--; } static void @@ -83,6 +98,7 @@ TransScrubOrphansFast (Transaction *trans, Account *root) { Split *split = node->data; Account *orph; + if (abort_now) break; if (split->acc) continue; @@ -110,6 +126,7 @@ xaccAccountScrubOrphans (Account *acc, QofPercentageFunc percentagefunc) guint current_split = 0; if (!acc) return; + scrub_depth++; str = xaccAccountGetName (acc); str = str ? str : "(null)"; @@ -120,12 +137,12 @@ xaccAccountScrubOrphans (Account *acc, QofPercentageFunc percentagefunc) for (node = splits; node; node = node->next) { Split *split = node->data; - if (current_split % 100 == 0) { char *progress_msg = g_strdup_printf (message, str, current_split, total_splits); (percentagefunc)(progress_msg, (100 * current_split) / total_splits); g_free (progress_msg); + if (abort_now) break; } TransScrubOrphansFast (xaccSplitGetParent (split), @@ -133,6 +150,7 @@ xaccAccountScrubOrphans (Account *acc, QofPercentageFunc percentagefunc) current_split++; } (percentagefunc)(NULL, -1.0); + scrub_depth--; } @@ -148,6 +166,7 @@ xaccTransScrubOrphans (Transaction *trans) for (node = trans->splits; node; node = node->next) { Split *split = node->data; + if (abort_now) break; if (split->acc) { @@ -183,9 +202,13 @@ void xaccAccountScrubSplits (Account *account) { GList *node; - + scrub_depth++; for (node = xaccAccountGetSplitList (account); node; node = node->next) + { + if (abort_now) break; xaccSplitScrub (node->data); + } + scrub_depth--; } void @@ -291,9 +314,11 @@ xaccSplitScrub (Split *split) void xaccAccountTreeScrubImbalance (Account *acc, QofPercentageFunc percentagefunc) { + scrub_depth++; xaccAccountScrubImbalance (acc, percentagefunc); gnc_account_foreach_descendant(acc, (AccountCb)xaccAccountScrubImbalance, percentagefunc); + scrub_depth--; } void @@ -305,6 +330,7 @@ xaccAccountScrubImbalance (Account *acc, QofPercentageFunc percentagefunc) gint split_count = 0, curr_split_no = 0; if (!acc) return; + scrub_depth++; str = xaccAccountGetName(acc); str = str ? str : "(null)"; @@ -316,6 +342,7 @@ xaccAccountScrubImbalance (Account *acc, QofPercentageFunc percentagefunc) { Split *split = node->data; Transaction *trans = xaccSplitGetParent(split); + if (abort_now) break; PINFO("Start processing split %d of %d", curr_split_no + 1, split_count); @@ -340,6 +367,7 @@ xaccAccountScrubImbalance (Account *acc, QofPercentageFunc percentagefunc) curr_split_no++; } (percentagefunc)(NULL, -1.0); + scrub_depth--; } static Split * @@ -1243,19 +1271,22 @@ scrub_trans_currency_helper (Transaction *t, gpointer data) static void scrub_account_commodity_helper (Account *account, gpointer data) { + scrub_depth++; xaccAccountScrubCommodity (account); xaccAccountDeleteOldData (account); + scrub_depth--; } void xaccAccountTreeScrubCommodities (Account *acc) { if (!acc) return; - + scrub_depth++; xaccAccountTreeForEachTransaction (acc, scrub_trans_currency_helper, NULL); scrub_account_commodity_helper (acc, NULL); gnc_account_foreach_descendant (acc, scrub_account_commodity_helper, NULL); + scrub_depth--; } /* ================================================================ */ @@ -1315,13 +1346,14 @@ xaccAccountTreeScrubQuoteSources (Account *root, gnc_commodity_table *table) LEAVE("Oops"); return; } - + scrub_depth++; gnc_commodity_table_foreach_commodity (table, check_quote_source, &new_style); move_quote_source(root, GINT_TO_POINTER(new_style)); gnc_account_foreach_descendant (root, move_quote_source, GINT_TO_POINTER(new_style)); LEAVE("Migration done"); + scrub_depth--; } /* ================================================================ */ @@ -1333,6 +1365,7 @@ xaccAccountScrubKvp (Account *account) gchar *str2; if (!account) return; + scrub_depth++; qof_instance_get_kvp (QOF_INSTANCE (account), &v, 1, "notes"); if (G_VALUE_HOLDS_STRING (&v)) @@ -1350,6 +1383,7 @@ xaccAccountScrubKvp (Account *account) qof_instance_slot_delete (QOF_INSTANCE (account), "placeholder"); qof_instance_slot_delete_if_empty (QOF_INSTANCE (account), "hbci"); + scrub_depth--; } /* ================================================================ */ diff --git a/libgnucash/engine/Scrub.h b/libgnucash/engine/Scrub.h index 9536f8c34b..bfaea845af 100644 --- a/libgnucash/engine/Scrub.h +++ b/libgnucash/engine/Scrub.h @@ -69,6 +69,16 @@ for orphaned inodes. @{ */ +/** The gnc_set_abort_scrub () method causes a currently running scrub operation + * to stop, if abort is TRUE; gnc_set_abort_scrub(FALSE) must be called before + * any scrubbing operation. + */ +void gnc_set_abort_scrub (gboolean abort); + +/** The gnc_get_ongoing_scrub () method returns TRUE if a scrub operation is ongoing. + */ +gboolean gnc_get_ongoing_scrub (void); + /** The xaccTransScrubOrphans() method scrubs only the splits in the * given transaction. */ From 3d98ba092f147bf738a5f5a746568a3a1941854a Mon Sep 17 00:00:00 2001 From: jean <27791933+jeanlaroche@users.noreply.github.com> Date: Thu, 27 Aug 2020 17:00:32 -0700 Subject: [PATCH 2/2] Fix reversed logic in gnc_quartz_should_quit --- gnucash/gnome-utils/gnc-main-window.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gnucash/gnome-utils/gnc-main-window.c b/gnucash/gnome-utils/gnc-main-window.c index f5cd1ab32d..5892130e67 100644 --- a/gnucash/gnome-utils/gnc-main-window.c +++ b/gnucash/gnome-utils/gnc-main-window.c @@ -3712,7 +3712,7 @@ static gboolean gnc_quartz_should_quit (GtkosxApplication *theApp, GncMainWindow *window) { if (gnc_main_window_all_finish_pending()) - return gnc_main_window_quit (window); + return !gnc_main_window_quit (window); return TRUE; }