[Gnucash-changes] r12999 - gnucash/trunk/lib/libqof/qof - Re-enable logging for GnuCash modules that haven't explicitly set their

[Chris Shoemaker]

On Sat, Jan 28, 2006 at 09:17:51PM +0000, Neil Williams wrote:
> Actually, it was in the docs all along:
> http://cvs.gnucash.org/docs/HEAD/group__Trace.html#ga7
> void qof_log_set_level_global   (   QofLogLevel   level    ) 
>      
>    Set the logging level for all known log_modules.
> Note: Unless a log_module has been registered using qof_log_set_level, it will 
> be unaffected by this change.

Think about this.  _When_ is this function supposed to be called?

> 
> http://cvs.gnucash.org/docs/HEAD/group__Trace.html#ga14
> void qof_log_module_foreach   (   QofLogCB   cb, 
>   gpointer   data
>   ) 
>      
>    Iterate over each known log_module
>  Only log_modules with log_levels set will be available.
> 
> http://cvs.gnucash.org/docs/HEAD/group__Trace.html#ga15
> gint qof_log_module_count   (   void    ) 
>      
>    Number of log_modules registered
> 
> 
> What that all means is that the API was not broken when it moved to libqof, 
> just clarified.

The docs are clear that the API is broken.  Setting a global level
that has no effect on logging modules you don't explicitly register
MAKES ZERO SENSE!  It's USELESS!

> Where before there was an assumption (based on the explicit list of defined 
> modules in gnc-trace.h) that everything listed was logged - QOF now makes it 
> explicit that we start with a clean slate each time. You decide which modules 
> you want logged - QOF makes no such assumptions. Chris, your patch 
> reintroduces this assumption by the backdoor and undermines the API. Did you 
> look at the Doxygen output? Is it unclear? I'll gladly enhance the 
> documentation if you don't find it sufficiently useful.
> 
> Note that even with gnc-trace.h, if a module was not listed in the hardcoded 
> list, it was also ignored.

Ah, but they _were_ listed and they all WORKED.  You're breaking
things and making GnuCash _harder_ to debug with changes that aren't
even a good idea.  The logging system _needs_ to work.

> Instead of changing gnc-trace all the time, the API allows a more flexible 
> approach that automatically keeps what we had from the old lists synchronised 
> AND ensures that any new module can be defined for logging WITHOUT any 
> changes to any file below lib/libqof.
> 
> This was an essential step in the use of QOF outside gnucash because the hard 
> coded list was completely irrelevant for any non-gnucash process.
> 
> I accept I did not post to the list about the removal of this assumption - 
> instead I did clearly document the results in the Doxygen output by making it 
> explicit in the qoflog.h file.
> 
> Please, Chris, can we revert your change and ditch the assumption?

No.  The assumption is sane and GnuCash depends on it.  There is _no_
other way to globally control the logging level in the presence of
dynamically loaded code.  

BTW, notice that the only way you could have kept GnuCash's logging
system working with this broken API would have been to collect all the
used logging modules into one place and register them before setting
the global level, which would have basically been trading the
gnc-trace.h for a bunch of function calls.  Not smart.

If anything should be reverted it's the commit that originally broke
the logging.  If you want to try to drastically change the way
gnucash's logging system works I suggest that you make your proposal
to gnucash-devel, build some consensus, and respond to constructive
criticism.

Your breaking things by making major changes with no approval is bad
enough.  Complaining when people fix things is BAD FORM.

> If there are issues you want clarified in the docs, I'll gladly do that, but 
> this patch is just plain wrong.

The docs do need to be updated - to reflect this change.

-chris