Created attachment 109107 [details] [review]
Adds some properties and support to get menus loading from builder

Ok, havent had time to put tests yet, but I think its pretty good:

  - added gtk_menu_item_set_label() and gtk_menu_item_set_use_underline()
  - added "label" and "use-underline" properties to menuitem
  - gtk_*_menu_item_new_with_[label/mnemonic]() now simply use
    the properties instead of all that duplicated code
  - gtk_menu_item is buildable and adds child menus correctly
  - gtk_image_menu_item() added construct-only "use-stock" property

The "use-stock" property is tricky and only works from builder
parse finished, since it needs to get the accelerators from the toplevel.

Still need to add tests.

Ok on second thought Ill have to give it another go.

in the image menu item parse_finished I need to get the
real toplevel window it was created for, so that I can 
correctly attach to the accel_group, shouldnt be too
tricky.

also one more thing to do to get menu support complete
is to add support for the custom image, I suppose I might
pull that off by using the IconFactory & stock ids.

I'm still don't think this is a good idea. 
There is already a way of constructing menus using GtkBuilder, there should be no need to have another way of doing the same thing.

The part which uses g_object_new is probably a good idea though, but it's not builder specific and should be filed in separate bug(s).

There is no way of building menus in GtkBuilder strictly speaking,
only a way of building UIManagers, there's quite a difference, when
building menus normally you will be allowed although possibly not
recommended to connect to normal signals etc, and set menu properties
directly, such as custom filename images for image menu items, not
having that these capabilities would be quite a regression.

Normally, you should be able to attach actions to your menu items
and tool items post menu/toolbar creation, also via properties,
I also plan to take care of this.

I agree that a dual standard is a bad thing and would be more than
happy to work on what would need to be done to deprecate the UIManager
and finally have a singe standard ui parser in gtk+: GtkBuilder only :)

As I understand it the only feature remaining would be menu/toolbar
mergeability, this seems a little complex but doable possibly at a 
GMergableIface level, implemented by menus and toolbars.

I *dont* think that full deprecation of GtkUIManager should be a 
requesite of integration of real, direct menu building in gtk+.

You know, I'm a little tired right now - after 14hrs bus, I've
had time to relax and think about it - I dont feel so strongly
about menus in builder anymore.

Taking into consideration that we can generate icons using the 
icon factory to generate menu items with custom icons, there isnt
really regressions concerning old menus, if the general sentiment
is that menus should be built from actions, then lets concentrate
on making sure actions rock in gtk+ instead.

I will definitly still support toolbars in glade, since toolbars
cant be assumed to be "the main application toolbar", and it can
still be usefull to add custom widgets to toolbaritems.

Other than that I think I will still formally propose, with a patch
to the list, that we add a "proxy-for-action" property to GtkWidget 
class, allowing us to discourage direct use of widget signals in 
general... also will come around to close this bug, all this after 
I take a good sleep and make sure I'm thinking straight ;-)

I'm not against the patch per se, just the builder specific parts.
There's really nothing in the patch that should be tied to GtkBuilder. Cleaning up the construction of GtkMenuItems and adding an additional constructors to GtkImageMenuItem sounds like a very sane approach.

However, the add_child(submenu) is a critical part of this patch which cannot easily be replaced by other means. *just* adding that builder specific part to GtkMenuItem would but okay I guess.

I'm not the maintainer of the menus in gtk though, so you'd have to get that part reviewed by Mattias.

The label and use-underline properties look like a nice cleanup for the various menuitem constructors. 
I'm not really sold on the use-stock property yet. At least, the current implementation looks like it can only work in GtkBuilder ? 
If we add such a property, then e.g.

g_object_new (GTK_TYPE_IMAGE_MENUITEM, 
              "label", GTK_STOCK_OK, 
              "use-stock", TRUE, NULL);

ought to work too.

Created attachment 121600 [details] [review]
Better patch

Ok, this patch now allows:

g_object_new (GTK_TYPE_IMAGE_MENU_ITEM,
              "label", text,
              "use-underline", TRUE,
              "use-stock", TRUE,
              "accel-group", group,
              NULL);

