Created attachment 328086 [details] [review]
scrolledwindow: Remove redundant use of MAX

Created attachment 328088 [details] [review]
scrolledwindow: Better size requisition for GTK_SCROLL_NATURAL children

Comment on attachment 328086 [details] [review]
scrolledwindow: Remove redundant use of MAX

Looks good :). The = vs += seems confusing without more patch context, but those variables are 0 at that point, so that's fine.

Review of attachment 328088 [details] [review]:

It looks right IMO, I'll be just marking as "reviewed" though, probably some other opinion is needed.

::: gtk/gtkscrolledwindow.c
@@ +1713,3 @@
   GtkBin *bin = GTK_BIN (scrolled_window);
+  GtkScrollablePolicy hscroll_policy = GTK_SCROLL_MINIMUM;
+  GtkScrollablePolicy vscroll_policy = GTK_SCROLL_MINIMUM;

for the sake of readability, I'd name those variables scrollable_v/hpolicy :), lest we get confused with the various policies around.

@@ +1771,3 @@
+                {
+		  minimum_req.width += min_child_size;
+		  natural_req.width += nat_child_size;

I was about to suggest folding those with the first branch, but I guess we want min_content_width/height to take effect before the scrollable policy.

Comment on attachment 328086 [details] [review]
scrolledwindow: Remove redundant use of MAX

Thanks for the review, Carlos!

Created attachment 328089 [details] [review]
scrolledwindow: Better size requisition for GTK_SCROLL_NATURAL children

Changed the variable names.

Attachment 328089 [details] pushed as 096bea3 - scrolledwindow: Better size requisition for GTK_SCROLL_NATURAL children

(In reply to Debarshi Ray from comment #0)
> GtkScrolledWindow leans towards using the minimum size of its child widget,
> unless the scrollbar policy is GTK_POLICY_NEVER. This is probably fine for
> most GtkScrollable implementations out there. Especially when using
> GTK_SCROLL_MINIMUM, which is the default for all implementations inside gtk+.
> 
> However, this is not good for GTK_SCROLL_NATURAL children. eg.,
> VteTerminal's minimum size is 1x1 and natural size is the number of visible
> rows and columns requested by the user. We really want to use the natural
> size unless the user has resized the window to change that.
> 
> The other option is to sub-class GtkScrolledWindow in gnome-terminal and
> replace its size requisition logic. See attachment 327779 [details] [review] [review]
> in bug 733210

I'm trying to wrap my head around this, I think the logic of this patch is flawed but I could be wrong.

Regarding GtkScrollablePolicy, I admit is not very clearly documented:

  "Defines the policy to be used in a scrollable widget when updating the 
   scrolled window adjustments in a given orientation."

And a bit more meaningful from gtk_scrollable_set_hscroll_policy():

  "Sets the GtkScrollablePolicy to determine whether horizontal scrolling 
   should start below the minimum width or below the natural width."

The purpose of the scrollable policy is to determine when the wrapping height for width content of a scrolled window should stop wrapping around and start scrolling instead.

Here is the original conversation:
   https://bugzilla.gnome.org/show_bug.cgi?id=468689#c22

Your patch however, actually effects the size requesting of the scrolled window, and I believe this is incorrect if I'm reading the patch right.

Basically:

  a.) A scrolled window that can scroll, will always be allowed to reduce
      its own size to 0px / 0px + scrollbar widths.

      A scrolled window that is not SCROLL_NEVER is always allowed to be
      smaller than it's child - that's sort of what viewports and scrolled
      windows are for. 

  b.) I may prefer that my height-for-width wrapping or ellipsis content
      in that scrolled window wrap down to it's minimum width before
      causing a horizontal scrollbar to appear; that is the default.

      The policy allows me to prefer to allocate the child always a minimum
      of it's natural size; so horizontal scrollbars appear when you shrink
      the child below it's natural size.

      This is especially important for ellipsis content, which would
      normally just disappear; forcing the user to resize the app if they
      want to see the full text in an ellipsis label embedded somewhere
      in a scrolled window.


