ofx and generic import: GUI, callbacks

[Christian Stimming]

Benoit Grégoire wrote:

>>Err... are you saying that the destination account matching *has* to be on
>>the same page as on the transaction matching/duplicate detection? 
>>I can easily imagine those happening in different
>>windows (or druid pages or whatever). 
> 
> Ok, here is an example.  [...] Since 
> your groceries and TV are unlikely to go into the same expense account, you 
> MUST have a way to override the destination account on a per transaction 
> basis. 


Oh absolutely. I totally agree that the destination account *has* to be 
presented to the user and chosen by the user on a per-transaction basis.


> Unless you mean for the first page to have one line per transaction, I don't 
> see how you could handle it on your previous druid page.  


As for the GUI for Destination Account matching/choosing, I absolutely 
have a listview in mind. The listview would have one line per 
transaction. Each line shows some details for the transaction (date, 
amount, description, ...) *and* the destination account. The destination 
account is either the automatically matched one, or empty by default, or 
some fixed account by default. The user then can click on each line's 
account to choose another account. In your example, the user would see 
one line for "WalMart Date xx Description xx Amount xx" and another line 
for "Walmart Date yy Description yy Amount yy" and can choose whatever 
destination account he/she considers appropriate.

All our discussion about "automatic destination account matching" IMHO 
is *only* about the suggested default. The user should *always* have the 
choice to choose some other destination account (and, BTW, this is the 
only way to learn the automatic matches in the first place).

>>* One thing that keeps buggin me about the gnc_import_add_trans function is
>>that it operates on static, hidden GUI data, which is obvious by the fact
>>that it only takes one argument, the Transaction to add. IMHO the natural
>>question here is "this thing adds transaction TO WHERE?" What I definitely
>>would prefer here is a function like
>>
>>void gnc_import_add_trans(GenericImporter *importer, Transaction *trans);
> 
> I don't actually mind if it's done like this, but I think it's only moving the 
> problem from one place to several place, which is usually a bad thing.  If 
> the structure is created from outside the matcher, I will need to keep a 
> static or global variable in the OFX module, so that each time a 
> ofx_proc_trans_cb is called from LibOFX, the transactions will go to the same 
> matcher.  


I agree that you will need to pass the pointer of the GenericImporter 
into each of your ofx_proc_transaction_cb (@§%$! could you please quote 
the correct name of your functions? Having to guess the right name on my 
own doesn't really increase the fun of understanding other people's 
code.) In fact you need mechanisms like that quite often. The usual 
implementation of that (in C !) is that a callback function has an 
additional parameter,

foo_callback(something *previous_args, void *user_data)

where the programmer of callback knows the type of what user_data points 
to and therefore can simply cast the pointer to the right type. Voilà, 
now you can pass anything to into your callback.

Speaking of passing into the callback: How does your libOFX know which 
functions it is supposed to call as callback? From inspecting 
src/import-export/ofx/gnc-ofx-import.c I can't tell. Actually since you 
don't use the function names (e.g. ofx_proc_transaction_cb) anywhere 
else in the gnucash code except in its definition it seems that libOFX 
simply calls those functions by their name and hopes that they exist 
"somewhere"?!? This is really not what I would consider good coding 
practice. Because it means that from only inspecting the gnucash code, I 
cannot know at all which functions are going to be called from libOFX. 
Also, how can libOFX know that those functions are really the right ones 
it wanted to call? Also, what if you have several possible usable 
callback functions and have to choose one out of them at runtime? The 
usual (correct?) way of callbacks in C from another object goes like 
this: At some place, the address of the cb function is passed to that 
other object:

other_object_set_cb ( & my_callback_function );

where you would define a type for the callback_function and the set_cb 
function as

typedef return_type (*Object_CB)(type1 arg1, type2 arg2);
void other_object_set_cb (Object_CB callback_function);

The definition of my_callback_function then has to match the typedef of 
Object_CB, otherwise you'll get a compiler error:

return_type my_callback_function (type1 arg1, type2 arg2);

Once you do this, it is also obvious how other (runtime-dependent) data 
can be passed from the other_object back to the callback function: You 
just add this void *user_data argument to both the typedef of the 
callback and the set_cb function. This modifies the above example to

typedef return_type (*Object_CB)(type1 arg1, type2 arg2, void *user_data);
void other_object_set_cb (Object_CB callback_function, void *user_data);

return_type my_callback_function (type1 arg1, type2 arg2, void *user_data);
other_object_set_cb ( & my_callback_function, my_user_data);

and the only thing to add is that at each point inside other_object 
where the callback is called, the value my_user_data has to be passed to 
the callback function.

