Re: update label patch, and gtktoolbar patch

[Owen Taylor <otaylor redhat com>]

jacob berkman <jacob ximian com> writes:

> On Fri, 2001-11-09 at 17:54, Owen Taylor wrote:
> > 
> > jacob berkman <jacob ximian com> writes:
> > 
> > > this patch makes GtkLabel actually default to what the properties say it
> > > should default to.
> > > 
> > > i've noticed things like this in the past - it begs for GObject to set
> > > the defaults itself somehow.
> > 
> > You need to fix what the property reports, not change the defaults.
> 
> ok, here's a fixed gtklabel patch, and is this gtktoolbar patch ok?

The label patch looks fine, _but_ Matt Wilson pointed out to
me today that although the default in GTK+-1.2 was CENTER for
justification, it turned out that CENTER only half worked -
it worked for explicit newlines in non-autowrapped labels, but not 
for auto-wrapped labels.

(I think this is an a artifact of an earlier state where autowrapping
implied fill justification.)

So, a lot of stuff gets justified strangly when you switch to
GTK+-2.0; CENTER justification is pretty rarely useful. So,
I think this means that we should switch the default 
to LEFT, which (IMO) is the correct default. Other opinions.

The tooltips patch looks basically OK (though I don't know why you'd
want to disable tooltips, especially programmatically...), with two
comments:

> Index: gtktoolbar.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtktoolbar.c,v
> retrieving revision 1.64
> diff -u -r1.64 gtktoolbar.c
> --- gtktoolbar.c	2001/10/15 13:59:34	1.64
> +++ gtktoolbar.c	2001/11/13 01:09:55
> @@ -53,6 +53,7 @@
>    PROP_0,
>    PROP_ORIENTATION,
>    PROP_TOOLBAR_STYLE,
> +  PROP_TOOLTIPS

I think "show_tooltips" would be a better significantly better
name since "tooltips" sounds like it sets what the tooltips 
are. (Worth violating the principle "same name as C accessor")

> @@ -1482,10 +1496,15 @@
>  {
>    g_return_if_fail (GTK_IS_TOOLBAR (toolbar));
>  
> +  if (enable == toolbar->tooltips->enabled)
> +    return;

You need to add 

 enable = enable != FALSE

here if you want this to be reliable ... gboolean parameters are
only required to be true, not TRUE.

>    if (enable)
>      gtk_tooltips_enable (toolbar->tooltips);
>    else
>      gtk_tooltips_disable (toolbar->tooltips);
> +
> +  g_object_notify (G_OBJECT (toolbar), "tooltips");
>  }

Also, noticed a bug in the existing code while looking at your
patch. gtk_tooltips_destroy() frees toolbar->tooltips, but
other parts of the widget assume that toolbar->tooltips 
exists unconditioanlly. There are basically two possibilities
here:

 1) Lazily create toolbar->tooltips with a 
    static GtkTooltips *gtk_widget_get_tooltips() so that
    if they are destroyed, we recreate them.

 2) Destroy the tooltips in finalize(), not destroy().

The choise here basically depends on whether toolbar->tooltips is
considered private or not. If it is publically accessible, then we
need to destroy it in destroy(), otherwise we can wait for finalize().

I think on balance I prefer 1), though either would be basically
OK.

Regards,
                                        Owen