Re: Review of accelerator changes

[Tim Janik <timj gtk org>]

On 17 Nov 2001, Owen Taylor wrote:

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

yep, gtkwidget.c basically listens on accel maps and updates key
bindings in accel groups accordingly.

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

i did consider the basic idea of always setting up closures
on the groups and defer actuall accel binding of them to path
changes. it didn't really improve things, as it comes with some
noticable drawbacks, e.g. ::accel_changed on a accel groups will
have as many signal connections as there are _possible_ accelerator
connections on this group, walking accel group entries needs
special casing, figuring bindings on accel closures of a widget
needs special casing etc...

but, as i wrote in an earlier private email already, if i'd see a
patch that changed things towards this end, simplified current API
and introduced no major drawbacks, i'd probably accept it ;)

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

eh? no, the variants you suggested are fine. the effect of
gtk_accel_groups_disconnect() is easily reproduced by
gtk_accel_group_from_closure() + gtk_accel_group_disconnect().

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

well, conceptually it should really be some
acceleratable (or similarly named) interface, since we *do* special
case windows internally. but since we don't expose that and merely
operate on objects, i guess there's no particular reason to call
the object anything besides "object", thus it'd be better to scratch
the "activate_".

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

that'll work only when the closure is handy. one case i had was checking
all the closures and match on:
  found_one = closure->data == my_object && closure->marshal == my_marshal.

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

you will need it for your plug keyboard table code, and i could imagine
that third party code may popup that'd also be interested in such a
keyboard table, accessibility?

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

i guess it should be in the "special, hard to fix" section then? ;)

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

convention. menu paths that formerly mapped accelerators had to follow
that convention, and i don't see any particular good reason to change
that now. in fact, most accel paths will be reusing item factory paths
anyways, and the rest, the directly setup portion (though that's probably
rare) doesn't gain anything from allowing freestyle. in fact, i think that
could lead to people not bothering enough, resulting in apps that  end
up having multiple paths that say "foo", "bar", "baz", "bla", "something"
and "hello".
so the easiest decision to make was simply to adopt the existing
menu path conventions for accel paths.

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

erm, is this about one missing or being extraneous, or can't we use
parenthesis at all in arg descriptions...?


>  
> Regards,
>                                         Owen

---
ciaoTJ