diff --git a/gnucash/import-export/import-backend.c b/gnucash/import-export/import-backend.c index 6ffdcf258a..a7ca8b9585 100644 --- a/gnucash/import-export/import-backend.c +++ b/gnucash/import-export/import-backend.c @@ -623,10 +623,6 @@ void split_find_match (GNCImportTransInfo * trans_info, Transaction *new_trans = gnc_import_TransInfo_get_trans (trans_info); Split *new_trans_fsplit = gnc_import_TransInfo_get_fsplit (trans_info); - // Do not consider transactions that have been previously matched. - if (gnc_import_split_has_online_id (split)) // JEAN THIS CAN NOW BE REMOVED. - return; - /* Matching heuristics */ /* Amount heuristics */ @@ -1081,7 +1077,7 @@ static gint check_trans_online_id(Transaction *trans1, void *user_data) } } -static gint collect_trans_online_id(Transaction *trans, void *user_data) //JEAN COLLECT +static gint collect_trans_online_id(Transaction *trans, void *user_data) { Split *split; GHashTable *id_hash = user_data; @@ -1101,7 +1097,7 @@ static gint collect_trans_online_id(Transaction *trans, void *user_data) //JEAN /** Checks whether the given transaction's online_id already exists in its parent account. */ -gboolean gnc_import_exists_online_id (Transaction *trans, GHashTable* id_hash) +gboolean gnc_import_exists_online_id (Transaction *trans, GHashTable* acct_id_hash) { gboolean online_id_exists = FALSE; Account *dest_acct; @@ -1113,15 +1109,13 @@ gboolean gnc_import_exists_online_id (Transaction *trans, GHashTable* id_hash) // Create a hash per account of a hash of all transactions IDs. Then the test below will be fast if // we have many transactions to import. dest_acct = xaccSplitGetAccount (source_split); - if (!g_hash_table_contains (id_hash, dest_acct)) + if (!g_hash_table_contains (acct_id_hash, dest_acct)) { GHashTable* new_hash = g_hash_table_new (g_str_hash, g_str_equal); - g_hash_table_insert (id_hash, dest_acct, new_hash); - printf ("CREATE HASH\n"); + g_hash_table_insert (acct_id_hash, dest_acct, new_hash); xaccAccountForEachTransaction (dest_acct, collect_trans_online_id, new_hash); - printf ("CREATE DONE\n"); } - online_id_exists = g_hash_table_contains (g_hash_table_lookup (id_hash, dest_acct), + online_id_exists = g_hash_table_contains (g_hash_table_lookup (acct_id_hash, dest_acct), gnc_import_get_split_online_id (source_split)); /* If it does, abort the process for this transaction, since it is diff --git a/gnucash/import-export/import-backend.h b/gnucash/import-export/import-backend.h index 3602476e02..3a78278b1f 100644 --- a/gnucash/import-export/import-backend.h +++ b/gnucash/import-export/import-backend.h @@ -64,7 +64,7 @@ typedef enum _action * * @param trans The transaction for which to check for an existing * online_id. */ -gboolean gnc_import_exists_online_id (Transaction *trans, GHashTable* id_hash); +gboolean gnc_import_exists_online_id (Transaction *trans, GHashTable* acct_id_hash); /** Evaluates the match between trans_info and split using the provided parameters. * diff --git a/gnucash/import-export/import-main-matcher.c b/gnucash/import-export/import-main-matcher.c index 2ef3fa512f..f08a606d5c 100644 --- a/gnucash/import-export/import-main-matcher.c +++ b/gnucash/import-export/import-main-matcher.c @@ -75,7 +75,7 @@ struct _main_matcher_info gboolean add_toggled; // flag to indicate that add has been toggled to stop selection gint id; GSList* temp_trans_list; // Temporary list of imported transactions - GHashTable* id_hash; // Hash table, per account, of list of transaction IDs. + GHashTable* acct_id_hash; // Hash table, per account, of list of transaction IDs. GSList* edited_accounts; // List of accounts currently edited. }; @@ -147,36 +147,29 @@ static void refresh_model_row ( static gboolean delete_hash (gpointer key, gpointer value, gpointer user_data) { - // Value is a hash table that needs to be destroyed. JEAN: I should free the memory used for the keys. + // Value is a hash table that needs to be destroyed. g_hash_table_destroy (value); return TRUE; } static void -commit_all (GNCImportMainMatcher *info) +update_all_balances (GNCImportMainMatcher *info) { - // Commit the edits for all accounts for which we've called inc_account_edit. for (GSList* iter = info->edited_accounts; iter; iter=iter->next) { - int level = qof_instance_get_editlevel (iter->data); - // This should not happen. - if (level != 1) - PERR ("import-main-macher.c: Edit level not 1"); - while (qof_instance_get_editlevel (iter->data) > 0) - xaccAccountCommitEdit (iter->data); + gnc_account_set_defer_bal_computation(iter->data,FALSE); + xaccAccountRecomputeBalance(iter->data); } g_slist_free (info->edited_accounts); info->edited_accounts = NULL; } static void -inc_account_edit (GNCImportMainMatcher *info, Account* acc) +defer_bal_computation (GNCImportMainMatcher *info, Account* acc) { - // Call BeginEdit on accounts that haven't been edited yet. This will speeds things up - // a lot by preventing commits until we're all done. - if (qof_instance_get_editlevel (acc) == 0) + if (!gnc_account_get_defer_bal_computation(acc)) { - xaccAccountBeginEdit (acc); + gnc_account_set_defer_bal_computation (acc, TRUE); info->edited_accounts = g_slist_prepend (info->edited_accounts, acc); } } @@ -186,7 +179,6 @@ void gnc_gen_trans_list_delete (GNCImportMainMatcher *info) GtkTreeModel *model; GtkTreeIter iter; GNCImportTransInfo *trans_info; - Split* first_split = NULL; if (info == NULL) return; @@ -219,14 +211,14 @@ void gnc_gen_trans_list_delete (GNCImportMainMatcher *info) else gnc_import_Settings_delete (info->user_settings); - // We've called xaccAccountBeginEdit on many accounts to defer the commits until now. - // Let's do it now that we're done. - commit_all (info); - g_slist_free_full (info->temp_trans_list,(GDestroyNotify) gnc_import_TransInfo_delete); info->temp_trans_list = NULL; - g_hash_table_foreach_remove (info->id_hash, delete_hash, NULL); - info->id_hash = NULL; + + // We've deferred balance computations on many accounts. Let's do it now that we're done. + update_all_balances (info); + + g_hash_table_foreach_remove (info->acct_id_hash, delete_hash, NULL); + info->acct_id_hash = NULL; g_free (info); } @@ -269,15 +261,14 @@ on_matcher_ok_clicked (GtkButton *button, GNCImportMainMatcher *info) /* Don't run any queries and/or split sorts while processing the matcher results. */ gnc_suspend_gui_refresh(); - printf("START OK LOOP\n"); do { gtk_tree_model_get (model, &iter, DOWNLOADED_COL_DATA, &trans_info, -1); - // JEAN: the account of all splits should be set as edited. IF THERE'S ONLY 1 SPLIT, A SPLIT - // WILL BE CREATED with unbalanced, and the account will not have its edit level increased... + // Note: if there's only 1 split (unbalanced) one will be created with the unbalanced account, + // and for that account the defer balance will not be set. So things will be slow. if (gnc_import_process_trans_item (NULL, trans_info)) { @@ -289,7 +280,6 @@ on_matcher_ok_clicked (GtkButton *button, GNCImportMainMatcher *info) } } while (gtk_tree_model_iter_next (model, &iter)); - printf("DONE OK LOOP\n"); gnc_gen_trans_list_delete (info); @@ -392,8 +382,8 @@ run_account_picker_dialog (GNCImportMainMatcher *info, &ok_pressed); if (ok_pressed) { - gnc_import_TransInfo_set_destacc (trans_info, new_acc, TRUE); // JEAN: BeginEdit on this account - inc_account_edit (info, new_acc); + gnc_import_TransInfo_set_destacc (trans_info, new_acc, TRUE); + defer_bal_computation (info, new_acc); } } @@ -547,8 +537,8 @@ gnc_gen_trans_assign_transfer_account (GtkTreeView *treeview, } if (ok_pressed) { - gnc_import_TransInfo_set_destacc (trans_info, *new_acc, TRUE); // JEAN Set Begin_edit on this account. - inc_account_edit (info, *new_acc); + gnc_import_TransInfo_set_destacc (trans_info, *new_acc, TRUE); + defer_bal_computation (info, *new_acc); } } break; @@ -623,7 +613,7 @@ gnc_gen_trans_assign_transfer_account_to_selection_cb ( } g_list_free_full (selected_rows, (GDestroyNotify) gtk_tree_path_free); - // now reselect the transaction rows. // JEAN THIS GETS REALLY SLOW AS WELL + // now reselect the transaction rows. This is very slow if there are lots of transactions. for (l = refs; l != NULL; l = l->next) { GtkTreePath *path = gtk_tree_row_reference_get_path (l->data); @@ -932,7 +922,7 @@ gnc_gen_trans_init_view (GNCImportMainMatcher *info, g_signal_connect (view, "popup-menu", G_CALLBACK(gnc_gen_trans_onPopupMenu_cb), info); - info->id_hash = g_hash_table_new (g_direct_hash, g_direct_equal); + info->acct_id_hash = g_hash_table_new (g_direct_hash, g_direct_equal); } static void @@ -1480,11 +1470,9 @@ void gnc_gen_trans_list_add_trans (GNCImportMainMatcher *gui, Transaction *trans Split* split = NULL; int i=0; - // JEAN call xaccAccountBeginEdit on trans_info->first_split->acc, but only once. - // This makes the deletion a lot faster because the commit is only done once at the end. split = xaccTransGetSplit (trans, 0); acc = xaccSplitGetAccount (split); - inc_account_edit (gui, acc); + defer_bal_computation (gui, acc); gnc_gen_trans_list_add_trans_with_ref_id (gui, trans, 0); return; @@ -1513,7 +1501,7 @@ void gnc_gen_trans_list_add_trans_with_ref_id (GNCImportMainMatcher *gui, Transa g_assert (gui); g_assert (trans); - if (gnc_import_exists_online_id (trans, gui->id_hash)) + if (gnc_import_exists_online_id (trans, gui->acct_id_hash)) return; else { @@ -1538,7 +1526,7 @@ create_list_of_potential_matches (GNCImportMainMatcher *gui, GtkTreeModel* model static const int secs_per_day = 86400; GSList* imported_trans; GList* potential_match; - GHashTable* lists_per_accounts = g_hash_table_new (g_direct_hash, g_direct_equal); + GHashTable* lists_per_accounts = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, (GDestroyNotify)g_slist_free); // Go through all imported transactions, gather the list of accounts, and min/max date range. for (imported_trans=gui->temp_trans_list; imported_trans!=NULL; imported_trans=imported_trans->next) @@ -1571,21 +1559,15 @@ create_list_of_potential_matches (GNCImportMainMatcher *gui, GtkTreeModel* model if (gnc_import_split_has_online_id (potential_match->data)) continue; split_account = xaccSplitGetAccount (potential_match->data); - // This will be NULL the first time around, which is fine for a GSList - per_account_list = g_hash_table_lookup (lists_per_accounts, split_account); + // g_hash_table_steal_extended would do the two calls in one shot but is not available until 2.58 + per_account_list = g_hash_table_lookup (lists_per_accounts, import_trans_account); + g_hash_table_steal (lists_per_accounts, import_trans_account); per_account_list = g_slist_prepend (per_account_list, potential_match->data); - g_hash_table_replace (lists_per_accounts, import_trans_account, per_account_list); + g_hash_table_insert (lists_per_accounts, import_trans_account, per_account_list); } return lists_per_accounts; } -static gboolean -destroy_helper (gpointer key, gpointer value, gpointer data) -{ - g_slist_free (value); - return TRUE; -} - typedef struct _match_struct { GNCImportTransInfo* transaction_info; @@ -1620,11 +1602,9 @@ gnc_gen_trans_list_create_matches (GNCImportMainMatcher *gui) GSList* imported_trans; query = qof_query_create_for (GNC_ID_SPLIT); - printf("CREATE LIST\n"); // Gather the list of splits that could match one of the imported transactions, this return a hash table. lists_per_accounts = create_list_of_potential_matches (gui, model, query, match_date_hardlimit); - printf("START LOOP\n"); // For each imported transaction, grab the list of potential matches corresponding to that account, // and evaluate how good a match they are. for (imported_trans=gui->temp_trans_list; imported_trans!=NULL; imported_trans=imported_trans->next) @@ -1649,10 +1629,8 @@ gnc_gen_trans_list_create_matches (GNCImportMainMatcher *gui) gtk_tree_store_append (GTK_TREE_STORE (model), &iter, NULL); refresh_model_row (gui, model, &iter, transaction_info); } - printf("END LOOP LIST\n"); qof_query_destroy (query); - g_hash_table_foreach (lists_per_accounts, (GHFunc) destroy_helper, NULL); g_hash_table_destroy (lists_per_accounts); return; } diff --git a/gnucash/import-export/test/gtest-import-backend.cpp b/gnucash/import-export/test/gtest-import-backend.cpp index eedcfba924..345f46eb2e 100644 --- a/gnucash/import-export/test/gtest-import-backend.cpp +++ b/gnucash/import-export/test/gtest-import-backend.cpp @@ -120,6 +120,8 @@ protected: m_dest_acc = new MockAccount(); m_trans = new MockTransaction(); m_split = new MockSplit(); + m_splitList = NULL; + m_splitList = g_list_prepend (m_splitList, m_split); using namespace testing; @@ -141,6 +143,7 @@ protected: MockAccount* m_dest_acc; MockTransaction* m_trans; MockSplit* m_split; + GList* m_splitList; }; @@ -160,6 +163,8 @@ TEST_F(ImportBackendTest, CreateTransInfo) // Define first split ON_CALL(*m_trans, getSplit(0)) .WillByDefault(Return(m_split)); + ON_CALL(*m_trans, getSplitList()) + .WillByDefault(Return(m_splitList)); // define description of the transaction ON_CALL(*m_trans, getDescription()) .WillByDefault(Return("This is the description")); @@ -228,6 +233,8 @@ TEST_F(ImportBackendBayesTest, CreateTransInfo) // Define first split ON_CALL(*m_trans, getSplit(0)) .WillByDefault(Return(m_split)); + ON_CALL(*m_trans, getSplitList()) + .WillByDefault(Return(m_splitList)); // Transaction has no further splits ON_CALL(*m_trans, getSplit(Gt(0))) .WillByDefault(Return(nullptr)); diff --git a/libgnucash/engine/Account.cpp b/libgnucash/engine/Account.cpp index 0cc63df107..fe49b2404d 100644 --- a/libgnucash/engine/Account.cpp +++ b/libgnucash/engine/Account.cpp @@ -1859,6 +1859,29 @@ gnc_account_set_balance_dirty (Account *acc) priv->balance_dirty = TRUE; } +void gnc_account_set_defer_bal_computation (Account *acc, gboolean defer) +{ + AccountPrivate *priv; + + g_return_if_fail (GNC_IS_ACCOUNT (acc)); + + if (qof_instance_get_destroying (acc)) + return; + + priv = GET_PRIVATE (acc); + priv->defer_bal_computation = defer; +} + +gboolean gnc_account_get_defer_bal_computation (Account *acc) +{ + AccountPrivate *priv; + if (!acc) + return false; + priv = GET_PRIVATE (acc); + return priv->defer_bal_computation; +} + + /********************************************************************\ \********************************************************************/ @@ -2214,7 +2237,7 @@ xaccAccountRecomputeBalance (Account * acc) priv = GET_PRIVATE(acc); if (qof_instance_get_editlevel(acc) > 0) return; - if (!priv->balance_dirty) return; + if (!priv->balance_dirty || priv->defer_bal_computation) return; if (qof_instance_get_destroying(acc)) return; if (qof_book_shutting_down(qof_instance_get_book(acc))) return; diff --git a/libgnucash/engine/Account.h b/libgnucash/engine/Account.h index e23401d999..8fb4bfa105 100644 --- a/libgnucash/engine/Account.h +++ b/libgnucash/engine/Account.h @@ -360,7 +360,18 @@ void gnc_account_set_balance_dirty (Account *acc); * * @param acc Set the flag on this account. */ void gnc_account_set_sort_dirty (Account *acc); - + +/** Set the defer balance flag. If defer is true, the account balance + * is not automatically computed, which can save a lot of time if + * multiple operations have to be done on the same account. If + * defer is false, further operations on account will cause the + * balance to be recomputed as normal. + * + * @param acc Set the flag on this account. + * + * @param defer New value for the flag. */ +void gnc_account_set_defer_bal_computation (Account *acc, gboolean defer); + /** Insert the given split from an account. * * @param acc The account to which the split should be added. @@ -403,6 +414,8 @@ const char * xaccAccountGetNotes (const Account *account); const char * xaccAccountGetLastNum (const Account *account); /** Get the account's lot order policy */ GNCPolicy *gnc_account_get_policy (Account *account); +/** Get the account's flag for deferred balance computation */ +gboolean gnc_account_get_defer_bal_computation (Account *acc); /** The following recompute the partial balances (stored with the * transaction) and the total balance, for this account diff --git a/libgnucash/engine/AccountP.h b/libgnucash/engine/AccountP.h index 995bc4cdc3..ea147aa833 100644 --- a/libgnucash/engine/AccountP.h +++ b/libgnucash/engine/AccountP.h @@ -126,6 +126,7 @@ typedef struct AccountPrivate * in any way desired. Handy for specialty traversals of the * account tree. */ short mark; + gboolean defer_bal_computation; } AccountPrivate; struct account_s diff --git a/libgnucash/engine/mocks/gmock-Transaction.cpp b/libgnucash/engine/mocks/gmock-Transaction.cpp index dd02f6ea80..ae3483cf1f 100644 --- a/libgnucash/engine/mocks/gmock-Transaction.cpp +++ b/libgnucash/engine/mocks/gmock-Transaction.cpp @@ -46,6 +46,13 @@ xaccTransGetSplit (const Transaction *trans, int i) return ((MockTransaction*)trans)->getSplit(i); } +SplitList * +xaccTransGetSplitList (const Transaction *trans) +{ + g_return_val_if_fail(GNC_IS_MOCK_TRANSACTION(trans), NULL); + return trans ? ((MockTransaction*)trans)->getSplitList() : NULL; +} + Split * xaccTransFindSplitByAccount(const Transaction *trans, const Account *acc) { diff --git a/libgnucash/engine/mocks/gmock-Transaction.h b/libgnucash/engine/mocks/gmock-Transaction.h index 0790a7c03f..5cb26a2507 100644 --- a/libgnucash/engine/mocks/gmock-Transaction.h +++ b/libgnucash/engine/mocks/gmock-Transaction.h @@ -52,6 +52,7 @@ public: MOCK_METHOD0(beginEdit, void()); MOCK_METHOD0(commitEdit, void()); MOCK_METHOD1(getSplit, Split *(int)); + MOCK_METHOD0(getSplitList, SplitList *()); MOCK_METHOD1(findSplitByAccount, Split *(const Account*)); MOCK_METHOD0(getDate, time64()); MOCK_METHOD1(setDatePostedSecsNormalized, void(time64));