On Gnucash, G2, and Architecture (was Re: [Gnucash-changes] Eliminate a double free of memory.)

[Chris Shoemaker]

On Fri, Jun 03, 2005 at 04:31:03PM -0400, Derek Atkins wrote:
> Chris Shoemaker <c.shoemaker at cox.net> writes:
> 
> >> > It's better to design out a bug than to patch it out. 
> >> 
> >> Normally I would agree with you.  In this particular case, I tried to do
> >> that three years ago and ended up throwing up my hands and declaring
> >> that I had better things to do.
> >
> > I can sympathize.
> 
> For the record, I looked at this a long time ago as well.  At the time
> I concluded that all destruction of data should happen in the GTK
> "destroy" callback, because not all dialogs use the component manager.
> This way all dialogs are consistent.  You see, all of them DO use
> gtk. :)
> 
> This means that all the component manager callback needs to do it
> destroy the widget (call gtk_widget_destroy() on the dialog) and
> return.
> 
> Consistency is a good thing.

BTW, FTR, there are some examples of better usage in the tree. E.g. I
just ran across gnc-ledger-display.c:

The CM close handler is:
static void close_handler (gpointer user_data)
{
  GNCLedgerDisplay *ld = user_data;

  if (!ld) return;

  gnc_unregister_gui_component (ld->component_id);

  if (ld->destroy) ld->destroy (ld);

  gnc_split_register_destroy (ld->reg);
  ld->reg = NULL;

  xaccFreeQuery (ld->query);
  ld->query = NULL;

  g_free (ld);
}

Notice how the component frees its own mem after unregistering with CM
and calling destroy on the register.

The register destroy is:
void gnc_split_register_destroy (SplitRegister *reg)
{
  if (!reg) return;

  gnc_split_register_cleanup (reg); // This grabs state and commits the split.

  gnc_table_destroy (reg->table);
  reg->table = NULL;

  /* free the memory itself */
  g_free (reg);
}

Notice how the register destroy doesn't know anything about the ledger
object.  Children of g_object are refcounted so they shouldn't do the
g_free step, but it's correct for the SplitRegister struct, since it's
not refcounted.

-chris