Re: Mnemonics patch

[Tim Janik <timj gtk org>]

On Wed, 21 Mar 2001, Alexander Larsson wrote:

> Ok. Here is the final patch for label uline accelerators, now called
> mnemonics. I really like it.

good.

> If nobody says otherwise before 20:00 CET today I will check this in,
> since I'll be gone for a while snowboarding and I don't want this to
> bitrot in my tree while I'm gone.

ok, i have a couple of comments on your patch,
though since you aren't around i will probably
fix them myself ;)

> Index: gtkwidget.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkwidget.c,v
> retrieving revision 1.195
> diff -u -p -r1.195 gtkwidget.c
> --- gtkwidget.c	2001/03/18 04:50:34	1.195
> +++ gtkwidget.c	2001/03/21 11:32:36
> @@ -62,6 +62,7 @@ enum {
>    DIRECTION_CHANGED,
>    ADD_ACCELERATOR,
>    REMOVE_ACCELERATOR,
> +  ACTIVATE_MNEMONIC,
>    GRAB_FOCUS,
>    EVENT,
>    BUTTON_PRESS_EVENT,
> @@ -188,6 +189,8 @@ static gint gtk_widget_event_internal
> 
>  static void gtk_widget_propagate_hierarchy_changed (GtkWidget *widget,
>  						    gpointer   client_data);
> +static gboolean gtk_widget_real_activate_mnemonic  (GtkWidget *widget,
> +						    gboolean   group_cycling);
> 
>  static GtkWidgetAuxInfo* gtk_widget_aux_info_new     (void);
>  static void		 gtk_widget_aux_info_destroy (GtkWidgetAuxInfo *aux_info);
> @@ -290,6 +293,7 @@ gtk_widget_class_init (GtkWidgetClass *k
>    klass->direction_changed = gtk_widget_direction_changed;
>    klass->add_accelerator = (void*) gtk_accel_group_handle_add;
>    klass->remove_accelerator = (void*) gtk_accel_group_handle_remove;
> +  klass->activate_mnemonic = gtk_widget_real_activate_mnemonic;
>    klass->grab_focus = gtk_widget_real_grab_focus;
>    klass->event = NULL;
>    klass->button_press_event = NULL;
> @@ -460,6 +464,14 @@ gtk_widget_class_init (GtkWidgetClass *k
>    widget_signals[REMOVE_ACCELERATOR] =
>      gtk_accel_group_create_remove (GTK_CLASS_TYPE (object_class), GTK_RUN_LAST,
>  				   GTK_SIGNAL_OFFSET (GtkWidgetClass, remove_accelerator));
> +  widget_signals[ACTIVATE_MNEMONIC] =
> +    gtk_signal_new ("activate_mnemonic",
> +		    GTK_RUN_LAST,
> +		    GTK_CLASS_TYPE (object_class),
> +		    GTK_SIGNAL_OFFSET (GtkWidgetClass, activate_mnemonic),
> +		    gtk_marshal_BOOLEAN__BOOLEAN,
> +		    GTK_TYPE_BOOL, 1,
> +		    GTK_TYPE_BOOL);
>    widget_signals[GRAB_FOCUS] =
>      gtk_signal_new ("grab_focus",
>  		    GTK_RUN_LAST | GTK_RUN_ACTION,
> @@ -2174,6 +2186,36 @@ gtk_widget_accelerator_signal (GtkWidget
>      return ac_entry->signal_id;
>    return 0;
>  }
> +
> +gboolean
> +gtk_widget_activate_mnemonic (GtkWidget *widget,
> +                              gboolean   group_cycling)
> +{
> +  gboolean handled = FALSE;
> +
> +  g_return_val_if_fail (GTK_IS_WIDGET (widget), FALSE);
> +
> +  if (!GTK_WIDGET_IS_SENSITIVE (widget))
> +    return TRUE;
> +
> +  gtk_signal_emit_by_name (GTK_OBJECT (widget),
> +                           "activate_mnemonic",

no need to use _by_name here, you have widget_signals[ACTIVATE_MNEMONIC].

> +                           group_cycling,
> +                           &handled);
> +  return handled;
> +}
> +
> +static gboolean
> +gtk_widget_real_activate_mnemonic (GtkWidget *widget,
> +                                   gboolean   group_cycling)
> +{
> +  if (group_cycling)
> +    gtk_widget_grab_focus (widget);
> +  else if (!group_cycling)
> +    gtk_widget_activate (widget);
> +  return TRUE;
> +}

in my original mail, i had this:

  if (!GTK_WIDGET_IS_SENSITIVE (widget))
    return TRUE;
  if (!GTK_WIDGET_CAN_FOCUS (widget) && group_cycling)
    return FALSE;
  if (!group_cycling && !GTK_WIDGET_GET_CLASS (widget)->activate_signal)
    return FALSE;

you're right in not adding this to gtk_widget_activate_mnemonic(),
since that would defeat e.g. GtkLabel overriding this signal,
but we still need it in the default implementation. since for
some widgets which aren't "activatable", installing mnemonics
could still make sense to have focus cycling, i guess the best
thing to do is:

static gboolean
gtk_widget_real_activate_mnemonic (GtkWidget *widget,
                                   gboolean   group_cycling)
{
  if (!group_cycling && GTK_WIDGET_GET_CLASS (widget)->activate_signal)
    gtk_widget_activate (widget);
  else if (GTK_WIDGET_CAN_FOCUS (widget))
    gtk_widget_grab_focus (widget);
  else
    {
      g_warning ("widget `%s' isn't suitable for mnemonic activation",
                 G_OBJECT_TYPE_NAME (widget));
      gdk_beep ();
    }
  return TRUE;
}


> @@ -174,6 +176,19 @@ GList*	   gtk_window_list_toplevels
> 
>  /* Get the "built-in" accel group (convenience thing) */
>  GtkAccelGroup* gtk_window_get_default_accel_group (GtkWindow *window);
> +
> +void     gtk_window_add_mnemonic          (GtkWindow *window,
> +					   guint      keyval,
> +					   GtkWidget *target);
> +void     gtk_window_remove_mnemonic       (GtkWindow *window,
> +					   guint      keyval,
> +					   GtkWidget *target);
> +gboolean gtk_window_activate_mnemonic     (GtkWindow *window,
> +					   guint      keyval,
> +					   guint      modifier);
> +void     gtk_window_set_mnemonic_modifier (GtkWindow *window,
> +					   guint      modifier);
> +

either add_mnemonic/remove_mnemonic need to be signals, or we need a
::mnemonics_changed signal for GtkWindow, otherwise the accel group
or plug/socket can't monitor mnemoics. also for that purpose,
we need a list_mnemonics function.
as for add_mnemonic/remove_mnemonic vs. mnemonics_changed, the former
is better to keep detailed track of the mnemonics list on a window,
i'm not sure however we really need that.
i don't know exactly what owen wants for plug/socket, but at least
for accel group negotiation, i'd be fine with gtk_window_add_mnemonic
and gtk_window_remove_mnemonic queueing an idle notifier to emit
::mnemonics_changed and then query the new mnemonics list from there.
the only thing that needs to be taken care about is that the idle notifier
priority, to avoid key events being handled before accel groups could
react to mnemonic changes, the idle notifier needs to be queued with
higher priority than the normal event handlers. since accel group
notification of mnemonic changes may involve accelerator changes,
which will most probably queue a resize on the widgets displaying
accelerator bindings, choosing a priority higher than GTK_PRIORITY_RESIZE
is prolly also not a bad idea.

owen, what do you think about:
#define GTK_PRIORITY_REDRAW     (G_PRIORITY_HIGH_IDLE + 20)
#define GTK_PRIORITY_RESIZE     (G_PRIORITY_HIGH_IDLE + 10)
#define GTK_PRIORITY_NOTIFY     (G_PRIORITY_HIGH_IDLE + 0)
?


> Index: gtkwindow.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkwindow.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 gtkwindow.c
> --- gtkwindow.c	2001/03/18 04:50:34	1.106
> +++ gtkwindow.c	2001/03/21 11:32:37

>  void
> +gtk_window_add_mnemonic (GtkWindow *window,
> +			 guint      keyval,
> +			 GtkWidget *target)
> +{
> +  GtkWindowMnemonic key;
> +  GtkWindowMnemonic *mnemonic;
> +
> +  g_return_if_fail (window != NULL);
> +  g_return_if_fail (GTK_IS_WINDOW (window));
> +  g_return_if_fail (GTK_IS_WIDGET (target));
> +
> +  key.window = window;
> +  key.keyval = keyval;
> +
> +  mnemonic = g_hash_table_lookup (mnemonic_hash_table, &key);
> +
> +  if (mnemonic)
> +    mnemonic->targets = g_slist_prepend (mnemonic->targets, target);

at least for now, where this code is new, it's probably not a bad idea
to add:
g_return_if_fail (g_slist_find (mnemonic->targets, target) == NULL);
here.

> +  else
> +    {
> +      mnemonic = g_new (GtkWindowMnemonic, 1);
> +      *mnemonic = key;
> +      mnemonic->targets = g_slist_prepend (NULL, target);
> +      g_hash_table_insert (mnemonic_hash_table, mnemonic, mnemonic);
> +    }
> +}
> +
> +void
> +gtk_window_remove_mnemonic (GtkWindow *window,
> +			    guint      keyval,
> +			    GtkWidget *target)
> +{
> +  GtkWindowMnemonic key;
> +  GtkWindowMnemonic *mnemonic;
> +
> +  g_return_if_fail (window != NULL);
> +  g_return_if_fail (GTK_IS_WINDOW (window));
> +  g_return_if_fail (GTK_IS_WIDGET (target));
> +
> +  key.window = window;
> +  key.keyval = keyval;
> +
> +  mnemonic = g_hash_table_lookup (mnemonic_hash_table, &key);
> +
> +  g_assert (mnemonic);

be carefull with asserts. if someone tries to remove a non-existing
mnemonic, gtk will abort with this.
instead, you should have used:
  g_return_if_fail (mnemonic && g_slist_find (mnemonic->targets, target) != NULL);
here.

> +
> +  if (mnemonic)
> +    {
> +      mnemonic->targets = g_slist_remove (mnemonic->targets, target);
> +
> +      if (mnemonic->targets == NULL)
> +	{
> +	  g_hash_table_remove (mnemonic_hash_table, mnemonic);
> +	  g_free (mnemonic);
> +	}
> +    }
> +}

> +gboolean
> +gtk_window_activate_mnemonic (GtkWindow *window,
> +                             guint      keyval,
> +                             guint      modifier)
> +{
> +  GtkWindowMnemonic key;
> +  GtkWindowMnemonic *mnemonic;
> +  GSList *list;
> +  GtkWidget *widget, *chosen_widget;
> +  gboolean overloaded;
> +
> +  g_return_val_if_fail (window != NULL, FALSE);
> +  g_return_val_if_fail (GTK_IS_WINDOW (window), FALSE);
> +
> +  if (modifier != window->mnemonic_modifier)
> +    return FALSE;

this breaks if NUM_LOCK is turned on, we actually need to:

  if (window->mnemonic_modifier != (modifier & gtk_accelerator_get_default_mod_mask ()))
      return FALSE;

> @@ -1358,6 +1529,24 @@ gtk_window_destroy (GtkObject *object)
>    GTK_OBJECT_CLASS (parent_class)->destroy (object);
>  }
> 
> +static gboolean
> +gtk_window_mnemonic_hash_remove (gpointer	key,
> +				 gpointer	value,
> +				 gpointer	user)
> +{
> +  GtkWindowMnemonic *mnemonic = key;
> +  GtkWindow *window = user;
> +
> +  if (mnemonic->window == window)
> +    {
> +      g_slist_free (mnemonic->targets);
> +      g_free (mnemonic);

if widgets were acting correctly upon ::heirarchy_changed, and removed
their mnemonics, this list should always be empty at finalization time,
so it's probably a good idea to puke if it isn't, e.g.:

      if (mnemonic->targets)
        {
          gchar *name = gtk_accelerator_name (mnemonic->keyval, 0);

          g_warning ("mnemonic \"%s\" wasn't removed for widget (%p)",
                     name, mnemonic->targets->data);
          g_free (name);
        }


> +
> +      return TRUE;
> +    }
> +  return FALSE;
> +}
> +
>  static void
>  gtk_window_finalize (GObject *object)
>  {
> @@ -1373,6 +1562,10 @@ gtk_window_finalize (GObject *object)
>    g_free (window->wmclass_name);
>    g_free (window->wmclass_class);
> 
> +  g_hash_table_foreach_remove (mnemonic_hash_table,
> +			       gtk_window_mnemonic_hash_remove,
> +			       window);
> +
>    G_OBJECT_CLASS (parent_class)->finalize (object);
>  }
> 



> Index: gtklabel.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtklabel.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 gtklabel.c
> --- gtklabel.c	2001/03/09 16:43:15	1.79
> +++ gtklabel.c	2001/03/21 11:32:34

> @@ -39,6 +43,7 @@ struct _GtkLabelSelectionInfo
>    gint selection_end;
>  };
> 
> +
>  enum {
>    PROP_0,
>    PROP_LABEL,
> @@ -49,7 +54,7 @@ enum {
>    PROP_PATTERN,
>    PROP_WRAP,
>    PROP_SELECTABLE,
> -  PROP_ACCEL_KEYVAL
> +  PROP_MNEMONIC_KEYVAL

we need a property PROP_MNEMONIC_WIDGET as well.

>  };

> +static gboolean
> +gtk_label_activate_mnemonic (GtkWidget *widget,
> +			     gboolean   group_cycling)
> +{
> +  GtkWidget *parent;
> +
> +  if (GTK_LABEL (widget)->mnemonic_widget)
> +    return gtk_widget_activate_mnemonic (GTK_LABEL (widget)->mnemonic_widget, group_cycling);
> +
> +  /* Try to find the widget to activate by traversing the widget
> +   * hierarachy.
> +   */
> +
> +  parent = widget->parent;
> +  while (parent)
> +    {
> +      if (GTK_WIDGET_CAN_FOCUS (parent) ||
> +	  (!group_cycling && GTK_WIDGET_GET_CLASS (parent)->activate_signal) ||
> +	  (parent->parent && GTK_IS_NOTEBOOK (parent->parent)) ||
> +	  (GTK_IS_MENU_ITEM (parent)))
> +	return gtk_widget_activate_mnemonic (parent, group_cycling);
> +      parent = parent->parent;
> +    }

ok, this isn't particularly nice, it's about ugly enough
to justify a GTK_WIDGET_FLAG_MNEMONIC_ACTIVATABLE flag.
gtknotebook should then set that automatically on all its
tab_labels, since it handles their mnemonic activation through
signal handlers.
that we actually need to special case MENU_ITEM here is due to menus
not using the normal focussing mechanisms, so menu items don't have
the CAN_FOCUS flag set. that's a bit odd, but definitely menu item
specific, it'd be taken care off by GTK_WIDGET_FLAG_MNEMONIC_ACTIVATABLE
as well though.

> +  g_warning ("Couldn't find a target for a mnemonic activation.");
> +  gdk_beep ();
> +
> +  return FALSE;
> +}
> +
> +static void
> +gtk_label_setup_mnemonic (GtkLabel *label, guint last_key)
> +{
> +  GtkWidget *toplevel;
> +
> +  if ((last_key != GDK_VoidSymbol) && label->mnemonic_window)
> +    gtk_window_remove_mnemonic  (label->mnemonic_window,
> +				 last_key,
> +				 GTK_WIDGET (label));
> +
> +  if (label->mnemonic_keyval == GDK_VoidSymbol)
> +    return;
> +
> +  toplevel = gtk_widget_get_toplevel (GTK_WIDGET (label));
> +
> +  if (GTK_IS_WINDOW (toplevel))
> +    gtk_window_add_mnemonic (GTK_WINDOW (toplevel),
> +			     label->mnemonic_keyval,
> +			     GTK_WIDGET (label));
> +}
> +
> +static void
> +gtk_label_hierarchy_changed (GtkWidget *widget)
> +{
> +  GtkLabel *label = GTK_LABEL (widget);
> +
> +  gtk_label_setup_mnemonic (label, label->mnemonic_keyval);
> +}
> +
> +
> +/**
> + * gtk_label_set_mnemonic_widget:
>   * @label: a #GtkLabel
> + * @widget: the target #GtkWidget
> + *
> + * If the label has been set so that it has an mnemonic key (using i.e.
> + * @gtk_label_set_markup_with_mnemonic, @gtk_label_set_text_with_mnemonic,
> + * @gtk_label_new_with_mnemonic or the use_underline property) the label can be
> + * associated with a widget that is the target of the mnemonic. When the label
> + * is inside a widget (like a #GtkButton or a #GtkNotebook tab) it is automatically
> + * associated with the correct widget, but sometimes (i.e. when the target is
> + * a #GtkEntry next to the label) you need to set it explicitly using this
> + * function.
> + *
> + * The target widget will be accelerated by emitting "activate_mnemonic" on it.
> + * The default handler for this signal will activate the widget if there are no
> + * mnemonic collisions and toggle focus between the colliding widgets otherwise.
> + **/
> +void
> +gtk_label_set_mnemonic_widget (GtkLabel         *label,
> +			       GtkWidget        *widget)
> +{
> +  g_return_if_fail (GTK_IS_LABEL (label));
> +  g_return_if_fail (GTK_IS_WIDGET (widget));
> +
> +  label->mnemonic_widget = widget;

we need to keep a reference count here and we must allow NULL
here, so this needs to read:

void
gtk_label_set_mnemonic_widget (GtkLabel         *label,
                               GtkWidget        *widget)
{
  g_return_if_fail (GTK_IS_LABEL (label));
  if (widget)
    g_return_if_fail (GTK_IS_WIDGET (widget));

  if (label->mnemonic_widget)
    gtk_widget_unref (label->mnemonic_widget);
  label->mnemonic_widget = widget;
  if (label->mnemonic_widget)
    gtk_widget_ref (label->mnemonic_widget);
}

also we must get rid of this widget at ::destroy time.

> +}


> +static gboolean
> +gtk_notebook_activate_mnemonic_switch_page (GtkWidget *child,
> +                                         gboolean overload,
> +                                         gpointer data)
> +{
> +  GtkNotebook *notebook = GTK_NOTEBOOK (data);
> +  GtkNotebookPage *page;
> +  GList *list;
> +
> +  list = g_list_find_custom (notebook->children, child,
> +                          gtk_notebook_page_compare_tab);
> +  if (!list)
> +    return TRUE;
> +
> +
> +  page = list->data;
> +
> +  gtk_notebook_switch_page (notebook, page,  -1);
> +  return TRUE;
> +}

since i made gtk_widget_real_activate_mnemonic() puke if there's nothing
to activate, this function needs:

  gtk_signal_emit_stop_by_name (GTK_OBJECT (child), "activate_mnemonic");

actually you should have added that already, since you just connected to
the notebook page's tab_label, switched pages on mnemonic activation and
then still left the tab_label's default handler running (which for HBoxes
as in testgtk doesn't do anything, but breaks for real labels).

i introduced a signal accumulator for ::activate_mnemonic now that
aborts the emission as soon as a handler returned TRUE, that's the same
thing we'll do for events in the future, once all event handlers are fixed
to properly return TRUE/FALSE.

btw, i think gtk_widget_mnemonic_activate() is a better name than
gtk_widget_activate_mnemonic(), since we don't activate the widget's
mnemonic key, but, due to a mnemonic key being pressed, activate the widget.
(if there're no further objections on this, i'll change this the next
few days)

---
ciaoTJ