Re: bonobo toolbar separator patch

[John Sullivan <sullivan eazel com>]

on 3/20/00 12:50 AM, Nat Friedman at nat@helixcode.com wrote:

> 
> John Sullivan <sullivan@eazel.com> writes:
> 
>> 2000-03-17  John Sullivan  <sullivan@eazel.com>
>> 
>> * bonobo/bonobo-uih-toolbar.c:
>> (toolbar_toplevel_item_create_widgets): Check for NULL
>> toolbar_item before reffing and adding to hash table.
>> The separator case does not create a widget. Added comments
>> explaining this and why it is bad (can't remove later).
> 
> This looks fine; we should make sure that we bail gracefully when we
> try to remove the widget later.  Can you look at this?

OK, here's the deal. The code was already trying to bail gracefully when
removing the widgets of an item with no widgets. But that code had a
memory-trashing bug, as follows:

Old (in toolbar_toplevel_item_remove_widgets):

    found = g_hash_table_lookup_extended
               (uih->top->path_to_toolbar_item_widget, path,
               (gpointer *) &orig_key, (gpointer *) &toolbar_item_widget);
    g_hash_table_remove (uih->top->path_to_toolbar_item_widget, path);
    g_free (orig_key);
 
    /*
     * Destroy the widget.
     */
    if (found && toolbar_item_widget != NULL)
        gtk_widget_destroy (toolbar_item_widget);


The bug is that "found" isn't respected by the g_free (orig_key) line.

Proposed fix:

    found = g_hash_table_lookup_extended
               (uih->top->path_to_toolbar_item_widget, path,
               (gpointer *) &orig_key, (gpointer *) &toolbar_item_widget);

    if (found) {
        g_hash_table_remove (uih->top->path_to_toolbar_item_widget, path);
        g_free (orig_key);

        /*
         * Destroy the widget.
         */
        if (toolbar_item_widget != NULL)
            gtk_widget_destroy (toolbar_item_widget);
    }

By the way, should that toolbar_item_widget != NULL test be an assert
instead of a test?

===

Should I go ahead and check this in?

John