Re: GtkButton patch for bug #58603

[Owen Taylor <otaylor redhat com>]

Alex Larsson <alexl redhat com> writes:

> Ok, this patch implements the GtkButton behaviour described in bug #58603, 
> and the posting by owen it references.
> 
> Ok?

> +/**
> + * gtk_button_get_label:
> + * @button: a #GtkButton
> + *
> + * Fetches the text from the label of the button, as set by
> + * gtk_button_set_label().
> + *
> + * Return value: the text of the label widget. This string is
> + *   owned by the widget and must not be modified or freed.
> + *   If the label text has not been set the return value
> + *   can be NULL. This can happen i.e. if you put a custom
        ^^^^                   
        will

> + *   child widget into the button.

This will be the case if you create an empty button with gtk_button_new()
to use as a container.

> +/**
> + * gtk_button_set_use_underline:
> + * @button: a #GtkButton
> + * @value: %TRUE if underlines in the text indicate mnemonics
> + *
> + * If true, an underline in the text of the button label indicates
> + * the next character should be used for the mnemonic accelerator key.
> + */
> +void
> +gtk_button_set_use_underline (GtkButton *button,
> +			      gboolean   value)
> +{
> +  g_return_if_fail (GTK_IS_BUTTON (button));
 +  
> +  button->use_underline = value;

This breaks if value is, say, 2.

 You should do:

  use_underline = use_underline != FALSE;
  if (use_underline != button->use_underline)
    { 
      [...]
    }

(Same for set_use_stock)

> +  
> +  gtk_button_construct_child (button);
> +  
> +  g_object_notify (G_OBJECT (button), "use_underline");
> +}


> +/**
> + * gtk_button_set_use_stock:
> + * @button: a #GtkButton
> + * @value: %TRUE if the button should use a stock item
> + *
> + * If true, the label set on the button is used as a
> + * stock id to select the stock item for the button.
> + */
> +void
> +gtk_button_set_use_stock (GtkButton *button,
> +			  gboolean   value)
> +{
> +  g_return_if_fail (GTK_IS_BUTTON (button));
> +  
> +  button->use_stock = value;
> +  
> +  gtk_button_construct_child (button);
> +  g_object_notify (G_OBJECT (button), "use_underline");
                                          ^^^^^^^^^^^^^

Oops. :-)

It is unfortunate that

 g_object_new (GTK_TYPE_LABEL, 
               "label", "open",
               "use_stock", TRUE,
               NULL);

creates and destroying an extra label. You probably can work around
this by adding

 guint constructed : 1;

To the GtkButton structure, make gtk_button_contruc_child() do nothing
if it is FALSE, then add a 'GObject::constructor' function that calls
gtk_button_construct_child() if necessary and sets this flag, and
mark label/use_stick/use_underline as CONSTRUCT (not CONSTRUCT_ONLY)
properties. 

If you do this, then you might as well make new_with_label() be:

 g_object_new (GTK_TYPE_BUTTON, "label", label, NULL);

Since GObject, as a dubious feature, sets all construct properties
to their default values, so you save nothing in efficiency by
bypassing the property interface. But this would be a code simplification
in any case.

Another thing that needs attention is the child classes - get_label()
needs to work if you create a checkbutton with gtk_check_button_new_with_label().

This is mostly really simple ... after all, the whole point of this
is to allow subclasses of GtkButton to use the GtkBUtton code for labels/stock
ids, etc.

The only troublesome part is that GtkButton and GtkToggleButton do:

  gtk_misc_set_alignment (GTK_MISC (label_widget), 0.5, 0.5);

While GtkCheckButton and GtkRadioButton do:

   gtk_misc_set_alignment (GTK_MISC (accel_label), 0.0, 0.5);

(Don't ask me why GtkCheckBUtton is using an GtkAccelLabel
here ... almost certainly a bug... but the crucial part is the
alignment parameters)

Note this is already a bug ... 

 g_object_new (GTK_TYPE_CHECKBUTTON, "label", "Hello", NULL);

gets the alignment wrong, and the alignment is wrong for check/radio buttons
with draw_indicator = FALSE.

I think the simplest fix would be, instead of setting a different alignment
on the child, to make check/radio buttons with draw_indicator=FALSE only give 
their children the space that the child requests. That will probably cause
minor problems for someone doing something really odd and unusual with
a checkbutton, but should make life better for the rest of us.

Can anybody think of a better way of handling this?

                                        Owen