Re: accelerators



On Thu, 1 Nov 2001, James Henstridge wrote:

> Tim Janik wrote:
> 
> >hi all,
> >
> >one of the outstanding API issues is fixing accelerator behaviour.
> >i'll start with the basic rationale and then come to the planned changes.
> >
> >accelerators are the basic mechamism through which certain key combinations
> >can be assigned to menu items, to emit those menu item's ::activate signal
> >through keyboard shortcuts. we've also had a couple of requests to bind
> >arbitrary callbacks to accelerators, so accelerators are supposed to
> >accomplish that functionality in the future as well.
> >
> Is it enough to be able to attach accelerators to signals of objects not 
> derived from GtkWidget?  That is already possible with the current API 
> (I made use of it in that menu actions demo I wrote a few months back), 
> so preserving this ability might be sufficient.

yeah, good idea to write that. i investigated your code somewhat before
writing this proposal, and one thing that struck me hard was the efford
you had to put into making a GObject have an accelerator (eventhough
accel group has a bunch some conveience fucntins for that).
that's why in my local tree, i went away from that approach and based
the accel group API solely on closures:

struct _GtkAccelGroupClass
{
  GObjectClass parent_class;

  /* activation signal (GClosure signature):
   * gboolean   (*accel_activate)       (GtkAccelGroup  *accel_group,
   *                                     GObject        *acceleratable,
   *                                     guint           keyval,
   *                                     GdkModifierType modifier);
   */
  void  (*accel_changed)        (GtkAccelGroup  *accel_group,
                                 guint           keyval,
                                 GdkModifierType modifier,
                                 GClosure       *accel_closure);
};

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

(no gtk_accel_group_add/gtk_accel_group_remove anymore).
thus, if you're having plain GObjects, you just write a new closure
for them and connect those, without having to introduce signals or
similar on the object type. (the GQuark path_quark argument is only
required for runtime changable accelerators).

then, the accel label now looks like:

void       gtk_accel_label_set_accel_widget  (GtkAccelLabel *accel_label,
                                              GtkWidget     *accel_widget);
void       gtk_accel_label_set_accel_closure (GtkAccelLabel *accel_label,
                                              GClosure      *closure);

i.e. you'd set your closure on an accel label, and it will monitor
changes on the closure via GtkAccelGroupClass.accel_changed().
the parameters to your closure are those of GtkAccelGroupClass.accel_activate()
internally the accel group uses a signal (without class method) to
activate accelerators now (signal detail being the accelerator
to activate).

> >in order to support runtime changable accelerators persistently (through
> >loading of accelerator assignments at program start and saving them upon
> >exit), gtk needs some kind of association of keyboard accelerator to
> >functionality. this currently works through item factory's menu paths,
> >e.g. we have a set of paths and accelerator pairs:
> >
> >("<DocumentWindow>/File/Quit", "<Ctrl>W")
> >("<DocumentWindow>/File/Open", "<Ctrl>O")
> >("<DocumentWindow>/Edit/Cut", "<Ctrl>X")
> >("<ImageWindow>/Edit/Cut", "<Ctrl>X")
> >("<ImageWindow>/Select/All", "<Ctrl>A")
> >
> >the paths consist of a window type in angle brackets and categorized
> >functionality. that's, so that changing e.g. <DocumentWindow>/Edit/Cut
> >to "<Alt>C" propagates to all <DocumentWindow>s of an application,
> >but the accelerator for <ImageWindow>/Edit/Cut for all windows which
> >display images can be changed independently from that.
> >
> >since a window can have multiple menus that come with different
> >sets of accelerators, we also have accelerator groups per menu
> >which get attached to the toplevel that uses this menu (e.g. in
> >a menu bar).
> >certain menus (e.g. right-click popups) may not be associated with
> >a single window, but are used for a set of similar windows, in those
> >cases, the menu's accelerator group needs to be added to all toplevels
> >of that set.
> >
> Or in the case of a merged set of menus, a few common menu items may 
> have accelerators specified in a shared accel group, which is probably 
> more common if you only use right click menus for context menus (as owen 
> suggested).

yeah, i've taken that into account now, multiple accelerators are
allowed in accel groups, though that might be irritating for the user
and is thusly not really recommended.

