Re: gtk-hp-patches

[Tim Janik <timj gtk org>]

On 8 Aug 2000, Owen Taylor wrote:

> 
> Here, after much delay, is my set of comments on the
> big gtk-hp-patches diff from a few weeks ago. Almost
> everything in here are;
>   
>  - stylistic issues
>  - suggestions for better ways of doing things
>  - bugs I noticed
>  
> There might be one or two naming issues as well. 
> 
> [ I'm not sure that this will make much sense to anybody but
> Havoc, and to Havoc only with a copy of the patch to
> grep through for comparison, but I'm mailing it to gtk-devel-list
> for completeness. ]

well it does a bit of good at least, since i have minor
coments on your comments ;)
for one, i've followed up on the inlining pixbufs thread with
reiterating my suggestions of using gimp's existing CSource code,
adopted to your's and havoc's comments. so i'm going to hack the
details of the inlining after havocs code.

> 
> Regards,
>                                         Owen
> 

> ===========
> +      if (keyval && accel_group)
> +        {
> +          gtk_widget_add_accelerator (button,
> +                                      "clicked",
> +                                      accel_group,
> +                                      keyval,
> +                                      GDK_MOD1_MASK,
> +                                      GTK_ACCEL_VISIBLE);
> ===========
> 
> While neither really has an effect here I think I would
> not include GTK_ACCEL_VISIBLE and would include GTK_ACCEL_LOCKED,
> since the accelerator is not visible, and cannot be changed.

you're right that GTK_ACCEL_LOCKED should be used, but also
GTK_ACCEL_VISIBLE. that takes effect if accel labels are used,
and for some people, especially newbies it can be quite helpfull
to use accel labels in buttons and display accelerators, at least
initially. so while for the case of a normal label and/or pixmap
being used it doesn't really affect things, it could if accel labels
are going to be used for some buttons.

> =========
> --- gtk/testgtk.c	2000/06/24 22:32:05	1.186
> +++ gtk/testgtk.c	2000/06/28 19:43:16	1.186.4.3
> @@ -166,8 +166,13 @@
>  
>    if (!window)
>      {
> +      GtkAccelGroup *accel_group;
> +      
>        window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
>  
> +      accel_group = gtk_accel_group_new ();
> +      gtk_window_add_accel_group (GTK_WINDOW (window), accel_group);
> +      
>        gtk_signal_connect (GTK_OBJECT (window), "destroy",
>  			  GTK_SIGNAL_FUNC (gtk_widget_destroyed),
>  			  &window);
> =======
> 
> You might as well use gtk_window_get_acel_group(), Also, that
> will avoid the leak of the accel group you have here.

s/gtk_window_get_acel_group/gtk_window_get_default_accel_group/


---
ciaoTJ