Re: Review of accelerator changes

[Owen Taylor <otaylor redhat com>]

[ To try to keep this of reasonable length, I've omitted places
  where we've come to agreement, or I've given up ]

Tim Janik <timj gtk org> writes:

> > I. Introduction
> > ===============
> 
> > 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
> 
> that's misinterpretation. basically accel group knows _nothing_
> about accel map, and accel map knows how accels relate to groups
> and how groups relate to windows. there's not much scattering going
> on there though ;)

I don't mind how AccelMap knows about AccelGroups, but I think 
you've traded AccelGroups knowing just a bit about AccelMaps
for GtkWidget knowing a lot about AccelMap and AccelGroup
in a complicated way that anybody else who wanted to use AccelMap
would have to duplicate.

And in fact, GtkAccelGroup has these weird "path quarks" that
don't make any sense unless you know about GtkAccelMap, so
I don't think you can really claim that it is independent 
of GtkAccelMap. (If there was any real benefit to that.)
 
> > it to work, how it actually does work, and how I think it
> > could be made a lot simpler than it is currently.
> 
> you seem to be missing the case of installing an accelerator on
> a menu item out of the blue though, which renders your idea pretty
> much unusable (i've also been there at some early design stages though ;)

Ah, but you've missed an easy way of handling this because you 
are stuck in the "old fashioned" idea that a GtkAccelGroup is
a list of key bindings with associated callbacks. If we 
view a GtkAccelGroup as a list of closures with associated
key bindings, then it's clear how we solve it.

As well as allowing

 closure => keybinding

and 

 closure => path => keybinding

we could allow:

 closure => path =>

That is, accelerators with no key modifiers.

> > 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>
> 
> not quite, it's more like:
> 
> GtkAccelGroup:
> 
>   GClosure  <=> (key,modifiers) + [optional path_quark for negotiation]
>   
> GtkAccelMap - a global map (didn't see a point in creating a singleton object) of:
>  
>     <accel-path>     <=>   <key/modifiers>

I believe that this is a fundementally clumsier way of thinking
about things -- we have the introduction of the paths into
the interface without enough structure for them to actually make
sense.
  
> > 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.
> 
> i don't quite understand... the "complicated internal logic" to detect/resolve
> clashes is all internal... the toplevel involving part is also help private
> through _gtk_window_query_nonaccels().

The non-internal part of the logic is the fact that it is base
on the set of notifiers that have been added to the GtkAccelMap.
That is, I'm fine with "complicated internal logic", but the
idea that the caller has to handle the propagation of changes
from GtkAccelMap to GtkAccelGroup bothers me.
 
> > 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.
> 
> yes, we need special code to react to changes in the mapping:
> 	
>     <accel-path>     <=>   <key/modifiers>
> 
> the possible cases to handle here are:
> 1) need to remove a closure
> 2) need to install a new closure
> 3) need to reconnect a closure from one accelerator to another
> all of these happen on the same group btw.
> 
> now creating a _new_ closure is actually the tricky part, since for
> 1) and 3), the accel_Group involved can be queried from the clposure.
> installing a new closure can't happen out of the blue however, for
> widgets they need to be specially created, and accel map has no
> knowledge about doing that. and it'd be false for it to have that
> knowledge, 'cause then accel-paths would only work for widgets.

See above. I don't believe that GtkAccelMap should ever need to
create new closures, just to reconnect closures for existing widgets.

> the naming "notifer" which was intended to be "notifier" is somewhat
> of a misnomer however, "Handler" might be more appropriate.

If I don't convince you to make more fundamental changes, I'd
certainly like to see this name change.
 
> > 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.
> 
> right, anyone going to support accel paths on objects other than
> widgets will need to supply those ~100 lines (<cough>, <sweat>)
> of code.
> as for the rationale, this is not just some random inconvenient
> code, but really embedds the widget specific parts of it, i.e.:
> - widgets need special closures. their closures emit the "activate"
>   signal on widgets, and need to make sure to _not_ activate insensitive
>   widgets, so they need to be specially created
> - widgets want to provide notification though ::accel-closures-changed
>   upon path changes that affect them
> - widgets want to know about their accel closures, so they can hand
>   them out via gtk_widget_get_accel_closures()
> - accel closures of widgets need to be invalidated once the widget
>   dies, since we don't want segfault-per-hotkey, do we? ;)
> 
> that's pretty much all the code does, it's widget specific on every
> edge, and i don't really see good ways to have accel map/group do
> that magically.

