Alex,
I don’t think it’s necessary to explicitly remove the slot in
xaccAccountGetTaxUSCopyNumber, the slot will get removed automatically in
guI_to_accounts. Just adding the *copy_number test is sufficient to deal with
0 getting stored in the slot.
Regards,
John Ralls
> On Feb 5, 2025, at 17:02, Alex Aycinena <alex.aycin...@gmail.com> wrote:
>
> John,
>
> Thanks for your quick response.
>
> If I followed you correctly, this is what I will change and test:
>
> gint64
> xaccAccountGetTaxUSCopyNumber (const Account *acc)
> {
> auto copy_number = get_kvp_int64_path (acc, {"tax-US", "copy-number"});
> if (copy_number && *copy_number == 0)
> /* this is here to clean up any KVPs set to zero due to prior bug
> */
> /* if copy_number exists and its value is zero get rid of KVP and
> return 1 */
> {
> set_kvp_int64_path (acc, {"tax-US", "copy-number"},
> std::nullopt);
> return 1;
> }
> return (copy_number && *copy_number) ? *copy_number : 1;
> }
>
> void
> xaccAccountSetTaxUSCopyNumber (Account *acc, gint64 copy_number)
> {
> set_kvp_int64_path (acc, {"tax-US", "copy-number"}, (copy_number &&
> *copy_number != 0) ? copy_number : std::nullopt);
> }
>
> In the 'Set' routine, I added the need for the copy_number to exist and the
> value to not be zero, otherwise the KVP is removed.
>
> Appreciate your feedback.
>
> Reagrds,
>
> Alex
>
> On Tue, Feb 4, 2025 at 8:55 PM John Ralls <jra...@ceridwen.us
> <mailto:jra...@ceridwen.us>> wrote:
>> 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
>>> <mailto: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 <mailto: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
