Re: Bug in get_preferred_height_for_width() [was Re: Minimum height for minimum width]

[Tristan Van Berkom <tristanvb openismus com>]

On Thu, 2010-10-21 at 13:57 +0900, Tristan Van Berkom wrote:
> On Mon, 2010-10-18 at 01:28 -0400, Havoc Pennington wrote:
> > Hi,
> > 
> > On Sun, Oct 17, 2010 at 11:58 PM, Tristan Van Berkom
> > <tristanvb openismus com> wrote:
> > >
> > > What happens when another subclass wants to use
> > > ->adjust_size_allocation() to realign itself further ? how
> > > can it cooperate with GtkWidgetClass and not cause bad side
> > > effects ?
> > 
> > In the patch I posted (assuming the FIXME is fixed), what would still be broken?
> > I'm sort of lost what problems are unsolved. Granted, I might find out
> > if I tested the patch ;-)
> > 
> 
> This part will be re-broken, in this patch you are not adjusting the
> allocated width for margins and alignments *before* getting the height
> for the real allocated width:
> 
> ---------------------------------------------------
>   adjusted_allocation = real_allocation;
>   if (gtk_widget_get_request_mode (widget) ==
>       GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH)
>     {
>       int min_width;
> 
>       gtk_widget_get_preferred_width (widget, &min_width,
>                                       &natural_width);
>       /* "natural_width" is a white lie; it's reduced to the
>        * actual allocation but kept above min.
>        */
>       if (natural_width > real_allocation.width)
>         natural_width = MAX(real_allocation.width, min_width);
>       gtk_widget_get_preferred_height_for_width (widget, natural_width,
>                                                  NULL, &natural_height);
>     }
>   else ...
> ---------------------------------------------------
> 
> And then it goes on too adjust the allocations after checking
> the height for width:
> ---------------------------------------------------
> 
>   GTK_WIDGET_GET_CLASS (widget)->adjust_size_allocation (widget,
>                                          GTK_ORIENTATION_HORIZONTAL,
>                                          &natural_width,
>                                          &adjusted_allocation.x,
>                                          &adjusted_allocation.width);
>   GTK_WIDGET_GET_CLASS (widget)->adjust_size_allocation (widget,
>                                          GTK_ORIENTATION_VERTICAL,
>                                          &natural_height,
>                                          &adjusted_allocation.y,
>                                          &adjusted_allocation.height);
> ---------------------------------------------------
> 
> Let's see what we can do with this api... it would have to be
> something more like:
> 

Actually even that is wrong, since we're passing a size that
has it's margins stripped to get_height_for_width(), which
will do the operation again on the for_size.

So it needs to add another line:

> ---------------------------------------------------
> 
> if (widget_is_height_for_width) {
> 
>   gtk_widget_get_preferred_width (widget, &min_width,
>                                   &natural_width);
>   GTK_WIDGET_GET_CLASS (widget)->adjust_size_allocation (widget,
>                                          GTK_ORIENTATION_HORIZONTAL,
>                                          &natural_width,
>                                          &adjusted_allocation.x,
>                                          &adjusted_allocation.width);

After adjusting adjusting the allocated width with the aid of the real
natural width... we get a new adjusted_allocation.width, before passing
that width to get_preferred_height_for_width we need to:

gint adjusted_allocated_width = adjusted_allocation.width;

GTK_WIDGET_GET_CLASS (widget)->adjust_size_request(widget, 
                                           GTK_ORIENTATION_HORIZONTAL,
					   /* for_size is never needed
                                              for this api afaics */
                                           &adjusted_allocated_width,
                                           NULL);

And then we need to call get_height_for_width () to obtain the natural
height for the allocated width... /after/ adding some margins to the
inner allocated width, since those margins will be removed from the
for_size deep inside get_height_for_width() and then others re-added 
to the output natural height.

Thanks for trying a patch out with this new API (it helps alot), 
I'll try updating the patch to do what I think it needs to do and 
send that one back.

I'll remove the for_size argument from ->adjust_size_request() as
well since it's not used anywhere and I cant imagine what it can
be used for.

Cheers,
      -Tristan

> 
>   gtk_widget_get_preferred_height_for_width (widget, 
>                                              adjusted_allocation.width,
>                                              NULL, &natural_height);
>   GTK_WIDGET_GET_CLASS (widget)->adjust_size_allocation (widget,
>                                          GTK_ORIENTATION_VERTICAL,
>                                          &natural_height,
>                                          &adjusted_allocation.y,
>                                          &adjusted_allocation.height);
> }



> 
> ---------------------------------------------------
> 
> >From first glance at the portion that touches 
> gtksizerequest.c, the answer to the FIXME comment is
> pretty much: yes it's going to be in the cache, we need
> to call gtk_widget_get_preferred_width() there directly
> to get a "probably cached" natural_width that has already been
> treated by ->adjust_size_request().
> 
> 
> > > ... and more importantly, why is this useful
> > > to anybody except GtkWidget and GtkContainer ?
> > 
> > It is useful in any abstract base class that wants to provide "stuff
> > around" whatever its subclasses draw.
> > 
> > I think GtkContainer is actually a good enough reason to have this.
> > border-width is deprecated sure, but it's not going away soon, it'd be
> > nice to clean up all the code that has to deal with it.
> > 
> > Another example in GTK is GtkMisc, though we want to deprecate that
> > too, you could use this vfunc to delete the align and pad handling
> > from its subclasses and delete some code, which would be nice.
> > 
> > Hypothetically you could do things like:
> >  * a base class that aligned in a more precise way than
> > left/right/center (like GtkAlignment)
> >  * a base class providing more complex CSS-like border/margin/pad
> > capability with colors for each
> >  * a base class that provided a frame
> >  * a base class that adds any kind of display or status next to subclass content
> > 
> > All of these could also be implemented as a container, granted. (That
> > is, GtkMisc and GtkAlignment solve the same problem.)
> > However, I think there can be good reasons to do this stuff in a base
> > class so your widgets can have the stuff "built in"
> 
> Hmm GtkMisc is another case that has to be handled I had overlooked
> that, possibly the GtkMisc classes have to implement the vfunc without
> ever chaining up to GtkWidgetClass (possibly doing something forceful
> by manually observing the GtkWidgetClass's "margins" to take them into
> account and then doing it's own custom alignment logic, thus ignoring
> the v/halign properties).
> 
> 
> > 
> > Havoc
> 
> 
> _______________________________________________
> gtk-devel-list mailing list
> gtk-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gtk-devel-list