Re: accelerators

[Owen Taylor <otaylor redhat com>]

OK, I'll start off with a bunch of issues, both philosophical
and practical, before getting to the details of your proposal.


The nature of an accelerator group
==================================

An accelerator group maps a keyboard accelerator to a concrete
callback, not to an abstract action. That is, an accelerator
group maps <Ctrl>X to "execute the cut command in this toplevel"
not to "cut". This is enforced by the fact that you attach the
callback to a particular menu item.

The idea has been advanced that you can share an accelerator
group between multiple windows when you are using a popup
menu by sharing the popup menu itself, but this is nonsense

 * Right click menus are "context menus" - they refer to operations 
   on a particular object, not on the entire window.

 * And even if they referred to the entire window, how would
   you know which entire window it referred to?

So, a accelerator group shared between toplevels is useful for 
a right click menu that has options:

 "New Window"
 "Quit"

on it. Nothing more.

Actually, the GIMP uses a single accelerator group (IIRC) 
by making menu items and accelerators refer to the "current"
image window, not the image window from which they are invoked,
but this is a practice I would not recommend on it.

While we probably need to retain the capability to have an
accelerator group attached to multiple windows, that really
should not be a fundemental part of the design.


What's the point of GtkAcceleratable?
=====================================

Accelerators live on windows. This is necessary because:

 - Key events are delivered to windows
 - Menu bars are in windows, so menu items are associate
   with particular windows.
 - GtkWindow doesn't know about anything but accelerator
   groups.

So I still don't understand the purpose GtkAcceleratable.

I'm also a little concerned about the fact that it looks like we'll
have a significant "default implementation" in GtkAcceleratable. Since
default implementation of interface methods is something that reallyl
doesn't map into languages with real interfaces (like Java), if we
have this feature, we should use it only for very trivial things where
the

You should never, for instance, _have_ to chain up to the default
implementation of an interface method.

If the idea of GtkAcceleratable is to separate out key handling from
GtkWindow for API and code cleanliness, it makes sense, but then we
should probably do it as a aggregate GtkKeyTable object, rather than
an interface.


What is the role of window types?
=================================

I must admit, that even with your lovely ascii-art diagram, I'm
still somewhat unsure of the relationship:

 Window type <=> GtkAccelGroup <=> GtkWindow, GtkAcceleratable

If I describe the three things:

 Window type:

   Window types serve as namespaces; mappings from "menu path" 
   or "action name" to modifier/key are unique within each
   window type.

 AccelGroup:

   An accel group holds a mapping from modifier/key to callback.

 GtkAcceleratable/GtkWindow:

   A GtkAcceleratable has a mapping from modifier/key to callback,
   which contains one or more GtkAccelGroup mappings and other
   miscellaneous mappings.

But these are "functional" descriptions - they say what the
things do. Not what they are meant for. In order to present
a comprehensible picture to the user, I think we need to have
a plan in our mind on the _typical_ way things will be used,
not the "possible ways" that things will be used.

I think the two comprehensible common use patterns are:

1) Each window has a GtkAccelGroup per window type it supports

    <Main>/File/Save            <DocumentWindow>/Image/Gamma
    <Main>/File/Open            <DocumentWindow>/Image/Colors
      /          \                           |
 accelgroup1  accelgroup 2               accelgroup3
     \       /            \             /
      \     /              \           /
    Window 1                 Window 2


2) Each window has one GtkAccelGroup for all it's menus

    <Main>/File/Save        <DocumentWindow>/Image/Gamma
    <Main>/File/Open        <DocumentWindow>/Image/Colors
       |          \              /        
       |           \            /
   accelgroup1       accelgroup2
       |                   |
       |                   |
    Window1             Window2

Is one of these canonical? How do you expect people to set
things up? I think if we document it as an "anything goes"
we'll just confuse people. A lot.


Multiple menu items for a single accelerator
============================================

James pointed out to me last night that if you have an action based
API, then accelerators conceptually point to actions, not to
particular menu items, so your paths should be action names, not
actual menu paths. It makes perfect sense to have multiple menu items
for the same action in, but in this case, they should share the same
key.

The easiest way to this goal is probably to allow the same accelerator
key/mods to be added to an accelerator group multiple times, as long
as it also had the same path. In this case, GtkAccelGroup would assume
that it didn't matter which of the menu items was activated, and leave
it up to the party creating the menus to make sure they all did the
same thing.


Unmodified accelerators
=======================

One of the issues in bugzilla that isn't covered by your
proposal is:
 
 #61285 - No way to have change-on-the-fly menus with unmodified accel