Looking at the patch, it seems that the scrolled window would now:

  1.) transmit the desired natural width of the child to it's parent
      during the request phase... which sounds like an interesting
      feature on it's own

  2.) has the incorrect side effect of making the minimum size of the
      actual scrolled window become the minimum size of the child,
      making it impossible for the user to shrink the scrolled window
      to a smaller size than it's childs minimum.


Considering that a scrolled window is usually set to expand and eat up any remaining space, I wonder if a better approach here would be to just change the size request code so that the scrolled window always reports enough natural width for it's child, and continue reporting the same minimum size.

This eliminates possible negative side effect outlined in (2) above, and also does not need any special casing on the scrollable policy.

Thoughts ?

didn't mean to push this, sorry.

(In reply to Tristan Van Berkom from comment #8)

> 
> Considering that a scrolled window is usually set to expand and eat up any
> remaining space, I wonder if a better approach here would be to just change
> the size request code so that the scrolled window always reports enough
> natural width for it's child, and continue reporting the same minimum size.

Sounds good to me, fwiw.

Created attachment 329115 [details] [review]
Report natural size unconditionally

This patch reports the natural size of the child unconditionally.

Created attachment 329116 [details]
Glade file demonstrating scrolled window reporting natural size

Attached glade file may be useful to observe the effect.

It has a simple setup as:

GtkWindow
  GtkBox
    GtkScrolledWindow
      GtkLabel (long text, ellipsis end)
  GtkLabel (short text, also ellipsis end)

Review of attachment 329115 [details] [review]:

ok

Review of attachment 329115 [details] [review]:

pushed as 0943c9f

I've reverted this - it caused unexpected fallout in gtk3-widget-factory and also seems to break the new max-content-size properties.

First look at this reveals that the test added in bug 742281 is incorrect, even though it did happen to pass before fixing the scrolled window to add scrollbar widths around the content size.

I get this failure:

ERROR:scrolledwindow.c:37:test_size: assertion failed (size == MIN_SIZE): (163 == 150)

This is because the said test makes the assumption that the scrolled window itself will never allocate any additional space surrounding the content.

While this is of course effected by the scrollbar widths (I'm not sure why they are being included here though, they should be overlay by default and nothing is setting GTK_OVERLAY_SCROLLING in the environment afaics), the calling C code cannot assume that the scrolled window will not have any border widths of any kind, either applied by other C code, possible changing of padding defaults, or values loaded from css, all of which can potentially change what the scrolled window may request in addition to it's child requisition.

Test case simply does:

  gtk_scrolled_window_set_min_content_width (GTK_SCROLLED_WINDOW (scrolledwindow), MIN_SIZE);
  gtk_widget_get_preferred_width (scrolledwindow, &size, NULL);
  g_assert_cmpint (size, ==, MIN_SIZE);

(In reply to Matthias Clasen from comment #15)
> I've reverted this - it caused unexpected fallout in gtk3-widget-factory and
> also seems to break the new max-content-size properties.

I'm curious how you came to the conclusion that this broke the max-content-width/height properties, although still looking into it...

Created attachment 329238 [details] [review]
scrolled window size request cleanup

This patch does a couple of things:
    
  o Removes the obscure 'extra_width' and 'extra_height' variables
    making the request code exceedingly difficult to read

  o Fixes the max-content-size properties introduced in bug 742281
    so that they do not grow the minimum request.

  o Cleanup of request code in general:
    - min/max content sizes are clamped around the child request as needed
    - scrollbar requests are only added in one place, after child request
      sizes are calculated and without the extra_width/height thing.

Created attachment 329239 [details] [review]
scrolledwindow: Support size requests before realize()

The priv->use_indicators cached value plays an important role in size requests but is never resolved until later, when the widget is actually realized.

In general, we want size request to be correct before realize, otherwise we have overhead of re-allocating GdkWindow resources during the initial display cycle.

This has the side effect of reporting the correct information for the new test case testsuite/gtk/scrolledwindow.c, added in bug 742281.

Created attachment 329240 [details] [review]
Fixed new scrolled window test case

Asides from this test case relying on:
  a.) scrolled windows not requesting any border/padding etc
  b.) scrolled windows being in overlay scrollbar mode

The test case had one essential flaw, which is that it expects the scrolled window to increase it's *minimum* size request in response to a setting of the max-content-width/height properties.

This patch fixes this, and additionally makes the case more strict by testing the reported natural size with == instead of >=.

The above patches clarify the request logic in scrolled window and have been tested for:

  a.) the new scrolledwindow test in testsuites/gtk
  b.) the popover on page 3 of the widget factory demo
  c.) the glade file I attached on this bug report
  d.) the glade file I attached to bug 767238