(and appropriately fixes gtk_image_menu_item_new_from_stock() to use
that constructor).

The parser_finished() for the imagemenuitem will resolve the accel_group
for stock items automatically.

As for gtk_image_menu_item_set_image(), i.e. the "image" property,
I will in glade make a conversion to actually use that property instead
of the internal-child method of libglade, so no worries there.

Looking pretty good.

Quick review:
 - buildertest should be extended, constructing a normal and one with type=submenu should work as expected. Making sure that everything in _parser_finished() is executed is a good idea
 - You can't add new fields to the end of a public struct, since the size of the struct is used in subclassing for instance. Use the private data if there is one, if there isn't add one.

Created attachment 121603 [details] [review]
Same patch, now frees a string in a newly added gtk_image_menu_item_finalize()

(In reply to comment #10)
> Created an attachment (id=121603) [edit]
> Same patch, now frees a string in a newly added gtk_image_menu_item_finalize()

Please add g_object_notify to all new accessors.

+  if (image_menu_item->label_copy)
+    g_free (image_menu_item->label_copy);

No need to check here, g_free does the NULL-check for free. You might even want to set the pointer to NULL in the _finalize handler.

+  gtk_menu_item_ensure_label (menu_item);
+  gtk_label_set_use_underline (GTK_LABEL (GTK_BIN (menu_item)->child), setting);

I would suggest you go the safe route and check if gtk_bin_get_child actually refers to a label. The current code will end up in a warning if a subclass placed a different widget there.

Created attachment 121609 [details] [review]
Revised patch again

Updated patch as per chirstians comments, now added property
notifications where needed (calling gtk_menu_item_set_label() from
gtkimagemenuitem.c will provoke a notificaion so I didnt add one
there), also made gtk_menu_item_* account for alien GTK_BIN(item)->child
widgets.

Also made GtkImageMenuItem's new struct members private
(I was somehow under the impression that GSEAL() made it
so I didnt need private structures... my bad).

I'll take care of adding tests to gtk/tests/builder.c 
hopefully later tonight...

I'm not sure I like the way in which the label property is duplicated between GtkMenuItem and GtkImageMenuItem. 

And I think the accelgroup handling in GtkImageMenuItem is at least dubious, if not outright wrong. I don't think gtk_image_menu_item_buildable_parser_finished 
has any business creating an accel group. Shouldn't it just do whatever gtk_image_menu_item_new_from_stock does ?

Also, the same comment I made about labels applies here too: menuitems are very common widgets, so I'd like to avoid blowing them up if at all possible.

Matthias, 
   thanks for reading the patch :)

a.) The label property was overridden, and only the value handled
    differently in the image menu item in the case of use_stock,
    I could try avoiding keeping the string around, but its risky,
    I need to have it stored somewhere if use-stock is changed,
    the resulting label will be corrupt (looking up in the stock
    for an ID based on an already rendered label), I dont see how
    that could work without the properties having a precise order
    in which they are to be setup.

b.) inasmuch as the parser_finished calls the same code that
    setting accel-group does with stock set, it works pretty much
    like new_from_stock(), it only exists to invent the accel_group,
    which is definitely a hack imported from libglade.

That being said, I'd love to do away with (b), I'll try removing
buildable completely from gtkimagemenuitem and building an explicit
accel_group for the toplevel window, and setting the property on
the menuitem, setting the "accel-group" should always result in
the menu item updating, and this way we could also afford to drop
the added struct member... I'll see what I can do...

Created attachment 121680 [details] [review]
New and improved

Ok I think I got it right this time :)

So differences from the last patch:
  - Now GtkMenuItemClass takes a set/get_label() vfunc
  - GtkImageMenuItem doesnt override properties, but has
    a different implementation of get/set_label() (I had
    it backwards before obviously).
  - GtkImageMenuItem is *not* a GtkBuildable, if there is
    ever an accel_group set, it will create its accel from the
    stock and connect to the group (provided stock is set
    and available)
  - GtkImageMenuItem knocks off the unneeded "accel_group" pointer
  - GtkWindow now takes an "accel-groups" custom tag to add
    accel groups to a window.

