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

797854 global interest pay bug1 #753

Merged

Conversation

jeanlaroche
Copy link
Contributor

Bug 797854 - Global Register Preference to prompt for interest payment is not being honored.

A new per-account preference is added to let the user decide whether the interest transaction dialog should be
opened automatically before the reconcile. This preference is only enabled for certain types of accounts and
the code that decides that was moved to Account.h as is it now used in two separate place.

libgnucash/engine/Account.cpp Outdated Show resolved Hide resolved
jean added 2 commits July 13, 2020 20:25
…t is not being honored.

A new per-account preference is added to let the user decide whether the interest transaction dialog should be
opened automatically before the reconcile. This preference is only enabled for certain types of accounts and
the code that decides that was moved to Account.h as is it now used in two separate place.
@jeanlaroche jeanlaroche force-pushed the 797854_global_interest_pay_bug1 branch from 7c1e285 to 705f0db Compare July 14, 2020 05:42
@jeanlaroche
Copy link
Contributor Author

Should be fixed now.

@jeanlaroche jeanlaroche requested a review from jralls July 14, 2020 06:03
@jralls
Copy link
Member

jralls commented Jul 14, 2020

When the edit-account dialog opens the auto-interest-transfer check box is unchecked regardless of whether the KVP is set.

@jeanlaroche
Copy link
Contributor Author

When the edit-account dialog opens the auto-interest-transfer check box is unchecked regardless of whether the KVP is set.

mmm, I don't see that. How do I reproduce the issue? Are you setting the KVP manually somehow?

@jralls
Copy link
Member

jralls commented Jul 14, 2020

Nope. Just edit an account, check the box, OK the dialog, edit again: box is unchecked. Close the dialog, reconcile the account and the transfer dialog comes up showing that the KVP is in fact set.

@jralls
Copy link
Member

jralls commented Jul 14, 2020

Interesting, works fine on Linux. What are you testing on?

@jeanlaroche
Copy link
Contributor Author

Nope. Just edit an account, check the box, OK the dialog, edit again: box is unchecked. Close the dialog, reconcile the account and the transfer dialog comes up showing that the KVP is in fact set.

Crap, I'm completely unable to repro. It works normally for me. I tried on several accounts, and several books (including some I haven't opened before with this PR)... If I edit the account, tick the check box, close the edit window, reopen it, the check box is checked... The whole things works normally for me. Let me rerun cmake in case something's not in sync.

@jeanlaroche
Copy link
Contributor Author

Interesting, works fine on Linux. What are you testing on?

Mac os, but I'm testing from the IDE

@jralls
Copy link
Member

jralls commented Jul 14, 2020

OK, I'll debug into it a bit.

@jeanlaroche
Copy link
Contributor Author

jeanlaroche commented Jul 14, 2020 via email

@jeanlaroche
Copy link
Contributor Author

BTW I'd much build on my windows machine, it's a lot more powerful, but I'm put off by the guide that essentially says "don't do it". Is that still the case? Or is building on windows not that hard any longer?

@jralls
Copy link
Member

jralls commented Jul 14, 2020

It's better than it was, but it's command line all the way. You could always set up a Linux VM.

@jeanlaroche
Copy link
Contributor Author

It's better than it was, but it's command line all the way. You could always set up a Linux VM.

Ah, no visual C++ or anything like that. Yeah, I'll setup a linux VM and be done with it. Developing on the mac isn't much fun on the mac I'm using, totally underpowered...

@jralls
Copy link
Member

jralls commented Jul 14, 2020

You can get a lot more mileage out of your mac by building from the command line instead of Xcode, which is quite a hog.

Debugging on MacOS and Linux side-by-side shows that opening the edit account dialog on MacOS fires gnc_account_type_changed_cb 4 times and on Linux it fires only once. Each time it fires it calls set_auto_interest_box. One of those 4 times it's triggered by gnc_account_parent_changed_cb that sets aw->type to ACCOUNT_TYPE_INVALID and that causes set_auto_interest_box to unset the check box. Another call sets aw->type back to ACCOUNT_TYPE_BANK, but set_auto_interest_box can only unset the check box it doesn't get set again before the dialog is shown.

