[Gnucash-changes] r13130 - gnucash/trunk/src/engine - Replace check_open() in setter API with internal qof_{begin, commit}_edit().

Chris Shoemaker c.shoemaker at cox.net
Thu Feb 9 13:52:55 EST 2006


On Thu, Feb 09, 2006 at 12:12:13PM -0500, Derek Atkins wrote:
> Chris,
> 
> Chris Shoemaker <chris at cvs.gnucash.org> writes:
> 
> > Log:
> >    Replace check_open() in setter API with internal qof_{begin,commit}_edit().
> >
> >
> > Modified: gnucash/trunk/src/engine/Transaction.c
> > ===================================================================
> > --- gnucash/trunk/src/engine/Transaction.c	2006-02-06 06:45:25 UTC (rev 13129)
> > +++ gnucash/trunk/src/engine/Transaction.c	2006-02-06 16:18:52 UTC (rev 13130)
> > @@ -1626,7 +1626,7 @@
> >    gint fraction, old_fraction;
> >  
> >    if (!trans || !curr || trans->common_currency == curr) return;
> > -  check_open (trans);
> > +  qof_begin_edit(QOF_INSTANCE(trans));
> 
> I think you want to be using xaccTrans{Begin,Commit}Edit here, and not
> qof_{begin,commit}_edit.  The resaon is clear in a moment...

Just so we're on the same page: I see the two as operating at very
different levels of abstraction.  The qof_{begin,commit}_edit is only
for storage coherency - it knows nothing about data consistency.  And
xaccTrans{Begin,Commit}Edit is the engine's external API for allowing
users to create consistent Transactions.  It _does_ care about data
consistency.

> [snip]
> > -  trans->inst.do_free = TRUE;
> > +  if (!xaccTransGetReadOnly (trans) || 
> > +      qof_book_shutting_down(trans->inst.book)) {
> > +      qof_begin_edit(QOF_INSTANCE(trans));
> > +      trans->inst.do_free = TRUE;
> > +      qof_commit_edit(QOF_INSTANCE(trans));
> > +  }
> 
> qof_commit_edit() knows nothing about inst.do_free.  The do_free is
> only handled by the object's Begin/Commit edit API, not by the lower
> level QOF begin/commit edit API.   

The macro versions actually do handle inst.do_free.  There's terrible
confusion about responsibility delegation between engine objects and
QOF, because the transactional semantics offered by QOF are
inconsistent.

Probably what _should_ happen is that the engine-level object should
maintain its own "deleteme" flag, and then use that during the
engine-level CommitEdit to set the QOF-level do_free, knowing that
once qof_commit_edit() is called on a do_free marked object, it's no
longer valid.

> So this can lead to a memory leak.

Well, I don't think it will leak if the xaccTransDestroy() is
surrounded by engine-level begin/commit.

The problem with using engine-level begin/commit inside the setter
api is that it may (and does, for Transaction) enforce constraints
that aren't valid after every field-set.  In fact, the
xaccTransCommitEdit will happily free any Trans that doesn't have any
split children yet.  That may make sense for engine-level data
consistency constraints, but it sure doesn't make sense to enforce
that inside xaccTransSetDescription().


> Also, by calling the qof command directly you've now made it such
> that you could lose transaction log information.  The log is written
> out in xaccTrans{Begin,Commit}Edit so by bypassing that you lose
> logging info.

I don't think you want to log _in between_ the
xaccTrans{Begin,Commit}Edit, only on the edges.  This is especially
clear when using "temporary" engine objects.  The gui very much wants
to distinguish between "Ok, I'm going to edit a real-live important
object. Please ensure everything remains consistent and coherent."
and "Just give me a temporary (new) object to play with.  Don't ever
save it anywhere or log it anywhere or enforce any inter-object
consistency on it.  Pretty much don't touch it.  I'll free it when I'm
done.  Thank-you-very-much."

I think the best way to make that distinction is to wrap the former
case in the engine-level begin/commit, but not the latter.

I know the current code is far from ideal, but I'm really struggling
to see a way to improve it incrementally without some invasive changes
to QOF.  If you see a path forward, (and it's not one that either a)
requires all external API users to wrap a parameter setting in between
{Begin/Commit}Edit or b) enforces inter-object constraints every each
and every parameter setting) I'm willing to implement it.

-chris


More information about the gnucash-devel mailing list