patch for removing menu item code

[John Sullivan <sullivan eazel com>]

Darin and I chased down a Bonobo menu problem we were experiencing in
Nautilus to the following cause:

In  bonobo_ui_handler_menu_toplevel_remove_item_internal, if there are
multiple entries for the same path, the code has a bug. Only the first
(head) entry for the path has a widget, by design. This routine calls
menu_toplevel_get_item, which returns this head entry, then checks
whether it is the head entry (yes) before calling
menu_toplevel_remove_widgets. So far so good. Now it calls
menu_toplevel_get_item again, which returns the next entry. This one was
not the head entry, and didn't have a widget, by design. But it
currently looks like the head entry, since we just removed the real head
entry, so the is_head test passes and menu_toplevel_remove_widgets is
called, but there is no widget, so you hit a return_if_fail.

We discussed three possible fixes:

(1) Change menu_toplevel_remove_widgets so it exits silently if it
doesn't find a widget. We rejected this because removing the
return_if_fail could mask other tricky bugs.

(2) _remove_item_internal could recurse with TRUE for the "replace"
parameter. This is the only caller that passes FALSE, so that parameter
could be eliminated. The parameter is an optimization to save the work
of creating widgets right before destroying them. We rejected this
because we didn't want to remove a deliberate optimization (perhaps the
person who wrote the optimization had reason to believe that it made a
noticeable impact on performance).

(3) Add a new private call that returns an entry for a path, like
menu_toplevel_get_item, but which doesn't return the head item unless
that's the only one. That way the removal code can walk through and
remove all the non-head non-widgeted items successfully before finally
removing the one head item with a widget. This solution has the drawback
that we have to add a new function, but the function is very small and
straightforward and we felt that this drawback was less than the
drawbacks of potential solutions (1) and (2). So that's what we did. The
patch follows. I will check this in later today if nobody tells me not
to.

John

Index: bonobo/bonobo-uih-menu.c
===================================================================
RCS file: /cvs/gnome/bonobo/bonobo/bonobo-uih-menu.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 bonobo-uih-menu.c
--- bonobo/bonobo-uih-menu.c 2000/08/09 19:40:16 1.21
+++ bonobo/bonobo-uih-menu.c 2000/08/17 18:51:52
@@ -593,6 +593,28 @@ menu_toplevel_get_item (BonoboUIHandler
  return (MenuItemInternal *) l->data;
 }

+/*
+ * This function grabs a menu item associated with a
+ * given menu path string. It will return the active item only if
+ * no non-active items exist. This lets callers clean up the list
+ * without mucking with the active item's widgets.
+ */
+static MenuItemInternal *
+menu_toplevel_get_item_prefer_non_head (BonoboUIHandler *uih, const
char *path)
+{
+ GList *l;
+
+ l = g_hash_table_lookup (uih->top->path_to_menu_item, path);
+
+ if (l == NULL)
+  return NULL;
+
+ if (l->next != NULL)
+  return (MenuItemInternal *) l->next->data;
+
+ return (MenuItemInternal *) l->data;
+}
+
 static MenuItemInternal *
 menu_toplevel_get_item_for_containee (BonoboUIHandler *uih, const char
*path,
           Bonobo_UIHandler containee_uih)
@@ -2107,7 +2129,7 @@ bonobo_ui_handler_menu_toplevel_remove_i
   while (internal->children != NULL) {
    MenuItemInternal *child_internal;

-   child_internal = menu_toplevel_get_item (uih, (char *)
internal->children->data);
+   child_internal = menu_toplevel_get_item_prefer_non_head (uih, (char
*) internal->children->data);
    bonobo_ui_handler_menu_toplevel_remove_item_internal (
     uih, child_internal, FALSE);
   }