[Fwd: [Gnucash-changes] r13084 - gnucash/trunk - Fix overall and ".log"-specific file-retention issues: Bug#329670 (++).]

[Neil Williams]

On Friday 03 February 2006 2:32 am, Josh Sled wrote:
> Hey Neil...

It shouldn't affect other backends but I'm not sure it needs to be done in 
qofsession.c - it causes duplication. Your patch has highlighted where the 
QOF Doxygen docs need more clarification.

The bug is actually in the gnc-backend-file - it should initialise the "be" 
configuration itself in the call to :
session->backend = (*(prov->backend_new))(); 
Repeating the call in the very next line should not be necessary.

> > Fix overall and ".log"-specific file-retention issues: Bug#329670 (++).
> > +                        session->backend = (*(prov->backend_new))();

Instead, put the initialising code in the function called by prov->backend_new 
- in the case of gnc-backend-file, that's gnc_backend_new in 
gnc_backend_file.c, src/backend/file.

I'll modify the documentation to make it clearer what QofBackendProvider 
expects to happen in the backend_new routine. Namely, that a new backend 
should fully setup it's own configuration before _backend_new returns.

> > +      /* Initialize be configuration. */
> > +        {
> > +               KvpFrame *config;
> > +              config = qof_backend_get_config(session->backend); 
> > +              qof_backend_load_config(session->backend, config); +                    
> >    }

The only real difference between the QSF and GNC backend new calls is that QSF 
initialised it's internal context (params) which *explicitly* sets the be 
config values. The GNC backend skipped that stage.

From only a cursory glance at the patch, I think the calls should not be in 
qofsession.c but in src/backend/file/gnc-backend-file.c in the 
gnc_backend_new function. Either directly or via some _init() function for 
other gnc_backend_file parameters.

_backend_new is called every time a backend is created so it would be called 
on the line above where your patch calls it: 

                session->backend = (*(prov->backend_new))();

(without requiring changes in qofsession.c)

(Note that it's not that qofsession.c can't be changed, just that the docs 
need to be clarified so that code is not run twice.)

> I know you'd rather be notified before commits to qof, but I figured
> it'd be easier to see the change in context; I'm in no way married to
> this particular change, but this does seem to work ... I'm curious as to
> your take on what the right solution is...

I suspect we should alter how the defaults are initialised in the 
gnc-backend-file because other backends do not require this step and I'll 
clarify the docs.

> Trying to debug Bug#329670, I noticed that the .xac and .log
> file-cleanup code wasn't being invoked appropriately since the
> file_retention_days wasn't being set. 

OK, I'll follow that up. I've noted that your change is primarily to set 
whatever value is retrieved from gconf as the on-load default instead of 
using static defaults.

> In fact, the gnc-file-backend's 
> get- and load-config code was never actually being invoked by either QOF
> or GnuCash...

Yes, it relied on the static defaults of zero and FALSE respectively. Your 
GConf solution is far better.

> It seems logical that QOF should configure a backend's options after
> loading it,

It's even better if it is done during the load itself. i.e. there should be no 
point at which a backend is available but not configured. I'll ensure the 
docs make this clear.

> and as such the change I made here seems right.  Is there a 
> better alternative?

Yes - a backend specific change and more detailed documentation in doxygen.

Thanks for spotting this, Josh. I'll revert the parts of r13084 that relate to 
qofsession.c and implement them in gnc-backend-file.c

This is the vital line in your addition:
+     option->value = GINT_TO_POINTER((int)gnc_gconf_get_float("general", 
"retain_days", NULL));

If the effect of this line is replicated in gnc_backend_new (with the line for 
the second option), all the problems will be resolved without changes to 
qofsession.c because gnc_backend_new is already called by qofsession in the 
line above your change.

We can either setup the config KvpFrame directly in gnc_backend_new or simply 
set the existing statics to the GConf values when the backend is loaded. If 
we initialise the frame directly, we may not need the statics at all.

How does that sound?

-- 

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20060203/b04046a1/attachment.bin