Re: GtkWidget::set_style proposal (was: [bug, v0.99.3] frame->label_width)

[Owen Taylor <owt1 cornell edu>]

Tim Janik <timj@gimp.org> writes:

> > > now, there would be an easy solution, that is to move the size computation
> > > of the frames label out of gtk_frame_set_label into gtk_frame_paint.
> > > but this will introduce a performance impact on every expose event (of which
> > > there are quite a lot coming from the x server during normal program
> > > operation), and it would only be a fix around the real problem, that is:
> > > widgets don't get informed when their style changes.
> > > 
> > > so i'm herewith proposing the need of yet another widget signal,
> > > run_type==GTK_RUN_FIRST as follows:
> > > 
> > > void	(*GtkWidget::set_style)	(GtkWidget *widget,
> > > 				 GtkStyle  *previous_style);
> > > 
> > > the new style will be already set if this signal is emitted.
> > > 
> > > objections?
> > 
> > From my TODO list:
> > 
> >  * Need to be able to make small alterations in the widgets style.
> >    1) gtk_style_copy
> >    2) A function to force the style to be set, _without_ realizing
> >       the widget. (And/or a way of telling widgets that the style has
> >       been changed so they can change their window's bg color)
> 
> hm, i'm not too sure how you would want to set the style on a widget
> without telling the widget->windows colormap and depth.
> would full notification be sufficient, so that we can stick to
> the scheme of attaching styles during widget realization?

No, it isn't sufficient. When I say "force the style to be set",
I don't mean "force it to be attached", what I mean is the code
from gtk_widget_realize:

      if (!GTK_WIDGET_USER_STYLE (widget))
	{
	  new_style = gtk_rc_get_style (widget);
	  if (new_style != widget->style)
	    gtk_widget_set_style_internal (widget, new_style);
	}

So somebody can copy the widgets style and know that they are
copying the right thing.
      
> > So, yes, it is a good idea. Further thoughts:
> > 
> > - I would call it "style_set" though, because it is notification,
> >   not a command. 
> 
> hm, what about the recently implemented GtkWidget::set_parent signal,
> that should be renamed GtkWIdget::parent_set to adhere to your
> idea of notification naming.

Yes it should. (I haven't paid much attention to the change, but you
originally proposed 'parent_set'; if you had proposed 'set_parent', I
would have suggested the other.) Note that the notebook's
"switch_page" signal, for instance, is OK, because the default handler
for this actually does switch the page.

> > - I don't see much point to having the previous_style argument -
> >   it may be harmless, but it also IMO useless.
> 
> i've similarly implemented previous_parent for the GtkWidget::set_parent
> signal. providing the old values might not be usefull if you are
> changing label sizes from e.g. (double)6 to (double)5 on a
> label_changed_size signal but as soon as structure references
> are concerned, especially such as
> GtkWidget or GtkStyle that do actual reference counting, the
> old values might come extremely in handy if you are keeping track
> of inter widget relationships from outside of the toolkit, like e.g.
> an interface builder or even a clever interpreter would do it.
> omitting previous_parent or previous_style would lead to
> gtk_object_set_data (widget,
>                      "xxx-private-key-old-parent-hint,
>                      widget->parent)
> in callbacks of GtkWIdget::set_parent just for the sake of
> later remebering the old parent, where the old value can just
> too easily by supplied by the signal emission code.

That sort of makes sense. But not really. If you _are_ keeping
a reference count, then you are keeping a pointer too, right?
If you aren't keeping a pointer, then there is no need to
keep a reference count.
 
Anyways, it is basically harmless. So if you want it, go ahead.

> > - Are you planning to emit it when the style is initially set
> >   or only later? I would tend to say that it it only needs to
> >   be emitted after the widget has been size_requested for the
> >   first time, though that isn't really flagged now.
> 
> kind of like the reasonings above, if i want to keep track of this
> from outside the toolkit, but even if i just want to put my
> label width calculation code only into gtk_*_style_set()
> the emission will be needed on any occasion where a style is
> attached to a widget. so i the emission is needed on the initial setting
> and on all subsequent changes (something that doesn't actually happen
> too often).
>
> > - Implementing it fully will require quite a bit of work - since
> >   almost every widget that isn't NO_WINDOW should in theory have
> >   a handler. 
> 
> hm, i think we can have an easy start by providing a default handler
> that will set the window background and perform a size request.

That will work for some widgets, not others. The background of
a widget may well _not_ be style->bg[...] - for quite a few widgets
it is style->base[...] It's only twenty minutes work in any case.
 
> > - It probably is a good idea to do the style initialization that
> >   is now done at the time of "realize" before the initial
> >   "size_request", because a lot of widgets will change their size
> >   depending on the style.
> 
> hm, you just gave a reason why GtkWidget::style_set should
> be emitted on the initial setting also. i seem to loose the point
> of your third argument ("- Are you planning to emit it when..").
> widgets need to be informed on subsequent changes anyway, now
> you gave the reasonings for the emission on the initial setting.
> seems like it should be emitted always as i state above, no? ;)

