This patch works fine for me, gtk_scrollable_set_Xscroll_policy changes the behavior as expected.

The only nitpick I would have is line length, need some newlines in there.

Created attachment 276391 [details] [review]
fix with line breaks

Review of attachment 276391 [details] [review]:

Matthias informally asked me to review this patch on IRC, this is a very
difficult review and I don't have time to give it very much attention but
here is what I have written up.

First of all I should mention that this ties directly into height-for-width,
we never landed height-for-width geometry into the treeview and in order
to keep things working as much as possible like it did in GTK+2, we simply
settled on a minimum-for-minimum default for the treeview area.

That said, text renderers DO height-for-width and will unwrap dynamically
if they are given more width (thus reducing their need for height).

For height-for-width to really work in GtkTreeView, the size request pattern
needs to be split into two separate iterations.

This is approximately what is happening right now:

  o The visible area is validated syncrhonously once for the purpose
    of responding to a size request and showing something on screen
    as fast as possible

  o The remaining rows are then traversed, for each row:

    o The minimum width is requested for the row
    o The height for that width is requested
    o The row heights are cached in the RBTree
    o The overall treeview 'width' and 'height' is recorded
    o The upper bounds of the adjustments are increased to allow for
      the additional rows

What needs to happen for height-for-width to be properly supported:

  o The treeview must cache the minimum/natural overall width/height
    instead of just the minimum in order to efficiently respond to
    size requests which can occur multiple times between any
    change in the treeview's underlying content.

  o The visible area must be validated synchronously as usual, however
    it must be done in two phases - all visible rows must report their
    minimum/natural width - and the allocated width should be used to
    request the heights of each visible row.

  o The remaining (invisible) rows must have their widths requested
    asyncrhonously, for each row:

      o The overall cached treeview minimum/natural width/height needs to
        be updated

  o Once we have requested all of the rows which we intend to request
    width for, we can finally know that we have an allocated width
    which can satisfy the request of our treeview - we receive an
    allocation.

  o When we receive an allocation and we know that all of our rows
    have been requested, now we need to start requesting height for
    those invisible rows we are interested in. (currently the treeview
    requests every row, but this should ideally be only one out of
    every 1000 rows or such, for a treeview with many millions of rows).

  o When the content changes (i.e. the text which would be rendered
    by a potentially wrapping text cell renderer), we need to restart
    the process depending on some conditions (such as grow-only and
    fixed height mode where we can make some optimizations).

  o It is possible that the content changes before we have even finished
    requesting horizontally, we still need to complete the process of
    requesting horizontally before we can start to consider requesting
    row heights.


diff --git a/gtk/gtktreeview.c b/gtk/gtktreeview.c
index 73c37c1..6672e61 100644
--- a/gtk/gtktreeview.c
+++ b/gtk/gtktreeview.c
@@ -2485,15 +2485,17 @@ gtk_tree_view_update_height (GtkTreeView *tree_view)
 
   for (list = tree_view->priv->columns; list; list = list->next)
     {
-      GtkRequisition     requisition;
+      GtkRequisition     minimum, natural;
       GtkTreeViewColumn *column = list->data;
       GtkWidget         *button = gtk_tree_view_column_get_button (column);
 
       if (button == NULL)
         continue;
 
-      gtk_widget_get_preferred_size (button, &requisition, NULL);
-      tree_view->priv->header_height = MAX (tree_view->priv->header_height, requisition.height);
+      gtk_widget_get_preferred_size (button, &minimum, &natural);
+      tree_view->priv->header_height = MAX (tree_view->priv->header_height,
+                                            tree_view->priv->vscroll_policy == GTK_SCROLL_MINIMUM
+                                            ? minimum.height : natural.height);

This part captures the natural height of the button, whereas before it would only capture the minimum.

It still does not do any height-for-width, however.
 
@@ -2682,7 +2684,9 @@ gtk_tree_view_size_allocate_columns (GtkWidget *widget,
   natural_width = tree_view->priv->natural_width;
   n_expand_columns = tree_view->priv->n_expand_columns;
 
-  width = MAX (widget_allocation.width, minimum_width);
+  width = MAX (widget_allocation.width,
+               tree_view->priv->hscroll_policy == GTK_SCROLL_MINIMUM
+               ? minimum_width : natural_width);

Here we are ensuring that the treeview bin window (as opposed to the
actual treeview allocation) will be at least 'natural_width' wide
in the case of GTK_SCROLL_NATURAL... the intent is correct, however
it will result in the unwrapping of text cell renderers - since the
treeview does not properly re-request for all rows asynchronously when
an allocated width change, this will result in inconsistencies in the
reported treeview height request (and probably some inconsistencies
in the stored row heights in the RBTree).

 
@@ -6602,18 +6606,22 @@ validate_visible_area (GtkTreeView *tree_view)
   /* update width/height and queue a resize */
   if (size_changed)
     {
-      GtkRequisition requisition;
+      GtkRequisition minimum, natural;
 
       /* We temporarily guess a size, under the assumption that it will be the
        * same when we get our next size_allocate.  If we don't do this, we'll be
        * in an inconsistent state if we call top_row_to_dy. */
 
       gtk_widget_get_preferred_size (GTK_WIDGET (tree_view),
-                                     &requisition, NULL);
+                                     &minimum, &natural);
       gtk_adjustment_set_upper (tree_view->priv->hadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment), requisition.width));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment),
