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 14:51 +0900, Tristan Van Berkom wrote:
> On Thu, 2010-10-21 at 14:10 +0900, Tristan Van Berkom wrote:
> > 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.
> > 
> 
> I've attached my patch here, with your API approach seems everything
> falls into place.
> 
> Besides the changes I outlined above I also had to change GtkContainer
> adjust_size_allocation() implementation to do it's own magic *first*
> and then chain up to GtkWidgetClass after.
> 
> Otherwise with extra-large allocations, a container border width
> would be *further removed* aligned allocations.
> 
> This should be the one.
> 

I updated the test case and found there was another bug, fixed
the other bug... you can test the new test-case with and without
the patch ... its all in bugzilla now:

   https://bugzilla.gnome.org/show_bug.cgi?id=632766

Cheers,
         -Tristan


> Cheers,
>          -Tristan
> 
> > 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
> > 
> > 
> > _______________________________________________
> > gtk-devel-list mailing list
> > gtk-devel-list gnome org
> > http://mail.gnome.org/mailman/listinfo/gtk-devel-list
> 
> _______________________________________________
> gtk-devel-list mailing list
> gtk-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/gtk-devel-list