Re: changing usize in ::size-request (bug #155139)

[Tim Janik <timj gtk org>]

On Tue, 12 Oct 2004, Owen Taylor wrote:

> On Tue, 2004-10-12 at 18:04 +0200, Tim Janik wrote:
>
> > that is, in the label's ::size-request handler tab_label_size_request_cb()
> > (at which point the label itself has already been size requested), the
> > testcase unconditionally sets the label's usize. that in turn queues a
> > new resize on the label.
> > however, due to the handling of the ::size-request signal inside
> > gtk, this request is later forgotten about (gtksizegroup.c):
> >
> > >       g_signal_emit_by_name (widget,
> > >                              "size_request",
> > >                              &widget->requisition);
> > >
> > >       GTK_PRIVATE_UNSET_FLAG (widget, GTK_REQUEST_NEEDED);
>
> How does this actually cause drawing to break? Is the problem that the
> requisition of the label is changing without a new call to
> gtk_label_size_request()? I don't see how that would matter, offhand.

i don't either, and i didn't bother debugging. basically, if queue_resize
got called after the ::size-request default handler and we then unconditionally
GTK_PRIVATE_UNSET_FLAG (widget, GTK_REQUEST_NEEDED); we enter an unsupported
state (since invariants got broken) after which anything can follow, so i
rather not continue debugging at this point.

> > basically, gtk doesn't support queueing new size requests from
> > ::size-request, and that's been that way for a looong time.
> >
> > [owen: FYI, applying my if (GTK_WIDGET_REQUEST_NEEDED (widget))
> >   gtk_widget_size_request (widget); fix from last week did indeed trigger an
> >   endless resizing loop with the test case and without the set_usize() guards.]
> >
> > the bug at hand can fairly easily be circumvented in gtk by not queueing a
> > resize from set_usize() if the new usize doesn't actually change anything.
> > since this is a reasonable efficiency improvement to make anyway,
>
> Yeah, looks good. (Though not understanding the problem, I don't
> understand how it fixes it.)

you're right. it actually *fixes* the endless loop mentioned above FYI,
and just "happens" to also cause the labels to be drawn with your
conditional GTK_REQUEST_NEEDED restting in CVS.
note that, with
  if (GTK_WIDGET_REQUEST_NEEDED (widget))
    gtk_widget_size_request (widget);
in gtk_widget_size_allocate(), the testcase wouldn't enter a situation
where the invariant "queue_resize() is followed by a ::size-request emission"
gets broken (because the required resize is re-queued after the flag reset
and thus the invariant breakage is "fixed"). re-queueing the resize does
risk endless resizing loops though, e.g. if the call to set_usize() would
change width in a ping-pong manner.
however with the current conditional GTK_PRIVATE_UNSET_FLAG (widget,
GTK_REQUEST_NEEDED); the mentioned invariant does get broken, so anything
may follow after that.

i'm not (yet) arguing for reverting the size_allocate() behaviour back
to re-queueing a resize again, but it should be added to my below
list of possible ways to support queue_resize from ::size-request.

> >  i've
> > comitted the appropriate guards to gtk_widget_set_usize_internal(). it
> > doesn't fix the more fundamental problem of people wanting to adjust widget
> > sizes around size requisition time though.
>
> What's the use case? Generally, I'd expect the example to use:
>
>    gtk_widget_set_size_request (label, label_width, -1);
>
> With
>
>    *requisition.width = label_width;
>
> This won't actually work because it will break GtkLabel's alignment
> code, but you could introduce an intermediate container and get it to
> work.

i don't understand you here. the use case is given by the bug report, the
label size depends on the toplevel allocation. i also gave another example
for a size-requisition patch up where i simply grew a requested
GtkTextView size.

> > ideally, the ::size-request signal would be RUN_LAST. it's not,
> > due to historic reasons though, and i'm afraid making it RUN_LAST
> > will significantly affect signal connections in exiting code:
>
> Yeah, I don't think we could do that compatibly.
>
> > so in order to deal with widget size adjustments around ::size-request time,
> > i see these possibilities:
> > 1) make ::size-request a RUN_LAST signal as it ought to be. this should only
> >     be done, when people are willing to put some work into porting their
> >     applications to a new gtk+ version anyway, e.g. around gtk+-3.0.
> > 2) introduce a ::custom-size or similar signal that is emitted directly
> >     before ::size-request, and can be used by users to setup resizing handler
> >     connections like the above (tab_label_size_request_cb, example_size_request_handler).
> > 3) preserve the current state of affairs, where size adjustments that queue
> >     a new resize during ::size-request are simply not supported.
> > 4) make ::size-request a NO_RECURSE signal, and introduce a new private flag for
> >     widgets (or similar mechanism like a private in-request widget list), which
> >     can be used to detect queueing resizes during emission of ::size-request.
> >     in that case, gtk_widget_queue_resize() could simply emit_by_name (widget,
> >     "size-request"); which will cause a the current emission of ::size-request
> >     to be restarted after the user handler queueing a resize returned.
> >     that is, make ::size-request deal with recursive resize requests the way we
> >     handle GtkAdjustment::value-changed.
> >     this may still be incompatible with some existing user code setups, but
> >     hopefully less so than simply making ::size-request RUN_LAST and as a
> >     consequence, the caveats from (1) about applying this around a 3.0 change
> >     only probably apply here as well.