+                                     tree_view->priv->hscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.width : natural.width));
       gtk_adjustment_set_upper (tree_view->priv->vadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment), requisition.height));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment),
+                                     tree_view->priv->vscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.height : natural.height));
       gtk_widget_queue_resize (GTK_WIDGET (tree_view));
     }

Here we do much the same as when allocating columns, withing the context of requesting
the size syncrhonously for the visible area of the treeview.

Again the intent looks correct, is the natural height also taken into accound while
asyncrhonously requesting row heights for the 1,000,000 rows that are not currently
visible ?

 
@@ -6809,8 +6817,7 @@ do_validate_rows (GtkTreeView *tree_view, gboolean queue_resize)
  done:
   if (validated_area)
     {
-      GtkRequisition requisition;
-      gint dummy;
+      GtkRequisition minimum, natural;
 
       /* We temporarily guess a size, under the assumption that it will be the
        * same when we get our next size_allocate.  If we don't do this, we'll be
@@ -6827,8 +6834,8 @@ do_validate_rows (GtkTreeView *tree_view, gboolean queue_resize)
        * untill we've recieved an allocation (never update scroll adjustments from size-requests).
        */
       prevent_recursion_hack = TRUE;
-      gtk_tree_view_get_preferred_width (GTK_WIDGET (tree_view), &requisition.width, &dummy);
-      gtk_tree_view_get_preferred_height (GTK_WIDGET (tree_view), &requisition.height, &dummy);
+      gtk_tree_view_get_preferred_width (GTK_WIDGET (tree_view), &minimum.width, &natural.width);
+      gtk_tree_view_get_preferred_height (GTK_WIDGET (tree_view), &minimum.height, &natural.height);
       prevent_recursion_hack = FALSE;
 
       /* If rows above the current position have changed height, this has
@@ -6838,9 +6845,13 @@ do_validate_rows (GtkTreeView *tree_view, gboolean queue_resize)
         gtk_widget_queue_draw (GTK_WIDGET (tree_view));
 
       gtk_adjustment_set_upper (tree_view->priv->hadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment), requisition.width));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment),
+                                     tree_view->priv->hscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.width : natural.width));
       gtk_adjustment_set_upper (tree_view->priv->vadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment), requisition.height));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment),
+                                     tree_view->priv->vscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.height : natural.height));


Yes to the above question... we have the same treatment in the process of
requesting row sizes asyncrhonously.


@@ -6870,15 +6881,19 @@ do_presize_handler (GtkTreeView *tree_view)
 
   if (tree_view->priv->fixed_height_mode)
     {
-      GtkRequisition requisition;
+      GtkRequisition minimum, natural;
 
       gtk_widget_get_preferred_size (GTK_WIDGET (tree_view),
-                                     &requisition, NULL);
+                                     &minimum, &natural);
 
       gtk_adjustment_set_upper (tree_view->priv->hadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment), requisition.width));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->hadjustment),
+                                     tree_view->priv->hscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.width : natural.width));
       gtk_adjustment_set_upper (tree_view->priv->vadjustment,
-                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment), requisition.height));
+                                MAX (gtk_adjustment_get_upper (tree_view->priv->vadjustment),
+                                     tree_view->priv->vscroll_policy == GTK_SCROLL_MINIMUM
+                                     ? minimum.height : natural.height));
       gtk_widget_queue_resize (GTK_WIDGET (tree_view));
     }
                   
And in the presize handler (the first size request while re-requesting the whole treeview), we've considered
the natural width in the equasion for fixed height mode (this basically 'bumps' the current adjustment values
to be at least as big as they are, it acts as a grow-only functionality unless before re-validating all rows
we've reset the internal width/height variables to 0).


I think this patch needs work, certainly all of the treeview tests in the ./tests directly should be
run and manually compared with a build without this patch applied (tests such as testtreeedit are
important to ensure we get all the pixels in the right places).

My primary worry about this is that it enables height-for-width functionality in cell renderers
implicitly by allocating cells/columns sizes which are greater than their minimum requests, this
means that row heights for those contextual widths will be cached in the RBTree and reused later,
potentially after a content change or a reallocation of the treeview - when those precached values
might not be correct anymore.

Comparisons should be made with treeviews that only use the vertical scrollbar and not the
horizontal scrollbar, ideally inside a GtkPaned as this allows one to resize the treeview
manually and watch the text renderers rewrap into the newly allocated width (of course these
tests should be made with wrapping cell renderers).

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/485.