(That paragraph reminds me of reading Kant ;-)

Point three was a genuine question, not an argument; but what
I'm addressing here is something a bit different.

In point three, I was asking whether the "style_set" signal should
always be emitted when the style was set, or only when it
would require action from the widget. You said "always", and
that made sense.

In the above, I was addressing something a little
different. Currently, the style information is not correct until
the widget is realized. However, widgets currently can be
size_requested _before_ they are realized. What I was suggesting
was that the style be set according to the RC (but not attached)
before the widget was size requested.

If this is not done, then whenever a widget has a style set
in the rc file, the sequence will look like:

 - "size request"
 - "size allocate"
 - "realize"
 - "style_set"
 - "size request"
 - "size allocate'

which is not going to make for ideal performance. (If the realize
happens before the size_request, this isn't a problem, but I
don't think that is necessarily true now, and that causes a 
different inefficiency - if the size is known before the realize,
you save a separate gdk_window_move_resize() call.

> > - There _should_ be a command that says "set the style now", 
> >   other than gtk_widget_realize(). Maybe we should add another
> >   widget flag as well...
> 
> 
> >   So people can do:
> > 
> 
> i assume you mean, one can do
>       g_assert (!GTK_WIDGET_REALIZED (widget)); /* here... */
> >     if (!GTK_WIDGET_STYLE_SET (widget))
> >       gtk_widget_set_default_style (widget); 
> >     new_style = gtk_style_copy (widget->style);
> >     new_style->fg_color[GTK_STATE_NORMAL] = my_color;
> >     gtk_widget_set_style (new_style)

There should be no reason to put the g_assert() in. It is more
efficient if it is done before the realization, but will work in any
case. If the widget has been realized, then the widget will have
STYLE_SET, and with your new signal, calling gtk_widget_set_style
(new_style) will work at any point.

> now, assume people are not going to modify
> new_style->fg_color[GTK_STATE_NORMAL], but
> new_style->fg_gc[GTK_STATE_NORMAL], because they want
> dotted lines or what the heck.

What people typically seem to want to do is modify the colors, and the
fonts. (See the current "Newbie Q, how to change label color" thread,
various postings by Rob Roebling, etc.)

They probably should just create new styles in the rc file
instead, but... Modifying the GC so that the widgets decoration's is
drawn with dotted lines is a pretty dubious way to do things.  If
someone wants an additional GC to do their own drawing with, they
should create it themselves.

> now the depth of the widget window is required that you need to
> know *before* actual realization. this is pretty much an
> impossibility, e.g. consider quartics GtkPixmap needs an own
> XWindow problem.
>
> owen, i know it would be nice to set this stuff in advance,
> and we might even be able to achive this for the style colors,
> since they can be parsed later on.
> 
> but it would only be a half working solution since the gc portion
> of styles cannot be set in advance since the depth is unknown.

So? The GC isn't part of the "key" of a style. The idea is to
enable people to make run-time variants on the styles that are
defined in the RC file. (I.e., for some odd reason, the programmer
wants a blue button, without having to create a special blue entry
in there RC file)
 
> actually there is a reason that people can connect to GtkWidget:realize
> (it is emitted RUN_FIRST). some stuff just can't be done in advance.

I don't quite follow. I think you are saying that people
should just connect to the "realize" signal. But this is

 a) inefficient (things are being created one way then modified)
 b) a nuisance to program.

> >   Adding the flag means that we can solve the problem mentioned
> >   above - the "style_set" would only be emitted _after_ the
> >   first time the style is set. Since that would be guaranteed
> >   to be before the first size_request, everything would work
> >   out fine. Until we run out of flags...
> 
> after the recent split up there's prettymuch room for flags now ;)

We have 17 public flags now - so we're more than half full. That's
still plenty of space, but I don't want get too proflifate with
them. ;-)

> >   Note that people will still get into trouble if they do:
> > 
> >     if (!GTK_STYLE_SET (widget))
> >       gtk_widget_set_default_style (widget); 
> >     my_color = widget->style_color;
> > 
> >   Because the style isn't _attached_ yet. For that usage, you
> >   will still have to realize the widget.
 
> hm, are you also proposing to have individual styles for all widgets,
> or do you want to copy the style of one out of two widgets, that
> have the same style set prior to realization but going to realize
> with different depths?

I'm not sure what the problem is. The style code handles all
this properly now. If necessary, the style code will make the
copy.

I'm not sure either of us has been to clear in the above discussion.
If it would be helpful, I could make up a summary.

Regards,
                                        Owen