Created attachment 208217 [details] [review]
main: don't use a BindConstraint for uiGroup

A BindConstraint on the size of uiGroup forces full redraws of the scene.
Instead, implement and use get_preferred_width and get_preferred_height.

Review of attachment 208217 [details] [review]:

<ebassi> drago01: the constraint will update the allocation, but not the preferred size; there may be something asking for the size of the uiGroup prior to the allocation, thus getting the preferred size? no idea
<drago01> ebassi: the lightbox uses a constraint on the uiGroup to sync it's size / position ... wouldn't that cause the allocation of uiGroup to be updated?
<ebassi> yes, that's how constraints work: we query them during clutter_actor_allocate()

Marking as needs-work based on that.

Can you retest whether this is till needed with the new paint_volume handling that landed in master?

See also Bug 673673

Review of attachment 208217 [details] [review]:

We should probably just go with that (it fixes a real issue after all) but add a comment in the code explaining why we can't use a constraint here.

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

drago01, I remember this being fixed with some of the patches with the new St... is that not the case? Also, this is a Clutter bug, is it not?

(In reply to comment #7)
> drago01, I remember this being fixed with some of the patches with the new
> St... is that not the case?

Apparently not Stefano could still reproduce it and it seems it happens for Alex too.

>  Also, this is a Clutter bug, is it not?

Ebassi thinks it isn't.

To add some more weirdness, replacing the signal handlers with this (as drago01 suggested IIRC):

const UiGroup = new Lang.Class({
    Name: 'UiGroup',
    Extends: Shell.GenericContainer,

    vfunc_allocate: function (box, flags) {
        let children = this.get_children();
        for (let i = 0; i < children.length; i++)
            children[i].allocate_preferred_size(flags);
    },

    vfunc_get_preferred_width: function(forHeight) {
        let width = global.stage.width;
        return [width, width];
    },

    vfunc_get_preferred_height: function(forWidth) {
        let height = global.stage.height;
        return [height, height];
    }
});

[...]

uiGroup = new UiGroup();

[...]


also fails to work.

I'm quite sure this is Clutter's fault. CCing ebassi.

(In reply to comment #9)

> also fails to work.

define "fails to work": the paint volume is not respected?

the default get_paint_volume() implementation will only return a valid paint volume *iff* the function pointers for paint() and get_paint_volume() are the ones installed by ClutterActor - to avoid breaking backward compatibility. if any of those two pointers are different, then the default implementation will return FALSE.

just to rule out any default behaviour, override get_paint_volume() with something like:

  vfunc_get_paint_volume : function(paintVolume) {
      let defaultPaintVolume = this.get_default_paint_volume();

      if (defaultPaintVolume) {
          paintVolume.set_origin(defaultPaintVolume.get_origin());
          paintVolume.set_width(defaultPaintVolume.get_width());
          paintVolume.set_height(defaultPaintVolume.get_height());
          paintVolume.set_depth(defaultPaintVolume.get_depth());
          return true;
      }
      else {
          return false;
      }
  },

and see if the clipped redraws work.

(In reply to comment #11)
> (In reply to comment #9)
> 
> define "fails to work"

1. $ CLUTTER_PAINT=redraws gnome-shell --replace
2. Go to overview and drag a window around
=> every actor has a green border

This happens both when using the bind constraint and when overriding the vfuncs (event with vfunc_get_paint_volume). OTOH, connecting the signal handlers as in the patch I see a blue border around most actors, except the dragged one (green border + slightly bigger red border).

(In reply to comment #9)
> To add some more weirdness, replacing the signal handlers with this (as drago01
> suggested IIRC):
> 
> const UiGroup = new Lang.Class({
>     Name: 'UiGroup',
>     Extends: Shell.GenericContainer,
>     ...
> });

Why are you subclassing Shell.GenericContainer? Just subclass ClutterActor.

(In reply to comment #13)
> (In reply to comment #9)
> > To add some more weirdness, replacing the signal handlers with this (as drago01
> > suggested IIRC):
> > 
> > const UiGroup = new Lang.Class({
> >     Name: 'UiGroup',
> >     Extends: Shell.GenericContainer,
> >     ...
> > });
> 
> Why are you subclassing Shell.GenericContainer? Just subclass ClutterActor.

Because layout.js makes use of the set_skip_paint method and I was lazy to check if this could be avoided somehow.

But actually the subclassing approach works, too, I just forgot to call set_allocation() in vfunc_allocate.

Created attachment 211552 [details] [review]
main: don't use a BindConstraint for uiGroup

A BindConstraint on the size of uiGroup forces full redraws of the scene.
Instead, implement and use get_preferred_width and get_preferred_height.

Unless I"m missing something, set_skip_paint should be the same as show/hide, no?

(In reply to comment #16)
> Unless I"m missing something, set_skip_paint should be the same as show/hide,
> no?

It is used as an override to hide "visible" actors without fiddling with their state, IIRC (useful for example when there is a fullscreen window). So, when in layout.js we have actor.set_skip_paint(false) it doesn't mean that the actor will be painted.

I tried once a naive replacement with show/hide, it broke dash tooltips and something on the message tray (a boxpointer?)

So, after a good two or three hours of tracking this down, I finally realized this was:

http://git.gnome.org/browse/clutter/tree/clutter/clutter-bind-constraint.c#n149

The stage is layed out every frame, which means that the uiGroup gets a relayout every frame as well. We need to track the source's allocation. Unfortunately, naively swapping the "queue-relayout" signal with "allocation-changed" causes "clutter_actor_queue_relayout() called while in allocation cycle" warnings.

Review of attachment 208217 [details] [review]:

Let's go with this for now, until Emmanuele looks into and solves this for real.

Review of attachment 211552 [details] [review]:

I'm not comfortable using the new JS GObject inheritance in a .1 release.

Comment on attachment 208217 [details] [review]
main: don't use a BindConstraint for uiGroup

Attachment 208217 [details] pushed as 8c94a5a - main: don't use a BindConstraint for uiGroup

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.