Created attachment 103765 [details]
QIF file demonstrating the bug

Created attachment 103766 [details]
Screenshot

If you go forward to the commodities page, then back to the currency page, then forward again it skips the commodities page the second time around.

Steps to Reproduce,

1.) Load this QIF: http://bugzilla.gnome.org/attachment.cgi?id=103399
2.) Click Next until you get to the Commodities page.
3.) Click Back to go back until you get to the currency page (1 or 2 backs).
4.) Click Next to go forward. Notice that this time the commodities page is skipped.

Now I'm going comment crazy. I meant to put that last comment in bug 503166

Created attachment 104948 [details] [review]
Proposed patch

Added a proposed patch to the bug. It works but may be a bit problematic on performance for large files as it checks imported transactions for payees/memos at least twice, when checking if the page should be displayed (gnc_ui_qif_import_categories_next_cb) and when preparing the page (gnc_ui_qif_import_memo_prepare_cb) and possibly again if the user hits back on the currency page (gnc_ui_qif_import_currency_back_cb).

Alternatives might be to jump pages during gnc_ui_qif_import_memo_prepare_cb or to save the list of payees/memos during the check step and save it to the window for reuse during the prepare step. The first wouldn't work well with the doc pages enabled and seems like it would break current design rules, so the second alternative seems like it might be better short of a full rewrite but would also need some extra code to make sure transactions are rechecked if a user goes back and adds more files etc.

I have been doing some large imports - large enough to run for more than an hour - and have not noticed any delay between mapping pages, so I think the performance hit shouldn't be significant.

But the performance hit could be eliminated, and in fact improved over the existing, unpatched code. Currently the three Scheme functions that determine what mappings are needed (qif-dialog:make-account-display, qif-dialog:make-category-display, and qif-dialog:make-memo-display) are called individually by the various "prepare" handlers (and in the proposed patch, the "next" handlers too). But they could all just be called once each by a "next" handler for "loaded_files_page". And if it were smart, that handler wouldn't launch those three Scheme functions again unless new QIF files were loaded. That way those Scheme functions wouldn't be getting called multiple times when flipping back and forth between pages.

This also fits with what I was planning to do some time down the road anyway, which is to combine those three Scheme functions mentioned above into one bigger one. It's more robust and efficient that way.

Not suggesting that we need to run with this right now... it's just an idea I had sketched in my head while digging through code to create the patch for bug 515163.

Charles,

Ok, since it works (actually, have you tested it?) and you seem to be saying you don't think it will be a big problem then let's just run with this patch and make improvements to the performance and code design farther forward in the future.

Sounds good to me. No, I haven't tested it yet. I'll try tomorrow.

Just out of curiosity, what do those urgency_hint properties do in the glade file? I wonder if they need to be removed for backward compatibility, along with the last_modification_time property. I see that Andreas was removing urgency_hint for some reason here:
http://lists.gnucash.org/pipermail/gnucash-changes/2007-April/005137.html

I wondered that too but just left them in, figuring that glade had the backwards compatibility issues worked out. It's a little annoying to have to edit the glade files by hand but Andreas' removal certainly seems deliberate and I'm sure there's a good reason for it so I'll remove them and submit an updated patch tonight.

Created attachment 105013 [details] [review]
Proposed patch

Updated proposed patch to remove the urgency_hint and last_modification_time properties that were added to qif.glade in the last version of the patch.

I haven't tried it, but I think you could eliminate the Scheme call in the "back" handler (gnc_ui_qif_import_currency_back_cb) by doing:

  if (!wind->memo_display_info ||
      SCM_NULLP(wind->memo_display_info))

instead of

  accts_left = scm_call_3(make_memo_display,
			  wind->imported_files,
			  wind->memo_map_info,
			  wind->gnc_acct_info);
  if (SCM_NULLP(accts_left))

I tested that out and it doesn't work somehow. It skips the payee page going forward but if you go back from the currency page, you see the empty payees/memos page with the attached QIF.

Oddly enough though, if you go all the way to the end and then back it seems to work and skips the payees/memos page though the druid still brings up a host of other pages that are not supposed to be shown.

Ah, I think I see why it doesn't work. It probably needs to be:

  if (!wind->memo_display_info ||
      (wind->memo_display_info == SCM_BOOL_F) ||
      SCM_NULLP(wind->memo_display_info))

Hopefully I can test the patch today. I couldn't get to it yesterday.

Created attachment 105191 [details] [review]
Proposed patch

Ah I see that. The memo_display_info is initialized to SCM_BOOL_F when the druid is created. Good catch. When you get a chance please test this new patch.

Ian, I just finished testing your patch. Works like a charm.

I removed all the references to last_modification_time in qif.glade (there was even a really old one in there), rebuilt GnuCash and retested. Works great!

Committed as r16956. Awaiting backport for 2.2.4.

Applied to branches/2.2 as r16974 for GnuCash 2.2.4.
Thanks a lot!

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=512173. Please update any external references or bookmarks.