[Gnucash-changes] r13132 - gnucash/trunk/src/engine - Keep QOF aware of the dirty-state of Transactions.

[Chris Shoemaker]

On Mon, Feb 06, 2006 at 01:47:12PM -0500, Derek Atkins wrote:
> Quoting Chris Shoemaker <c.shoemaker at cox.net>:
> 
> >mark_trans() is horribly named.  It's a fundamentally different
> >operation than qof_instance_set_dirty().  _Most_ of the places we call
> >qof_instance_dirty (11 of them) we don't really want to dirty the
> >splits.
> 
> I admit I haven't looked at the code recently..  Part of the issue is that
> in XML the Splits aren't listed on their own -- they only exist within
> a Transaction -- so you kinda need to mark the transaction as dirty when
> you change a split.

Yeah, I don't know why, but, looking at the code more closely, we
don't even actually keep dirty state on Splits at _all_.  mark_split()
just flags the account _balance_ and _sorting_ as dirty.  I don't see
any good reason not to actually keep dirty state for the splits.  Then
you can ask a transaction if it has dirty splits, something we can't
currently do.

> I'm not sure in what cases you ever want to or need to mark a split
> dirty just because the transaction changed.

Well, like I said, the two existing cases are actually just dirtying
computed-state (not stored-state) in the Account.  I think they're
both correct: changing the trans date, and the trans currency.  But, I
don't think these cases should ever dirty the stored-state of the
Account, and I think it should dirty the stored-state of the Split iff
the split value has to be re-expressed in a new denominator.

> >>On a separate (but related) note, I want to separate extend the events
> >>in order to differentiate between "Account data modified" and "account
> >>contents modified" -- where the former is emitted by the new/edit account
> >>dialog, and the latter is emitted by the register...
> >
> >Absolutely, and this is _very_ related, because the same case applies
> >to Transactions.  The mark_trans() cases should correlate with
> >emitting the "child_changed" signal (or whatever), while the
> >qof_instance_set_dirty() cases should correlate with emitting just the
> >"changed" signal (or whatever).  And in two places, we should emit both.
> 
> We need to come up with a good set of signals here.  "MODIFIED" isn't
> sufficiently robust.  :)   Unfortunately "INSERTED" and "REMOVED" are
> already used, and used IMHO in weird ways.  :(

Definitely.

> >This is a very useful distinction for both Accounts and Transactions.
> 
> ... and Invoices.
> 
> Indeed, another question is who owns the link.  For example, I think
> the Split<->Account mapping is backwards.. Right now you need to 
> Begin/Commit
> on the Account, but it's really the Split that contains the mapping.
> The XML (and SQL) data for an account does not keep track of the splits.
> The split keep track of the account.  So, shouldn't it be 
> xaccSplitSetAccount()
> instead of xaccAccountInsertSplit(), and shouldn't that be wrapped around
> a Split/Trans Begin/Commit edit?

Yeah, I think you're right.  I can't think of any case where modifying
a Split should require a begin/commit edit on an Account.

> (I'll note that we still need the xaccAccountInsertSplit(), but I think that
> only should be called "interally" by xaccSplitSetAccount(), and it doesn't
> necessarily need a begin/commit edit because it's only structural data,
> not real data).

Exactly.  I'd be more emphatic.  s/doesn't necessarily/really shouldn't/  :)

-chris