Making GtkMenuMerge nice to demo

[Owen Taylor <otaylor redhat com>]

Here's some thoughts I wrote down yesterday looking at appwindow.c;
my basic premise is that if we can't write a demo that looks easy
to use and understand, then the API needs some changes.

Action definitions
==================

Right now, if you look at the action definitions appwindow.c, you see:

[ Making an edited selection of a few typical actions ]

  { "FileMenu", N_("_File"), NULL, NULL, NULL, NULL, NULL },
  { "New", N_("_New"), GTK_STOCK_NEW, "<control>N", N_("Create a new
file"), G_CALLBACK (activate_action), NULL },
  { "About", N_("_About"), NULL, "<control>A", N_("About"), G_CALLBACK
(activate_action), NULL },
  { "Red", N_("_Red"), NULL, "<control>R", N_("Blood"), G_CALLBACK
(activate_action), NULL, GTK_ACTION_RADIO },
  { "Green", N_("_Green"), NULL, "<control>G", N_("Grass"), G_CALLBACK
(activate_action), NULL, GTK_ACTION_RADIO, "Red" },

Which is clearly not going to make people think "what an elegant
API!". So, what can we do to clean up defining actions. First, there
is no reason to have N_() in the demo. While showing how to do
translation is an important piece of documentation, translation isn't
something most people need initially in there first app. Menus are.

But even getting rid of the N_(), things are still unreadable.

To really make things readable, it would be nice to use C99 named
initializers:

  { .name = "FileMenu",   .label = "_File",
  { .name = "New", .stock_id = GTK_STOCK_NEW,
    .tooltip = "Create a new file",
    .callback = G_CALLBACK (activate_action)
  },
  { .name = "About",
    .label = "_About", .accelerator = "<control>A",
    .tooltip = "About",
    .callback = G_CALLBACK (activate_action)
  },
  { .name = "Red",
    .label = "_Red", .accelerator = "<control>R",
    .tooltip = "Blood",
    .callback= G_CALLBACK (activate_action),
    .entry_type = GTK_ACTION_RADIO
  },
  { .name = "Green",
    .label = "_Green", .accelerator = "<control>G",
    .tooltip = "Grass",
    .callback = G_CALLBACK (activate_action),
    .entry_type = GTK_ACTION_RADIO, .extra_data = "Red"
  },

However, it's still a bit early to assume that's portable, so we
need to make do as we can. What can we do?

 * Swap the stock_id and label fields. Logically we have
   either stock_id or label+accelerator. Seldom both.

 * Get rid of the user_data field; it's seldom or never useful, so
   we should unclutter. Requiring one separate function for action
   is not a problem. (See later for a more useful user_data idea)

So, with those changes, adding some comments, and taking advantage
of default initialization, we get:

  { "FileMenu", NULL, "_File" }, /* name, ..., label */
  { "New", GTK_STOCK_NEW,	 /* name, stock_id */
    NULL, NULL,			 /* label, accelerator */
    "Create a new file",	 /* tooltip */
    G_CALLBACK (activate_action)
  },
  { "About", NULL,		 /* name, stock_id */
    "_About", "<control>A",	 /* label, accelerator */
    "About",			 /* tooltip */
    G_CALLBACK (activate_action)
  },
  { "Red",			/* name, stock_id */
    "_Red", "<control>R",	/* label, accelerator */
    "Blood",			/* tooltip */
    G_CALLBACK (activate_action),
    GTK_ACTION_RADIO,           /* entry_type */
  },
  { "Green", NULL,		/* name, stock_id */
    "_Green", "<control>G",	/* label, accelerator */
    "Grass",			/* tooltip */
    G_CALLBACK (activate_action),
    GTK_ACTION_RADIO, "Red"     /* entry_type, radio_group */
  },

Which is vastly more legible than where we started.

GtkMenuMerge
============

* First off, the name. GtkMenuMerge handles both menus and toolbars.
And the merging functionality, while critical to the way it is
implemented and some of it's advanced functionality, is something
that isn't going to make a lot of sense to someone encountering
it for the first time.

Some brainstorming about possible names that try to fix up 
one or both of the objections above:

 GtkMenuManager
 GtkUIManager
 GtkMenuSet
 GtkUISet

MenuMananager is more idiomatic than UIMananger, but if we
already have:

  gtk_menu_merge_add_ui_from_string (merge, ui_info, -1, NULL);

We've already made people learn what a "UI" is, so:

  gtk_ui_manager_add_ui_from_string (merge, ui_info, -1, NULL);

works reasonably well.

* GtkMenuMerge should g_warning() on errors if error == NULL;
you shouldn't have to handle programmatic errors.

* While it might logically belong more in a "AppWindow" or "Dock"
class, I don't think it would hurt to add a singleton per
toplevel GtkMenuMerge.

  window = gtk_window_new ();
  merge = gtk_window_get_menu_merge ();

  gtk_menu_merge_insert_action_group (merge, action_group, 0);  
  gtk_menu_merge_add_ui_from_string (merge, ui_info, -1, NULL);
  
  menu_bar = gtk_menu_merge_get_widget (merge, "menu");

this would automatically take care of memory management and adding
the menu merges accelerator group to the window.

* The 'dockitem' type in the XML seems confusing to me, if it actually
means 'toolbar'. Unless we actually have 100% compatibility with
the BonoboUI format, I think it makes sense to rename it to
'toolbar'.

* Using "verb" in the XML and "action" in the API is clearly a
mistake.

* I think it might be better to make "action" (verb) the default value 
for "name" than vice versa.

  <menu name='MainMenu'>
    <submenu action='FileMenu'>
      <menuitem action='New'/>
      <menuitem action='Open'/>
    </submenu>
  <menu/>

Is more obvious to me than:

  <menu name='MainMenu'>
    <submenu name='FileMenu'>
      <menuitem name='New'/>
      <menuitem name='Open'/>
    </submenu>
  </menu>

The fact that some names magically get interpreted as action 
names is very unclear at first glance. (The fact that you have
actions for menu items with submenus is weird to me, but probably
a necessary oddity.)

* I'm not that happy about the menu/submenu naming - it seems to
me to be in conflict with the names of the GTK+ widgets. I'm
not sure if doing:

 menu => menus/menubar
 submenu => menu

would make sense.

* For gtk_menu_merge_add_ui_from_string (), I think we could play the 
same trick as we do for PangoMarkup and add the Root element when
missing. If you have to put XML documents into your C program,
you want to keep them as concise as possible. (But see below for
how Root can be made useful by holding attributes that are inherited
by the whole UI.)

* It also seems to me that Root should be removed from the paths used
in gtk_menu_merge_get_widget (); it doesn't add any information.

 gtk_menu_merge_get_widget (merge, "Root/menu")

might as well be:

 gtk_menu_merge_get_widget (merge, "menu")

* "Root" shouldn't capitalized.

* "Root" should be renamed to something else. It's a semi-intimidating
  term. "ui" would be better and would fit in with 
  gtk_menu_merge_add_ui_from_string.

* I don't think that using a add_widget signal connection
in appwindow.c adds anything but confusion. Just getting the
widgets by name makes it a lot easier to understand. If
typical users of the API need to use signal connections to
::add-widget, then we are in trouble.

* I think avoiding anonymous node names in appwindow.c would be best.

* I think it's better to go for clarity in the source and omit the
  "\n" from inline UI strings than to go for clarity when dumping
  the UI strings out - we can always have an autoformatting dumper.

GtkActionGroup
==============

* It's really important to be able to get back to the application data 
structure from an action callback; this is a more useful use of the
user_data field of the GtkAction callback then a fixed value you
stick into the GtkActionGroupEntry structure.

What I think might be best is:

  void
  gtk_action_group_add_actions (GtkActionGroup      *action_group,
			        GtkActionGroupEntry *entries,
 			        guint                n_entries,
                                gpointer             user_data)

passing it to gtk_action_group_new() might also be possible, but then
the interaction with gtk_action_group_add_action() is unclear.

An add_actions_full() would be useful for language bindings; I think the
extra simplicity is worth not just adding a GDestroyNotify to the 
main add_actions.

Then you can simply have:

 static void 
 on_file_save (GtkAction        *action,
               MyDocumentWindow *document_window)
 {
 }

* The docs for gtk_action_group_new() and the comments in appwindow.c
need to make clear what the name of the action group is for: for
associating keybindings with the actions.

* Should it be possible (or even required?) to use the action group
  name to disambiguate in a GtkMenuMerge? 

    <menuitem group='MainMenu' action='New'/>
    <menuitem action='MainMenu::New'/>
    <menuitem action='MyApp::MainMenu::New'/>

  if we are going to do inter-widget, and possibly interprocess
  merging with this, 'New' doesn't seem good enough, though we
  could try to establish a convention of names like
  'MyAppMainNew'. If we used multiple actions, then we could use
  inheritance to make this non-painful.

   <Root application='MyApp' group='MainMenu'/>
 
* Perhaps rename GtkActionGroupEntry to GtkActionEntry?

Radio action items
==================

* Radio action items suffer from the same problem as GtkMenuItem
currently - they fire both when they are activated and when they
are deactivated.

What if we:

 - Added an extra signal for GtkRadioMenuItem - "changed" - which
   fires when the current item in the group changes.
 - Make the callback slot of GtkActionGroupEntry hook up "changed"
rather
   than activate for radio menu items

So, then you just specify a callback for the first of your radio
action items, and omit it for the rest. 

The question then becomes how to handle the getter that the "changed"
callback calls. An automatically assigned integer index assumes a 
predictable ordering, something we might not have, especially with a 
UI builder involved:

String comparisons aren't awful:

 static void
 on_color_changed (GtkRadioAction   *action,
                   MyDocumentWindow *document_window) 
 {
   GtkAction *current = gtk_radio_action_get_current (action);
   const char *name = gtk_action_get_name (current);

   if (strcmp (name, "Red") == 0)
     /* ... */
   else if (strcmp (name, "Blue") == 0)
     /* ... */
  }

Anything else is going to require an extra field in GtkActionGroupEntry
which is unattractive. We could pass in the current action 
(or even the name) as an extra parameter to the "changed" action
if we wanted to simplify.

The only other approach I can think of is to use something other
than GtkActionGroupEntry for radio actions.

 static const GtkActionGroupRadioEntry color_actions = {
   { "Red",    "_Red",   "<control><shift>R", "Blood", COLOR_RED },
   { "Green",  "_Green", "<control><shift>G", "Grass", COLOR_GREEN },
   { "Blue",   "_Blue",  "<control><shift>S", "Sky",   CLOR_BLUE }
 };

 gtk_action_group_add_radio_actions (group,
                                     color_actions, G_N_ELEMENTS
(color_actions),
                                     G_CALLBACK (on_color_changed),
                                     document_window);

 static void
 on_color_changed (GtkRadioAction   *action,
                   MyDocumentWindow *document_window) 
 {
   MyColor color = gtk_radio_action_get_current_value (action);
   switch (color)
     {
       case COLOR_RED:
       case COLOR_GREEN:
       case COLOR_BLUE:
     }
  }

Which, while it ruins the elegance of one huge table of all the
actions, removes some complexity from the standard
GtkActionGroupEntry, makes having a single callback for the entire
group natural, and provides a convenient place to put an integer
enumeration.

It also works pretty well for UI builders, since we can make the
value field just another property of the action.