None of the above implies any particular complexity. If the code
simply created the closure and added it to the group when the
path/group was set, and invalidated when the path/group was
unset/changed then the code would be a lot shorter, and a lot
easier to understand. (It literally took me hours to figure out
what was going on with the current code.)
 
> > 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.
> 
> it unfortunately can't, as much as i'd like to make this simplification.
> the reason being the widget specific parts that need to be implemented as
> outlined above.

The only fundamental objection I see is the need to create closures
which is pretty easily solved by allowing closures to be created
and not connected to any key binding.

> > III. A ramble about gtk_widget_set_accel_path
> > =============================================
> > 
> > [ Don't expect much of a point to this section ]
> 
> erm, now, should i respond to this or not? ;)

The only point I really cared about there is the discussion of
gtk_menu_set_accel_path(), but this mail is already far too
long, so let me let just let that pass; I still think it's
a bad idea, but it isn't a bad idea that has consequences
elsewhere.

> > 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.
> 
> as stated above, i don't quite agree here.
> i think it's conceptually unclean to have the accel gorup upon
> gtk_accel_group_connect_path() automatically figure the accel key
> and mods through the accel-path, but be completely dumb and unaware
> of accel paths (their creation, changes) otherwise.

This is again, a "see above, I think it can work fine". 
 
> >  * 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.
> 
> yes, the reason i pass a quark here is burried deep in the negotiation code.
> i don't actually want to make a huge amount of string comparisons or
> hashtable lookups there with quarks i get a decent integer comparison, and
> the accel-path changed notifier already has the quark available.
> 
> the problem might very well lie in the type and name i choosed, things
> will still work quite well, if we have:
 
[...]

> /* new function: */
> void            gtk_accel_group_connect_with_hint (GtkAccelGroup  *accel_group,
>                                                  guint           accel_key,
>                                                  GdkModifierType accel_mods,
>                                                  GtkAccelFlags   accel_flags,
>                                                  GClosure       *closure
>                                                  guint           map_hint);

How would you describe the function of the hint in any way that wouldn't
basically describe it's use as an accelerator path. Unlike the
notifier/handler change, this name change doesn't actually remove
a misleading name, what it does is pretend something that isn't
generic is generic. Which doesn't really help anyways.
 
> >  * Instead of gtk_accel_group_disconnect, gtk_accel_group_disconnect_closure(),
> 
> accel_groupS_disconnect_closure, i.e.:
> gboolean    gtk_accel_groups_disconnect_closure (GClosure       *closure);
> 
> not to be anal about a spelling error you made in response, but to
> point out that this function automatically fetches the accel group
> from the closure and disconnects it from there, so basically this
> is a convenience function.
>
> >    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.
> 
> yeah, i was actually pondering something like this. then decided that
> a) the current way is good enough for the moment, b) i'd rather get
> the widget/menu API and accel map negotiation right, c) most importantly,
> owen is goint to catch this anyway and make a good suggestion.
> so, i'm fine with that.

So, do you want to have gtk_accel_groups_disconnect (GClosure *closure);
as well as gtk_accel_group_disconnect() as above?
 
> >    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().
> 
> sure, go ahead.

Any preference between

 gtk_accel_groups_from_object():

and 

 gtk_accel_groups_from_activate_object():

The first name seems less clumsy and since we don't have objects
for callbacks, not all that confusing.
> 
> >  * 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.
> 
> at this point, GtkAccelLabel remains the only valid external user,
> if you think the accel label should rather walk the list itself
> (accessing GtkAccelGroup.priv_accels, which IMHO is not such a
> good idea), we can make this private.
> keep in mind that accel label shouldn't just use a private function
> _gtk_accel_group_find(), since third party code will want to do
> the same thing, e.g. the accessibility code.

