Quoting Chris Shoemaker <[EMAIL PROTECTED]>:
> 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.
This goes back to the conversation we had on IRC the other night. Right
now the API isn't very clear about the distinction between storage coherency
and data consistency. It's muddied even worse due to the way the code
logs changes.
As we agreed on IRC, each setter operation needs to use begin/commit
semantics to make sure you have data consistency. Ideally we'd also
log when the change is committed. For the short term I'm willing to
potentially miss some log messages.
HOWEVER, we do need to deal with the fact that qof_commit_edit() doesn't
destroy an object. So that means we really do need to use xaccTransCommitEdit
at least in xaccTransDestroy().
[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.
The problem is that qof_begin_edit() and qof_commit_edit() were added
to the external API. In general you're just NOT supposed to use them,
and you're supposed to use the object-specific begin/commit edit.
The macro is there to help you implement your object-specific begin/commit
edit functions, because frankly they're all pretty much the same.
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.
Nah, no need to do that.
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.
IFF! If it's not, well, the object wont get destroyed. This is what
the check_open() was for in that particular function. ;)
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().
That's arguably true... But remember that those checks are only performed
when the commit nest level reaches zero. The engine user should be
calling BeginEdit() before filling in the transaction so it shouldn't
be an issue. But yes, the db-transactional semantics aren't very clear
and unfortunately you ran smack-dab into the middle of the incoherencies
of the API. :(
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."
The engine really has no concept of a non-saved object or temporary object.
Perhaps this is a bug. I don't know. But it's certainly true that
you cannot create a temporary object. IMHO this is a GOOD THING -- it
prevents accidental data loss.
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.
I see no need to make /ANY/ changes to QOF. QOF is fine. It's the
users of QOF (Transactions, in this case) that are causing the problem.
A simple change would be a couple new private functions that wrap the
QOF object storage consistency and the logging, and then you can use those
internally.. But it WILL share a significant amount of code with
xaccTransBeginEdit and xaccTransCommitEdit -- so it may just make sense
to refactor those functions, or use the QOF BEGIN/COMMIT macros.
-chris
-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
[EMAIL PROTECTED] PGP key available
_______________________________________________
gnucash-devel mailing list
[email protected]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
