Re: #52434 - Lock accelerators by default

[Havoc Pennington <hp redhat com>]

Can we get this patch in? Comments?

Havoc

Owen Taylor <otaylor redhat com> writes:
> I took a quick look at this bug - the idea being that accelerators
> should only be changeable for menu items if the entity creating
> the menu item wants them to be changeable.
> 
> Changing gtkwidget.c so accelerators are locked by default
> is easy enough - patch appended.
> 
> However this isn't quite right because we don't really want
> prevent the accelerators on the widget being changed programatically,
> just from being changed by the user.
> 
> I think the right thing to do might be to change things so:
> 
>  a) gtk_widget_add_accelerator/remove_accelerator() ignore the
>     locked flag and always work.
> 
>  b) gtkmenu.c explicitely checks the locked flag (already does so) 
>     before changing an accelerator.
> 
>  c) gtkmenu.c checks to see if there are any locked accelerators with
>     the same key before setting the accelerator on a widget to avoid
>     removing accelerators that you can't add back.  (Requires a small
>     GtkAccelGroup API addition. Ugly, yes, but internally ugly)
> 
> c) is not all that important, IMO, since I think having a mix of
> changeable and non-changeable accelerators on the same toplevel
> is pretty broken. But its a corner case we probably should handle.
> 
> Anyways, I think you might have said you had some interest in working
> on this, so if that's the case, I'll let you decide what we should
> do. Just thought I'd throw out my thoughts since I spent a bit of time
> looking on it.
> 
> Regards,
>                                         Owen
> 
> Index: gtk/gtkitemfactory.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkitemfactory.c,v
> retrieving revision 1.37
> diff -u -r1.37 gtkitemfactory.c
> --- gtk/gtkitemfactory.c	2001/03/24 00:07:16	1.37
> +++ gtk/gtkitemfactory.c	2001/03/29 23:57:39
> @@ -1221,6 +1221,7 @@
>  						    type_id != quark_type_title),
>  			   "GtkWidget::parent", parent,
>  			   NULL);
> +  gtk_widget_unlock_accelerators (widget);
>    if (option_menu && !option_menu->menu_item)
>      gtk_option_menu_set_history (option_menu, 0);
>  
> Index: gtk/gtkprivate.h
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkprivate.h,v
> retrieving revision 1.22
> diff -u -r1.22 gtkprivate.h
> --- gtk/gtkprivate.h	2001/03/08 06:14:42	1.22
> +++ gtk/gtkprivate.h	2001/03/29 23:57:39
> @@ -49,7 +49,8 @@
>    PRIVATE_GTK_IN_REPARENT       = 1 <<  6,
>    PRIVATE_GTK_DIRECTION_SET     = 1 <<  7,   /* If the reading direction is not DIR_NONE */
>    PRIVATE_GTK_DIRECTION_LTR     = 1 <<  8,   /* If the reading direction is DIR_LTR */
> -  PRIVATE_GTK_ANCHORED          = 1 <<  9    /* If widget has a GtkWindow ancestor */
> +  PRIVATE_GTK_ANCHORED          = 1 <<  9,   /* If widget has a GtkWindow ancestor */
> +  PRIVATE_GTK_ACCELS_LOCKED     = 1 <<  10   /* If accelerators are locked in widget */
>  } GtkPrivateFlags;
>  
>  /* Macros for extracting a widgets private_flags from GtkWidget.
> @@ -64,6 +65,7 @@
>  #define GTK_WIDGET_DIRECTION_SET(obj)	  ((GTK_PRIVATE_FLAGS (obj) & PRIVATE_GTK_DIRECTION_SET) != 0)
>  #define GTK_WIDGET_DIRECTION_LTR(obj)     ((GTK_PRIVATE_FLAGS (obj) & PRIVATE_GTK_DIRECTION_LTR) != 0)
>  #define GTK_WIDGET_ANCHORED(obj)          ((GTK_PRIVATE_FLAGS (obj) & PRIVATE_GTK_ANCHORED) != 0)
> +#define GTK_WIDGET_ACCELS_LOCKED(obj)     ((GTK_PRIVATE_FLAGS (obj) & PRIVATE_GTK_ACCELS_LOCKED) != 0)
>  
>  /* Macros for setting and clearing private widget flags.
>   * we use a preprocessor string concatenation here for a clear
> Index: gtk/gtkwidget.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkwidget.c,v
> retrieving revision 1.201
> diff -u -r1.201 gtkwidget.c
> --- gtk/gtkwidget.c	2001/03/29 23:02:26	1.201
> +++ gtk/gtkwidget.c	2001/03/29 23:57:39
> @@ -168,10 +168,21 @@
>  static gboolean gtk_widget_real_focus_out_event   (GtkWidget     *widget,
>  						   GdkEventFocus *event);
>  
> -static void gtk_widget_style_set		 (GtkWidget	    *widget,
> -						  GtkStyle          *previous_style);
> -static void gtk_widget_direction_changed	 (GtkWidget	    *widget,
> -						  GtkTextDirection   previous_direction);
> +static void gtk_widget_style_set               (GtkWidget        *widget,
> +						GtkStyle         *previous_style);
> +static void gtk_widget_direction_changed       (GtkWidget        *widget,
> +						GtkTextDirection  previous_direction);
> +static void gtk_widget_real_add_accelerator    (GtkWidget        *widget,
> +						guint             accel_signal_id,
> +						GtkAccelGroup    *accel_group,
> +						guint             accel_key,
> +						GdkModifierType   accel_mods,
> +						GtkAccelFlags     accel_flags);
> +static void gtk_widget_real_remove_accelerator (GtkWidget        *widget,
> +						GtkAccelGroup    *accel_group,
> +						guint             accel_key,
> +						GdkModifierType   accel_mods);
> +
>  static void gtk_widget_real_grab_focus           (GtkWidget         *focus_widget);
>  
>  static GdkColormap*  gtk_widget_peek_colormap      (void);
> @@ -313,8 +324,8 @@
>    klass->hierarchy_changed = NULL;
>    klass->style_set = gtk_widget_style_set;
>    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->add_accelerator = gtk_widget_real_add_accelerator;
> +  klass->remove_accelerator = gtk_widget_real_remove_accelerator;
>    klass->activate_mnemonic = gtk_widget_real_activate_mnemonic;
>    klass->grab_focus = gtk_widget_real_grab_focus;
>    klass->event = NULL;
> @@ -1087,6 +1098,7 @@
>  			GTK_PARENT_SENSITIVE |
>  			(composite_child_stack ? GTK_COMPOSITE_CHILD : 0) |
>  			GTK_DOUBLE_BUFFERED);
> +  GTK_PRIVATE_SET_FLAG (widget, GTK_ACCELS_LOCKED);
>  
>    widget->style = gtk_widget_peek_style ();
>    gtk_style_ref (widget->style);
> @@ -2090,41 +2102,13 @@
>       }
>  }
>  
> -static void
> -gtk_widget_stop_add_accelerator (GtkWidget *widget)
> -{
> -  g_return_if_fail (widget != NULL);
> -  g_return_if_fail (GTK_IS_WIDGET (widget));
> -
> -  gtk_signal_emit_stop (GTK_OBJECT (widget), widget_signals[ADD_ACCELERATOR]);
> -}
> -
> -static void
> -gtk_widget_stop_remove_accelerator (GtkWidget *widget)
> -{
> -  g_return_if_fail (widget != NULL);
> -  g_return_if_fail (GTK_IS_WIDGET (widget));
> -
> -  gtk_signal_emit_stop (GTK_OBJECT (widget), widget_signals[REMOVE_ACCELERATOR]);
> -}
> -
>  void
>  gtk_widget_lock_accelerators (GtkWidget *widget)
>  {
>    g_return_if_fail (widget != NULL);
>    g_return_if_fail (GTK_IS_WIDGET (widget));
> -  
> -  if (!gtk_widget_accelerators_locked (widget))
> -    {
> -      gtk_signal_connect (GTK_OBJECT (widget),
> -			  "add_accelerator",
> -			  GTK_SIGNAL_FUNC (gtk_widget_stop_add_accelerator),
> -			  NULL);
> -      gtk_signal_connect (GTK_OBJECT (widget),
> -			  "remove_accelerator",
> -			  GTK_SIGNAL_FUNC (gtk_widget_stop_remove_accelerator),
> -			  NULL);
> -    }
> +
> +  GTK_PRIVATE_SET_FLAG (widget, GTK_ACCELS_LOCKED);
>  }
>  
>  void
> @@ -2132,28 +2116,16 @@
>  {
>    g_return_if_fail (widget != NULL);
>    g_return_if_fail (GTK_IS_WIDGET (widget));
> -  
> -  if (gtk_widget_accelerators_locked (widget))
> -    {
> -      gtk_signal_disconnect_by_func (GTK_OBJECT (widget),
> -				     GTK_SIGNAL_FUNC (gtk_widget_stop_add_accelerator),
> -				     NULL);
> -      gtk_signal_disconnect_by_func (GTK_OBJECT (widget),
> -				     GTK_SIGNAL_FUNC (gtk_widget_stop_remove_accelerator),
> -				     NULL);
> -    }
> +
> +  GTK_PRIVATE_UNSET_FLAG (widget, GTK_ACCELS_LOCKED);
>  }
>  
>  gboolean
>  gtk_widget_accelerators_locked (GtkWidget *widget)
>  {
>    g_return_val_if_fail (GTK_IS_WIDGET (widget), FALSE);
> -  
> -  return gtk_signal_handler_pending_by_func (GTK_OBJECT (widget),
> -					     widget_signals[ADD_ACCELERATOR],
> -					     TRUE,
> -					     GTK_SIGNAL_FUNC (gtk_widget_stop_add_accelerator),
> -					     NULL) > 0;
> +
> +  return GTK_WIDGET_ACCELS_LOCKED (widget);
>  }
>  
>  void
> @@ -3555,6 +3527,44 @@
>  			      GtkTextDirection  previous_direction)
>  {
>    gtk_widget_queue_resize (widget);
> +}
> +
> +static void
> +gtk_widget_real_add_accelerator (GtkWidget        *widget,
> +				 guint             accel_signal_id,
> +				 GtkAccelGroup    *accel_group,
> +				 guint             accel_key,
> +				 GdkModifierType   accel_mods,
> +				 GtkAccelFlags     accel_flags)
> +{
> +  /* We now only stop interactive changing through the menus when an accelerator
> +   * is locked.
> +   */
> +#if 0  
> +  if (gtk_widget_accelerators_locked (widget))
> +    return;
> +#endif  
> +
> +  gtk_accel_group_handle_add (GTK_OBJECT (widget), accel_signal_id, accel_group,
> +			      accel_key, accel_mods, accel_flags);
> +}
> +
> +static void
> +gtk_widget_real_remove_accelerator (GtkWidget        *widget,
> +				    GtkAccelGroup    *accel_group,
> +				    guint             accel_key,
> +				    GdkModifierType   accel_mods)
> +{
> +  /* We now only stop interactive changing through the menus when an accelerator
> +   * is locked.
> +   */
> +#if 0
> +  if (gtk_widget_accelerators_locked (widget))
> +    return;
> +#endif  
> +
> +  gtk_accel_group_handle_remove (GTK_OBJECT (widget), accel_group,
> +				 accel_key, accel_mods);
>  }
>  
>  static void
> 
> _______________________________________________
> gtk-devel-list mailing list
> gtk-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gtk-devel-list