Re: [PATCH 1/4] Popup var inspector size requisition code rework

[Kalev Lember <kalev smartlink ee>]

Hey Dodji,

Thanks for the review. I have replied to some of the comments in-line.
If you agree with these, I'll update this and the following gtk3 patch
and resend both to the list.


>> 	* src/uicommon/nmv-popup-tip.cc (PopupTip::Priv::Priv): Set the
>> 	window as not resizable so its size would shrink back along with
>> 	its contents.
> 
> This works nicely for the vertical dimension.  I.e, it shrinks back
> /vertically/ along with its content.  But it doesn't shrink back
> /horizontally/, for me.  I wonder if there is something to do for this.
> Your patch is an improvement over what's in there already, so I agree
> with that particular change.  I am just wondering if there is something
> to do for the horizontal dimension as well.

Note that the resizable property is for the window (Gtk::Window), not
the container (Gtk::ScrolledWindow). When the window's resizable
property is false, it shrinks back along with its contents. The reason
why the window doesn't shrink back for the horizontal dimension is that
its contents (Gtk::ScrolledWindow) isn't shrinking back.

Having said that, it's of course entirely possible to also do it for the
horizontal dimension. However, I'd rather leave the horizontal shrinking
out of this patch for two reasons:
a) this patch doesn't (or at least shouldn't) change the current
   behaviour;
b) size request code is noticeably different in gtk3, and putting
   the horizontal size calculations here here would make the following
   gtk3 patch slightly less straight forward.


>> +            const Gtk::Widget *child = get_child ();
>> +            THROW_IF_FAIL (child);
>> +            Gtk::Requisition child_req = child->size_request ();
>> +
>> +            // If the height of the container is too big so that
>> +            // it overflows the max usable height, clip it.
>> +            if (child_req.height > max_height) {
>> +                req->height = max_height;
>> +            } else {
>> +                req->height = child_req.height;
>> +            }
> 
> Now that we are doing this on the scrolled window (the container)
> rather than on the popup variable inspector itself, it seems to me
> that it is the height of the scrolled window that we want to stay
> smaller than max_height, not necessarily the height of its child
> widget, or am I missing something ?
> 
> FWIW, I have tested the code snipped below and it seems to behave
> fine, unless I have missed a corner case:
> 
>             // If the height of the container is too big so that
>             // it overflows the max usable height, clip it.
>             if (req.height > max_height) {
>                 req->height = max_height;
>             } else {
>                 req->height = child_req.height;
>             }

No, that would let it grow past the max_height.

As an example, lets say that the container itself thinks it should be 20
pixels high, the child widget thinks it should be 600 pixels high, and
we've calculated that the max_heigh is 400. When we check if
(req.height > max_height), we are checking if (20 > 400), which is
false. But we should really be comparing the child widget's height
request here.

I think my original code wasn't very clear, because it mixed both
setting and clipping the container's size. Would it be more clear if it
was written like this?

            req->height = child_req.height;

            // If the height of the container is too big so that
            // it overflows the max usable height, clip it.
            if (req->height > max_height) {
                req->height = max_height;
            }


Thanks,
Kalev