Forgotten patch: qofid.diff

[Chris Shoemaker]

On Wed, Oct 26, 2005 at 07:14:08PM +0100, Neil Williams wrote:
> On Tuesday 25 October 2005 3:18 pm, Chris Shoemaker wrote:
> > Subject: [qofid.diff] Minor tweaks to Qof Code in src/engine.
> >
> > Neil, I don't know if this patch messes up your plans.
> 
> No, but I am curious about why some of it is needed.
> 
> Paid employment commitments prevented me replying earlier.
> 
> > The only 
> > essential thing here is qof_collection_get_list().  If we have to, I
> > suppose we could pull this into the budget namespace until the next
> > qof release.  What do you think?
> 
> I'm not sure about the GList - I'd like to know how you want to use it and why 
> qof_collection_foreach isn't adequate.

It's certainly not strictly necessary, since I could always use
qof_collection_foreach to generate a GList.  I just noticed that other
objects were jumping through various hoops to obtain a GList like
this.  (For example, when they want to get a count of how many such
objects there are, or when writing them to file.)  I also use it to
populate a temporary ListStore for the Budget Selection Dialog.

In every place I used qof_collection_get_list, I could have just
written a static add_to_list callback, and used qof_collection_foreach
to get a GList, but I'm lazy and I didn't want to redo this every
place I needed it, nor even once in Budget's API.  Since other objects
were doing similar things, or worse (like managing a GList stored in
the collection's generic data field) I thought it might be convenient
for everybody.

Also, I didn't want to rely on known relationships letting me get a
GList of Budgets from some other object (like the book) as some
objects do.  And, I certainly didn't want to store a GList in the
generic book data ala commodity_table.

All said, it's just a convenience function.  If you don't like it, we
can just put it in the Budget API.  (Although there /is/ a weird
naming issue: other objects have a GList *
gnc_book_get_{thisobject}(QofBook *book) or something stranger.  That
just seems awkward and not too difficult to avoid if you have
something like qof_collection_get_list.)

> 
> >    - Introduced qof_collection_get_list() for getting GList of entity
> > GUIDs.
> 
> It's QofEntity that you are actually storing in the list rather than the GUID. 
> In which case, I'm not sure I see the advantage over qof_collection_foreach.

Yeah, that description is outdated and irrelavant.  It just so happens
I use get GUIDs, among other things, from the entity.  I think an
earlier implementation actually returned a GList of GUIDs, but I
decided entities was more useful.

> 
> > @@ -158,8 +158,8 @@ void
> >  qof_collection_destroy (QofCollection *col)
> >  {
> >    CACHE_REMOVE (col->e_type);
> > -  g_hash_table_destroy(col->hash_of_entities);
> >    col->e_type = NULL;
> > +  g_hash_table_destroy(col->hash_of_entities);
> 
> (separate minor point):
> Why move the g_hash_table_destroy call?

Heh, actually, I moved the col->e_type = NULL.  :)  It's invalid
immediately, so it's best to drop the pointer immediately.

> 
> > +static void
> > +add_to_list(gpointer key, gpointer val, gpointer data) {
> > +    GList **list = data;
> > +    *list = g_list_append(*list, QOF_ENTITY(val));
> > +}
> > +
> > +GList *
> > +qof_collection_get_list(QofCollection *col) {
> > +    GList *list = NULL;
> > +    g_return_val_if_fail (col, NULL);
> > +
> > +    g_hash_table_foreach(col->hash_of_entities, add_to_list, &list);
> > +    return list;
> > +}
> 
> Why do you need this GList call at all? I'm not sure I see why you need to 
> convert the QofCollection hashtable into a list. What does it gain compared 
> to qof_collection_foreach?
> 
> > +/** Get a list of entities in the collection.  Caller owns the list. */
> > +GList * qof_collection_get_list(QofCollection *col);
> 
> To me, this defeats the main purposes of a primary collection - to keep the 
> e_type identifier and the GUID hash table key. You can't look up entities 
> from the converted list and the list isn't limited to only one e_type.

Well, you can, but that's not what it's for.  It's just another type
of iteration.  And sometimes, you *do* need a list.  It's just a
question of who should make it.

-chris