Skip to content

Commit

Permalink
Bug 797900 - Crash caused by Quitting while Check and Repair All is r…
Browse files Browse the repository at this point in the history 
…unning

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.
  • Loading branch information
@jeanlaroche
jeanlaroche committed Aug 27, 2020
1 parent a50c188 commit bbdd4f3
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 8 deletions.
5 changes: 5 additions & 0 deletions gnucash/gnome-utils/gnc-main-window.c
View file
Expand Up @@ -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;
}

Expand Down
32 changes: 29 additions & 3 deletions gnucash/gnome/gnc-plugin-page-account-tree.c
View file
Expand Up @@ -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?");

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Sep 6, 2020

Contributor

They need the gnc_verify_dialog to select the parent window properly, although I am not entirely sure why:

window = GTK_WINDOW (gnc_plugin_page_get_window (GNC_PLUGIN_PAGE (page)))

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Sep 6, 2020

Member

Who's "they"?
You're correct about the parent window. It should be set so that the dialog appears over it instead of in the top left corner of the left-most monitor.

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Sep 6, 2020

Contributor

"They" means @Gnucash/gnucash-developers of course :)

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Sep 6, 2020

Member

Which includes you, so "we" would be more appropriate. ;-)
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)
{
Expand All @@ -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",
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
26 changes: 26 additions & 0 deletions gnucash/gnome/gnc-plugin-page-register.c
View file
Expand Up @@ -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)
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand All @@ -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 (" ");
}

Expand Down
44 changes: 39 additions & 5 deletions libgnucash/engine/Scrub.c
View file
Expand Up @@ -57,17 +57,32 @@
#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;
}

/* ================================================================ */

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
Expand All @@ -83,6 +98,7 @@ TransScrubOrphansFast (Transaction *trans, Account *root)
{
Split *split = node->data;
Account *orph;
if (abort_now) break;

if (split->acc) continue;

Expand Down Expand Up @@ -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)";
Expand All @@ -120,19 +137,20 @@ 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),
gnc_account_get_root (acc));
current_split++;
}
(percentagefunc)(NULL, -1.0);
scrub_depth--;
}


Expand All @@ -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)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)";
Expand All @@ -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);
Expand All @@ -340,6 +367,7 @@ xaccAccountScrubImbalance (Account *acc, QofPercentageFunc percentagefunc)
curr_split_no++;
}
(percentagefunc)(NULL, -1.0);
scrub_depth--;
}

static Split *
Expand Down Expand Up @@ -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--;
}

/* ================================================================ */
Expand Down Expand Up @@ -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--;
}

/* ================================================================ */
Expand All @@ -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))
Expand All @@ -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--;
}

/* ================================================================ */
Expand Down
10 changes: 10 additions & 0 deletions libgnucash/engine/Scrub.h
View file
Expand Up @@ -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.
*/
Expand Down

0 comments on commit bbdd4f3

Please sign in to comment.