New QOF type, QOF_TYPE_COLLECT and recursive entity copying

[Derek Atkins]

MOved to gnucash-devel....

Quoting Neil Williams <linux at codehelp.co.uk>:

> On Monday 09 May 2005 9:46 pm, Derek Atkins wrote:
> > Neil,
> >
> > A couple of comments on this patch.  I'll note that I haven't looked very
> > closely at it, but in my cursory examination I see a number of issues:
> 
> I committed this too early - there were bits that were unfinished and bits 
> that should have been cleaned up. My bad.
> 
> However, I simply don't understand your objection to GncEntry being a 
> QOF_TYPE_COLLECT from GncInvoice - see later.
> 
> > 1) You introduce a number of memory leaks.  For example, when you just
> > reset the address pointer to a passed-in object you potentially leak
> > memory.
> 
> Is this the correct solution?
> 
> void
> qofCustomerSetAddr (GncCustomer *cust, QofEntity *addr_ent)
> {
> 	GncAddress *addr;
> 
> 	if(!cust || !addr_ent) { return; }
> 	addr = (GncAddress*)addr_ent;
> 	if(addr == cust->addr) { return; }
> 	if(cust->addr != NULL) { gncAddressDestroy(cust->addr); }
> 	cust->addr = addr;
> }

Yes, this would be more correct. :)

> > You have similar issues in  places like qofOwnerGetType() where I
> > really don't think you should be returning g_strdup'd strings when the
> data
> > is constant.
> 
> Fixed. Thanks.
> 
> > 2) You add a number of APIs that really don't belong.  There is absolutely
> > no reason to export the invoices list of entries as a QofCollection (or a
> > KVP frame).
> 
> The KVP frame was redundant, it wasn't meant to be committed. The 
> QofCollection, however, I need.
> 
> GList* gncInvoiceGetEntries(invoice) already existed - it was already public.
> 
> I didn't change that. QOF cannot deal with the GList directly so I use a 
> QofCollection instead. Then all I've done is a set routine to complement the
> 
> get routine that already existed.

Why can't QOF be changed to deal with the GList directly?  Why does the "convert
the GList into a collection" code have to be duplicated in every place where it
has a list of objects, rather than putting that code in a central place in
QOF/QSF?

> QOF_TYPE_COLLECT (QofCollection) is the new QOF type, it's not a regular 
> collection, it's a secondary collection that is used to pass references to 
> many entities of the same type in one parameter. It doesn't contain all 
> entities from the book, it contains all entities associated with the 
> parameter. In this case, it's every (QofEntity*)GncEntry in the GList* of the
> 
> specific GncInvoice*.  The collection is written out as a series of tags and
> 
> passed back to the invoice as one. The collection is also compared as one - 
> using a deep compare routine. During a merge, the collection is updated as a
> 
> single block.

I don't agree that it should work this way.  When you merge accounts do you
merge the whole list of transactions as a single block?

> QOF_TYPE_COLLECT is being used, in this example, as a "typed list". It's a 
> block of QofEntity pointers with a guarantee that all entities are of the 
> same e_type. In the backend, this is converted to a block of pointers to 
> QofEntityReference that contains the GUID and the type of each. This is then
> 
> written to QSF. I cannot simply write out the GUID using the list, QOF needs
> 
> the type, hence a collection.
> 
> > I'd be less concerned if this were a private API -- but it 
> > certainly does not belong in the .h file and exported from the module.
> 
> ? This is the only problem that remains.

I don't know.  (sorry, I'm on the road so I only have limited time to actually
ponder these issues).

> ?? How else am I to obtain and set the entries for the invoice? GncEntry is 
> exported in the current v2 XML so it isn't transparent like Owner. QOF has to
> be able to create the entries for the invoice and this means accessing the 
> list. Doesn't it?

In the v2 gnc-xml the GncEntry is a top-level object, just like a Transaction. 
It's exported separately (just like transactions) and loaded and linked via
GUID references.  It's not exported as a subtree.

Note that it IS possible for a single GncEntry to be referenced by TWO invoices!
 In particular it can be referenced by a Vendor Invoice and then again by a
Customer Invoice.

> What have I missed? QOF is working from the invoice down - it locates the 
> Customer etc. via the GncOwner Collection, it locates the billterms using a 
> one-to-one reference. The invoice owns the entries (yes?), so whether the 
> invoice has been posted or not, QOF should be able to access the entries from
> the invoice object. How else can I create a new invoice - with entries - in 
> from QOF?

I don't know offhand.  I'm just pointing out that there are issues with the
current approach.  That doesn't mean I know how to solve those issues, I'm
afraid.

> The business objects are written to v2 XML via Scheme and it's complete 
> mumbo-jumbo to me (even the generated C code). I can't tell how the Invoice 
> and the Entry are currently processed from the v2 XML and there's absolutely
> *no* documentation.
> :-(

HUH???  There's no scheme at all involved in writing the business objects into
v2 gnc-xml.  All the C code was hand written (although it was mostly a
copy-and-paste job by yours truly)..  Look in src/business/business-core/file
for the source code.  No, there's not much documentation in how the objects are
stored.  Hint: each first-class object is stored by itself.

> I don't understand why I cannot use a secondary collection typed list for 
> pointers to the entries within the invoice. OK, I know it's a hash table 
> internally, but that is why it is so useful - it's so easy to lookup and 
> ensure you aren't referencing the same entity twice.

See above about how you handle the fact that the same entry could be in multiple
invoices.  Also, I don't think that code belongs in the invoice module, and it
certainly doesn't belong in the public api.

> It's quite separate from qof_book_get_collection - at no point is the entity
> itself changed, it remains within it's primary collection. Adding an entity 
> to a secondary collection does not change the primary. It's about references
> and recursion.

Fair enough..  Except that in the case of a GncOwner how do you make sure that
you have a collection with one and only one entity?  (I've already mentioned
the multiple-reference thing ;)

> > 3) I don't see why you need a "qof_temp" storage in the GncOwner object. 
> > It seems superfluous.
> 
> I changed qofOwnerSetEntity and didn't twig that qofOwnerSetType and 
> qofOwnerSetOwner were left hanging without much to do. Removed.
> 
> > 4) You change a lot of style structure which makes it harder to read your
> > patches.  :)
> 
> Now there's two things going on there, first the majority of all those
> changes 
> are Doxygen fixes. Anywhere that I've changed:
> -/** @name Create/Destroy Functions */
> -/** @{ */
> +/** @name Create/Destroy Functions 
> + @{ */
> 
> Those were generating horrible artefacts in the Doxygen output - prefixing 
> function declarations in the named block with * - confusion was inevitable.
> const char* myfunc(); // OK
> *const char* myfunc(); // NOT OK
> 
> Only in GncInvoice did I inadvertently change the spacing in the declarations
> 
> - that was only 5 lines.
> :-(

There were a number of places where you changed:

   foo *      fooobj;

to

   foo       *fooobj;

Is doxygen stupid enough not to understand the former?  I also noticed a place
where you changed:

   foo *fooobj = g_new0(foo, 1);

to:

   foo *fooobj;
   foo = g_new0(foo,1);

There's no reason to do that...   IMHO...  It's just a style thing.  Unless, of
course, doxygen is so stupid that it can't recognize that..  :(

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available