In GTK+-1.2, we keyed whether unmodified accelerators were allowed
on !gtk_menu_get_uline_accel_group (GTK_MENU (menu_shell)). 
But now we've gotten rid of uline_accel_group in favor of monikers,
if we still want to support unmodified accelerators, as in the
GIMP, we need to add an API to enable this explicitely.

[ I think it probably should just be some global function like:

  gtk_menu_enable_unmodified_accel() ]

> so in terms of what we currently have, i plan to change things like this:
> 
> - remove the GtkWidget::add_accelerator and GtkWidget::remove_accelerator
>   signals

Does that mean we can remove gtk_accel_group_handle/create_add/remove
as well?
 
> - remove: gtk_widget_remove_accelerators(), gtk_widget_accelerator_signal(),
>   gtk_widget_lock_accelerators(), gtk_widget_unlock_accelerators(),
>   gtk_widget_accelerators_locked(),
>   and probably also:
>   gtk_widget_add_accelerator(), gtk_widget_remove_accelerator()
>   though the latter two could be left for convenience if people feel overly
>   annoyed by their removal

My instinct would be to deprecate the last two rather than to remove
them - these functions are used a lot compared to any of the others.
Probably 80% of the cases where they are used are places where
mnemonics should be used now, but keeping that work initially will
ease the pain of porting.
 
> - implement a global (path,shortcut) map with the public API:
>   /* save/load accelerators to/from a file */
>   void gtk_accelerator_map_load (const gchar *file_name);
>   void gtk_accelerator_map_save (const gchar *file_name);
>   /* in case someone needs to operate on partial files (e.g. beast
>    * combines the accelerator dump with its rc file):
>    */
>   void gtk_accelerator_map_load_from_fd (gint fd);
>   void gtk_accelerator_map_save_to_fd (gint fd);
>   /* if saving is not possible/desired, this prevents runtime changable
>    * accelerators
>    */
>   void gtk_accelerator_map_lock    (void);
>   void gtk_accelerator_map_unlock  (void);
>   /* in case someone needs to implement his own parser (iirc, gsumi
>    * wanted to save in xml syntax or somesuch):
>    */

Not XML, but yes, I wanted to integrate the saved accelerators with
my own config data.

>   void gtk_accelerator_map_foreach (GtkAccelPoolForeach foreach_func,
>                                     gpointer            data,
>                                     gboolean            ignore_filters);

I assume the GtkAccelPoolForeach function here will properly take
path/key/modifiers rather than a string. Looking at the gsumi code, I
wrote:

static void 
menu_print_func (gpointer func_data, gchar *str)
{
  /* Fix ItemFactory! Now! */

  gchar *path;
  gchar *accelerator;
  gchar *pos;

  /* Extract path */
  str = strchr (str, '"');
  g_return_if_fail (str != NULL);
  str++;

  [...]

(BTW, what's an AccelPool?)

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().

>   void gtk_accelerator_map_enter   (const gchar        *accel_path,
>                                     const gchar        *accel_key);

_add_entry() would be more standard for GTK+ than enter(). enter()
is used for things like gdk_threads_enter(), not for adding an
entry.

>   /* semi public API, only needed for GtkModules that create their own
>    * menus, but don't want it to be saved into the application's
>    * accelerator map dump (e.g. GLE)
>    */
>   void gtk_accelerator_map_install_filter (const gchar *pattern);

Modules need to use public APIs. As long as the API is accessible
from external code portions without #defining something, I'd
consider it a public API. (Comments in header files do little good,
as you've discovered in the past with GtkAccelGroup.)

> - add
>   void gtk_menu_set_accel_path (GtkMenu     *menu,
>                                 const gchar *accel_path);
>   this is for menus on which accelerators should be runtime-changable,
>   but which aren't created by GtkItemFactory. the accel_path is used to
>   create accelerator paths for all those menu items that have a label.

As I said before, I think this is a bad idea. If people support paths,
they will pass paths into gtk_accel_group_add().

> - apps that do not bother saving/loading should call
>   gtk_accelerator_map_lock() after gtk_init()

You are saying, you either have to call gtk_accelerator_map_lock()
or load and save accelerators to be correct. Note that this means
that apps that do nothing are defined as buggy. If the default
state is locked, then apps that do nothing are less-functional,
and when the app author discovers the FAQ entry describing how
to load/save, they will also find there that they need to
add gtk_accelerator_map_unlock().
 
Regards,
                                        Owen