> Which implies that we will also have to define a 
> gnc_import_importer_complete_cb(GenericImporter *importer) to tell the OFX 
> module that the matcher has 
> been closed by the user and should now be destroyed.  


No you don't need that. Please first implement the correct callback 
mechanism as described above. Otherwise we'll pretty much talk about 
different things in our discussion.

If you want to see how this works in practice, have a look at 
src/import-export/hbci/hbci-interaction.c. At the very end, in the call 
to HBCI_InteractorCB_new, all the callback functions are set. Also, the 
very last argument to that function is precisely that user_data thingy 
mentioned above. You can see how the user_data is cast to the actual 
type in each of the actual callback functions.

>>* Same point (different programming styles): I would kindly ask you that if
>>you define function prototypes in a header file, *please* put the
>>implementation in a .c file *of the same name*. 
> 
> I don't really understand the benefit.  To have a coherent and easy to 
> understand API for the generic import infrastructure, I think all public 
> functions should be in a single header file, so that this file can be 
> included by the protocol specific module.  


No. If each function of the generic importer does *not* depend on any 
other function, then it is perfectly valid to have several totally 
independent header files, and have the protocol module include only 
those headers files of functions that it actually uses. This is not a 
confusing API but instead this is just the perfect representation of 
what is actually going on in the code. In C, an API will always consist 
of a *bunch* of header files. There's no point in "trying to work around 
that" or similar, as long as you stay in C. If, on the other hand, some 
generic-importer functions *do* depend on each other, then you would 
include their headers in each other's headers (... got the point? :), so 
that the headers will form a dependency graph. And that again would only 
be the perfect representation of what is actually going on in the code.

> I think 
> one header for each C file is usefull if you want to have prototypes for 
> private functions, but I've never seen that anywhere in GnuCash.  


No. First, at some places in Gnucash this in fact does exist 
(src/engine/*P.h). Second, the criterion simply is that every function 
that appears in more then one file belongs into the header (which means 
"private header files" only make sense if you have some "private" 
functions that are used from more than one file, as is the case in 
src/engine). Third, every function that is *not* used from more than one 
file (i.e. is only used in the same file where it's defined) should 
*not* go into the header -- this is what you usually see all through 
Gnucash and for the whole lot of GUI functions. Forth, we come back to 
my comments above: If there is a C function, and it is used from 
somewhere else, then for the shoot of it there needs to be a header. And 
by convention (which I'm now asking you to adhere to) the header file 
has to have the same name as the implementation file, cause otherwise 
we'll have to search around like stupid. If those headers files end up 
to be totally independent from each other, requiring the protocol-module 
to include a whole lot of headers,  then this is just the representation 
of what's going on. If those headers end up to depend on each other like 
crazy, then the protocol-module will include only the "last leaf" of the 
dependency tree and by this will have all headers included.

>>confirming that). In other words: You shouldn't feel disappointed because
>>nobody else is editing the files you do -- instead, this has just proven to
>>be the most effective way of dividing up the work, especially in OpenSource
>>projects. 
> 
> Well, from that point of view, I think you are right.  But it would still have 
> been nice to have negative comments on the API design while it was only that: 
> a design document.  I might have ignored some of it, but i could have 
> justified it as i went.  I received only thumbs up for the design document, 
> and started receiving negative feedback only once I had working code, which 
> was kind of demoralizing...


I totally understand your point. Nevertheless, after I thought about it 
for some more time, I came to the conclusion that the *real* point here 
are the objectives: What objectives do we have at Gnucash? The objective 
is to deliver a good application to the user. Therefore it's quite 
pointless to have a discussion about API design here -- if at the end of 
the day the user experience is satisfying, I don't give a darn shoot 
about whatever API was used to achieve this. Really. With the obvious 
exception that for the important data types (Transaction, Split, 
Account) we have to get them right in the first place. But an "API for 
the generic importer"? Honestly, the design of that API doesn't interest 
me at all as long as the user experience is fine. Which, in your case, 
by definition couldn't be tested until after you started coding. Which, 
in turn, explains why "negative feedback" hasn't been brought up earlier.


> Oh well, the hard part is behind us in any case.  Once destination matching is 
> implemented, I'll be able to turn my attention back to LibOFX, for which I 
> have big projects (And since GnuCash is my testbed, it will also gain OFX and 
> QIF export, and possibly OFC import in the process...)  


Sounds good. BTW I might still get into some "coding flash" this weekend 
  and quickly implement the destination matching. In that case it would 
already be in 1.8.0.

Christian