> >what we end up with is a set of accelerator assignments, a set
> >of accelerator groups and a set of toplevels with n:n relations, e.g.:
> >
> >Paths and accelerators:
> >	(<A>/Bar,<Ctrl>X)  (<C>/Foo,<Shft>Y)  (<B>/Baz,<Alt>Z)
> >                 \                /|\                /
> >                  \              / | \              /           
> >M*=MenuItems       M1           /  M3 \            M2
> >C*=Callbacks        \          C1  M4  C2         /
> >                     \        /    M5   \        /              
> >                      \      /     |     \      /
> >AccelGroups:           Group1    Group2   Group3
> >                          |       /|\       |                  
> >                          |      / | \      |                  
> >                          |     /  |  \     |                  
> >                          |    /   |   \    |                  
> >                          |   /    |    \   |                 
> >AcceleratableObjects:  Window1  Window2  Window3
> >
> >
> >based on this observation, it makes most sense to maintain:
> >1) a global map of (path,shortcut) pairs which tracks what
> >   accel groups use what pair
> >2) accelerator changed notification on accel groups (not widgets
> >   like we currently do)
> >3) a GtkAcceleratable interface that object types can implement which
> >   whish to support shortcuts
> >
> Is there likely to be anything other than GtkWindows that implement 
> GtkAcceleratable?

pondered this somewhat, and the honest answer is "not that i could
think of". still it feels a bit wrong to special GtkWindow in the accel
group/map implementations (which is what i ended up with) to figure
whether new accelerators would conflict with mnemonics.

