[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