Created attachment 169517 [details] [review]
default impls of width_for_height,hfw should chain directly not use wrapper API

In GtkBin and GtkWidget we tried to provide handy defaults that
call get_width if there's no get_width_for_height and
get_height for get_height_for_width.

However, they used the wrapper API on GtkSizeRequest instead of
chaining directly to the other method implementation.

This could result in all kinds of surprising behavior, for example,
get_width_for_height() would now already include the effects of set_size_request().

If nothing else it's inefficient. But it's just conceptually wrong,
because to chain to another implementation, we should call the other
implementation, not call a wrapper around the other implementation
(when we're already inside a previous invocation of the wrapper,
i.e. compute_size_for_orientation() ends up reinvoking itself
in the same orientation on the same object which it pretty
likely isn't intending to do)

Created attachment 169518 [details] [review]
Warn about recursively calling size req wrappers on the same object and orientation

We are not re-entrant and there is no reason for widgets to
do this, most likely they'll just get unexpected bugs because
the wrappers may modify the request.

Computing the request should logically rely only on the
widget itself, not on any adjustments caused by set_size_request,
size groups, and so forth.

Makes sense to me. With these patches, I get a lot of warnings when running testheightforwidth or testwrapbox:

(lt-testwrapbox:30586): Gtk-WARNING **: GtkComboBox 0x8c71b0: widget tried to gtk_size_request_get_height_for_width inside GtkSizeRequest::get_height implementation. Should just invoke GTK_SIZE_REQUEST_GET_IFACE(widget)->get_height_for_width directly rather than using gtk_size_request_get_height_for_width

(lt-testwrapbox:30586): Gtk-WARNING **: GtkWrapBox 0x856970: widget tried to gtk_size_request_get_height_for_width inside GtkSizeRequest::get_height implementation. Should just invoke GTK_SIZE_REQUEST_GET_IFACE(widget)->get_height_for_width directly rather than using gtk_size_request_get_height_for_width

(lt-testheightforwidth:30572): Gtk-WARNING **: GtkFrame 0x181c3c0: widget tried to gtk_size_request_get_width inside GtkSizeRequest::get_width_for_height implementation. Should just invoke GTK_SIZE_REQUEST_GET_IFACE(widget)->get_width directly rather than using gtk_size_request_get_width

(lt-testheightforwidth:30632): Gtk-WARNING **: GtkMenu 0x9d1030: widget tried to gtk_size_request_get_height_for_width inside GtkSizeRequest::get_height implementation. Should just invoke GTK_SIZE_REQUEST_GET_IFACE(widget)->get_height_for_width directly rather than using gtk_size_request_get_height_for_width


So I guess we need a third patch that cleans up the various hfw implementations for this.

Created attachment 169541 [details] [review]
Fix more SizeRequest implementations to avoid recursive calls to wrapper API

GtkFrame, GtkComboBox, GtkExpander, GtkMenu, GtkWrapBox

These are all the examples I could find so far.

I did some grepping and opened all the screens in testgtk and gtk-demo, this is what I found.

There are a fair number of warnings about other stuff in testgtk and gtk-demo but hopefully none of it is related to my patches. (didn't look related, but can be hard to know.) Anyway this particular warning about recursive size request is no longer triggered that I could find.

Comment on attachment 169518 [details] [review]
Warn about recursively calling size req wrappers on the same object and orientation

Kind of wondering why I made this keep h and v request separate. no part of the GtkSizeRequest implementation should call the wrappers (on the same "this" widget), period, I think. Anyone disagree?

Also the warning message could use a line wrap.

Review of attachment 169541 [details] [review]:

Works for me, no more recursion warnings. I'll let Tristan comment, but as far as I'm concerned, the 3 patches are fine.

(In reply to comment #0)
> It isn't semantically correct for the get_width_for_height vfunc 
> to have the semantics of the wrapper, instead of the vfunc.
> The patch includes height_for_width fix also of course.

Hmmm, it was my intention to use the wrapper API as much as possible 
from the implementations.

The rationale is that a "height-for-width" widget most of the time
will simply compute get_width() for any get_width_for_height() requests
(which may still be multiple requests per allocation cycle):

in this case it makes sense to call the get_width() wrapper api and 
hopefully return a cached value instead of actually invoking the iface 
method.

In other cases where the "height-for-width" widget returns something
for the initial get_height() request, it will sometimes call get_width()
on itself and then get_height_for_width() on itself... to ensure enough
height when being requested as "width-for-height" the widget needs to
return the height for its minimum width.

Without thinking of the latter case too deeply I still have a feeling
we are saving some important cycles by calling the cached wrapper API
instead of unconditionally executing the request code.

The problem is the cache doesn't have the right thing in it, so it may be faster, but... The wrappers add at least:

* the 1x1 constraint
* set_size_request
* size groups
* with my patch, arbitrary adjust_size_request

It was sort of OK with size request and size groups because those set constraints rather than making a relative adjust, so applying them twice isn't a catastrophe. 
However I still wonder if stuff was subtly messed up. It's just really hard to think about correctness in this framework where the vfuncs may not return a property of the widget but may (or may not) already have the wrapper logic added.

With my widget padding patch, of course, using the wrappers inside the vfuncs cannot work... the widget padding is added twice.

If we want to cache the chaining, I think a better way would be to do it up in the wrappers somehow. For example, you could have a flag that all for_width including -1 are equivalent. (Basically a "no height for width" flag.) That might work, right? Conceptually similar to an optimization like gtk_widget_set_redraw_on_allocate().

Or, alternatively, the cache could be strictly of the vfunc return values, *without* any post-processing; and there could be another set of wrappers, 
get_raw_width_cached() or something, that would hit that cache. These wrappers would be for use only where currently the code invokes a vfunc directly.
You would then have to always do the post-process sizegroup/set_size_request/adjust work (which may be fine, it's not that expensive) or you could have two caches one with and one without postprocess (seems like the cache overhead could be higher than redoing the postprocess).

Created attachment 169566 [details] [review]
Warn about recursively calling size req wrappers on the same object and orientation

Updated patch making the warning span both h and v orientation, and fixing the really long line

(In reply to comment #9)
> The problem is the cache doesn't have the right thing in it, so it may be
> faster, but... The wrappers add at least:
> 
> * the 1x1 constraint
> * set_size_request
> * size groups
> * with my patch, arbitrary adjust_size_request
> 
> It was sort of OK with size request and size groups because those set
> constraints rather than making a relative adjust, so applying them twice isn't
> a catastrophe. 
> However I still wonder if stuff was subtly messed up. It's just really hard to
> think about correctness in this framework where the vfuncs may not return a
> property of the widget but may (or may not) already have the wrapper logic
> added.

Yes it was OK because values from set_size_request(), sizegroups and
fallbacks to the old "size-request" signal only result in clamping
of the returned values.

I have not looked into your added padding patches at all but if you
plan on making a relative adjustment automatically deep in the guts
of a widget's own request code... things will surely start to break
at that point (i.e. I suppose you mean to make size requests and allocations 
both padding inclusive...)

> 
> With my widget padding patch, of course, using the wrappers inside the vfuncs
> cannot work... the widget padding is added twice.
> 
> If we want to cache the chaining, I think a better way would be to do it up in
> the wrappers somehow. For example, you could have a flag that all for_width
> including -1 are equivalent. (Basically a "no height for width" flag.) That
> might work, right? Conceptually similar to an optimization like
> gtk_widget_set_redraw_on_allocate().
> 
> Or, alternatively, the cache could be strictly of the vfunc return values,
> *without* any post-processing; and there could be another set of wrappers, 
> get_raw_width_cached() or something, that would hit that cache. These wrappers
> would be for use only where currently the code invokes a vfunc directly.
> You would then have to always do the post-process
> sizegroup/set_size_request/adjust work (which may be fine, it's not that
> expensive) or you could have two caches one with and one without postprocess
> (seems like the cache overhead could be higher than redoing the postprocess).

Bah, I dont think it will slow things down too too much if we must make this
change... I just thought it better to avoid recalculating things whenever possible.

It's much more of a hit to add any complexity to the overall request
cycle (better to cache only the end result of the size request with padding and
sizegroups and set_size_request() all calculated).

As soon as you start to consult the aux_info and compare it to the cached
values before returning... things definitely start to get visibly slower.
(I suppose just getting the aux_info from the widget qdata for every
request instead of just pulling the whole thing from the cache just makes
things crawl...).

The patches have all landed now.