I have a skeleton for tests in there, that I have tested
as a file, but I havent quite figured how to test it yet,
since I cant find public apis to test the things that need
testing...

Created attachment 121681 [details] [review]
The docs

This covers GtkMenuItem child "submenus" and GtkWindow "accel-groups"

Created attachment 121682 [details] [review]
good docs

oops that was the outdated docs patch, this is the good one ;-)

Created attachment 121683 [details] [review]
docs

ok Im obviously not on the ball with the docs, and I have to 
admit I havent went though compiling them, which I know takes
a long time... sorry for the spam guys.

Just noting, theres something I can do more for the priv->label,
for it to work right I believe I still need the pointer, but
at least I can avoid the dupped string in the case of
use-stock=False... its alot of code now so for the moment, I'll 
wait for a review for the next patch.

Looks basically ok to me now. 

I don't think you can avoid duping the string.


Some details:

+  if (strcmp (element_name, "group") == 0)
+    for (i = 0; names[i]; i++)
+      if (strcmp (names[i], "name") == 0)
+	data->items = g_slist_prepend (data->items, g_strdup (values[i]));
+  else if (strcmp (element_name, "accel-groups") == 0)
+    return;
+  else
+    g_warning ("Unsupported tag type for GtkWindow: %s\n",
+	       element_name);

I'd insert some {} here, just for clarity.


  if (strcmp (tagname, "accel-groups"))
+    return;
+  

Please add an "!= 0" here.


+      g_value_set_object (value, (GObject*) image_menu_item->image);

(Not your fault, but) the cast is not needed here, g_value_set_object takes a gpointer.


+  if (priv->label != label)
+    {
+      g_free (priv->label);
+      priv->label = g_strdup (label);
+    }
+
+  gtk_image_menu_item_recalculate (GTK_IMAGE_MENU_ITEM (menu_item));
+
+  g_object_notify (G_OBJECT (menu_item), "label");

Should move the recalculate and the notify into the if as well.


+  void (* set_label)            (GtkMenuItem *menu_item,
+				 const gchar *label);
+  G_CONST_RETURN gchar *(* get_label) (GtkMenuItem *menu_item);
 
   /* Padding for future expansion */
-  void (*_gtk_reserved1) (void);
   void (*_gtk_reserved2) (void);
   void (*_gtk_reserved3) (void);
   void (*_gtk_reserved4) (void);


Looks to me like you need to remove one more padding here. But please take away from the other end so that the reserved slots still start at 1.

The docs patch looks fine.

Created attachment 121743 [details] [review]
Latest patch

This patch is a bit more tidy, fixes the last comments
from Matthias + a possible double free when destroying
windows (using set_qdata_full() on an allocated list... fixed),
and a fixed crash in buildable warning messages.

I'm including commented test cases in this patch for 
buildable menus incase someone has a good idea how they
can be implemented.

Created attachment 121744 [details] [review]
Latest patch

This patch is a bit more tidy, fixes the last comments
from Matthias + a possible double free when destroying
windows (using set_qdata_full() on an allocated list... fixed),
and a fixed crash in buildable warning messages.

I'm including commented test cases in this patch for 
buildable menus incase someone has a good idea how they
can be implemented.

Created attachment 121758 [details] [review]
Another bugfix

The last patch was broken in cases where the GtkImageMenuItem has
to invent the child label 
(i.e. g_object_new (IMAGE_MENU_ITEM, "label", label, "image", image, NULL)).

In this update GtkImageMenuItem always uses GtkMenuItemClass->set_label to
update the text, so the menuitem class is still the only one responsable for
creating the label.

gtk_window_buildable_custom_tag_start needs to chain up

Looking at the test code, maybe you can use gtk_accelerator_parse to parse the label and compare the key and mods instead of formatting keys and mods and compare the strings...

Created attachment 121791 [details] [review]
Latest update

Ok fixed window to chain up from custom_tag_start/finish (my
mistake copy/pasting from sizegroup, which doesnt need that
treatment).