let me add:
5) change gtk_widget_size_allocate() to re-queue a resize if GTK_REQUEST_NEEDED
   remains set upon function exit. this keeps the invariant of ::size-request
   having to follow every queue_resize (for DRAWABLE widgets).
   this may trigger endless resizing loops, but only if users unconditionally
   cause queue_resize from ::size-request signal handlers, in which case it
   can be argued they get what they deserve (requested). note that the testcase
   from #155139 is such a scenrio though. it only happens to work with
   recent CVS because i made set_usize() queue a resize conditionally.

> > for the first three cases, it might actually make sense to add some magic to the
> > signal code, to issue a warning once per runtime, for !AFTER signal connections
> > to ::size-request.
>
> I'd like to get a clearer idea of what we are trying to support before
> commenting on the above. To some extent it would be nice to simply
> support calls to queue_resize() at any point. But really changing the
> size of a widget in response to size_request() is pretty odd.

yes, i know. and this has been our line at least since 1.2, which is why i listed
option (3) above. i'd also like to support queue_resize from ::size-request
out of the box though, so bug reports like #155139 don't come up again, and
for that i currently favour (4) or (5).

however, to some extend, this carries the risk, that people are doing unsupported/
buggy things, never notice and at some point run into other subtle bugs.

i think it can be argued for #155139 to be such a case:
- the allocation of a window depends on its requisition
- the requisition of a window depends on the size of all the widgets it contains
- the requisition of the label from #155139 depends on the allocation of the
  toplevel window (tab_label_set_size() as called from tab_label_size_request_cb())
this introduces a circular dependancy that can't be resolved in all cases (namely,
if the label requisition changed in response to the window requisition, which changed
in response to the label requisition, etc...)

mundane cases can easily be constructed where such circular scenarios either:
- loop endlessly due to ping-pong size requests (the lable<->window dependancy might
  request alternating sizes like: 4,5,4,5,4,5,4,5), or
- get into a feedback loop (with size requests: 8,...,256,...,65536,...)
and due to float-round-off, font-sizes, theme settings, etc. these pathologic cases
might not even always be triggered.

if we want to completely prevent such scenarios, i might even add:
(6) explicitely catch queue_resize() during ::size-request emissions by a mechanism
    similar to that suggested in (4), and issue a warning.

i think we should stop to look for more use cases for queue_resize() from ::size-request
though. experience shows that code is usually used in unintended or not planned for
ways, so we should rather make a general decision on how to deal with recursive resize
requests, than to focus on getting arbitrary cases to work and ignore others.

> Regards,
> 							Owen

---
ciaoTJ