New QOF type, QOF_TYPE_COLLECT and recursive entity copying

[Neil Williams]

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;
}

> 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.

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.

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.

?? 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?

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?

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.
:-(

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.

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.

> 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.
:-(


-- 

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-patches/attachments/20050510/e681d348/attachment.bin