Re: update label patch, and gtktoolbar patch
- From: Owen Taylor <otaylor redhat com>
- To: jacob berkman <jacob ximian com>
- Cc: gtk-devel-list gnome org
- Subject: Re: update label patch, and gtktoolbar patch
- Date: 13 Nov 2001 20:44:20 -0500
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
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]