Alex, That commit is part of https://github.com/Gnucash/gnucash/pull/1965.
Your first proposed change isn’t quite right because a the std::optional<T>::operator==(T) returns false if the optional doesn’t contain a value (see cases 21-33 in https://en.cppreference.com/w/cpp/utility/optional/operator_cmp). You want (copy_number && *copy_number) ? *copy_number : 1; *But* the assumption behind the original code is that if the copy_number was 0 then get_kvp_int64_path would have returned std::nullopt because set_kvp_path<T>(path, nullptr) calls (indirectly) KvpFrame::;set_impl(https://github.com/Gnucash/gnucash/blob/b5c1937702ca7a8fba772a47b2ac7fa6b8857bf5/libgnucash/engine/kvp-frame.cpp#L101) and that removes the path if it’s passed a false value; nulltpr is false. qof_instance_get_path_kvp https://github.com/Gnucash/gnucash/blob/b5c1937702ca7a8fba772a47b2ac7fa6b8857bf5/libgnucash/engine/qofinstance.cpp#L1068) bases that on getting a nullptr from KvpFrame::get_slot (https://github.com/Gnucash/gnucash/blob/b5c1937702ca7a8fba772a47b2ac7fa6b8857bf5/libgnucash/engine/kvp-frame.cpp#L144). I think you’re right about > void > xaccAccountSetTaxUSCopyNumber (Account *acc, gint64 copy_number) > { > set_kvp_int64_path (acc, {"tax-US", "copy-number"}, copy_number); > } The problem is that it’s set_kvp_int64_path (Account *acc, const Path& path, std::optional<gint64> value) So gint64 copy_number is getting implicitly converted to std::optional<gint64> and that’s what’s passed to qof_instance_set_kvp_path<T> (https://github.com/Gnucash/gnucash/blob/b5c1937702ca7a8fba772a47b2ac7fa6b8857bf5/libgnucash/engine/qofinstance.cpp#L1076) it’s (quite reasonably) the optional that’s tested, not the optional’s contained value. After all, it’s copy_number that’s the special case where 0 isn’t a valid value. The fix (and answer to your question about how to get rid of the KVP) is void xaccAccountSetTaxUSCopyNumber (Account *acc, gint64 copy_number) { set_kvp_int64_path (acc, {"tax-US", "copy-number"}, copy_number ? std::make_optional<gint64>(copy_number) : std::nullopt); } so that it will evaluate false in qof_instance_set_kvp_path. Regards, John Ralls > On Feb 4, 2025, at 18:04, Alex Aycinena <alex.aycin...@gmail.com> wrote: > > I would request some advice in solving a problem that has been discussed on > the gnucash-users list recently. > > The symptom: a US-based user has reported that on the Accounts tab, when the > Tax Info column is displayed, some accounts show 'Sched B: Total dividend > income' for their assigned tax codes and others show 'Sched B(0): Total > dividend income'. The number in parentheses is the 'copy number' which should > never be zero and should only show up if it is greater than 1 (and the copy > number issue doesn't even apply to Schedule B). > > The problem: investigation shows that not only is the display in the Tax Info > column on the Accounts tab wrong and confusing, but the US Tax Report can > show incomplete information and the TXF export also comes out with incomplete > data and invalid data (there is a copy number field that comes out as 'C0', > which is not valid). > > The cause: a commit was made on Oct 17, 2024, commit 63deaad, that changed > logic, in Account.cpp: > > From: > > gint64 > xaccAccountGetTaxUSCopyNumber (const Account *acc) > { > gint64 copy_number = 0; > GValue v = G_VALUE_INIT; > g_return_val_if_fail(GNC_IS_ACCOUNT(acc), FALSE); > qof_instance_get_path_kvp (QOF_INSTANCE(acc), &v, {"tax-US", > "copy-number"}); > if (G_VALUE_HOLDS_INT64 (&v)) > copy_number = g_value_get_int64 (&v); > > return (copy_number == 0) ? 1 : copy_number; > } > > to: > > gint64 > xaccAccountGetTaxUSCopyNumber (const Account *acc) > { > auto copy_number = get_kvp_int64_path (acc, {"tax-US", "copy-number"}); > return copy_number ? *copy_number : 1; > } > > If I read it correctly, this changed the logic from "if there is a copy > number of '0', return '1'", to "if there is a copy number (even if it is > zero) just return it, otherwise return '1'". > > Also changed in commit 63deaad, > > From: > > void > xaccAccountSetTaxUSCopyNumber (Account *acc, gint64 copy_number) > { > g_return_if_fail(GNC_IS_ACCOUNT(acc)); > xaccAccountBeginEdit (acc); > if (copy_number != 0) > { > GValue v = G_VALUE_INIT; > g_value_init (&v, G_TYPE_INT64); > g_value_set_int64 (&v, copy_number); > qof_instance_set_path_kvp (QOF_INSTANCE (acc), &v, {"tax-US", > "copy-number"}); > } > else > { > qof_instance_set_path_kvp (QOF_INSTANCE (acc), nullptr, {"tax-US", > "copy-number"}); > } > mark_account (acc); > xaccAccountCommitEdit (acc); > } > > to > > void > xaccAccountSetTaxUSCopyNumber (Account *acc, gint64 copy_number) > { > set_kvp_int64_path (acc, {"tax-US", "copy-number"}, copy_number); > } > > Again, if I read it correctly, this changed the logic from "if the copy > number is not zero, create a new KVP for it and save it or, if there is > already a KVP for 'copy number', change it to the new value, otherwise, > delete the KVP (but do not save a zero value)" to "just save the value, even > if it is zero". > > This change caused the problem and symptoms. > > The fix: first correct/retore the logic in ' xaccAccountGetTaxUSCopyNumber' > and ' xaccAccountGetTaxUSCopyNumber'. Second, if any of our users have used > 'Edit->Tax Report Options' to modify their tax code assignments for releases > since Oct 17, 2024, they may have data files with 'copy number' KVPs with > zeros and inaccurate US Income Tax Reports/TXF Exports. We need to figure out > how to clean up their data files. > > The first correction (I implemented this first in 2009, so not sure of > current syntax, thus need your feedback): > > for ' xaccAccountGetTaxUSCopyNumber' > > - return copy_number ? *copy_number : 1; > + return (copy_number == 0) ? 1 : copy_number; > > for 'xaccAccountSetTaxUSCopyNumber' > > - set_kvp_int64_path (acc, {"tax-US", "copy-number"}, copy_number); > + if (copy_number != 0) > + { > + set_kvp_int64_path (acc, {"tax-US", "copy-number"}, copy_number); > + } > + else > + { > + Get rid of KVP {"tax-US", "copy-number"}); <- how do I do this now! > + } > > For the second correction (clean up users data files), I was thinking to add > logic to 'xaccAccountGetTaxUSCopyNumber' that would, if it gets a zero value, > delete the KVP, so it would look like this: > > gint64 > xaccAccountGetTaxUSCopyNumber (const Account *acc) > { > auto copy_number = get_kvp_int64_path (acc, {"tax-US", "copy-number"}); > if (copy_number == 0) > { > Get rid of KVP {"tax-US", "copy-number"}); <- how do I do this > now! > return 1; > } > return copy_number; > } > > Since 'xaccAccountGetTaxUSCopyNumber' is basically used in only three places > ('Edit->Tax Report Options', the 'Tax Info' column of the Accounts tab, and > the 'US Tax Report"), the problem would simply be cleaned up by being called > through normal activity (looking at the data or running a report). > > Feedback welcome, > > Alex > _______________________________________________ > gnucash-devel mailing list > gnucash-devel@gnucash.org > https://lists.gnucash.org/mailman/listinfo/gnucash-devel
_______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
