Re: #59885: patch

[Owen Taylor <otaylor redhat com>]

Kristian Rietveld <kristian planet nl> writes:

> Hi all,
> 
> Appended patch fixes bug #59885 (gtk-criticals in testgtk, (severity:
> major)). ChangeLog is seperate from the diff at the moment:
> 
> 2001-09-19  Kristian Rietveld  <kristian planet nl>
> 
> 	* tests/testgtk.c (struct OptionMenuItem): get rid of func,
> 
> 	(build_option_menu): add func argument, connect ::changed
> 	signal to option menu instead of connecting the ::activate
> 	signal to the menu items,
> 
> 	(toplevel): get rid of RADIOMENUTOGGLED macro,
> 
> 	(list_toggle_sel_mode), (clist_toggle_sel_mode),
> 	(ctree_toggle_line_style), (ctree_toggle_expander_style),
> 	(ctree_toggle_justify), (ctree_toggle_sel_mode),
> 	(progressbar_toggle_orientation), (progressbar_toggle_bar_style):
> 	use gtk_option_menu_get_history() instead of RADIOMENUTOGGLED,
> 
> 	(notebook_type_changed): merged standard_notebook(),
> 	notabs_notebook(), scrollable_notebook() and borderless_notebook()
> 	into notebook_type_changed()
> 
> 	(create_list), (create_ctree), (create_notebook),
> 	(create_progress_bar): changes to the OptionMenuItem arrays
> 	to reflect change in OptionMenuItem structure.
> 
> 	All these changes fixes bug #59885
> 
> 
> Ok to commit?
> 
> regards,

Looks fine, with various stylistic nits noted below.

Regards,
                                             Owen

> Index: testgtk.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/tests/testgtk.c,v
> retrieving revision 1.275
> diff -u -r1.275 testgtk.c
> --- testgtk.c	2001/09/10 18:54:20	1.275
> +++ testgtk.c	2001/09/19 18:04:27
> @@ -55,7 +55,6 @@
>  typedef struct _OptionMenuItem
>  {
>    gchar        *name;
> -  GtkSignalFunc func;
>  } OptionMenuItem;

I'd just get rid of this structure.

> @@ -4476,10 +4467,9 @@
>    if (!GTK_WIDGET_MAPPED (widget))
>      return;
> 
> -  RADIOMENUTOGGLED ((GtkRadioMenuItem *)
> -		    (((GtkOptionMenu *)list_omenu)->menu_item), i);
> +  i = gtk_option_menu_get_history (GTK_OPTION_MENU (widget));
> 
> -  gtk_list_set_selection_mode (list, (GtkSelectionMode) (3-i));
> +  gtk_list_set_selection_mode (list, (GtkSelectionMode) (i));
>  }

Actaully, SelectionMode is only 0-2 now - so the "extended" option
needs to be removed from the menus where it is used.
 
>  static void
> @@ -4489,10 +4479,10 @@
> 
>    static OptionMenuItem items[] =
>    {
> -    { "Single",   GTK_SIGNAL_FUNC (list_toggle_sel_mode) },
> -    { "Browse",   GTK_SIGNAL_FUNC (list_toggle_sel_mode) },
> -    { "Multiple", GTK_SIGNAL_FUNC (list_toggle_sel_mode) },
> -    { "Extended", GTK_SIGNAL_FUNC (list_toggle_sel_mode) }
> +    { "Single"    },
> +    { "Browse"    },
> +    { "Multiple"  },
> +    { "Extended"  }


> @@ -5096,7 +5087,9 @@
>        label = gtk_label_new ("Selection Mode :");
>        gtk_box_pack_start (GTK_BOX (hbox), label, FALSE, TRUE, 0);
> 
> -      clist_omenu = build_option_menu (items, 4, 3, clist);
> +      clist_omenu = build_option_menu (items, 4, 3,
> +				       (GtkSignalFunc)clist_toggle_sel_mode,
> +				       clist);

We tend to prefer GTK_SIGNAL_FUNC() for such casts. But actually, I
might change build_option_menu() to take the correct prototype, 
since we don't have the issue that gtk_signal_connect() has with
multiple possible prototypes.

> @@ -5582,11 +5573,10 @@
>    if (!GTK_WIDGET_MAPPED (widget))
>      return;
> 
> -  RADIOMENUTOGGLED ((GtkRadioMenuItem *)
> -		    (((GtkOptionMenu *)omenu3)->menu_item), i);
> +  i = gtk_option_menu_get_history (GTK_OPTION_MENU (widget));
> 
>    gtk_clist_set_column_justification (GTK_CLIST (ctree), ctree->tree_column,
> -				      (GtkJustification) (1 - i));
> +				      (GtkJustification) (i));
                                                         ^ ^

