Re: accelerators

[James Henstridge <james daa com au>]

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.

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

> 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?

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.

> 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
>
> - 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
>
> - remove: gtk_item_factory_print_func, gtk_item_factory_parse_rc, 
>  gtk_item_factory_parse_rc_string, gtk_item_factory_parse_rc_scanner,
>  gtk_item_factory_add_foreign, gtk_item_factory_dump_items,
>  gtk_item_factory_dump_rc
>
> - 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):
>   */
>  void gtk_accelerator_map_foreach (GtkAccelPoolForeach foreach_func,
>                                    gpointer            data,
>                                    gboolean            ignore_filters);
>  void gtk_accelerator_map_enter   (const gchar        *accel_path,
>                                    const gchar        *accel_key);
>  /* 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);
>
> - introduce, and for toplevels implement, the new interface:
>  struct GtkAccelKey {
>    guint keyval;
>    guint modifier;
>    guint fixed : 1; /* !removable */
>  };
>  struct GtkAcceleratableClass {
>    GTypeInterface base_iface;
>    gboolean	 (*remove_accel)	(GtkAcceleratable *acceleratable,
>                                         guint             keyval,
>                                         guint             modifier);
>    /* list accels and mnemonics */
>    GtkAccelKey* (*list_accels)		(GtkAcceleratable *acceleratable,
>  					 guint            *n_accels);
>    void	 (*queue_changed)	(GtkAcceleratable *acceleratable,
>  					 guint             keyval,
>  					 guint             modifier);
>    /* signals */
>    void	 (*accels_changed)	(GtkAcceleratable *acceleratable);
>  };
>  gboolean	gtk_acceleratable_remove_accel	(GtkAcceleratable *acceleratable,
>                                                 guint             keyval,
>                                                 guint             modifier);
>  GtkAccelKey*	gtk_acceleratable_list_accels	(GtkAcceleratable *acceleratable,
> 	  					 guint            *n_accels);
>  void		gtk_acceleratable_queue_changed	(GtkAcceleratable *acceleratable,
> 	  					 guint             keyval,
> 	  					 guint             modifier);
>  a few notes on the purpose of the above functions:
>  listing all accelerators and getting changed notification is required for
>  plug/socket code to figure what key combos need to be passed between
>  embedded apps. removing accelerators (and returning whether removal was
>  successfull, i.e. the accelerator was unlocked) is needed to change and
>  override accelerators at runtime.
>  for GtkWindow's GtkAcceleratable implementation, removal and listing will
>  also account for mnemonics (they are basically treated as locked
>  accelerators)
>
> - nuke gtk_accel_group_get_default(), there's no point in having a default
>  accelerator group, it just enables unmaintained shortcuts in the wrong
>  cases, which is one of the major bugs that drove some people into always
>  locking accelerators
>
> - for accelerator installation/removal, offer as public API:
>    void        gtk_accel_group_add             (GtkAccelGroup  *accel_group,
>                                                 const gchar    *path,
>                                                 guint           accel_key,
>                                                 GdkModifierType accel_mods,
>                                                 GtkAccelFlags   accel_flags,
>                                                 GObject        *object,
>                                                 const gchar    *accel_signal);
>    void        gtk_accel_group_remove          (GtkAccelGroup  *accel_group,
>                                                 guint           accel_key,
>                                                 GdkModifierType accel_mods,
>                                                 GObject        *object);
>    void        gtk_accel_group_connect         (GtkAccelGroup  *accel_group,
>                                                 const gchar    *path,
>                                                 guint           accel_key,
>                                                 GdkModifierType accel_mods,
>                                                 GClosure       *closure);
>    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?

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

> - for internal changed notification from GtkAccelGroup->GtkAcceleratable,
>  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?

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

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

--
Email: james daa com au
WWW:   http://www.daa.com.au/~james/