Created attachment 152892 [details] [review]
Fix resizing problems with DashDocDisplay

- Handle non-uniform child heights properly - use a constant
  grid size as the maximum of all child heights; with the
  previous code, the children might not line up in the two
  columns and the last item could be lost if the second column
  was taller than the first column.

- Call set_skip_paint(child, false) on children that we do
  want visible to override any previous hiding of that child.

- Correctly handle the DashDocDisplay not being allocated at 0, 0;
  children should be allocated starting at 0, 0, not at origin
  of the allocation box passed in.

The problem here was that due to a combination of problems, various recent documents were getting tagged with "skip" paint.

 a) we computed sizes wrong so the last item in the second column was often
    chopped off and set to skip paint
 b) we added the items and allocated one by one due to a serious bug in
    StWidget which calls clutter_get_allocation_box() when an actor is
    initially mapped, so each added item triggered a reallocation.
 c) We never cleared the skip paint flag

So this would result say, in every other item getting marked to skip painting. The attached patch fixes a) and c) and thus the symptoms. b) is harder to fix so I'll deal with it separately. The third fix in this patch actually is irrelevant to this bug, it's just something Colin noticed when we were looking at the code.

Comment on attachment 152892 [details] [review]
Fix resizing problems with DashDocDisplay

looks good

>         // Two columns, where we go vertically down first.  So just take
>         // the height of half of the children as our preferred height.

While still technically true, that comment is now sort of misleading. The code is pretty clear. You could just remove the comment.

>+        // properly updated for changes in children. It's not clear
>+        // if this is guaranteed as an invarient of Clutter; does

"invariAnt"

If you're concerned, you could check that this._itemWidth matches the allocation, and assume that this._itemHeight is correct as well if it does (and call getPreferredHeight() with the new width if not, to compute a new this._itemHeight.)


Also, while looking at the existing code, I noticed a useless line:

        let skipPaint = [];

near the end of _allocate() that can be removed.

(In reply to comment #3)
> (From update of attachment 152892 [details] [review])
> looks good
> 
> >         // Two columns, where we go vertically down first.  So just take
> >         // the height of half of the children as our preferred height.
> 
> While still technically true, that comment is now sort of misleading. The code
> is pretty clear. You could just remove the comment.

I rewrote it, then decided that you are right, and when rewritten to make sense it was duplicating the code. So removed it.

> >+        // properly updated for changes in children. It's not clear
> >+        // if this is guaranteed as an invarient of Clutter; does
> 
> "invariAnt"

Fixed.

> If you're concerned, you could check that this._itemWidth matches the
> allocation, and assume that this._itemHeight is correct as well if it does (and
> call getPreferredHeight() with the new width if not, to compute a new
> this._itemHeight.)

I don't think that wuite works out: this._itemHeight isn't just a function of the allocated width - it's also a function of the contents of the list; just because the allocated width hasn't changed doesn't mean that _itemHeight is correct. But actually, calling get_preferred_height is a good idea - 

  if (priv->needs_height_request ||
      priv->request_height_for_width != for_width)

in ClutterActor will cause our code to be called iff. it needs to be called.
(There's some potential for fail for floating-point rounding reasons, but that's not peculiar to this, and would need to be fixed inside clutter.)

So, that reduces the comment to:

+        // Make sure this._itemWidth/Height have been computed, even
+        // if the parent actor didn't check our size before allocating.
+        // (Not clear if that is required or not as a Clutter
+        // invariant; this is safe and cheap because of caching.)
+        actor.get_preferred_height(width);

Where the second two lines of the comment are sort of optional. I checked that caching was actually working as expected, and the added call optimized out.
 
> Also, while looking at the existing code, I noticed a useless line:
> 
>         let skipPaint = [];
> 
> near the end of _allocate() that can be removed.

Removed.

Attachment 152892 [details] pushed as 9afb091 - Fix resizing problems with DashDocDisplay