I'd tend to prefer gtk_accel_group_lookup_by_closure() or something
like that... that is, I don't think we need a general find function.
 
> >  * 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.
> 
> it needs to be public for things like the widget part of the accel
> implementation, which might very well be a menu action object in the
> future. however, this function is not of much use besides accel map
> handler implementation, which is why it's in the category i personally
> like to call semi-public eventhough you keep jumping on that ;)

Well, OK, let's just say /* Not typically needed */ rather than
/* Internal */ then, and describe in the doc comment when you
would actualy need it.

> >  * We need doc comments for:
> > 
> >     gtk_accel_groups_from_acceleratable()
> 
> do we need this function to be public? anyone?

I don't see any reason why anybody would need it.
 
> >     gtk_accel_group_find()
> >     gtk_accel_groups_disconnect_closure()
> 
> or better yet, your variants (which is why this has no docs
> yet ;)
> 
> >     gtk_accel_group_query()
> 
> grumble, but this is a semi-public guru-entry point ;))

Even gurus need docs some times ;-)


> >  * 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.
> 
> damn, i mean to finish this up with you on irc. ;(
> change on the fly should definitely be on by default. yes, i know, that'S
> not what windows does, but then i don't play a jingle everytime i open
> a window or flush my toilet either. this is still gtk land and i'm not working
> for MS, we've been having runtime chnagable accels since pre-1.0 and should
> deinfitely preserve it.

We had Motif-style scrollbar arrows since before 1.0, and I have no
regrets about removing that... 

Should we kick the question of what the default should be to the
useability project? since I don't think we'll really get anywhere
with the:

 "It's confusing for users"

vs.

 "It's a cool feature that everybody who has been using GTK+ since
  before 1.0 wants"

argument. Just much not data there.

> i do buy your argument of accidental chnages however, but i'd like to
> treat it with different measures:
> - one 1.0/1.2 problem is that accidental accel installation can delete
>   this accel if it was setup for other menu items. this can be circumvented
>   with an rc-file property:
>     can-replace-accelerators = yes/no
>   i don't particularly care the default here
> - accidental changes are probably way more likely without modifiers, so lets
>   just fix this issue with:
>     accelerators-need-modifiers = yes/no
>   it'll just become a gimp FAQ entry to put
>     accelerators-need-modifiers = no
>   into ~/.gtkrc ;)

Hmmm, making whether the feature exists or not configurable can
make sense. Making how it behaves configurable seems to imply
that we can't figure out how it should work. I think the
above changes might just cripple the behavior for the experienced
users without actually fixing the useability problems.

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

> >  * gtk_accel_map_load/save_fd() have portability problems.
> 
> well, you didn't adress that in my initial proposal, how would
> you go about them?

Well, that's why it is in the trivial section ... because I don't
have a good alternative. 

> >  * In gtk_accel_path_is_valid(), 
> > 
> >    "<<a>" is not a valid accel path, but "<a<>" is?
> 
> nope, i didn't want to add the full parsing code from item factory
> there though. proper accel paths are documented, and this is only
> for a g_return_if_fail() check which don't need to check every
> insane case. if you want to take a look at code that tries
> to catch any possible misuse, take gsignal.c, it goes to quite
> some lengths in that regard, so much that it hurts readability
> and gets ugly.
> i'll not complain if you change it into checking real validity.
> then, and after GQuark from add_entry() has been removed, it should
> probably be exported so gtkwidget.c can return_if_fail() on it.

An question I have is why GtkAccelMap enforces any sort of validity at
all on the paths since it doesn't interpret them at all... seems to me
to treat them (in GtkAccelMap, if not in GtkItemFactory) as arbitrary
strings.
 
> >  * Extra ) in arguments descriptions for gtk_accel_map_add_notifier()
> 
> hm? i don't see an extra parenthesis there...

Ah, it's actually in GtkAccelMapNotify which is immediately above.
 
Regards,
                                        Owen