On Tue, 2007-01-30 at 22:23 -0500, Peter McAlpine wrote: > I look forward to hearing your feedback regarding the patch and > feature proposal.
The patch generally looks fine. Thanks for sticking (mostly) to the coding style convention in the existing files ... even if it's often my old, silly, everything( before_has_too_much_whitespace_and_after ); convention. :) I've applied it as r15486, with the following notes and changes: One problem I have is with the revised interface(s) for 'gnc_sx_get_instances(...)'. By definition, disabled things don't generate instances, so I don't see much need to have that spelled out in the UI; the body of your 'gnc_sx_get_current_enabled_instances' can just move into 'gnc_sx_get_instances(GDate*)' ... at least in spirit; I have stylistic quibbles with the implementation [1]. I've made this change. [1a]: there's no real win by constructing the list backwards and reversing it; computers are sufficiently fast, especially for the very small values of 'n' we're talking about here. [1b]: while loops suck; iterations are a for-loop idiom. [1c]: 'iterator' is never a good name; 'sx_iter' is better. [1d]: 'filtered' isn't a great name; 'enabled_sxes' is better. '_gnc_sx_instance_event_handler' should be modified to ignore non-enabled SXes that it receives updates about; I've added a fixme in the code about it. It'd be nice to add an 'enabled' column to the sx-list model and view; it'll become an optional column when that view switches over to using the GncTreeView, at some point here. There's no need to have configuration, let alone a gconf key, for the default enabled state. SXes are enabled by default. I've removed this. Similarly, there's no need for the "newsxP" branch regarding enabled in the SX Editor setup; the SX is already in the correct default state (enabled) for new SXes, and its value can be set directly from the SX. ---------- The new "enabled" field represents a non-backward-compatible file change, but that's alright ... we'll probably have others. We should probably just release a 2.0.5 that fixes the "XML reader doesn't ignore elements" problem, and buy ourselves some breathing room in the rest of the 2.x series. Tim raises a good point regarding SXes with variables. Similar is multi-currency template transactions (which are actually implmented by creating "fake" variables for the currency conversion rates). I think "estimation" is the right solution for both, though with the pricedb, there's maybe a better chance of forecasting pricedb values. In any case, there's two major options for handling the problem: 1/ handle at the SX level: in the SX editor, and probably a first-order part of the SX itself, allow the user to define the estimation of variables and exchange rates. - Pros: makes sense; general purpose; durable data entry - Cons: not used elsewhere; heavyweight; big code changes (schema, expression parser, SX editor) 2/ handle in the report: somewhere in the report, allow the user to define the estimation for variables. - Pros: simple; isolated; solves the problem at hand - Cons: impermanent data entry; reports are already nasty Reports aren't naturally formatted anything like the register, though some of the General Ledger/Jounral reports -- with the right stylesheet -- might be a reasonable facsimile. Do you really want it to look like a register, or just reflect the values of the accounts? While imagining the report itself, I note that presently the only way to create Transactions from a set of SX instances is by actually creating them. There's no notion of "proposed" transactions [2], and the interface that the GncSxInstanceModel provides is only the variables that need binding. Certainly the SXes themselves have API to get the transaction detail, but it seems like it'd be better to have the template-transaction -> real-transaction conversion only be in one place. Thus, we should probably factor out that code. [2] GnuCash (QOF, really), makes this a bit hard since it's non trivial to create objects that simply aren't on the Books, like Transactions and whatnot. Sometimes code just wants to create a Transaction and Splits that aren't real, and shouldn't be returned in a Query or whatever. On IRC we've talked about some notion of an "excursion", whereby code (hey, exactly like this report! :) could create "provisional" engine objects that have some sort of visibility or scope. We probably don't need to solve that problem right now, but it bears noting. -- ...jsled http://asynchronous.org/ - a=jsled;b=asynchronous.org;echo [EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
_______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