@jeanlaroche
Copy link
Contributor Author

OK I added refresh code to set_auto_interest_box. The drawback is that if you set the flag then change the account type to a type that does not support the flag, the checkbox is unchecked, if you go back to a type that allows it, the flag will not set itself back, because it will just reflect the preference you had before you opened the edit dialog.
This is really not a problem in my view.

@jralls
Copy link
Member

jralls commented Jul 16, 2020

I agree that it's not a problem, but I think that the following is a bit more straightforward:

--- a/gnucash/gnome-utils/dialog-account.c
+++ b/gnucash/gnome-utils/dialog-account.c
@@ -157,6 +157,7 @@ void gnc_account_name_insert_text_cb (GtkWidget   *entry,
                                       gint         length,
                                       gint        *position,
                                       gpointer     data);
+static void set_auto_interest_box(AccountWindow *aw);

 /** Implementation *******************************************************/

@@ -278,9 +279,7 @@ gnc_account_to_ui(AccountWindow *aw)
     gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (aw->hidden_button),
                                   flag);

-    flag = xaccAccountGetAutoInterest (account);
-    gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (aw->auto_interest_button),
-                                  flag);
+    set_auto_interest_box (aw);
     LEAVE(" ");
 }

@@ -1098,12 +1097,13 @@ static void
 set_auto_interest_box(AccountWindow *aw)
 {
     Account* account = aw_get_account (aw);
-    gboolean flag = account_type_has_auto_interest_xfer (aw->type);
-    gtk_widget_set_sensitive (GTK_WIDGET (aw->auto_interest_button), flag);
-    if (!flag)
-        gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (aw->auto_interest_button), FALSE);
-    gtk_widget_set_sensitive (GTK_WIDGET (aw->auto_interest_button_label), flag);
-
+    gboolean type_ok = account_type_has_auto_interest_xfer (aw->type);
+    gboolean pref_set = xaccAccountGetAutoInterest (account);
+    gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (aw->auto_interest_button),
+                                  type_ok && pref_set);
+    gtk_widget_set_sensitive (GTK_WIDGET (aw->auto_interest_button), type_ok);
+    gtk_widget_set_sensitive (GTK_WIDGET (aw->auto_interest_button_label),
+                              type_ok);
 }

 static void

@jeanlaroche
Copy link
Contributor Author

jeanlaroche commented Jul 16, 2020

I agree that it's not a problem, but I think that the following is a bit more straightforward:

I agree will make the changes.

@jeanlaroche jeanlaroche force-pushed the 797854_global_interest_pay_bug1 branch from b8d0031 to 91b6bb3 Compare July 17, 2020 01:03
@jeanlaroche
Copy link
Contributor Author

I adopted your suggestions. Annoyingly I tried to use your diff as a git patch but wasn't successful in applying it. Not sure what git was complaining about. I probably don't know of a very easy way to do that... :)
J.

@jralls
Copy link
Member

jralls commented Jul 17, 2020

Annoyingly I tried to use your diff as a git patch but wasn't successful in applying it. Not sure what git was complaining about.

That's because my diff was against the PR before your refresh commit. I'd think you'd have gotten a fairly simple conflict to resolve.

@jeanlaroche
Copy link
Contributor Author

Annoyingly I tried to use your diff as a git patch but wasn't successful in applying it. Not sure what git was complaining about.

That's because my diff was against the PR before your refresh commit. I'd think you'd have gotten a fairly simple conflict to resolve.

Possibly, but git wouldn't even let me apply it. I was only able to apply part of it. Git would say "corrupt patch at line $" (usually the last line). But no worries.

@code-gnucash-org code-gnucash-org merged commit b07d093 into Gnucash:maint Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants