Review of accelerator changes

[Owen Taylor <otaylor redhat com>]

This is long, so let me provide a table of contents:

 I.   Introduction
 II.  GtkAccelMap and GtkAccelGroup 
 III. A ramble about gtk_widget_set_accel_path
 IV.  Small things, more or less important
 V.   Some trivial comments


I. Introduction
===============

I spent a good portion of today reading through the GtkAccelGroup,
and GtkAccelMap code and figuring out how it all works.

Mostly, it looks quite good. It's a big improvement on GTK+-1.2
and is mostly quite easy to understand. I have various small
API suggestions below, along with spelling fixes, a few code
cleanups, etc. There are two things in the API that I dislike:

 - gtk_menu_set_accel_path()
 - The filters setup in GtkAccelMap

But these are relatively minor things, and if I can't convince
you to change them, they will do no great harm.
 
But there was one aspect of the code that I had a lot of trouble
figuring out how it works -- that was the connection between
GtkAccelMap and GtkAccelGroup. It seemed to be scattered all
through the code and done in ways that I just didn't expect
and that didn't seem convenient. Part II explores how I expected
it to work, how it actually does work, and how I think it
could be made a lot simpler than it is currently.


II. GtkAccelMap and GtkAccelGroup
=================================

The path <=> closure connection is not done at all how I would expect.
It seems to me a that it should be fairly simple problem to create
an appropriate API.

We have:

 GtkAccelGroup - a map of:

                    <key/modifiers>
   <closure>  <=>        or
                       <path>

 GtkAccelMap - a (global singleton) map of:

   <path>     <=>   <key/modifiers>

To add entries to GtkAccelGroup we have:

 void gtk_accel_group_connect (GtkAccelGroup	*accel_group,
			       guint		 accel_key,
			       GdkModifierType   accel_mods,
			       GtkAccelFlags	 accel_flags,
			       GClosure	        *closure,
			       GQuark		 accel_path_quark);

To add entries to GtkAccelMap we have:

 GQuark gtk_accel_map_add_entry	(const gchar *accel_path,
                		 guint	      accel_key,
				 guint	      accel_mods);

and to change them:

 gboolean gtk_accel_map_change_entry (const gchar	*accel_path,
				      guint		 accel_key,
				      GdkModifierType	 accel_mods,
				      gboolean		 replace);

Now, there is complicated internal logic for how add_entry() and
change_entry() work with keeping uniqueness of changeable accelerators
on toplevels, but I would expect that to be handled via API's private
to GtkAccelMap and GtkAccelGroup.

Then for monitoring, via something like GtkAccelLabel, we have 
the signal on GtkAccelGroup:

  void	(*accel_changed) (GtkAccelGroup	*accel_group,
			  guint           keyval,
			  GdkModifierType modifier,
			  GClosure       *accel_closure);

Given a closure, I can find it's group with:

 GtkAccelGroup*	gtk_accel_group_from_accel_closure (GClosure *closure);

and then watch changes on that group. If I want to watch for changes
in accelerators for a widget and the closures for that widget might
change, then it's a little more tricky but still easy - I need
something like:

 GSList* _gtk_widget_get_accel_closures (GtkWidget *widget);
 ::accel_closures_changed (GtkWidget *widget);

All the above API exists, and makes a lot of sense to me, except
for a few small quibbles. (E.g., I don't think 
_gtk_widget_get_accel_closures should be private.)


But unfortunately, things aren't this pretty. Instead of 
GtkAccelMap and GtkAccelGroup maintaining their relationship
internally, it's spilled out into an ugly public API:

 void gtk_accel_map_add_notifer	(const gchar		*accel_path,
				 gpointer		 notify_data,
				 GtkAccelMapNotify	 notify_func,
				 GtkAccelGroup		*accel_group);
 void gtk_accel_map_remove_notifer (const gchar		*accel_path,
				    gpointer		 notify_data,
				    GtkAccelMapNotify	 notify_func);

Despite the "notifier" names, these functions aren't actually
just about notification. The accel_group parameter here 
actually is used to figure out what AccelGroups a path is
used in. If you don't call add_notifier(), then things won't
work properly.

Not only that, but to get propagation of changes in the AccelMap
to work properly we have to connect a notifier that is responsible
for handling them manually. There is about 100 lines of code
in GtkWidget behind _gtk_widget_set_accel_path() responsible
for this back propagation that would have to be duplicated
by anyone else who wanted to use GtkAccelMap.


Since GtkAccelMap already is quite familiar with the details
of GtkAccelGroup, and has expectations of how things work,
a private Map/Group API would be much simpler. All we'd
need to add is

 gtk_accel_map_path_add_group    (const gchar  *path,  /* Or a quark */
                                  GtkAccelGroup group);
 gtk_accel_map_path_remove_group (const gchar  *path,  /* Or a quark */
                                  GtkAccelGroup group);

And GtkAccelMap would simply keep a list of groups for each path
and handle everything from there.


III. A ramble about gtk_widget_set_accel_path
=============================================

[ Don't expect much of a point to this section ]

With the first change above, adding and watching changeable
accelerators for closures would be quite easy. But we still
have the question of how we set up the traditional 
widget::activate signals and associate them with paths.

I might expect the API for this to be something like:
 
 void gtk_widget_add_changeable_accelerator (GtkWidget           *widget,
                                             const gchar         *accel_path,
					     const gchar         *accel_signal,
					     GtkAccelGroup       *accel_group);

Or variants, say perhaps always using the activate_id signal, and
setting a path and group for the widget.

The actual API is different - there is no public API on GtkWidget,
instead it is set up as:

 void gtk_menu_item_set_accel_path (GtkMenuItem	   *menu_item,
				    const gchar	   *accel_path);
 void gtk_menu_set_accel_path      (GtkMenu        *menu,
			            const gchar    *accel_path);

Where gtk_menu_set_accel_path() allows GtkMenuItem to magically make
up a path for all the menu items in the menu based on their
labels. Now, let me say plainly, I think gtk_menu_set_accel_path() is
a useless feature:
  
 - Menu item labels are translated, menu paths cannot be translated
 - Menu item labels contain mnemonics and markup, menu paths
   should not contain such.
 - If people have paths for their menus, they will have paths
   for the menu items.

But that being said, wanting to have gtk_menu_set_accel_path()
does explain why gtk_widget_set_accel_path() API wouldn't work
very well ... GtkMenuItem and GtkMenu can cooperate so that
the magic path only takes effect if gtk_menu_item_set_accel_path()
hasn't been called explicitely; this is harder for GtkWidget.

And, gtk_menu_item_set_accel_path() does have other advantages:

 - It can get the accel group automatically from the GtkMenuItem
 - It is obvious what the correct signal to use is.



IV. Small things, more or less important
========================================

 * With my suggestion above in mind (AccelGroup/AccelMap handle
   relationship automatically) it would be clearer if
   gtk_accel_group_connect() were split into:

    gtk_accel_group_connect (GtkAccelGroup  *group,
        	             guint	     accel_key,
			     GdkModifierType accel_mods,
			     GtkAccelFlags   accel_flags,
			     GClosure	    *closure);
    gtk_accel_group_connect_path (GtkAccelGroup  *group,
        			  GQuark	  accel_path_quark,
			          GtkAccelFlags   accel_flags,
			          GClosure	 *closure);

   Since it _is_ conceptually only one or the other.

 * I consider using a GQuark for the path argument to gtk_accel_group_connect
   quite dubious. This is an implementation detail, and even if the
   caller actually has a quark, a one extra quark => string => quark 
   conversion per accelerator will not hurt us.

 * _gtk_widget_get_accel_closures() should be exported, see mail 
   exchanged with Padraig earlier.

 * Mispellings:

    refresh_accel_paths_froeach()
    gtk_accel_map_foreach_unfilterd()
    gtk_accel_map_add_notifer()
    gtk_accel_map_remove_notifer()

 * Instead of gtk_accel_group_disconnect, gtk_accel_group_disconnect_closure(),
   I'd expect something like:

    gtk_accel_group_disconnect     (GtkAccelGroup *group, 
                                    GClosure      *closure);
    gtk_accel_group_disconnect_key (GtkAccelGroup  *group,
				    guint	    accel_key,
				    GdkModifierType accel_mods);

   Since AccelGroup, as the caller sets it up is a map from closure to
   _either_ key/mods or to path, so the closure is more primary.

 * I find "acceleratable" to be a word that:

    a) Doesn't mean anything to me
    b) Keeps on looking like a misspelling of GtkAcceleratorTable

   Can't we rename change gtk_accel_groups_from_acceleratable() back
   to the 1.2 gtk_accel_groups_from_object(), or perhaps
   gtk_accel_groups_from_activate_object(). Similarly, I'd like
   to rename the acceleratable paramete to gtk_accel_groups_activate().

 * What is the use case for exporting gtk_accel_group_find()?
   It seems to me that you either want to go key => closure or
   closure => key and that a general iteration interface is
   a little clumsy.

 * If gtk_accel_group_query is internal, it should be called _gtk. 
   If it is not internal, than it shouldn't have a comment above
   it that says so. In order to have reliable source/binary compatiblity
   we need to know exactly what is public and what isn't. Comments
   in header files are _not_ sufficient means of marking this.
 
   We need to either not export, or use #defines to hide the
   prototypes, as in Pango or GtkTextLayout.

 * The arrangement of return values from gtk_accel_group_query()
   is wrong. (Yes, I know you don't think so, but I need to keep 
   repeating this in case my point will get through some day ;-)

 * The GQuark return from gtk_accel_map_add_entry() looks a bit pointless,
   since it is done as:

     return g_quark_try_string (entry->accel_path);

   The caller could do the g_quark_try_string() if they needed it,
   and most of the callers don't actually need the quark...

 * I don't understand why gtk_accel_map_lookup_entry returns a 
   GQuark rather than a boolean. The quark could easily be
   computed by the caller if they needed it; the one caller 
   currently there doesn't use it at all. 

 * I never got an answer about my comment about the filters in
   GtkAccelMap

   > The 'ignore_filters' argument here seems a convuluted way of doing
   > things to me and I also question whether it's a good idea to force
   > parties installing filters to use gtk_accelerator_map_foreach().
   > 
   > Perhaps we should add a 'const gchar *root' argument to
   > gtk_accelerator_map_save(), where NULL, means 'everything
   > except what has been disabled by gtk_accelerator_map_install_filter()',
   > but GLE could do "gtk_accelerator_map_save (filename, "<GLE>");
   > And maybe rename gtk_accelerator_map_install_filter() 
   > gtk_accelerator_map_make_root_private().

 * We need doc comments for:

    gtk_accel_groups_from_acceleratable()
    gtk_accel_group_find()
    gtk_accel_groups_disconnect_closure()
    gtk_accel_group_query()

 * We need a Changes-2.0.txt entry, for things like g
   gtk_accel_group_get_default()

 * Didn't we agree to call ::accels_changed on GtkWindow, ::keys_changed?

 * In order to handle key propagation for GtkPlug, I'm going 
   to have to add a private API like _gtk_window_list_keys(),
   which will have to look into GtkAccelGroup->priv_accels.

   (Just warning you, so you don't accuse me of doing evil 
   things later ;-)

 * API Bug #61285 - No way to have change-on-the-fly-menus with
   unmodified accels is still not resolved.

   You proposed simply always allowing unmodified accelerators when
   changeable accelerators are allowed. This significantly increases
   the chance of accidentally changing accelerators, and the harm that
   an accidentally installed accelerator does, but if change on the
   fly accelerators are off for novice users, this probably doesn't
   matter.

V. Some trivial comments:
=========================

 * _gtk_menu_refresh_accel_paths should be static to gtkmenu.c

 * The replace_accels gboolean in gtk_menu_key_press() looks 
   like it could be removed to advantage.

 * gtk_accel_map_load/save_fd() have portability problems.

 * There is a // FIXME in gtK_accel_map_lookup_entry(), need
   to fix before 1.3.11

 * GQuark quark_path - gtk_accel_map_add_entry (accel_path, 0, 0);
  if (!quark_path)
    return;        /* pathalogic anyway */

  Are we going to start writing
   
    label = gtk_label_new ("Hello!");
    if (!label)
      return;      /* pathalogic anyway */

  If a function cannot return a value, we should _not_ check for
  it. It just bloats code and makes  the reader of the code
  work harder.

 * Step 6) of internal_change_entry could be skipped
   if !can_change. This function would be easier to read
   with a few gotos. Things like:

    for (slist = removable ? replace_list : NULL; slist; slist = slist->next)

   To skip blocks of code don't make for pleasant reading.

 * In gtk_accel_path_is_valid(), 

   "<<a>" is not a valid accel path, but "<a<>" is?

 * Extra ) in arguments descriptions for gtk_accel_map_add_notifier()