I need this for Nautilus long operations popover and for the new GtkPlacesView gsoc project.

FWIW code of Christian looks good to me. If we don't have a strong reason to not have these properties, it would be nice to have this on master as soon as possible.

The code looks rather simple when done in a subclass, but merging it into the very messy details of GtkScrolledWindow is probably what is holding this up.

Strictly speaking, we could just copy it in. But that doesn't help us from a maintainability standpoint.

Created attachment 328919 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

The GtkScrolledWindow has support to set the minimum content size (both
width and height) which controls the minimum space allocated, but does
not exposes any way to control the maximum size the content can grow.

After the introduction of GtkPopover, which always uses the minimum
size of it's children widgets, the lack of max-content-width and -height
properties became a concrete use case.

This patch introduces the GtkScrolledWindow::max-content-width and
-height properties. The properties will alter the minimum size of
the scrolled window, making it grow up to the set value. They also
respect the previously set ::min-content-width and -height.

Created attachment 328920 [details] [review]
tests: adapt testscrolledwindow to have max content sizes

This patch adpats the current test to show a popover with
max-content-width and -height properties set.

Review of attachment 328919 [details] [review]:

::: gtk/gtkscrolledwindow.c
@@ +1812,3 @@
+                        priv->min_content_width > width)
+                      {
+

minimum_req.width no?

@@ +1816,3 @@
+                    else
+                      {
+                     * It'll grow at least until min_content_width, and at most until

dito

Review of attachment 328920 [details] [review]:

The test in the following patch doesn't really cover the mistake done in the previous patch. Worth expanding the test to cover all cases.

Created attachment 328949 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Updated and simplified the patch.

(In reply to Carlos Soriano from comment #5)
> Review of attachment 328919 [details] [review] [review]:
> 
> ::: gtk/gtkscrolledwindow.c
> @@ +1812,3 @@
> +                        priv->min_content_width > width)
> +                      {
> +
> 
> minimum_req.width no?

No.

(In reply to Georges Basile Stavracas Neto from comment #7)
> Created attachment 328949 [details] [review] [review]
> scrolledwindow: add ::max-content-width and -height properties
> 
> Updated and simplified the patch.
> 
> (In reply to Carlos Soriano from comment #5)
> > Review of attachment 328919 [details] [review] [review] [review]:
> > 
> > ::: gtk/gtkscrolledwindow.c
> > @@ +1812,3 @@
> > +                        priv->min_content_width > width)
> > +                      {
> > +
> > 
> > minimum_req.width no?
> 
> No.

No? But you actually changed it in your last patch. Are you sure you checked the line I was reviewing? (Don't trust what is printed in the comment, click the "review" button)

Created attachment 328962 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Done

It's "bigger than", not "bigger then". But more importantly, may I mention at this point that there's a size requisition test for GtkRevealer[0] which makes sure that GtkRevealer preferred sizes make sense? *hint* *hint*




[0] https://git.gnome.org/browse/gtk+/tree/testsuite/gtk/revealer-size.c

Created attachment 329010 [details] [review]
testsuite: add tests for scrolledwindow

This patch adds a set of tests for scrolled window
sizing, covering both min- and max-content width and
height.

Created attachment 329011 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Fixed the typos.

Review of attachment 329011 [details] [review]:

Refreshingly simple patch, I have to say.

I'm not happy with the content-width/height naming of these properties - its really about the size of the viewport that you get on the child ('the content'). But too late now, since we already have the min-content-width/height properties.

::: gtk/gtkscrolledwindow.c
@@ +1810,3 @@
+                     */
+                    minimum_req.width = MAX (minimum_req.width, width);
+                    natural_req.width = width;

I think this should be

natural_req.width = MAX (natural_req.width, width)

if only for clarity (but I am not 100% convinced that you can't end up with natural_req.with < minimum_req.width without it.

@@ +1850,3 @@
+                     */
+                    minimum_req.height = MAX (minimum_req.height, height);
+                    natural_req.height = height;

Same here

@@ +4547,3 @@
+ *
+ * It is a programming error to set the max_content_width to a value bigger
+ * than min_content_width.

You mean smaller here, certainly ? Thats what your assertion is about, anyway.

And, if you consider it a programming error here, you should do the same in 

gtk_scrolled_window_set_min_content_width

@@ +4599,3 @@
+ *
+ * It is a programming error to set the max_content_height to a value bigger
+ * than min_content_height.

Same here: smaller

Review of attachment 329011 [details] [review]:

::: gtk/gtkscrolledwindow.c
@@ +1839,3 @@
 		}
+
+                if (priv->max_content_height > -1 &&

There's an indentation bug here - the whole block is off-by-2-spaces

Created attachment 329084 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Fixed the indentation issue, as well as the docs and the natural height
request.

Review of attachment 329084 [details] [review]:

Did you want to add the same assert and documenation to the min-content setters ?

::: gtk/gtkscrolledwindow.c
@@ +4553,3 @@
+ *
+ * It is a programming error to set the max_content_width to a value smaller
+ * than min_content_width.

Purely cosmetic, but I'd avoid te underscores in the text here and may say:

It is a programming error to set the maximum content width to a value smaller than #GtkScrolledWindow:min-content-width.

@@ +4606,3 @@
+ * It is a programming error to set the max_content_height to a value smaller
+ * than min_content_height.
+ *

Same here

Created attachment 329086 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Improved the documentation.

Review of attachment 329086 [details] [review]:

::: gtk/gtkscrolledwindow.c
@@ +4415,3 @@
  *
+ * It is a programming error to set the maximum content width to a
+ * value smaller than #GtkScrolledWindow:min-content-width.

I would actually flip these around here and say:

It is a programming error to set the minimum content width to a
value bigger than #GtkScrolledWindow:max-content-width.

Man, I'm picky today...

@@ +4470,3 @@
+ * It is a programming error to set the maximum content height to a
+ * value smaller than #GtkScrolledWindow:min-content-height.
+ *

Same here

Created attachment 329091 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

(In reply to Matthias Clasen from comment #18)
> Review of attachment 329086 [details] [review] [review]:
> I would actually flip these around here and say:
> 
> It is a programming error to set the minimum content width to a
> value bigger than #GtkScrolledWindow:max-content-width.
> 
> Man, I'm picky today...

Absolutely. Fixed.
 
> + *
> 
> Same here

Done.

Review of attachment 329084 [details] [review]:

Overall, the patch makes perfect sense.

I don't think we should land this with the current g_assert() expressions, that's a bit overkill,
it may be a programming error to set min > max as mentioned in the new docs, but return_if_fail() 
to police the values should be used here.

Created attachment 329130 [details] [review]
scrolledwindow: add ::max-content-width and -height properties

Thanks for the review, Tristan. This patch changes from g_assert() to g_return_if_fail().

Review of attachment 329130 [details] [review]:

Looks good to me :)

(In reply to Tristan Van Berkom from comment #22)
> Review of attachment 329130 [details] [review] [review]:
> 
> Looks good to me :)

I should add one more note to this, in light of bug 767238 which I discovered looking into scrolled windows this weekend, equal care must be taken for the new max content size.

I.e. the added clause which 'clamps' the child request size should be performed before adding any space for optional scrollbars (content size, max or min, is not scrollbar inclusive).

That said I think it's fine to merge this with the existing mis-behavior in place and just do the right thing in bug 767238.

Review of attachment 329130 [details] [review]:

.

Review of attachment 329010 [details] [review]:

more tests always good!

Review of attachment 328920 [details] [review]:

ok. Ideally we'd also find a place in gtk3-demo or gtk-widget-factory to demonstrate this

Thanks for the reviews.

Attachment 328920 [details] pushed as 657fcd0 - tests: adapt testscrolledwindow to have max content sizes
Attachment 329010 [details] pushed as 3962b90 - testsuite: add tests for scrolledwindow
Attachment 329130 [details] pushed as 4e5ecb7 - scrolledwindow: add ::max-content-width and -height properties