These parens are extraneous (Lots of other places in the patch have
the same minor problem)

> @@ -5985,21 +5974,27 @@
>        hbox = gtk_hbox_new (TRUE, 5);
>        gtk_box_pack_start (GTK_BOX (mbox), hbox, FALSE, FALSE, 0);
> 
> -      omenu1 = build_option_menu (items1, 4, 2, ctree);
> +      omenu1 = build_option_menu (items1, 4, 2,
> +				  (GtkSignalFunc)ctree_toggle_line_style,
> +				  ctree);
>        gtk_box_pack_start (GTK_BOX (hbox), omenu1, FALSE, TRUE, 0);
>        gtk_tooltips_set_tip (tooltips, omenu1, "The tree's line style.", NULL);
> 
> -      omenu2 = build_option_menu (items2, 4, 1, ctree);
> +      omenu2 = build_option_menu (items2, 4, 1,
> +				  (GtkSignalFunc)ctree_toggle_expander_style,
> +				  ctree);
>        gtk_box_pack_start (GTK_BOX (hbox), omenu2, FALSE, TRUE, 0);
>        gtk_tooltips_set_tip (tooltips, omenu2, "The tree's expander style.",
>  			    NULL);
> 
> -      omenu3 = build_option_menu (items3, 2, 0, ctree);
> +      omenu3 = build_option_menu (items3, 2, 0,
> +				  (GtkSignalFunc)ctree_toggle_justify, ctree);
>        gtk_box_pack_start (GTK_BOX (hbox), omenu3, FALSE, TRUE, 0);
>        gtk_tooltips_set_tip (tooltips, omenu3, "The tree's justification.",
>  			    NULL);
> 
> -      omenu4 = build_option_menu (items4, 4, 3, ctree);
> +      omenu4 = build_option_menu (items4, 4, 3,
> +				  (GtkSignalFunc)ctree_toggle_sel_mode, ctree);
>        gtk_box_pack_start (GTK_BOX (hbox), omenu4, FALSE, TRUE, 0);
>        gtk_tooltips_set_tip (tooltips, omenu4, "The list's selection mode.",
>  			    NULL);
> @@ -7248,57 +7243,51 @@
>  }
> 
>  static void
> -standard_notebook (GtkButton   *button,
> -                   GtkNotebook *notebook)
> +notebook_type_changed (GtkWidget *optionmenu, GtkNotebook *notebook)

Function definition arguments should be vertically aligned.
> -static void
> -notabs_notebook (GtkButton   *button,
> -                 GtkNotebook *notebook)
> -{
> -  gint i;
> +  switch (c)
> +    {
> +    case 0:

I'd probably introduce a local enumeration instead of using the
index directly.

> @@ -9071,7 +9058,9 @@
>  			5, 5);
>        gtk_misc_set_alignment (GTK_MISC (label), 0, 0.5);
> 
> -      pdata->omenu1 = build_option_menu (items1, 4, 0, pdata);
> +      pdata->omenu1 = build_option_menu
> +	(items1, 4, 0, (GtkSignalFunc)progressbar_toggle_orientation,
> +	 pdata);

You should avoid breaking lines between the function name and the 
opening brace.