probably another "we were accidentally depending on clutter queuing a redraw that it didn't need to" bug

poked at this a bit... it's not "needs a queue_redraw". The actor is painting, it's just painting nothing for some reason.

*** Bug 644274 has been marked as a duplicate of this bug. ***

OK, I poked around a little and I got a vague idea what's going on - I should be able to come up with a patch later ...

Created attachment 184577 [details] [review]
app-menu: Update clip on icon size changes

To keep the app icon from overlapping the panel's (border-image)
border, a custom property for clipping the app menu icon's bottom
was introduced. But if the clip region is set before the initial
icon is set, the entire actor ends up clipped. Also the double
meaning of clutter_actor_get_height() (e.g. preferred height versus
allocated height), the clip region may end up too large and the icon
overlaps the panel's border-image.
Fix both problems by updating the clip region on size changes as
well, rather than on style changes only.

*** Bug 646760 has been marked as a duplicate of this bug. ***

Review of attachment 184577 [details] [review]:

It looks like an improvement to me, but maybe only half a fix?

::: js/ui/panel.js
@@ +347,3 @@
             this._iconBox.set_clip(0, 0,
                                    this._iconBox.width,
+                                   this._iconBox.height - this._iconBottomClip);

the width seems to come into this as well? Maybe switch to using notify::allocation? (and get_allocation_box() if you want to avoid the breakage that is the clutter width/height properties, which when a resize is queued magically change to the size request, then change back to the allocation once the actor is allocate.)

Created attachment 186223 [details] [review]
app-menu: Update clip on icon size changes

(In reply to comment #7)
> the width seems to come into this as well? Maybe switch to using
> notify::allocation? (and get_allocation_box() if you want to avoid the breakage
> that is the clutter width/height properties, which when a resize is queued
> magically change to the size request, then change back to the allocation once
> the actor is allocate.)

Yeah - the width shouldn't change at all, but using the actual allocation rather than whatever clutter reports as height at a given moment seems like a good idea anyway ...

Review of attachment 186223 [details] [review]:

Sentence in the message body no verb.

 "Also the double
  meaning of clutter_actor_get_height() (e.g. preferred height versus
  allocated height), the clip region may end up too large and the icon
  overlaps the panel's border-image."

::: js/ui/panel.js
@@ +345,3 @@
+    _updateIconBoxClip: function() {
+        let allocation = this._iconBox.get_allocation_box();
+        if (allocation && this._iconBottomClip > 0)

result of allocation() is always a rectangle, so check is a little over-conservative.

One thing that's now bothering me is that get_allocation_box() actually synchronously forces a stage layout whenever we get the style-changed signal, which I think is *before* we are supposed to allocate the stage on initial creation. There's no way I know of to just say "give me the allocation to the best of your knowledge and tell me when it changes later". 

Ugh.

So, maybe we need to go back to your version - width and ::height which have their own issues, but don't cause an extra allocation to occur at a weird time on shell startup.

Created attachment 186601 [details] [review]
app-menu: Update clip on icon size changes

(In reply to comment #9)
> So, maybe we need to go back to your version - width and ::height which have
> their own issues, but don't cause an extra allocation to occur at a weird time
> on shell startup.

*Sigh*

Re-attaching previous patch version (with an additional signal connection to notify::width) ...

(In reply to comment #9)
> Review of attachment 186223 [details] [review]:
> 
> Sentence in the message body no verb.

Woops, still present in the re-attached version ...


>  "Also the double
>   meaning of clutter_actor_get_height()

Changed to:
"Also due to the double meaning ..."

(In reply to comment #9)
> One thing that's now bothering me is that get_allocation_box() actually
> synchronously forces a stage layout whenever we get the style-changed signal,
> which I think is *before* we are supposed to allocate the stage on initial
> creation. There's no way I know of to just say "give me the allocation to the
> best of your knowledge and tell me when it changes later".

Hmm, on 2nd thought - the allocation property doesn't trigger a stage relayout (but may not be set, so over-conservative check would be necessary then) ... maybe worth using that instead?

(In reply to comment #12)
> (In reply to comment #9)
> > One thing that's now bothering me is that get_allocation_box() actually
> > synchronously forces a stage layout whenever we get the style-changed signal,
> > which I think is *before* we are supposed to allocate the stage on initial
> > creation. There's no way I know of to just say "give me the allocation to the
> > best of your knowledge and tell me when it changes later".
> 
> Hmm, on 2nd thought - the allocation property doesn't trigger a stage relayout
> (but may not be set, so over-conservative check would be necessary then) ...
> maybe worth using that instead?

Ah, that would work. It actually is always set though:

    case PROP_ALLOCATION:
      g_value_set_boxed (value, &priv->allocation);
      break;

&priv->allocation can never be NULL.

Created attachment 186615 [details] [review]
app-menu: Update clip on icon size changes

(In reply to comment #13)
> Ah, that would work. It actually is always set though:

Oh, right - I removed the check.

Review of attachment 186615 [details] [review]:

Looks good.

::: js/ui/panel.js
@@ +348,3 @@
             this._iconBox.set_clip(0, 0,
+                                   allocation.x2 - allocation.x1,
+                                   allocation.y2 - allocation.y1 - this._iconBottomClip);

This could set a clip with negative width/height, which in my reading of clutter which seems to be allowed but have undefined semantics (depending on the exact code paths followed it can do differnet things). But I think it's OK since we should never have a negative rectangle when we're actually painting.

Attachment 186615 [details] pushed as d97657b - app-menu: Update clip on icon size changes

(In reply to comment #15)
> This could set a clip with negative width/height, which in my reading of
> clutter which seems to be allowed but have undefined semantics (depending on
> the exact code paths followed it can do differnet things). But I think it's OK
> since we should never have a negative rectangle when we're actually painting.

Right. We used to get a negative height with the original code, but now it should only happen if we fail to get an icon (in which case I suppose we don't really care about the icon's clip anyway).