Re: The Table menu patch; another try

[Owen Taylor <otaylor redhat com>]

Bunch of small comments below. I couldn't think of anything that
was wrong on a larger scale; it looks nice and clean and like
it should work.

gtk_menu_attach() needs docs, of course.

Regards,
					Owen


====
+  /* info used for the table */
+  guint rows;
+  guint columns;
+
+  guint *height;
===

'heights' might be better naming.

===
+#define CHILD_INFO_KEY "gtk-menu-child-info"
+
===

This looks unused.

===
+static void
+gtk_menu_free_private (gpointer data)
+{
+  GtkMenuPrivate *priv = (GtkMenuPrivate *)data;
+
+  g_free (priv->height);
+}
===

Should GtkMenu over to instance-private-data, then this
can just be done in the finalize.

===
+ gtk_container_class_install_child_property (container_class,
+                                             CHILD_PROP_LEFT_ATTACH,
+                                             g_param_spec_uint
("left_attach",
+                                                               _("Left
Attach"),
+                                                               _("Left
attach position of the menu item"),
+                                                               0,
UINT_MAX, 0,
+                                                              
G_PARAM_READWRITE));
==

The GtkTable blurbs are a bit better:

								 _("The column number to attach the left side of the child to"),

(GtkTable uses "Left Attachment" for the name, but I prefer "Left
Attach" as
you have it here.)


+static void
+gtk_menu_set_child_property (GtkContainer *container,
+                             GtkWidget    *child,
+                             guint         property_id,
+                             const GValue *value,
+                             GParamSpec   *pspec)
+{
+  GtkMenu *menu = GTK_MENU (container);
+  GtkMenuPrivate *priv;


====
+  AttachInfo *ai = g_object_get_data (G_OBJECT (child),
ATTACH_INFO_KEY);
+
+  if (!ai)
+    {
+      ai = g_new0 (AttachInfo, 1);
+      g_object_set_data (G_OBJECT (child), ATTACH_INFO_KEY, ai);
+    }
+
+  g_assert (ai != NULL);
====

 A) I think it would be good to encapsulate this in a function,
    you have it in two places.
 B) The assertion seems a little bit silly to me
 C) Although I don't think it practically speaking matters,
    it would seem cleaner to remove the data in remove().

 

+  priv = gtk_menu_get_private (menu);
+
+  switch (property_id)
+    {
+      case CHILD_PROP_LEFT_ATTACH:
+        ai->left_attach = g_value_get_uint (value);
+        break;
+      case CHILD_PROP_RIGHT_ATTACH:
+        ai->right_attach = g_value_get_uint (value);
+
+        if (ai->right_attach >= priv->columns)
+          gtk_menu_resize (menu, priv->rows?priv->rows:1,
ai->right_attach);

The argument here to gtk_menu_resize() isn't necessary, since 
the loop in gtk_menu_resize() actually goes over all children
including this one.

The logic in gtk_menu_resize() seem far too complex as well.
I think all you need is, at the end of this function, somethign
like:

 priv->columns = MAX (priv->columns, ai->right_attach);
 priv->rows = MAX (priv->rows, ai->bottom_attach);

====
@@ -768,10 +946,53 @@ gtk_menu_real_insert (GtkMenuShell     *
 		      GtkWidget        *child,
 		      gint              position)
 {
+  gint i;
+  GList *children;
+  GtkMenuPrivate *priv;
+
   if (GTK_WIDGET_REALIZED (menu_shell))
     gtk_widget_set_parent_window (child, GTK_MENU
(menu_shell)->bin_window);
===

Doesn't gtk_menu_attach() handle this?
 
===
+  priv = gtk_menu_get_private (GTK_MENU (menu_shell));
+  if (priv->in_attach)
+    return;
===

I'd like to see a comment about here ... it's not at all
clear to me how this recursion happens.

+
+  if (position < 0)
+    {
+      /* attach after the last row */
+      i = g_list_length (menu_shell->children) - 1;
+      gtk_menu_attach (GTK_MENU (menu_shell), child,
+                       0, priv->columns?priv->columns:1,
                                        ^^           ^^
                                           add spaces
(occurs elsewhere as well)

===
   attributes.colormap = gtk_widget_get_colormap (widget);
   
   attributes.event_mask = gtk_widget_get_events (widget);
+
   attributes.event_mask |= (GDK_EXPOSURE_MASK | GDK_KEY_PRESS_MASK |
 			    GDK_ENTER_NOTIFY_MASK | GDK_LEAVE_NOTIFY_MASK );
===

Random whitespace change

====
+      gtk_container_child_get (GTK_CONTAINER (menu), child,
+                               "left_attach", &l,
+                               "right_attach", &r,
+                               "top_attach", &t,
+                               "bottom_attach", &b,
+                               NULL);

This occurs in a lot of places, maybe have

 get_child_attach (child, &l, &r, &t, &b);

? (You could bypass gtk_container_child_get() or just use
child->parent, either way no reason to pass in menu)

===
+       /* handle all different span types */
+       if (l == (r - 1))
+         {
+           /* spans a single column */
+           requisition->width = MAX (requisition->width,
+                                     child_requisition.width);
+         }
+       else
+         {
+           gint part = child_requisition.width / (r - l);
+           requisition->width = MAX (requisition->width, part);
+         }
===

Isn't the math the same in both cases here?

===
+static void
+gtk_menu_resize (GtkMenu *menu,
+                 guint    rows,
+		 guint    columns)
===

see comments above.

===
+  GList *i;
===

Various parts of GLib use 'tmp_list', 'l' or 'list'. Pick
one of those. 

===
+  for (i = GTK_MENU_SHELL (menu)->children; i; i = i->next)
+    if (i->data == child)
+      break;
+
+  priv->in_attach = TRUE;
+
+  if (!i)
+    gtk_menu_shell_append (GTK_MENU_SHELL (menu), child);
===

How about simply

===
 if (!child->parent)
    gtk_menu_shell_append (GTK_MENU_SHELL (menu), child);
===

====
+static GtkWidget *
+find_super_child (GtkMenuShell *menu_shell,
+                  int           left,
+                  int           right,
+                  int           top,
+                  int           bottom)
+{
+  GList *i;
===

As above.

===
+
+  /* find a superchild which includes the child given by
+   * left, right, top, bottom.
+   */
====

It's not "the child given by", but the "the area given
by", right?

I think find_child_containing() would be a better name
than find_super_child().

===
+      if (match)
+        {
+	  gtk_menu_shell_select_item (menu_shell, match);
+          handled = TRUE;
+        }
+    }
+
+  if (handled)
+    return;
+
+  GTK_MENU_SHELL_CLASS (parent_class)->move_current (menu_shell,
direction);
==

Why not just replace handled = TRUE with return?