Unfortunately GtkAccelLabel makes a cute displayable string,
while gtk_accelerator_parse() hand checks for the '<' and '>'
(and accel-label doesnt create any of those).

Hmm, why do your patches not apply to gtk trunk here ?

Anyway, another idea for the test: 

- You can check the hierarchy (ie

  imagemenuitem1's parent is menu1, 
  menu1 is attached to menuitem1, 
  menuitem1's parent is menubar1,
  etc

- For the accel label check: how about contructing another stock gtk-new accellabel, and compare its accel_string to the menuitems ?

Created attachment 122033 [details] [review]
Complete with tests

Ok this one includes the accel_label test by way of creating
another imagemenuitem and checking that both accels are properly
set.

Also, I had made sure from gtk_menu_item_*() apis that alien children
of menuitems would be safe, but I had a remaining bug in buildable->add_child(),
thats fixed and a test is added to ensure alien children are allowed.

-  g_warning ("'%s' is not a valid child type of '%s'", type, g_type_name (G_OBJECT_TYPE (type)))
+  g_warning ("'%s' is not a valid child type of '%s'", type, g_type_name (G_OBJECT_TYPE (object)))

Please commit this fix separately.

Also, it would be good to commit the buildable implementation and tests separately from the new menuitem api.

Can you do that ?

I also have the feeling that gtk_menu_item_buildable_add_child can be simplified. Shouldn't it chain up to the parent for starters?

Created attachment 122122 [details] [review]
Improved add_child() approach/improved warnings

This one makes all the relavent warnings handled in the
gtkcontainer fallback implementation of buildable_add_child() instead
of doing anything from menuitems buildable_add_child().

With that, I committed it all seperately, here is the related 
changelog entries.

I made that last change to gtkmenuitem.c:buildable_add_child() to
offload the warnings to the container implementation, it was a minor
change and were deffinitly better off, I hope thats ok, I wont be
too far if theres any problems :)


2008-11-06  Tristan Van Berkom <tvb@gnome.org>

	* gtk/gtkmenuitem.c: Made buildable and added support for adding children
	of type "submenu"

	* gtk/gtkwindow.c: Added support for custom tag "accel-groups" to add GtkAccelGroups
	to the window.

	* gtk/gtkcontainer.c: Added builder contextual warnings in buildable_add_child()

	* gtk/tests/builder.c: Added tests for buildable menus (test that accelerators are
	properly connected on stock items, test the menu hierarchy, test permission to
	add alien/custom menuitem children).

	* docs/reference/gtk/tmpl/gtkbuilder.sgml, docs/reference/gtk/tmpl/gtkwindow.sgml,
	docs/reference/gtk/tmpl/gtkmenuitem.sgml: Updated docs for buildable submenus
	and accel groups.

2008-11-06  Tristan Van Berkom <tvb@gnome.org>

	* gtk/gtkmenuitem.[ch]: added new apis gtk_menu_item_[set/get]_label() and
	gtk_menu_item_[set/get]_use_underline() with "label" and "use-underline"
	properties, constructors cleaned up to use g_object_new().
	GtkMenuItemClass take new vfuncs ->get/set_label().

	* gtk/gtkcheckmenuitem.c: constructors cleaned up to use g_object_new().

	* gtk/gtkimagemenuitem.[ch]: added new apis gtk_image_menu_item_[get/set]_use_stock()
	and gtk_image_menu_item_set_accel_group() with "use-stock" and write-only
	"accel-group" properties. constructors cleaned up to use g_object_new().

2008-11-06  Tristan Van Berkom <tvb@gnome.org>

	* gtk/gtkbuilder.h: Fixed a crasher in GTK_BUILDER_WARN_INVALID_CHILD_TYPE()

(In reply to comment #33)
> I made that last change to gtkmenuitem.c:buildable_add_child() to
> offload the warnings to the container implementation, it was a minor
> change and were deffinitly better off, I hope thats ok, I wont be
> too far if theres any problems :)

I ran gimp and midori on trunk today, and so far I didn't see any regressions at least.

Note that there are two attachments still not marked as obsolete, would be nice if you could check that just to make sure nothing was left open.