> I don't know if it would be worth putting this here or in the GtkMenu 
> keypress code, but it would be useful to make a menu item a `proxy' for 
> some other object as far as accelerator changes.  With my menu actions 
> demo, the menu items only act as views of the action objects, and 
> accelerators were set on the actions rather than menu items.

certainly not the menu keypress code. to gain runtime changable
accelerators, you now have to call:

/* set the accel group to be used for accelerators of this menu's children */
void           gtk_menu_set_accel_path    (GtkMenu             *menu,
                                           const gchar         *accel_path);


/* associate an accel path with a shortcut, e.g.
 * ("<DocumentWindow>/File/Open", 'O', GDK_MOD_CONTROL)
 */
GQuark     gtk_accel_map_add_entry      (const gchar            *accel_path,
                                         guint                   accel_key,
                                         guint                   accel_mods);

/* sync a menu item's accelerator with an accel path, e.g.:
 * (fopen_menu_item, "<DocumentWindow>/File/Open")
 */
void       gtk_menu_item_set_accel_path (GtkMenuItem         *menu_item,
                                         const gchar         *accel_path);

/* or, as an alternative to gtk_menu_item_set_accel_path(), you can just
 * call gtk_menu_set_accel_path (menu, "<DocumentWindow>/File"); which
 * will automatically construct menu item accel paths with text from
 * their labels. this is especially usefull for menus where only a small
 * amount of menu items need accelerators initially, but the user should
 * still be able to newly install some.
 */
void           gtk_menu_set_accel_path    (GtkMenu             *menu,
                                           const gchar         *accel_path);

with that, the menu key press handler shrunk to basically:

  changed = gtk_accel_map_change_entry (menu_item_accel_path,
                                        accel_key, accel_mods,
                                        replace_accels);
  if (!changed)
    gdk_beep ();


so going for proxying actionobject<->menuitem should just involve:
- implementing a special closure for proxy menu items, that activates
  the action object instead.
- propagating the action object accelerator back to the menu item

> >    void        gtk_accel_group_disconnect      (GtkAccelGroup  *accel_group,
> >                                                 guint           accel_key,
> >                                                 GdkModifierType accel_mods,
> >                                                 GClosure       *closure);
> >  (NULL paths are allowed, they'll instantly lock the accelerators though,
> >   as we can't save/load accelerators without path specification)
> >
> Sounds good.  So the object passed to gtk_accel_group_add() could be any 
> object, not having to implement any special signals or anything?

just a closure to be passed in now ;)

> What would the argument list for the closure in the connect/disconnect 
> pair be?  I don't know what I think about the naming 
> (gtk_accel_group_add vs gtk_accel_group_connect).

solved by getting rid of gtk_accel_group_add/remove().
the gtkwidget functions:

void       gtk_widget_add_accelerator     (GtkWidget           *widget,
                                           const gchar         *accel_signal,
                                           GtkAccelGroup       *accel_group,
                                           guint                accel_key,
                                           guint                accel_mods,
                                           GtkAccelFlags        accel_flags);
gboolean   gtk_widget_remove_accelerator  (GtkWidget           *widget,
                                           GtkAccelGroup       *accel_group,
                                           guint                accel_key,
                                           guint                accel_mods);

remain though. but accelerators set through them cannot be runtime
changable. (runtime changable accelerators on widgets (menu items)
are bound to having an accel path and always emitting the signal
GtkWidgetClass.activate_signal).

> >
> >  introduce:
> >  void (*GtkAccelGroup::accel_changed)	(GtkAccelGroup *group,
> >                                         guint           accel_key,
> >                                         GdkModifierType accel_mods);
> >
> I could imagine things like bonobo possibly wanting to know about accel 
> changes.  Would this really be internal, or just something apps rarely use?

this is internal, for things like GtkAccelLabel and similar code.
bonobo, or the plug/socket code should use the GtkWindow::accels_changed
which proxies and coalesces notification occouring on the window's accel
groups.

> >- change GtkAccelGroup functions that currently take a GObject towards
> >  taking GtkAcceleratable (not the above _add/_remove of course). a few
> >  functions might also get deprecated and move into the GtkAcceleratable
> >  interface, such as:
> >  gboolean   gtk_accel_group_activate     (GtkAccelGroup  *accel_group,
> >                                           guint           accel_key,
> >                                           GdkModifierType accel_mods);
> >
> >- 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.
> >
> What would be the best way to proxy accel changes for runtime accel 
> changes?  Currently, my demo just hooked add_accelerator and 
> remove_accelerator of the menu item, but this wasn't very good (removing 
> accelerators didn't work, because the menu item itself had no 
> accelerators to remove).

that's solved by purely going through:

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

your objects would just need to implement a bit of glue code to react
to notifcation about accel map entry (accel path) changes.
i.e. gtkwidget.c in its implementation of
void         _gtk_widget_set_accel_path   (GtkWidget           *widget,
                                           const gchar         *accel_path,
                                           GtkAccelGroup       *accel_group);
installs hooks on the accelmap through:
typedef void (*GtkAccelMapNotify)               (gpointer        data,
                                                 GQuark          accel_path_quark,
                                                 guint           accel_key,
                                                 guint           accel_mods,
                                                 GtkAccelGroup  *accel_group,
                                                 guint           old_accel_key,
                                                 guint           old_accel_mods);
void       gtk_accel_map_add_notifer    (const gchar            *accel_path,
                                         gpointer                notify_data,
                                         GtkAccelMapNotify       notify_func,
                                         GtkAccelGroup          *accel_group);

but as i said earlier, it'd probably be better and easier to
just propagate the accel_path from your action objects to menu items
and let the menu items use a closure that activates your action
object directly.

> >though this looks like a lot of changes, internally this is mostly moving
> >code from GtkMenu and GtkItemFactory into GtkAccelGroup and the accelerator
> >map. in terms of the public API changes, the expected amount of adaption
> >for third party code is also moderately small:
> >- apps that currently save/load accelerators through the item factory should
> >  simply use gtk_accelerator_map_load/gtk_accelerator_map_save instead
> >  (GNOME applications do this automatically through gnome-libs)
> >- apps that do not bother saving/loading should call
> >  gtk_accelerator_map_lock() after gtk_init()
> >- explicit accelerator installation (most people just do this through
> >  GtkItemFactory and won't notice) should be done through
> >  gtk_accel_group_add() and gtk_accel_group_remove() instead of
> >  gtk_widget_add_accelerator() and gtk_widget_remove_accelerator()
> >  unless we decide to keep those
> >- monitoring accelerator changes will work through a GtkAccelGroup
> >  signal now and not GtkWidget signals, i highly doubt people are
> >  actually doing that in their apps though (if so, i'd be interested
> >  in hearing why)
> >
> Overall, this sounds like a good change.  People get very confused 
> looking at the GtkAccelGroup header at the moment, as the majority of 
> the functions found in it are not meant to be used by normal apps :) 

jup, those are gone or made private now. it'll go into CVS as soon
as i have the docs on the remaining functions done ;)

>  Things get even more confusing when demos distributed with gtk use the 
> supposedly internal functions.  If this results in less APIs (it sounds 
> like it does), then that would be good.
> 
> James.
> 

---
ciaoTJ




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]