Re: Two insights about underline accelerators

[Havoc Pennington <hp redhat com>]

Alexander Larsson <alla lysator liu se> writes:
> I added an extra argument to gtk_label_set_accel_widget to select if
> you want to emit "activate" (and "grab_focus" if overloaded) or just
> "grab_focus". I had to do this, because for some label targets
> "activate" doesn't make sence (like for a GtkEntry or a GtkNotebook
> page).

This is bad, it makes the programmer do something we can do. We need a
mechanism to "just do the right thing" for the various widget types.

So the obvious mechanism again is a GtkAcceleratable interface,
which widgets override to do the right thing for that widget. 
Since overriding interfaces in derived classes isn't implemented yet,
you could simulate this with GTK_IS_FOO() hacks for now.

The GtkAcceleratable::accelerate() method would of course take a bool
indicating whether the accel is overloaded.

To minimize changes to accel group and get things working now, we
could also use the old "signal id" hack for interfaces that we
currently use to simulate Activatable and Scrollable. i.e. have a
signal on GtkWidget "accelerate" or whatever and put its signal ID in
the class struct.
 
> There are still some questions on the order of accelerators, and exactly
> how overloading works. In the patch accelerator groups attached to an
> object are activated in this order: first all groups not allowing
> conflicts, then all groups allowing conflicts, and then the default group.
> This means:
> * overloaded accelerators in different accelerator groups are not handled
>   well. The group that happens to be first is the only one used.

With this new way of doing things, you are supposed to always use the
"default uline accel group" for uline accelerators. So it's OK if we
only handle within accel groups.

> * menu accelerators in the default accel group "application wide" can be
>   overridden by label accelerators. I don't know if this is a
> problem.

I'm not sure we should keep gtk_window_get_default_accel_group(),
since that was created primarily to make this uline stuff convenient,
and now we are fixing it properly. If we do keep it, it should be for
menu accelerators, so maybe you should run it with the other menu
accel groups.

I'm not sure whether the uline or menu accels are suppposed to win in
case of conflict.
 
> I didn't implement automatic association of labels with accelerator
> widgets, because doing this an always emitting "activate" seems kinda
> broken to me? 

You don't emit activate, instead you call
GtkAcceleratable::accelerate() for the widget, so it's vritualized to
do the right thing for that widget.

This feature is absolutely essential, it means that to do uline
accelerators you just say gtk_label_new_with_accel() or
gtk_button_new_with_accel() and it magically works. That's the only
way to make it easy enough to add accels that people will do it. ;-)

Windows is even less sophisticated than our suggestion, it simply
accelerates the widget next in the focus order after the label, and
you modify things by changing the focus order.

A couple points:
 - remember that Owen suggested making parse_uline() _not_ 
   set the accel keyval on the label; so legacy code calling
   parse_uline will not set up the accel automagically, 
   only new code calling with_accel
 - obviously if you explicitly set the accel widget on the label, it
   would override the automagic finding of the accel widget

> What if the target found was an entry with
> set_activates_default set? Then this could close the dialog or
> something.

In this particular case, accelerating an entry just focuses it. But if
the accel widget is a button that closes the dialog, then yes it
should close the dialog to hit the accel for that button, I think
that's right.

> gtk_button_from_stock() previosly set up the stock items accelerator too,
> but when i removed the accel_group argument to the function i no longer
> know where to install the accelerator. Shall i just ignore it, or install
> a ::hierarchy_changed handler to automatically find the toplevel window of
> the button and use gtk_window_get_default_accel_group()?

My thought here is that putting a Ctrl+XX menu accel on the button 
is probably silly.

> gtk_image_menu_item_new_from_stock() still takes an accel_group
> argument. This is probably necessary, and is not normally a problem
> because you use GtkItemFactory, which has a accel group.

Yep.

> +	  if (choosen_entry)

Only one "o" in that ;-)

> +GtkAccelGroup* gtk_window_get_default_uline_accel_group (GtkWindow
> *window);

I wonder if we shouldn't think of a better name than "uline" and use
it consistently - "mnemonic" seems the obvious choice. Then I guess
we'd rename all the with_accel() to with_mnemonic().

Opinions?

> +      group = gtk_accel_group_new (FALSE);

Adding an arg to this existing function is somewhat questionable,
should probably have gtk_accel_group_new_allow_conflicts() or
something.
  
>  struct _GtkLabelClass
>  {
>    GtkMiscClass parent_class;
> +
> +  void (* run_accelerator) (GtkLabel *label);

Forgot the args to the signal here.

> +  signals[RUN_ACCELERATOR] =
> +    gtk_signal_new ("run_accelerator",

Need to use the new g_signal_newc() stuff.

> +static void
> +gtk_label_run_accelerator (GtkLabel *label,
> +			   gboolean overloaded,
> +			   gpointer accel_data)
> +{
> +  if (label->accel_widget)
> +    {
> +      guchar *signal;
> +
> +      signal = "grab_focus";
> +
> +      if (!overloaded && label->accel_activate)
> +	signal = "activate";
> +      
> +      gtk_signal_emit_by_name (GTK_OBJECT (label->accel_widget),
> +			       signal);
> +    }
> +}

OK, so here's where the intelligence goes - first by finding an accel
widget if it has none, second by virtualizing the action on the
widget.

I think the default implementation of Acceleratable::accelerate() for
GtkWidget should be:

 if (overloaded)
   "grab_focus"
 else if (widget->klass->activate_signal != 0)
    widget->klass->activate_signal

GtkEntry needs to override to avoid the activate, buttons should just
work out of the box, as should most other widgets.

> + * If @activate is FALSE pressing the accelerator will emit "grab_focus" on
> + * the target widget. If it is TRUE, "activate" will be emitted, unless there
> + * are several accelerators with the same accelerator key, then "grab_focus"
> + * will be emitted.

So these docs can be fixed to simply say "the widget will be
accelerated," simplifying things for people a lot.

> + **/
> +void
> +gtk_label_set_accel_widget (GtkLabel         *label,
> +			    GtkWidget        *widget,
> +			    gboolean          activate)

Again, no need for the "activate" arg.

If people want to do something really custom, they can set the accel
keyval to 0 so that the label doesn't do anything with accelerators,
then they hook things up manually using the accel group, as they would
have in GTK 1.2. But my guess is that people will only very, very
rarely want to do anything besides the default behavior, since we're
making it all just work, at least with the stock GTK widgets.

Havoc