Created attachment 137818 [details] [review]
Add partial implementation of dynamic width/height layout to TidyGrid

This is not a complete patch; it doesn't attempt to handle the homogenous
property or column major.

Created attachment 137819 [details] [review]
Switch to dynamic layout for Dash

Instead of laying out the dash contents "manually" and having
the content items explicitly passed their height, just give them
a set width.

(In reply to comment #1)
> Created an attachment (id=137818) [edit]
> Add partial implementation of dynamic width/height layout to TidyGrid
> 
> This is not a complete patch; it doesn't attempt to handle the homogenous
> property or column major.

      if (iter->next) natural_width += priv->column_gap;

Missing newline.

+  g_printerr ("%p req nat width %f (from height %f)\n", self, (double) natural_width, for_height);

Leftover printerr.

   if (min_width_p)
-    *min_width_p = natural_width;
+    *min_width_p = 0;

min_width should actually be the min of the min-widths of the children, right?

+  if (priv->column_major)
+    gap = priv->row_gap;
+  else
+    gap = priv->column_gap;
+

That doesn't seem to be right (even given the fact that you are not handling column_major) - if you were column-major, then the logic would be in width-for-height. And you never use 'gap'

      gboolean wrapped;

Shadowed variable

      if (iter == priv->list)
	{
          current_natural_width = child_natural_w;
          natural_height = row_height;
        }

Assigning the natural_height here is clumsy - better to do it after the last item (then you don't need the if (!wrapped), and you don't double the natural height for a single-item grid) Then the need for 'wrapped' falls out entirely.

+          if (iter->next)
+            natural_height += priv->row_gap;

I don't think the iter->next check is right - we are wrapping the *current* item onto the next line, so we definitely want the row_gap.

+          g_printerr ("wrap, row: %f\n", row_height);

Another leftover.

+      if (!wrapped && iter->next)
+        current_natural_width += priv->column_gap;

Doesn't look right to me. I interpret it as:

  "if we're not the first item on the line, except for the first item
  on the first line", and there is a following item, which may or
  maybe not be on this same line, add a column gap."

I think instead you want:

      else
        {
-          current_natural_width += child_natural_w;
+          current_natural_width += child_natural_w + priv->column_gap;
        }

Will attach a version fixed up for the above comments.

Created attachment 137829 [details] [review]
Add partial implementation of dynamic width/height layout to TidyGrid

This is not a complete patch; it doesn't attempt to handle the homogenous
property or column major.

(Based on patch by Colin Walters <walters@verbum.org>)

(In reply to comment #2)
> Created an attachment (id=137819) [edit]
> Switch to dynamic layout for Dash
> 
> Instead of laying out the dash contents "manually" and having
> the content items explicitly passed their height, just give them
> a set width. 

+        // If it's empty, we need to provide a minimum drop target
+        if (running.length == 0)
+          this._runningArea.actor.set_size(this._width, 50);
+        else
+          this._runningArea.actor.set_size(-1, -1);

This just looks strange to me. If there are no extra running items, the area should be zero-size and the separator hidden.

         this._appsSection.append(this._appsText, Big.BoxPackFlags.EXPAND);

-        this._docsSection.append(this._docsText, Big.BoxPackFlags.EXPAND);
+        this._docsSection.append(this._docsText, Big.BoxPackFlags.NONE);

Doesn't matter which, but would be good to be consistent.

There reason why there is still a gap under the whole thing is that you needed:

-        this._docsSection.append(moreDocsBox, Big.BoxPackFlags.EXPAND);
+        this._docsSection.append(moreDocsBox, Big.BoxPackFlags.NONE);

I'll attach follow-on patches that fix those issues. Other than that looks reasonable to me. I think there are now more variables and computations that can be removed. E.g. Dash._itemDisplayHeight is unused.

Created attachment 137830 [details] [review]
Hide the running-apps area when empty

It looks funny to have the "more running apps area" there as a gap
when empty and dragging to it is an unintuitive way to remove stuff
from the favorites list, in any case, so just hide the area when
empty.

Created attachment 137831 [details] [review]
Fix expand flags for the contents of the Dash

Pack everything we don't want to expand with BigBoxPackFlags.NONE;
this fixes the More... button for the docs section ending up with
a gap underneath it.

Since the More... button for the docs now ends is right at the bottom
of the dash, add some padding to it.

Created attachment 137832 [details] [review]
Hide the running-apps area when empty

This version actually works (was showing the wrong thing)