There is an interesting side effect of reporting natural size in a setting where a scrolled window is competing for natural request space and has dynamic content, especially content which calculates size in a timeout such as a text view.

Notice when loading the widget factory initially, the window will renegotiate space and the text view will grow while it queues resizes in it's timeout. Turn to page 2 and see it happen again. On page 2 the icon view has an extremely large natural width and is requested synchronously - while the text view will compete for some space but not as much as one might expect.

At this junction, I'm a bit torn about making this the default, maybe we need some extra semantic for this.

As explained in comment 8 above, this should not be enabled as a hidden side effect of the GtkScrollablePolicy being NATURAL, as this effects how the child is allocated inside the scrolled window when the child's minimum/natural size differs.

Review of attachment 329239 [details] [review]:

Looks ok to me

Review of attachment 329238 [details] [review]:

this didn't apply here, but it looks cleaner indeed.

Review of attachment 329240 [details] [review]:

ok

(In reply to Tristan Van Berkom from comment #20)

> Notice when loading the widget factory initially, the window will
> renegotiate space and the text view will grow while it queues resizes in
> it's timeout. Turn to page 2 and see it happen again. On page 2 the icon
> view has an extremely large natural width and is requested synchronously -
> while the text view will compete for some space but not as much as one might
> expect.

Yeah, these are the oddities I saw.

> At this junction, I'm a bit torn about making this the default, maybe we
> need some extra semantic for this.

I agree.

Review of attachment 329238 [details] [review]:

pushed as d7e242e

Review of attachment 329239 [details] [review]:

Pushed as 34feba1

Review of attachment 329240 [details] [review]:

Pushed as d6187c9

(In reply to Matthias Clasen from comment #24)
[...]
> > At this junction, I'm a bit torn about making this the default, maybe we
> > need some extra semantic for this.
> 
> I agree.

So I pushed these, I think it's certainly the right direction... but we may need to add some API here, even if the fallout is minimal in real interfaces I'm not sure what existing API we could recommend to avoid scrolled windows competing for horizontal space, in the cases where for instance; we have some height-for-width wrapping text on the left and scrolled window with textview on the right.

Using max-width-chars and the like to work around this would not be ideal.

Leaving this open for now...

Comment on attachment 329238 [details] [review]
scrolled window size request cleanup

This patch breaks Login History dialog in gnome-control-center (g-c-c). Scrollbar is not shown and dialog is resized to fit GtkListBox height instead. See source:
https://git.gnome.org/browse/gnome-control-center/tree/panels/user-accounts/data/history-dialog.ui#n79

I am not sure, whether this is bug in your patch, or in g-c-c. What needs to be done in order to fix it?

Created attachment 331854 [details]
History dialog screenshot before and after

(In reply to Ondrej Holy from comment #29)
> Comment on attachment 329238 [details] [review] [review]
> scrolled window size request cleanup
> 
> This patch breaks Login History dialog in gnome-control-center (g-c-c).
> Scrollbar is not shown and dialog is resized to fit GtkListBox height
> instead. See source:
> https://git.gnome.org/browse/gnome-control-center/tree/panels/user-accounts/
> data/history-dialog.ui#n79
> 
> I am not sure, whether this is bug in your patch, or in g-c-c. What needs to
> be done in order to fix it?

Hi Ondrej,

The patch in question does a couple of different things while simplifying the size requesting logic at the same time, I am almost certain that what is happening here is the dialog is being allocated it's calculated natural height, and having changed the scrolled window behavior to report its child's natural size is what caused this (which is the reason we've left this bug open).

As far as I can see, there are some (few) cases which benefit from the natural size of the child being propagated through, some cases where this doesnt change anything significantly, and some cases where this is harmful, like your history dialog in g-c-c and the side effects described in comment 20.

I think it's clear we need some semantic to be able enable the scrolled window reporting natural widths/heights of it's child (and keep the default off as it always was), we've just been reluctant to add new API to cover this.

This change of behavior also affects the Search, Notifications, and Printers, panels in GNOME Control Center.

An easy UI workaround would be setting a "max-content-height" value, but this is obviously hackish.

With gtk+ 3.21, tracker-preferences looks like this by default.
https://bugzilla.gnome.org/attachment.cgi?id=333304
It is automatically allocated the maximum height possible for the "Ignored content" tab without even switching to that tab.
Under gtk+ 3.20, it allocates just enough height for the initial tab and I can scroll down in the "Ignored content" tab.
It won't look very elegant from an end-user's perspective when a dialog takes a lot more size than it has to.

IMO these patches break way more than they actually improve (that might just be GtTextView relying on the old behavior of course, but as long as nobody fixes that...). Open the widget-factory with GTK_OVERLAY_SCROLLING=0 for example and see how Page 2 looks completely different.

git bisect shows commit d7e242e (attachment 329238 [details] [review]) is the root cause of bug 770430 which was crashing mutter/gnome-shell under Wayland (bug 770387)

Created attachment 334495 [details] [review]
Add API to make propagation of child natural sizes optional

GtkScrolledWindow: Bug 766569 - Make propagation of natural child sizes optional
    
Making propagation of child natural sizes mandatory (or default, even) was
evidently a mistake as this causes dynamic content in a scrolled window
to resize it's parent when the scrolled window is competing for space
with an adjacent widget.
    
This patch instead adds API to control whether natural width and
height of the child should be propagated through the scrolled windows
size requests.

(In reply to Tristan Van Berkom from comment #36)
> Created attachment 334495 [details] [review] [review]
> Add API to make propagation of child natural sizes optional
> 
> GtkScrolledWindow: Bug 766569 - Make propagation of natural child sizes
> optional
>     
> Making propagation of child natural sizes mandatory (or default, even) was
> evidently a mistake as this causes dynamic content in a scrolled window
> to resize it's parent when the scrolled window is competing for space
> with an adjacent widget.
>     
> This patch instead adds API to control whether natural width and
> height of the child should be propagated through the scrolled windows
> size requests.

Bit late since it has been pushed, but the types are all mis-matched on this. The header for the getters return gboolean, the function returns gint (which i know are equal), and the actual type is guint. Ideally the actual type should just be gboolean, cleaner that way at least.

FYI if the various applications that now have too small scrolled windows are too arcane, there's one on page 2 of the widget factory, namely the one with the icon view on the right, that is now way smaller than it was before (before meaning in 3.20).

(In reply to Patrick Griffis (tingping) from comment #37)
[...]
> 
> Bit late since it has been pushed, but the types are all mis-matched on
> this. The header for the getters return gboolean, the function returns gint
> (which i know are equal), and the actual type is guint. Ideally the actual
> type should just be gboolean, cleaner that way at least.

Hi Patrick,

Fixed this with commit eaa6ca4a4920ee2146a91a20dbacc321ea55908b, thanks for
pointing out my mistake (likely a copy paste error).

To clarify; the "actual type" is an ambiguous concept here, the param spec (property) type is boolean, the outward facing API is all boolean (now that I've fixed the error).

Internally we use a single bit to store the boolean state, that's what you are seeing on the private data structure "guint : 1" (using gboolean for storage would cost 32 or 64 bits, but we only need 1 bit to store a boolean property value).

(In reply to Timm Bäder from comment #38)
> FYI if the various applications that now have too small scrolled windows are
> too arcane, there's one on page 2 of the widget factory, namely the one with
> the icon view on the right, that is now way smaller than it was before
> (before meaning in 3.20).

Should we reopen, or create a new bug? Tristan?

(In reply to Bastien Nocera from comment #40)
> (In reply to Timm Bäder from comment #38)
> > FYI if the various applications that now have too small scrolled windows are
> > too arcane, there's one on page 2 of the widget factory, namely the one with
> > the icon view on the right, that is now way smaller than it was before
> > (before meaning in 3.20).
> 
> Should we reopen, or create a new bug? Tristan?

I'm ambivalent about either or, so I'll just reopen for now.

I'm not entirely sure that this mentioned behavioral change is related to these commits but it's likely related. Tomorrow I'll take a look at page 2 of the widget factory and see how it behaves in older GTK+ vs master, and try to discern why.

fwiw, the iconview on page 2 can be fixed by setting propagate-natural-height to TRUE

(In reply to Timm Bäder from comment #38)
> FYI if the various applications that now have too small scrolled windows are
> too arcane, there's one on page 2 of the widget factory, namely the one with
> the icon view on the right, that is now way smaller than it was before
> (before meaning in 3.20).
[...]

(In reply to Matthias Clasen from comment #42)
> fwiw, the iconview on page 2 can be fixed by setting
> propagate-natural-height to TRUE

In reply to both comments: The scrolled window on page 2 of the widget factory changed since 3.20 because of commit 37e913d76bea6b58a38db1b7996958b81984a114, which tried to adapt to the new behavior by limiting the request sizes of that scrolled window using the new max-content-width/height properties.

I've reverted the commit in question, now that the behavioral change in the unstable series has been made optional and the scrolled windows appear exactly the same size in gtk-3-20 branches and master at this time. At least this behavioral change in particular was not caused by changes in the scrolled window, but by changes in the widget factory application.

Closing this bug with the following commit:

commit 6be8979c645387a205f9fbbd91aa669c651ca791
Author: Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
Date:   Tue Sep 6 20:23:20 2016 +0900

    Revert "widget factory: Adapt to new scrolled window behavior"
    
    This reverts commit 37e913d76bea6b58a38db1b7996958b81984a114.
    
    This is no longer needed since the natural size propagation of
    scrolled window children is now an optional behavior. Reverting
    this also makes the widget factory scrolled window sizes behave
    the same as with the gtk+ 3.20 branch.


Please let us know about scrolled window behavioral API breaks between gtk 3.20 and master and I will look into them.

(In reply to Tristan Van Berkom from comment #8)

Sorry for disappearing, Tristan. I got preempted by some other things and didn't get a chance to page gtk/gtkscrolledwindow.c and friends back into my brain until now.

I think that we are one step closer to using an unmodified GtkScrolledWindow in gnome-terminal, but there are still some cases that I am not sure about.

> Regarding GtkScrollablePolicy, I admit is not very clearly documented:
> 
>   "Defines the policy to be used in a scrollable widget when updating the 
>    scrolled window adjustments in a given orientation."
> 
> And a bit more meaningful from gtk_scrollable_set_hscroll_policy():
> 
>   "Sets the GtkScrollablePolicy to determine whether horizontal scrolling 
>    should start below the minimum width or below the natural width."
> 
> The purpose of the scrollable policy is to determine when the wrapping
> height for width content of a scrolled window should stop wrapping around
> and start scrolling instead.

Ok.

(By the way, thanks for the cleanup in attachment 329238 [details] [review] or commit d7e242eec03a1. It does make the code a lot easier to read. )

I am trying to understand the original logic behind VteTerminal (which implements GtkScrollable) defaulting to GTK_SCROLL_NATURAL. Earlier, I assumed that it was meant to be a hint to GtkScrolledWindow's size request code to respect/transmit the terminal's minimum and natural sizes. Since, that's not what GtkScrollablePolicy is meant to be, I wonder if:

(a) there should be a way for a GtkScrolledWindow to look at a GtkScrollable and enable the propagate-natural-* behaviour

(b) there should be API to also propagate the minimum sizes

(b) VteTerminal defaulting to GTK_SCROLL_NATURAL (unlike all other GtkScrollable implementations) has any significance at all

?

What do you think?

Some background:

VteTerminal's minimum request is a single cell (not pixels). Its natural request is something like 80x24 cells. ie. whatever the user has set the visible terminal size to be. Therefore, when a VteTerminal is placed inside a GtkWindow without a GtkScrolledWindow (like it is in gnome-terminal today), the window will grow to accommodate the natural size, and can't be resized to show anything smaller than a single cell.

However, if VteTerminal is allocated a size bigger than its natural request, it doesn't mean that its adjustments won't scroll. eg., if you ran 'seq 500' your terminal will still scroll even if it is bigger than its natural size.

> Looking at the patch, it seems that the scrolled window would now:
> 
>   1.) transmit the desired natural width of the child to it's parent
>       during the request phase... which sounds like an interesting
>       feature on it's own
> 
>   2.) has the incorrect side effect of making the minimum size of the
>       actual scrolled window become the minimum size of the child,
>       making it impossible for the user to shrink the scrolled window
>       to a smaller size than it's childs minimum.

(2) might also be important for VteTerminal. I don't know how strongly chpe feels about not respecting the 'a VteTerminal can't be smaller than a single cell' rule.

In the worst case, if we really can't use an unmodified GtkScrolledWindow in gnome-terminal, I don't mind overriding its size request logic in a derived object inside the application (see attachment 327779 [details] [review] in bug 733210). chpe might have an opinion on that, though.

Review of attachment 334495 [details] [review]:

::: gtk/gtkscrolledwindow.c
@@ +1840,3 @@
 
+	  if (priv->propagate_natural_width)
+	    natural_req.width += nat_child_size;

Doesn't this mean that when propagate-natural-* is FALSE and scrollbar-policy is NEVER, the natural size of GtkScrolledWindow will not be the natural size of the child?

Instead ...

@@ +1881,3 @@
+  /* Ensure we make requests with natural size >= minimum size */
+  natural_req.height = MAX (minimum_req.height, natural_req.height);
+  natural_req.width  = MAX (minimum_req.width,  natural_req.width);

... it will be the minimum size of the child.

I don't know if this is a problem in practice. Or maybe I am completely mistaken.

Hi Debarshi :)

[...]
> I am trying to understand the original logic behind VteTerminal (which
> implements GtkScrollable) defaulting to GTK_SCROLL_NATURAL. Earlier, I
> assumed that it was meant to be a hint to GtkScrolledWindow's size request
> code to respect/transmit the terminal's minimum and natural sizes. Since,
> that's not what GtkScrollablePolicy is meant to be, I wonder if:
> 
> (a) there should be a way for a GtkScrolledWindow to look at a GtkScrollable
> and enable the propagate-natural-* behaviour
> 
> (b) there should be API to also propagate the minimum sizes
> 
> (b) VteTerminal defaulting to GTK_SCROLL_NATURAL (unlike all other
> GtkScrollable implementations) has any significance at all
> 
> ?
> 
> What do you think?

Regarding (a)

I'm pretty confident that choosing to have the scrolled window to propagate the 
natural size of the child is not something the child widget should be deciding. 
This seems clearly to be a decision that an application or container makes 
about how it uses that child.

Regarding (b)

I'd like to avoid this if at all possible, the API is getting already bigger 
than I'd like. That said, below you make an interesting argument for it.

Keep in mind that as a rule, GTK+ will never allocate a widget less than it's 
request, and scrolled windows and viewports are exactly the exception to handle
this in a sane way.

Propagating the minimum size of a child seems to defeat this purpose at face
value, but with the distinction between minimum vs natural sizes it might
make sense; if so I wonder if we could have a better API for this, there seems
to be more knobs and dials on this API than desirable.

Regarding (c)

It serves a purpose. You mention that the VteTerminal which you place into a
scrolled window requests one cell as minimum width and 80 cells as natural
width. 

If you were using the GTK_SCROLL_MINIMUM policy, the result would be that when
the user shrinks the surrounding scrolled window, the VteTerminal would wrap all
the way down to 1 cell.

The horizontal scrollbar would only ever appear when the scrolled window is allocated less space than one cell (you might try this with huge 64pt fonts to see the effect of needing to scroll the window in order to view both sides of the given cell ;-) ).

I hope this helps to explain the meaning of the scrollable policy better.

> 
> Some background:
> 
> VteTerminal's minimum request is a single cell (not pixels). Its natural
> request is something like 80x24 cells. ie. whatever the user has set the
> visible terminal size to be. Therefore, when a VteTerminal is placed inside
> a GtkWindow without a GtkScrolledWindow (like it is in gnome-terminal
> today), the window will grow to accommodate the natural size, and can't be
> resized to show anything smaller than a single cell.
> 
> However, if VteTerminal is allocated a size bigger than its natural request,
> it doesn't mean that its adjustments won't scroll. eg., if you ran 'seq 500'
> your terminal will still scroll even if it is bigger than its natural size.
> 
> > Looking at the patch, it seems that the scrolled window would now:
> > 
> >   1.) transmit the desired natural width of the child to it's parent
> >       during the request phase... which sounds like an interesting
> >       feature on it's own
> > 
> >   2.) has the incorrect side effect of making the minimum size of the
> >       actual scrolled window become the minimum size of the child,
> >       making it impossible for the user to shrink the scrolled window
> >       to a smaller size than it's childs minimum.
> 
> (2) might also be important for VteTerminal. I don't know how strongly chpe
> feels about not respecting the 'a VteTerminal can't be smaller than a single
> cell' rule.

This rule remains true no matter what, however you are making a subtle
distinction here: While the VteTerminal might be as large as a single cell,
the surrounding scrolled window is allowed to be smaller than the size of
the embedded VteTerminal.

Perhaps what you mean to define the rule as: the container in which you place a VteTerminal must allocate at least enough space for the VteTerminal to display, a single cell on screen.

This is simply not an assertion that a child widget can make, however; and scrolled windows expressed purpose is to break that rule (to allow displaying only a part of a child's allocated size).

With that said: we did add the "minimum-content-width" and 
"minimum-content-height" properties to the scrolled window to allow application
authors to set a size below which the scrolled window's child area must not
shrink.

I.e. it tells the scrolled window to allocate "at least this much space to the child". This might accommodate your use case but I understand it is not completely ideal.


> In the worst case, if we really can't use an unmodified GtkScrolledWindow in
> gnome-terminal, I don't mind overriding its size request logic in a derived
> object inside the application (see attachment 327779 [details] [review] [review] in
> bug 733210). chpe might have an opinion on that, though.

I'm curious, it seems the only problem here is that it's possible the user might shrink the terminal to a size smaller than is required to display a single "cell", is this a realistic use case ?

Also, would an arbitrary but small size work equally when setting the minimum content width/hight properties ?

(In reply to Tristan Van Berkom from comment #46)

Btw, since I never clearly mentioned this before, the original problem was that if you put VteTerminal in a GtkScrolledWindow (with horizontal=GTK_POLICY_NEVER, vertical=GTK_POLICY_ALWAYS) in gnome-terminal, then you basically get a 80x2 strip. So, the problem here is the height.

> Regarding (a)
>
> I'm pretty confident that choosing to have the scrolled window to propagate
> the natural size of the child is not something the child widget should be
> deciding. This seems clearly to be a decision that an application or container makes
> about how it uses that child.

Ok.

> Regarding (c)
>
> It serves a purpose. You mention that the VteTerminal which you place into a
> scrolled window requests one cell as minimum width and 80 cells as natural
> width.
>
> If you were using the GTK_SCROLL_MINIMUM policy, the result would be that
> when the user shrinks the surrounding scrolled window, the VteTerminal would wrap
> all the way down to 1 cell.

I think the terminal is a bit unusual.

Note:
 (i) GtkViewport is not involved.
 (ii) Horizontal scrolling is not involved.
 (iii) VteTerminal is GTK_SIZE_REQUEST_CONSTANT_SIZE.

A VteTerminal's initial size request is whatever the user set his preferences to. Then when you resize it, its natural height request is updated to match the current size allocation. ie. VteTerminal::size-allocate calls vte_terminal_set_size with the number of rows that it can now accommodate, which is used for the subsequent natural height request.

Thus, under non-pathological conditions, its natural height request is always satisfied.

The natural width request is also similarly adjusted, but a VteTerminal cannot scroll horizontally because, I guess, a physical terminal won't scroll horizontally either. When shrunk, the width of the emulated terminal decreases and the lines re-wrap to take up more rows (ie. the vertical adjustment's range increases, but not the height request).

Since
  (i) there are no horizontal scrollbars
  (ii) neither the minimum nor the natural request can exceed the allocation,
regardless of the GtkScrollPolicy, the initial scrollbar visibility guessing code in gtk_scrolled_window_allocate will always think that there is no need to show the vertical scrollbar. It is only when it looks at the adjustments during the allocation loop that it realizes whether there is any history in the terminal that requires scrolling.

Hence I think VteTerminal's GtkScrollPolicy has no significance. :)

> > > Looking at the patch, it seems that the scrolled window would now:
> > >
> > >   1.) transmit the desired natural width of the child to it's parent
> > >       during the request phase... which sounds like an interesting
> > >       feature on it's own
> > >
> > >   2.) has the incorrect side effect of making the minimum size of the
> > >       actual scrolled window become the minimum size of the child,
> > >       making it impossible for the user to shrink the scrolled window
> > >       to a smaller size than it's childs minimum.
> >
> > (2) might also be important for VteTerminal. I don't know how strongly chpe
> > feels about not respecting the 'a VteTerminal can't be smaller than a single
> > cell' rule.
>
> This rule remains true no matter what, however you are making a subtle
> distinction here: While the VteTerminal might be as large as a single cell,
> the surrounding scrolled window is allowed to be smaller than the size of
> the embedded VteTerminal.
>
> Perhaps what you mean to define the rule as: the container in which you
> place a VteTerminal must allocate at least enough space for the VteTerminal
> to display, a single cell on screen.

Yes, that's what I meant. I should have been more explicit. Sorry about that.

> With that said: we did add the "minimum-content-width" and
> "minimum-content-height" properties to the scrolled window to allow
> application
> authors to set a size below which the scrolled window's child area must not
> shrink.
>
> I.e. it tells the scrolled window to allocate "at least this much space to
> the child". This might accommodate your use case but I understand it is not
> completely ideal.

minimum-content-height is one option, yes. The other, more mathematically correct, option would be to bubble the VteTerminal's minimum height request in a derived GtkScrolledWindow class.

> I'm curious, it seems the only problem here is that it's possible the user
> might shrink the terminal to a size smaller than is required to display a
> single "cell",

Yes. :)

> is this a realistic use case ?

I don't know. It is certainly possible for the user to do that if they prefer to hide the scrollbar. Whether it is something we care about, I don't know.

Ultimately, it depends on chpe.