GNOME Bugzilla – Bug 313782
Moving an account does not properly fire the row-deleted event on the GtkTreeModel
Last modified: 2018-06-29 20:53:27 UTC
Please describe the problem: The GNC_EVENT_REMOVE event will queue up the the actual removal of the original account path, but GNC_EVENT_DESTORY is never called (because the original account is never freed). Steps to reproduce: 1. Start GnuCash with a hierarchy of accounts 2. Move an account from its current location to be parented by another account. 3. Move the mouse over the accounts below the moved account's original position, especially over the last item in the list, should exhibit unusual display artifacts, and critical errors. Actual results: Expected results: Does this happen every time? Yes, this happens every time. Other information:
Created attachment 50882 [details] [review] Simple (possibly incorrect) patch Simply ensures that GNC_EVENT_DESTORY is called on the original account whether or not the old and new account parents are members of the same book. I'm not sure what other implications this may have, but it appears to solve the problem in my local build, and I haven't noticed any ill effects yet. I haven't extensively tested the patch though, I just wanted to get it shared since I'm rather inexperienced with the GnuCash code base, and I'm assuming others will be better equiped to understand where this patch may fall short, and what may be a more appropriate solution.
"destroy" usually means that the object has been, well, destroyed. I'm not sure that's quite the right thing here, but I'll look at more context to see what I think. It's possible that something isn't looking for the Remove event when it should. I also added the PATCH keyword.
I understand. In the current code path, gnc_tree_model_account_event_handler (which does receive the GNC_EVENT_REMOVE event fired) doesn't actually raise a row-deleted event on the GtkTreeModel. It simply appends the removed element to a pending_removals glist. Then, when the DESTROY event is called, it calls a helper function which finds the destroyed item in this pending_removals list and fires the row-deleted event on the saved path. I believe this is done because you have to have actually removed an account from the model before you fire the row-deleted event. In the case of a move though, as soon as you get the account has been reparented, you could fire row-deleted on the old path. The only other way I saw to go about tackling this was to create another GNC_EVENT, GNC_EVENT_MOVED which would be fired in lieu of the GNC_EVENT_REMOVE and GNC_EVENT_CREATE. Then the event handler would be able to properly special case this so that it could fire the row-deleted event.
I haven't looked at the model code, but I dont see why you couldn't fire the row-deleted event at the model when the REMOVE event happens... I don't see why it has to wait to delete it from the view/model. Another option is to key off the ADD event; if the account is in the remove_pending list when an ADD occurs then you can kick off the row-deleted event then. I see no reason to add a new event for this.
Created attachment 51025 [details] [review] Flush pending_removals for added entities I'm not sure why the pending_removals stuff is necessary either, but I'm just trying to make this patch as benign as possible, so I didn't want to rework that bit. Perhaps I'll test whether that is still needed at a later date. For now, I've updated the patch as Derek suggested. Rather than calling GNC_EVENT_DESTORY inappropriately or creating a new event, I've just modified the handler for GNC_EVENT_ADD in src/gnome-utils/gnc-tree-model-account.c to cycle through the pending_removals as is done in the GNC_EVENT_DESTROY handler. I've also included a small changelog entry, in case that is the desired patch format, but this change is relatively minor. It does cycle through the pending_removals GList on GNC_EVENT_ADD now as well, but I don't imagine that will have any performance impact, since I don't imagine that list will ever grow considerably.
Fixed with a similar patch. The same steps are taken, but from an idle loop callback instead of when the account is added back in its new location.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=313782. Please update any external references or bookmarks.