GNOME Bugzilla – Bug 567709
Exchange rate entry dialog pulls wrong data in Transaction Journal view
Last modified: 2018-06-29 22:15:58 UTC
Open an account register which already contains entries, some of which are in a foreign currency. Choose the Transaction Journal view. Click on the main ledger entry (green bar) of any transaction. Then, look on the screen for a *different* transaction, where one of that transaction's splits is in another currency. Right-click directly on that split, and select "Edit Exchange Rate". On the dialog that appears, all of the data under the "Basic Information" section except for the "Amount" will be from the previous transaction, not the one containing the split that you just right-clicked on. Basically, the data is always being pulled from the last main ledger entry (green bar) you clicked on. Also -- in the Transaction Journal view, Gnucash is frequently ignoring changes that I try to make to an existing exchange rate that I have set for a transaction. I have been doing a lot of testing (on a brand new file I created), and I can not find any pattern to how it records or ignores changes, but it happens quite frequently and is extremely aggravating. This is happening under 2.2.6 release (Ubuntu package), 2.2.8 release (compiled from source), and latest SVN. I don't notice this happening in Basic Ledger view. I am using Ubuntu 8.10.
Could I politely request that this bug be triaged ASAP? It is personally blocking me from using Gnucash. I have about four months' worth of overseas transactions, many of which have splits due to an exchange fee assessed by VISA or my credit union, and I basically can't enter them at all because of the behavior described where changes are being ignored. I would be happy to assist in troubleshooting or providing more information, although I have not worked with the Gnucash code base at all.
Reproduced in trunk r18057 (first paragraph). I haven't tried the 2nd paragraph yet. Are you reporting 2 problems here? If so, I'm going to clone this if I find that they have different causes, because we might need to keep one piece open longer than the other. Also, is it the 2nd piece that is causing you the most problem?
Can the first bug be worked around (for now) by left clicking on the split before right clicking on it?
Can the second problem still be reproduced with the trunk SVN code? There have been quite a few register fixes lately in trunk.
Thanks for the replies, and sorry for my delayed response because it's final exams week here...I really appreciate you working on this because it's still really affecting me. I haven't thought about balancing my checkbook in months with the hope that this would get fixed. Phil, you're right, I would consider them as separate bugs. It's more difficult to reproduce the behavior in the second bug at will, so I was kind of hoping that it was all related to whatever was causing the first one, so that it would be easier to see if a fix really happened...but I certainly realize that may not be the case. If you think a fix may already be in place, I'll update to the latest SVN and give it a try as early as this weekend and let you know (I'm sure I would catch it because I have a lot to enter).
To clarify, yes the second part is the real problem for me. And that is what I was assuming there might already be a fix for that I would try out, from Charles's comments.
I doubt the first problem would be fixed. I'll have to look at that. Does the workaround described in comment 4 work? For the second bug, it might be fixed. But it still happens, does using the comment 4 workaround make any difference?
Sorry, where I said comment 4, I meant comment 3.
If I remember correctly that was the workaround that I used too (although I'd inadvertently click elsewhere anyway out of habit). I don't know of any second workaround for the second bug though. I can try that workaround again this weekend to verify.
I've cloned this bug as 580968 to handle the second part (changes ignored). This bug will remain for the first part (wrong data). I've also copied the comments and pasted them in the new bug.
The bug is occurring in gnc_table_move_cursor_internal(), located in register/register-core/table-allgui.c. Within this function, table->layout->cells is changed if the green row from another transaction (not a split) is clicked, but it is NOT updated if a split of another transaction is clicked directly. The value of table->layout->cells is what is later used to populate the exchange rate dialog.
I can reproduce the problem. From comment 11, I'd guess that there may be something wrong in gnc_split_register_move_cursor() in register/ledger-core/split-register-control.c, which is the callback reached via gnc_table_move_cursor_internal().
Thanks for reproducing Charles, and pointing out gnc_split_register_move_cursor() which I didn't realize was being called. I have a very limited understanding of what is going on (I haven't worked with the Gnucash code at all before this), but after looking at this a little longer, what seems to be happening from my point of view is: - There is a "transaction cursor" (aka cellblock) and a separate "split cursor". - When clicking on a transaction, the cells in the transaction cursor are updated (using the table). When clicking on a split (including a split on a different transaction than the current one), the cells in the split cursor are updated -- but the transaction cursor is not updated. The cells in the respective cursor are updated in the double for-loop at the end of gnc_table_move_cursor_internal(). - table->layout->cells contains a linked list of pointers that always point to cells in the transaction cursor (don't understand this part yet). This is used to populate the exchange rate dialog, which takes place in gnc_split_register_handle_exchange() in register/ledger-core/split-register-control.c. However I don't think I understand the code design well enough yet to know how it is intended to work and thus where the problem is -- does this provide enough information for one of the developers to see what the problem is and produce a fix? Thanks so much.
FWIW I used the GNU Debugger (gdb) to trace through the code to come to my conclusions in the previous two posts, so I'm confident that I'm seeing where the problem is happening, just not what to do about it.
Although I've made most of the changes to the register code over the past few months, I'm honestly quite new to it myself. And there is very little documentation, especially in the guts of it. So we are both learning. Sorry, but I have not had time to read the gnc_split_register_move_cursor() code in detail yet in the context of this bug. It may have to be gone over with a fine-toothed comb to figure out exactly where the problem is. I would not be surprised if this was a one-line fix, actually. Hopefully I will get some time later today.
By the way, your remarks in comment 11 and comment 13 certainly provide a few clues. I don't have a full grasp of all the code in register/register-core -- only the bits that I've had to touch to fix specific bugs that were crashing the register. Fortunately those crashers are all gone, I believe. But I guess we'll really find out as more people test 2.3.x, since none of my register fixes were included in the 2.2.x releases.
Charles, thanks so much for your help on this. Any luck? It seems that either: - the transaction cursor needs to be updated whenever the split cursor is, or - the exchange dialog should not be using table->layout->cells for populating its data...whatever that is Do you have an idea which it is?
Behavior still appears in 2.3.1 alpha release, marking as such.
Created attachment 136605 [details] [review] Use xaccTrans/xaccSplit functions to populate dialog This patch fixes bug 567709 with my initial testing. This patch removes the only references to the functions gnc_split_register_get_cell_string() and gnc_split_register_get_cell_date() anywhere in the source code. Consider removing those functions; I'm not sure they are necessary or reliable.
Charles, can you check out the patch. I don't know the register code.
Sure, I will take a look.
Hopefully I will get to this tomorrow, but my first impression is that it is probably better in the long run to make the transaction-related cells reflect the split's transaction, rather than avoiding them. I'm thinking this patch is more of a workaround than a fix. Otherwise other functionality could be susceptible to the same sort of trouble. Removed blocking of bug 580968.
The more I think about this, there are two possibilities: 1. You are not supposed to assume that cells outside the current cursor are valid. So when the cursor is on a split, all cells that are not in that cursor, such as transaction-level cells like the date, should not be trusted. If this is the case, then your patch has the right idea. 2. All cells are supposed to be up-to-date, even if they are not part of the current cursor. This would mean that when you move from a split in one transaction to a split in another, you'd have to visit the transaction cursor first to make sure all those cells are up-to-date. I think I might ask about this on the developer's list. I don't really see any assumptions in the code that #2 is correct, except for in the exchange rate handling, which is obviously broken. So maybe #1 is correct. It makes more sense to me anyway. As far as your patch, assuming that #1 is correct, then the patch fixes the problem for transaction journal mode. But I don't think it is correct for basic ledger mode, because there the current cursor DOES contain the transaction data, and it might have been changed by the user but not saved yet. So you would have to pull from the cells rather than the saved transaction.
No response yet on the developer's list, so I might just assume #1 since that works either way.
Fix committed in r18177. Setting target to 2.3.2.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=567709. Please update any external references or bookmarks.