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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Josh Sled
Hey Neil...

-------- Forwarded Message --------

> Author: jsled
> Date: 2006-02-02 21:09:29 -0500 (Thu, 02 Feb 2006)
> New Revision: 13084
> Trac: http://svn.gnucash.org/trac/changeset/13084
>
> Modified:
>    gnucash/trunk/ChangeLog
>    gnucash/trunk/lib/libqof/qof/qofsession.c
>    gnucash/trunk/src/backend/file/gnc-backend-file.c
> Log:
> Fix overall and ".log"-specific file-retention issues: Bug#329670 (++).

> Modified: gnucash/trunk/lib/libqof/qof/qofsession.c
> ===================================================================
> --- gnucash/trunk/lib/libqof/qof/qofsession.c 2006-02-03 00:47:40 UTC (rev 13083)
> +++ gnucash/trunk/lib/libqof/qof/qofsession.c 2006-02-03 02:09:29 UTC (rev 13084)
> @@ -888,7 +888,13 @@
>   continue;
>   }
>   /* Use the providers creation callback */
> -          session->backend = (*(prov->backend_new))();
> +                        session->backend = (*(prov->backend_new))();
> +                        /* Initialize be configuration. */
> +                        {
> +                                KvpFrame *config;
> +                                config = qof_backend_get_config(session->backend);
> +                                qof_backend_load_config(session->backend, config);
> +                        }
>   session->backend->provider = prov;
>   /* Tell the books about the backend that they'll be using. */
>   for (node=session->books; node; node=node->next)

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...

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

It seems logical that QOF should configure a backend's options after
loading it, and as such the change I made here seems right.  Is there a
better alternative?

--
...jsled
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Neil Williams-2
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/


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Josh Sled
On Fri, 2006-02-03 at 22:10 +0000, Neil Williams wrote:
> 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.

Why does the framework want backends to define interface methods that
the framework doesn't actually use?  What's the point of having a
backend declare its options to QOF?

--
...jsled
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Neil Williams-2
In reply to this post by Neil Williams-2
On Friday 03 February 2006 10:10 pm, Neil Williams wrote:
> On Friday 03 February 2006 2:32 am, Josh Sled wrote:
> > Hey Neil...

OK, after a second look:

This is what I think we should have instead of the change in qofsession.c:
(It's virtually identical, just that it only affects gnc-backend-file.)

Index: gnc-backend-file.c
=======================================
--- gnc-backend-file.c  (revision 13089)
+++ gnc-backend-file.c  (working copy)
@@ -963,6 +963,13 @@
        be = (QofBackend*) gnc_be;
        qof_backend_init(be);

+        /* Initialize be configuration. */
+        {
+             KvpFrame *config;
+             config = qof_backend_get_config(be);
+             qof_backend_load_config(be, config);
+        }
+
        be->session_begin = file_session_begin;
        be->session_end = file_session_end;
        be->destroy_backend = file_destroy_backend;

Or we could simply not bother with the temporary frame and call the GConf
values directly, setting the data in the statics. Either way, a separate
KvpFrame is created when any function outside the backend actually ASKS for
the config options.

+        /* Initialize be configuration. */
+        {
+              file_retention_days = (int)gnc_gconf_get_float("general",
+                   "retain_days", NULL);
+              file_compression = gnc_gconf_get_bool("general",
+                    "file_compression", NULL);
+        }
+

Preferences?

The direct calls to GConf would appear adviseable.

gnc_backend_new is then called automatically by qofsession.c via
prov->backend_new()

> 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.

(Ignore that, we will need the statics - not that this is a problem.)

I'll commit when I've got the docs improved for both this and Chris' original
question about qoflog.c - probably tomorrow evening. (busy week)

--

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


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Neil Williams-2
In reply to this post by Josh Sled
On Friday 03 February 2006 10:36 pm, Josh Sled wrote:
> On Fri, 2006-02-03 at 22:10 +0000, Neil Williams wrote:
> > 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.
>
> Why does the framework want backends to define interface methods that
> the framework doesn't actually use?

Data-centric design.

Just like many other components of QofBackend, this is supported to provide an
interface between the backend and the application. QOF mirrors the
information from the backend (which cannot be linked against the application
directly) to the application.

It's also how the translated tooltip and description strings for each backend
are made available to the application (the main reason for using a KvpFrame
in the first place).

http://www.data-freedom.org/explain.html#example
Therefore any program that can exchange data with QOF can use whichever
backend is suitable. Any other QOF compatible program can read the data, even
if it doesn't use that backend normally. This increases platform
independence, data accessibility and program flexibility.

> What's the point of having a
> backend declare its options to QOF?

So that any application using QOF can query the configuration options for any
QOF backend without having to be linked against the backend itself (which it
can't be in any portable way).

Remember that QOF backends can be installed independently of any application
and vice-versa. gnucash relies on gnc-backend-file and QSF but future
backends will not have such strict limits. Applications will need to be able
to read the configuration of these backend modules at runtime and decide how
to use the available options.

e.g. I plan to use libgda to provide any QOF process with access to any of the
databases supported by the gnome-db project. The various configuration
options of these modules will be passed up to the application via the
KvpFrame. The application will not be able to determine at compile time which
precise backend module is to be used (other than sql://) as it will depend on
which modules the user has installed. All the application will see is a
standard QofBackend that can run queries in the backend, has a local cache of
entities etc., and which has N configuration options that the application may
or may not choose to use.

The KvpFrame is sufficiently flexible for this because although the
application can set any option, the backend only reads the options which it
understands. The reverse is also true - the application can choose to present
the user with only those options it understands, the others are left as
defaults.

QOF processes can only communicate with the backends via qof_session_* and the
supported configuration options. Equally, QofBackends can only receive config
options and a QofSession/QofQuery. QOF must act as the gopher for the config
options, just as it does for the objects themselves.

The application must declare it's objects to QOF so that the backend can
operate; the backend must declare it's options, if any, to the application so
that the application can use the options.

--

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


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Josh Sled
In reply to this post by Neil Williams-2
On Fri, 2006-02-03 at 22:40 +0000, Neil Williams wrote:
> I'll commit when I've got the docs improved for both this and Chris' original
> question about qoflog.c - probably tomorrow evening. (busy week)

That's okay ... I'll fix it up as you describe along side some other
cleanups that I realized should be made.  Thanks for the info about the
backend's semantics.

--
...jsled
http://asynchronous.org/ - `a=jsled; b=asynchronous.org; echo ${a}@${b}`
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Neil Williams-2
On Saturday 04 February 2006 5:26 pm, Josh Sled wrote:
> On Fri, 2006-02-03 at 22:40 +0000, Neil Williams wrote:
> > I'll commit when I've got the docs improved for both this and Chris'
> > original question about qoflog.c - probably tomorrow evening. (busy week)
>
> That's okay ... I'll fix it up as you describe

Still need to revert the change in lib/libqof/qof/qofsession.c - I'll do that
later.

> along side some other
> cleanups that I realized should be made.  Thanks for the info about the
> backend's semantics.

No problem.

I saw the patch - then realised that I'd missed something myself: it will
force a GConf dependency on cashutil if it's shares gnc-backend-file.
:-(

Ooh-err. Wasn't planning on that, but ne'er mind, as long as depending on
gconf is no more cumbersome than depending on libxml2 or libglib-2.0 then I
can cross that particular bridge later - i.e. when I've actually got some
time to develop on cashutil.
:-)

Adding libgconf2-4 appears to only add liborbit2 and libidl0 to the dependency
list - on Debian anyway.

As gnc-backend-file is to be replaced, it may not be too much of a problem.
Besides, it may make sense for cashutil to read at least some gnucash
preferences - I will promise NOT to change any preference data! I'm not that
crazy!
:-)

--

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


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (196 bytes) Download Attachment