From 8a7c53925870501c8f103c8f09961ecba3341c84 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Mon, 27 Feb 2023 14:54:46 -0800 Subject: [PATCH] Bug 798748 - Transaction Notes field's value does not appear in... reverse transaction. The proximate cause was that xaccTransBeginEdit put the KVP cache variable of the new transaction in a state that prevented the value from being copied. More generally the KVP cache variables didn't handle any invalidating events. With the change to GValue usage in qof_instance_get_kvp it's now a simple memory dereference with no copying except for POD types so caching is no longer useful. This commit removes caching from Transaction, eliminating the notes problem. --- libgnucash/engine/Transaction.c | 164 ++++++-------------- libgnucash/engine/TransactionP.h | 16 +- libgnucash/engine/mocks/gmock-Transaction.h | 2 - 3 files changed, 53 insertions(+), 129 deletions(-) diff --git a/libgnucash/engine/Transaction.c b/libgnucash/engine/Transaction.c index 4b1888cc36..987706f74f 100644 --- a/libgnucash/engine/Transaction.c +++ b/libgnucash/engine/Transaction.c @@ -255,9 +255,6 @@ void gen_event_trans (Transaction *trans) } } -static const char* -is_unset = "unset"; - /* GObject Initialization */ G_DEFINE_TYPE(Transaction, gnc_transaction, QOF_TYPE_INSTANCE) @@ -274,11 +271,6 @@ gnc_transaction_init(Transaction* trans) trans->date_posted = 0; trans->marker = 0; trans->orig = NULL; - trans->readonly_reason = (char*) is_unset; - trans->isClosingTxn_cached = -1; - trans->notes = (char*) is_unset; - trans->doclink = (char*) is_unset; - trans->void_reason = (char*) is_unset; trans->txn_type = TXN_TYPE_UNCACHED; LEAVE (" "); } @@ -821,24 +813,12 @@ xaccFreeTransaction (Transaction *trans) /* free up transaction strings */ CACHE_REMOVE(trans->num); CACHE_REMOVE(trans->description); - if (trans->readonly_reason != is_unset) - g_free (trans->readonly_reason); - if (trans->doclink != is_unset) - g_free (trans->doclink); - if (trans->void_reason != is_unset) - g_free (trans->void_reason); - if (trans->notes != is_unset) - g_free (trans->notes); /* Just in case someone looks up freed memory ... */ trans->num = (char *) 1; trans->description = NULL; trans->date_entered = 0; trans->date_posted = 0; - trans->readonly_reason = NULL; - trans->doclink = NULL; - trans->notes = NULL; - trans->void_reason = NULL; if (trans->orig) { xaccFreeTransaction (trans->orig); @@ -2047,7 +2027,7 @@ xaccTransSetDatePostedGDate (Transaction *trans, GDate date) * the future a date which was set as *date* (without time) can * clearly be distinguished from the time64. */ g_value_init (&v, G_TYPE_DATE); - g_value_set_boxed (&v, &date); + g_value_set_static_boxed (&v, &date); qof_instance_set_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_DATE_POSTED); g_value_unset (&v); /* mark dirty and commit handled by SetDateInternal */ @@ -2105,7 +2085,7 @@ xaccTransSetDateDue (Transaction * trans, time64 time) GValue v = G_VALUE_INIT; if (!trans) return; g_value_init (&v, GNC_TYPE_TIME64); - g_value_set_boxed (&v, &time); + g_value_set_static_boxed (&v, &time); xaccTransBeginEdit(trans); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_DATE_DUE_KVP); qof_instance_set_dirty(QOF_INSTANCE(trans)); @@ -2126,7 +2106,7 @@ xaccTransSetTxnType (Transaction *trans, char type) g_value_unset (&v); return; } - g_value_set_string (&v, s); + g_value_set_static_string (&v, s); xaccTransBeginEdit(trans); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_TXN_TYPE_KVP); qof_instance_set_dirty(QOF_INSTANCE(trans)); @@ -2142,10 +2122,6 @@ void xaccTransClearReadOnly (Transaction *trans) qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, TRANS_READ_ONLY_REASON); qof_instance_set_dirty(QOF_INSTANCE(trans)); xaccTransCommitEdit(trans); - - if (trans->readonly_reason != is_unset) - g_free (trans->readonly_reason); - trans->readonly_reason = NULL; } } @@ -2156,16 +2132,12 @@ xaccTransSetReadOnly (Transaction *trans, const char *reason) { GValue v = G_VALUE_INIT; g_value_init (&v, G_TYPE_STRING); - g_value_set_string (&v, reason); + g_value_set_static_string (&v, reason); xaccTransBeginEdit(trans); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, TRANS_READ_ONLY_REASON); qof_instance_set_dirty(QOF_INSTANCE(trans)); g_value_unset (&v); xaccTransCommitEdit(trans); - - if (trans->readonly_reason != is_unset) - g_free (trans->readonly_reason); - trans->readonly_reason = g_strdup (reason); } } @@ -2218,25 +2190,16 @@ xaccTransSetDocLink (Transaction *trans, const char *doclink) { if (!trans || !doclink) return; - if (trans->doclink != is_unset) - { - if (!g_strcmp0 (doclink, trans->doclink)) - return; - - g_free (trans->doclink); - } xaccTransBeginEdit(trans); if (doclink[0] == '\0') { - trans->doclink = NULL; qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, doclink_uri_str); } else { GValue v = G_VALUE_INIT; - trans->doclink = g_strdup (doclink); g_value_init (&v, G_TYPE_STRING); - g_value_set_string (&v, doclink); + g_value_set_static_string (&v, doclink); //Gets copied at the other end qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str); g_value_unset (&v); } @@ -2257,18 +2220,9 @@ xaccTransSetNotes (Transaction *trans, const char *notes) { GValue v = G_VALUE_INIT; if (!trans || !notes) return; - if (trans->notes != is_unset) - { - if (!g_strcmp0 (notes, trans->notes)) - return; - - g_free (trans->notes); - } g_value_init (&v, G_TYPE_STRING); - g_value_set_string (&v, notes); + g_value_set_static_string (&v, notes); xaccTransBeginEdit(trans); - - trans->notes = g_strdup (notes); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str); qof_instance_set_dirty(QOF_INSTANCE(trans)); g_value_unset (&v); @@ -2288,12 +2242,10 @@ xaccTransSetIsClosingTxn (Transaction *trans, gboolean is_closing) g_value_set_int64 (&v, 1); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str); g_value_unset (&v); - trans->isClosingTxn_cached = 1; } else { qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, trans_is_closing_str); - trans->isClosingTxn_cached = 0; } qof_instance_set_dirty(QOF_INSTANCE(trans)); xaccTransCommitEdit(trans); @@ -2431,50 +2383,46 @@ const char * xaccTransGetDocLink (const Transaction *trans) { g_return_val_if_fail (trans, NULL); - if (trans->doclink == is_unset) - { - GValue v = G_VALUE_INIT; - Transaction *t = (Transaction*) trans; - qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str); - t->doclink = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL; - g_value_unset (&v); - } - return trans->doclink; + + GValue v = G_VALUE_INIT; + Transaction *t = (Transaction*) trans; + qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, doclink_uri_str); + const char* doclink = G_VALUE_HOLDS_STRING (&v) ? g_value_get_string (&v) : NULL; + g_value_unset (&v); + + return doclink; } const char * xaccTransGetNotes (const Transaction *trans) { g_return_val_if_fail (trans, NULL); - if (trans->notes == is_unset) - { - GValue v = G_VALUE_INIT; - Transaction *t = (Transaction*) trans; - qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str); - t->notes = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL; - g_value_unset (&v); - } - return trans->notes; + + GValue v = G_VALUE_INIT; + Transaction *t = (Transaction*) trans; + qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str); + const char *notes = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL; + g_value_unset (&v); + + return notes; } gboolean xaccTransGetIsClosingTxn (const Transaction *trans) { if (!trans) return FALSE; - if (trans->isClosingTxn_cached == -1) - { - Transaction* trans_nonconst = (Transaction*) trans; - GValue v = G_VALUE_INIT; - qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str); - if (G_VALUE_HOLDS_INT64 (&v)) - trans_nonconst->isClosingTxn_cached = (g_value_get_int64 (&v) ? 1 : 0); - else - trans_nonconst->isClosingTxn_cached = 0; - g_value_unset (&v); - } - return (trans->isClosingTxn_cached == 1) - ? TRUE - : FALSE; + + Transaction* trans_nonconst = (Transaction*) trans; + GValue v = G_VALUE_INIT; + gboolean rv; + qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, trans_is_closing_str); + if (G_VALUE_HOLDS_INT64 (&v)) + rv = (g_value_get_int64 (&v) ? 1 : 0); + else + rv = 0; + g_value_unset (&v); + + return rv; } /********************************************************************\ @@ -2604,15 +2552,12 @@ xaccTransGetReadOnly (Transaction *trans) if (!trans) return NULL; - if (trans->readonly_reason == is_unset) - { - GValue v = G_VALUE_INIT; - qof_instance_get_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_READ_ONLY_REASON); - trans->readonly_reason = G_VALUE_HOLDS_STRING (&v) ? - g_value_dup_string (&v) : NULL; - g_value_unset (&v); - } - return trans->readonly_reason; + GValue v = G_VALUE_INIT; + qof_instance_get_kvp (QOF_INSTANCE(trans), &v, 1, TRANS_READ_ONLY_REASON); + const char *readonly_reason = G_VALUE_HOLDS_STRING (&v) ? + g_value_get_string (&v) : NULL; + g_value_unset (&v); + return readonly_reason; } static gboolean @@ -2810,16 +2755,13 @@ xaccTransVoid(Transaction *trans, const char *reason) else g_value_init (&v, G_TYPE_STRING); - g_value_set_string (&v, _("Voided transaction")); + g_value_set_static_string (&v, _("Voided transaction")); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, trans_notes_str); - g_value_set_string (&v, reason); + g_value_set_static_string (&v, reason); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str); - if (trans->void_reason != is_unset) - g_free (trans->void_reason); - trans->void_reason = g_strdup (reason); gnc_time64_to_iso8601_buff (gnc_time(NULL), iso8601_str); - g_value_set_string (&v, iso8601_str); + g_value_set_static_string (&v, iso8601_str); qof_instance_set_kvp (QOF_INSTANCE (trans), &v, 1, void_time_str); g_value_unset (&v); @@ -2841,15 +2783,13 @@ const char * xaccTransGetVoidReason(const Transaction *trans) { g_return_val_if_fail (trans, NULL); - if (trans->void_reason == is_unset) - { - GValue v = G_VALUE_INIT; - Transaction *t = (Transaction*) trans; - qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str); - t->void_reason = G_VALUE_HOLDS_STRING (&v) ? g_value_dup_string (&v) : NULL; - g_value_unset (&v); - } - return trans->void_reason; + + GValue v = G_VALUE_INIT; + qof_instance_get_kvp (QOF_INSTANCE (trans), &v, 1, void_reason_str); + const char *void_reason = G_VALUE_HOLDS_STRING (&v) ? g_value_get_string (&v) : NULL; + g_value_unset (&v); + + return void_reason; } time64 @@ -2889,8 +2829,6 @@ xaccTransUnvoid (Transaction *trans) qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, void_reason_str); qof_instance_set_kvp (QOF_INSTANCE (trans), NULL, 1, void_time_str); g_value_unset (&v); - g_free (trans->void_reason); - trans->void_reason = NULL; FOR_EACH_SPLIT(trans, xaccSplitUnvoid(s)); @@ -2927,7 +2865,7 @@ xaccTransReverse (Transaction *orig) /* Now update the original with a pointer to the new one */ g_value_init (&v, GNC_TYPE_GUID); - g_value_set_boxed (&v, xaccTransGetGUID(trans)); + g_value_set_static_boxed (&v, xaccTransGetGUID(trans)); qof_instance_set_kvp (QOF_INSTANCE (orig), &v, 1, TRANS_REVERSED_BY); g_value_unset (&v); diff --git a/libgnucash/engine/TransactionP.h b/libgnucash/engine/TransactionP.h index 3e107ff390..0a22be0cc3 100644 --- a/libgnucash/engine/TransactionP.h +++ b/libgnucash/engine/TransactionP.h @@ -111,23 +111,11 @@ struct transaction_s */ Transaction *orig; - /* The readonly_reason is a string that indicates why a transaction - * is marked as read-only. If NULL, the transaction is read-write. - * This value is stored in kvp, but we cache a copy here for - * performance reasons. + /* A flag to indicate when a transaction represents an invoice, a payment, + * or a link between the two. */ - char * readonly_reason; - - char * doclink; - char * void_reason; - char * notes; - char txn_type; - /* Cached bool value to indicate whether this is a closing txn. This is - * cached from the KVP value because it is queried a lot. Tri-state value: -1 - * = uninitialized; 0 = FALSE, 1 = TRUE. */ - gint isClosingTxn_cached; }; struct _TransactionClass diff --git a/libgnucash/engine/mocks/gmock-Transaction.h b/libgnucash/engine/mocks/gmock-Transaction.h index ce82437fa7..e9dcd53fc2 100644 --- a/libgnucash/engine/mocks/gmock-Transaction.h +++ b/libgnucash/engine/mocks/gmock-Transaction.h @@ -32,8 +32,6 @@ public: date_posted = 0; marker = 0; orig = nullptr; - readonly_reason = nullptr; - isClosingTxn_cached = -1; } void* operator new(size_t size) {