Benoit Gr�goire wrote:
Err... are you saying that the destination account matching *has* to be onOk, 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.
the same page as on the transaction matching/duplicate detection? I can easily imagine those happening in different
windows (or druid pages or whatever).
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).
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.* 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 likevoid gnc_import_add_trans(GenericImporter *importer, Transaction *trans);
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 ifI 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.
you define function prototypes in a header file, *please* put the
implementation in a .c file *of the same name*.
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 becauseWell, 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...
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.
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
_______________________________________________
gnucash-devel mailing list
[EMAIL PROTECTED]
http://www.gnucash.org/cgi-bin/mailman/listinfo/gnucash-devel
