Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register preference "Jump behaviour when there are multiple accounts" #1657

Open
wants to merge 6 commits into
base: stable
Choose a base branch
from

Conversation

nomis
Copy link
Contributor

@nomis nomis commented Jun 3, 2023

Make the behaviour when the "Jump" operation encounters a transaction with multiple splits configurable.

This can be configured as:

  • Do nothing (this is the default)
  • Use the first split with the largest value
  • Use the first split with the smallest value

This makes it much easier to jump between accounts for stock transactions that include splits for fees and taxes, or dividend transactions that have a zero value split for the stock account.

Edit: The aim is that jump works for these transactions without any extra steps. Making this configurable allows different options to be added in the future (such as a menu, which I don't want to use myself). I will not be implementing those options.


I have also detected the case where there are only splits involving one other account so we can jump to that, added warnings, and now prevent all jumping to the current register by selecting one of its splits.

Example expanded transaction register rows for:

  1. Transaction: same existing behaviour, can "jump to the other account"
  2. Split A (this account): new behaviour, does nothing so that it's consistent with selecting a different split in this transaction (or any split in a transaction with 3+ splits) instead of the previous behaviour of also doing "jump to the other account (same as split B)"
  3. Split B (other account): same existing behaviour, can "jump to the other account"

@nomis
Copy link
Contributor Author

nomis commented Jun 3, 2023

This is incomplete because the preferences dialog doesn't support enums. I think it should be an enum so that other options could be added later. Proposed "display a menu" behaviour could be one of these options (or become the default).

There are no other existing preferences that are handled as real enums. Where there are enums they're implemented as groups of related boolean options where only one is expected to be enabled at any one time.

Please advise on how this preference should be implemented:

  • GtkRadioButton enum (current type; not yet supported)
  • GtkComboBox enum (not yet supported; comments in dialog-preferences.c state to use radio boxes for fewer than 5 choices)
  • GtkRadioButton int (not yet supported; comments in dialog-preferences.c state to use radio boxes for fewer than 5 choices)
  • GtkComboBox int
  • GtkRadioButton multiple booleans (not ideal for this preference)

I'd have to implement support in the preferences dialog for some of these and I don't want to do that until I know which type should be used (because I'm only going to add support for one of them).

The preference can be configured manually with dconf-editor.

@nomis nomis marked this pull request as draft June 4, 2023 14:43
@nomis nomis changed the title Draft: Register preference "Jump behaviour when there are multiple splits" Register preference "Jump behaviour when there are multiple splits" Jun 4, 2023
@dawansv
Copy link
Contributor

dawansv commented Jun 5, 2023

In some ways this is related to my proposal for a more "aggressive" split-transaction match in reports (instead of just showing "split transaction") in PR #1637, but my logic there goes beyond smaller or bigger. It follows the following selection tree:

  • First, multiple splits on the same credit/debit side as main split are excluded.
  • Second, splits with zero value are excluded unless main split is zero as well.
  • Third, if there is still more than one split on the other credit/debit side, then a Split on the other side with same value (opposite sign) is matched unless there is of course still more than one with same value.
  • If no obvious single split is found with these extra rules, then there is still no jumping but that's a smaller subset.

Note that this is in addition to the trading account splits which are already ignored (both in the reports as well as here in the Jump feature).

Now in my PR I say that while this would be good in a report, we certainly don't want to show that "best guess" matching split as the register transfer account because unlike a report, the register also allows to change the info right in that view. If I have more than one split, it's not safe to change that info in the limited transaction view, it should be done in the split view. Hence why my proposal is for reports only.

But here we are not changing but jumping. Yet I think we should ask ourselves whether or not it's risky to allow jumping from a multisplit transaction without the user going in the split view and having a full understanding of the transaction. Since it's not the default option, possibly it's OK to offer it. Regardless, another alternative of doing nothing for the default option would be to at least display a message inviting the user to repeat the jump from the split view. I imagine a new user might be confused with the current situation, as to why some transactions are jumping while others are not, and this message would explain the reason (instead of making it look like it's broken).

@nomis
Copy link
Contributor Author

nomis commented Jun 5, 2023

Regardless, another alternative of doing nothing for the default option would be to at least display a message inviting the user to repeat the jump from the split view. I imagine a new user might be confused with the current situation, as to why some transactions are jumping while others are not, and this message would explain the reason (instead of making it look like it's broken).

There are two options here:

  1. Change the default to "display error message" with an extra "do nothing" option.
  2. Use the existing warning framework to tell the user that they can't jump because there's more than one other split. This can then be dismissed but the user may forget they've done this the next time they try to jump and it does nothing.

The second option would be more consistent with how GnuCash normally behaves.

@dawansv
Copy link
Contributor

dawansv commented Jun 5, 2023

2. Use the existing warning framework to tell the user that they can't jump because there's more than one other split. This can then be dismissed but the user may forget they've done this the next time they try to jump and it does nothing.

The second option would be more consistent with how GnuCash normally behaves.

#2 would be my choice. Consistency is good.

@nomis
Copy link
Contributor Author

nomis commented Jun 5, 2023

I've added remember-able warnings. I've also had to change the existing behaviour. If you expand a transaction and "jump" on a split that refers to the current account, it will now do nothing and not tell you.

Previously it would jump to the other account if it was a transaction with only two splits, but that is inconsistent with splits for other accounts and how splits on transactions that have three or more splits behave.

@nomis nomis changed the title Register preference "Jump behaviour when there are multiple splits" Register preference "Jump behaviour when there are multiple accounts" Jun 5, 2023
@nomis
Copy link
Contributor Author

nomis commented Jun 5, 2023

But here we are not changing but jumping. Yet I think we should ask ourselves whether or not it's risky to allow jumping from a multisplit transaction without the user going in the split view and having a full understanding of the transaction. Since it's not the default option, possibly it's OK to offer it.

I need this as my default behaviour because I don't want to have to expand multiple stock transactions to jump to the individual stocks just because there's a third split for fees.

Other people will want different default behaviour, which is why it's a preference.

@dawansv
Copy link
Contributor

dawansv commented Jun 6, 2023

Other people will want different default behaviour, which is why it's a preference.

Yes in the case of the stock transaction with a fee then my decision tree would still be unable to pick it.

Still the decision tree is relevant I think. Even for your options, I assume when you say "split with largest/smallest value" you mean split on the opposite side (as it would be with your stock transaction example). I haven't looked at the code yet, just trying to figure out how we want it to behave.

So taking this more complex splits; somebody bough a movie ticket and some food with a debit card and also left a tip in cash with the food:

                          DEBIT        CREDIT
Checking Account                        $55
Cash in Wallet                          $10
Dining                     $50
Entertainment              $15

Now when I am on Checking Account and jumping, with "Use the split with the smallest value" selected, I assume you want it to jump to Entertainment, not Cash in Wallet. Hence the "only jump to split to the other side" is probably always true regardless of option.
Then what about splits with a zero value? When selecting smallest, should they be ignored? I think they should. Splits with zero value are used in some cases to connect an additional account to the split, but that account doesn't participate in the transaction value-wise such as for instance to connect a dividend payment to a stock account, hence the zero value. I think zero should not be considered a value with the "Use the split with the smallest value" option.

So then we are left with same value. Should that not take priority over "smaller" or "bigger"? Consider this transaction:

                          DEBIT        CREDIT
Checking Account                        $50
Cash in Wallet                          $15
Dining                     $50
Entertainment              $15

When selecting "Use the split with the smallest value" does it make really make sense to jump to Entertainment from Checking considering Dining is a direct match? Same with "Use the split with the largest value", does it really make sense to jump to Dining from Cash in Wallet instead of the exact match in Entertainment?

Hence at the end possibly the way you put it together is the rules I have set:

  • Look at other credit/debit side only
  • Ignore zero (unless main value is zero)
  • Match equal value first
  • If no equal value, match either smaller or bigger as per option;

Then what about other options such as matching the closest value instead of smallest or biggest? Doesn't that make more sense in most cases anyway?

At the end you are still left with an inability to match when you have 2 or more equals values, either as a direct match or smallest/biggest/closest. I don't see what you would do in these cases.

                          DEBIT        CREDIT
Checking Account                        $30
Cash in Wallet                          $30
Dining                     $30
Entertainment              $30
                          DEBIT        CREDIT
Checking Account                        $40
Cash in Wallet                          $20
Dining                     $30
Entertainment              $30

@nomis
Copy link
Contributor Author

nomis commented Jun 6, 2023

I assume you mean split on the opposite side

No, it must be for a different account; the side is irrelevant.

Then what about splits with a zero value? When selecting smallest, should they be ignored?

No, they are the smallest.

So then we are left with same value. Should that not take priority over "smaller" or "bigger"?
... does it make really make sense to jump to Entertainment from Checking considering Dining is a direct match?
... does it really make sense to jump to Dining from Cash in Wallet instead of the exact match in Entertainment?
Then what about other options such as matching the closest value instead of smallest or biggest?

No, because this is the "use the [first] split with the largest/smallest value" option.

Exact matches and closest value on the opposite side would be a different option. The current account may also have a split on both sides.

It is not easy to cover all cases and pick the right split in all cases. Either a menu or a multiple-choice orderable set of preference rules is needed for more complex cases. This change is to add an easily understood option that will work in some scenarios that doesn't change the default behaviour.

At the end you are still left with an inability to match when you have 2 or more equals values, either as a direct match or smallest/biggest/closest. I don't see what you would do in these cases.

I have now added the word "first" to the name to make this clear. That should be the first split in the order displayed.

@christopherlam
Copy link
Contributor

Instead of using heuristics to guess the desired target account, how about a keyboard-accessible popup to choose one? See gnc_plugin_page_register_cmd_jump_linked_invoice.

@nomis
Copy link
Contributor Author

nomis commented Jun 8, 2023

Instead of using heuristics to guess the desired target account, how about a keyboard-accessible popup to choose one? See gnc_plugin_page_register_cmd_jump_linked_invoice.

The aim is that jump works for these transactions without any extra steps.

I know some people want a menu, that's why it's configurable.

@jralls
Copy link
Member

jralls commented Dec 21, 2023

@Bob-IT Any comments?

@jralls
Copy link
Member

jralls commented Dec 21, 2023

@nomis wrote on IRC:

I need someone to comment on #1657 (comment)

GtkRadioButtons are special GtkToggleButtons. They're boolean. You can write a wrapper class that creates N radio buttons given a set of enums and presents a nice API to return one of the enums when queried. You can with less effort do the same with GtkComboBox. If you really want to do that be sure to write the wrappers so that they'll migrate easily to Gtk4, we'll probably be forced to migrate in the next 2-3 years.

The HIG requirement about using a radio group for fewer than 6 choices is I think a good one, so a GtkRadioButton is better for the current PR. Writing code on speculation that it might be useful someday is a waste of time now because it's not used and a waste of time later because it will bitrot and have to be fixed when someone decides to use it.

@Bob-IT
Copy link
Contributor

Bob-IT commented Dec 22, 2023

@Bob-IT Any comments?

Will have a look after Christmas.

@Bob-IT
Copy link
Contributor

Bob-IT commented Jan 10, 2024

Had a look at this today and it does not work. I am getting the following in the trace file...

  • 15:05:01 ERROR g_settings_bind: property 'active' on class 'GtkRadioButton' has type 'gboolean' which is not compatible with type 's' of key 'jump-multiple-splits' on schema 'org.gnucash.GnuCash.general.register'
  • 15:05:01 ERROR g_settings_bind: property 'active' on class 'GtkRadioButton' has type 'gboolean' which is not compatible with type 's' of key 'jump-multiple-splits' on schema 'org.gnucash.GnuCash.general.register'
  • 15:05:01 ERROR g_settings_bind: property 'active' on class 'GtkRadioButton' has type 'gboolean' which is not compatible with type 's' of key 'jump-multiple-splits' on schema 'org.gnucash.GnuCash.general.register'

The simplest solution would be to add an entry for each radio button in the scheme file just like 'Preference/Accounts/Reverse Balanced Accounts' but it would mean you would have to retrieve a max of 2 entries to know the correct value.

As mentioned by @jralls, a GtkRadioButton wrapper class would be better and could be used in a number of places in the preference dialog.

If you've selected a split for this account, for consistency with selecting
the split of another account we should do nothing. You're already on the
account for the split you selected.

Previously it would still go to the "other" account but that's going to
make the "multiple split" options confusing.
@nomis nomis marked this pull request as ready for review September 21, 2024 18:36
The "=" in the widget name will separate the pref name from the enum
string value that is used when the radio button is activated. When the
radio button is deactivated, its preference update is ignored because
another button must have been made active.
Make the behaviour when the "Jump" operation encounters a transaction with
multiple splits configurable.

This can be configured as:
* Do nothing (this is the default)
* Use the split with the largest value
* Use the split with the smallest value

This makes it much easier to jump between accounts for stock transactions
that include splits for fees and taxes, or dividend transactions that have
a zero value split for the stock account.
There may be multiple splits but only one other account. In this scenario
we can jump to the first split for that account.
All existing dialogs are either QUESTION or WARNING.

Allow INFO, ERROR and OTHER to remember being dismissed without asking a
question.
@nomis
Copy link
Contributor Author

nomis commented Sep 21, 2024

I am getting the following in the trace file...

* 15:05:01 ERROR  g_settings_bind: property 'active' on class 'GtkRadioButton' has type 'gboolean' which is not compatible with type 's' of key 'jump-multiple-splits' on schema 'org.gnucash.GnuCash.general.register'

Configuring the preference hadn't been fully implemented yet. I'm now using g_settings_bind_with_mapping() to convert the enum values so this error is now gone.

As mentioned by @jralls, a GtkRadioButton wrapper class would be better and could be used in a number of places in the preference dialog.

I've been able to map the existing radio buttons to the enum values, so no additional wrapper is needed.

@nomis
Copy link
Contributor Author

nomis commented Sep 21, 2024

@Bob-IT this is now ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants