Created attachment 162571 [details] [review]
[workspaceSwitcher] Constrain to monitor size

Currently the workspaceSwitcher does not take the screen size into account
which could result into overflowing the screen.

Fix that but using Shell.GenericContainer instead of St.BoxLayout which takes
the monitor size into account when allocating.

ctrl+alt+<arrows> finall works fine after applying the patch. thanks!

Review of attachment 162571 [details] [review]:

::: js/ui/workspaceSwitcherPopup.js
@@ +34,3 @@
         this._container = new St.BoxLayout({ style_class: 'workspace-switcher-container' });
+        this._list = new Shell.GenericContainer({ style_class: 'workspace-switcher' });
+        this._list.spacing = 0;

better to make spacing just a property on the JS object (this._listSpacing) rather than an "expando" property on the GObject - if ClutterActor or ShellGenericContainer added a property called 'spacing', this would cause very unexpected side effects.

(When we do need an expando property, we use an underscore prefix)

@@ +69,3 @@
+        }
+
+        width += this._list.spacing * (global.screen.n_workspaces - 1) - this._list.get_theme_node().get_vertical_padding();

I don't understand why you subtract of the vertical padding here and the horizontal padding below. (Either the subtract or the horz/vert flip). If it's right, it very much needs a comment.

@@ +72,3 @@
+
+        if (width > availwidth) {
+            this._scaleWidth *= availwidth / width;

By modifying this._scaleWidth, I think you aren't going to handle the case where the workspace count drops. (Probably what you should do is use _scaleWidth/_scaleHeight only from getPreferredWidth() to the the other allocation functions and move the initialization of the scale to the screen/monitor ratio to here)

@@ +74,3 @@
+            this._scaleWidth *= availwidth / width;
+            if (this._scaleHeight == 1)
+                this._scaleHeight = availwidth / width;

Looks like scaleHeight would stick at the first value other than 1?

@@ +90,3 @@
+            if (childMinHeight < childHeight)
+                continue;
+            childHeight = childMinHeight;

Much clearer as the reverse:

 if (childMinHeight > childHeight)
     childHeight = childMinHeight;

Or:

 childHeight= Math.max(childHeight, childMinHeight);

@@ +102,3 @@
+        let childBox = new Clutter.ActorBox();
+
+        let x = 0;

You need to start at box.x1/box.y1 not 0,0 to handle padding/etc. correctly

@@ +108,3 @@
+            childBox.x2 = x + childNaturalHeight * this._scaleWidth;
+            childBox.y1 = 0;
+            let [found, border] = children[i].get_theme_node().get_length('border', false);

'border' isn't a length - it's a complicated composite property. you would need get_border_width(<side>). But why do we care about the border of the child? - the child will manage its own border?

@@ +137,3 @@
     _position: function() {
         let primary = global.get_primary_monitor();
+        this._scaleWidth = global.screen_width / global.screen_height;

Do we really want the workspace indicators to be screen-shaped - maybe it would be better to make them primary-monitor-shaped?

(In reply to comment #3)
> Review of attachment 162571 [details] [review]:
> 
> ::: js/ui/workspaceSwitcherPopup.js
> @@ +34,3 @@
>          this._container = new St.BoxLayout({ style_class:
> 'workspace-switcher-container' });
> +        this._list = new Shell.GenericContainer({ style_class:
> 'workspace-switcher' });
> +        this._list.spacing = 0;
> 
> better to make spacing just a property on the JS object (this._listSpacing)
> rather than an "expando" property on the GObject - if ClutterActor or
> ShellGenericContainer added a property called 'spacing', this would cause very
> unexpected side effects.

OK, that makes sense.

> (When we do need an expando property, we use an underscore prefix)
>
> @@ +69,3 @@
> +        }
> +
> +        width += this._list.spacing * (global.screen.n_workspaces - 1) -
> this._list.get_theme_node().get_vertical_padding();
> 
> I don't understand why you subtract of the vertical padding here and the
> horizontal padding below. (Either the subtract or the horz/vert flip). If it's
> right, it very much needs a comment.

It was a workaround for a bug I added but didn't notice, see below.
 
> @@ +72,3 @@
> +
> +        if (width > availwidth) {
> +            this._scaleWidth *= availwidth / width;
> 
> By modifying this._scaleWidth, I think you aren't going to handle the case
> where the workspace count drops. (Probably what you should do is use
> _scaleWidth/_scaleHeight only from getPreferredWidth() to the the other
> allocation functions and move the initialization of the scale to the
> screen/monitor ratio to here)

What do you mean by that?
It does not handle on the fly workspace changes (i.e while you switch).
So "dropping workspace count" should be an non issue here.

> @@ +74,3 @@
> +            this._scaleWidth *= availwidth / width;
> +            if (this._scaleHeight == 1)
> +                this._scaleHeight = availwidth / width;
> 
> Looks like scaleHeight would stick at the first value other than 1?

Yes.

> @@ +90,3 @@
> +            if (childMinHeight < childHeight)
> +                continue;
> +            childHeight = childMinHeight;
> 
> Much clearer as the reverse:
> 
>  if (childMinHeight > childHeight)
>      childHeight = childMinHeight;
> 
> Or:
> 
>  childHeight= Math.max(childHeight, childMinHeight);

Yeah that makes sense.

> @@ +102,3 @@
> +        let childBox = new Clutter.ActorBox();
> +
> +        let x = 0;
> 
> You need to start at box.x1/box.y1 not 0,0 to handle padding/etc. correctly

Yeah that allows me to get rid of the "subtract the paddings" hack.

> @@ +108,3 @@
> +            childBox.x2 = x + childNaturalHeight * this._scaleWidth;
> +            childBox.y1 = 0;
> +            let [found, border] =
> children[i].get_theme_node().get_length('border', false);
> 
> 'border' isn't a length - it's a complicated composite property. you would need
> get_border_width(<side>). But why do we care about the border of the child? -
> the child will manage its own border?

It isn't needed it can be fixed in css (which I did now).

> @@ +137,3 @@
>      _position: function() {
>          let primary = global.get_primary_monitor();
> +        this._scaleWidth = global.screen_width / global.screen_height;
> 
> Do we really want the workspace indicators to be screen-shaped - maybe it would
> be better to make them primary-monitor-shaped?

Yeah Jon said that we want monitors as units and not screens so that makes sense.

Created attachment 162670 [details] [review]
[workspaceSwitcher] Constrain to monitor size

*) Made spacing a property of the JS object rather than _list
*) Don't subtract padding and start by box.x1/y1 in allocate rather than 0,0
*) Remove border hack and fix the CSS (96 + border-width=2 should be 100 not 98)
*) Use the monitor as unit rather than the screen
*) Cleanups

Created attachment 162685 [details] [review]
[workspaceSwitcher] Constrain to monitor size

*) Fix small bug

Review of attachment 162685 [details] [review]:

still not happy with the handling of scaleWidth/scaleHeight - it produces the right result, but it's not transparently correct.

::: js/ui/workspaceSwitcherPopup.js
@@ +74,3 @@
+            this._scaleWidth *= availwidth / width;
+            if (this._scaleHeight == 1)
+                this._scaleHeight = availwidth / width;

If scaleHeight wasn't 1 here, that meant that _getPreferredWidth was called twice with different conditions - different number of workspaces, or different primary monitor geometry - without _position() being called in between. So either this check is unnecessary, or we have the problem I pointed out with scaleWidth gettig "stuck" at a too small value. 

The whole handling of scaleWidth and scaleHeight is just confusing - this is why I'm suggesting to initialize them to primary monitor aspect ratio / 1 in *this* function rather than in the _init() and in _position()

I think it would also be clearer rather than to have scaleWidth and scaleHeight have childAspect and childScale - so compared to the current scaleWidth = cihldScale * childAspect; scaleHeight = childAspect.

@@ +106,3 @@
+            childBox.x2 = x + childNaturalHeight * this._scaleWidth;
+            childBox.y1 = box.y1;
+            childBox.y2 = box.y1 + childNaturalHeight * this._scaleHeight;

I think it would be good to round the child position to an integer here - I don't know if we draw rounded rectangles correctly at non-integer positions - the corner pieces may get fuzzy. The simple approach is to just round here when assigning to childBox (but leave x unrounded. If you round the widths you add into x, then the cumulative error will show up at the right edge of the screen.)

Make sure to see if the spacing between the children looks sufficiently regular once you get into the overflow condition: with the rounding of the children positions, you'll probably get spacings like 6,6,7,6,6,6,7,6,6,7.

If it doesn't look sufficiently regular, then the fix is to compute:

 spacing = Math.round(this._childScale * this._childSpacing);

and then compute childBox.x2 as above (round(x + childNaturalHeight * this._scaleWidth), but compute childBox.x1 as:

 prevChildX1 + spacing.

So in other words, we move the irregularity from the spacings into the children - since the children are considerably wider than the spacing, it's less visible to the naked eye.

Created attachment 162762 [details] [review]
[workspaceSwitcher] Constrain to monitor size

*) Get rid of _scaleWidth/Height, as a result calculations should be much simpler now.
*) Align at integer positions

Review of attachment 162762 [details] [review]:

::: js/ui/workspaceSwitcherPopup.js
@@ +64,3 @@
+        let width = 0;
+        for (let i = 0; i < children.length; i++) {
+            let [childMinHeight, childNaturalHeight] = children[i].get_preferred_height(-1);

The contract for Clutter actors isn't precisely defined. But I would consider it a good form to make sure that when the parent calls getPreferredWidth() then getPreferredHeight(), you call both get_preferred_width() and get_preferred_height() on the child, in either height-for-width or width-for-height order.

To keep things standard, I would replace this with:

  let [childMinWidth, childNaturalWidth] =  children[i].get_preferred_width(-1);
  let [childMinHeight, childNaturalHeight] = children[i].get_preferred_height(childNaturalWidth);

If that doesn't work for some reason, you can alternately do:

  children[i].get_preferred_width(-1); // Ignore result
  let [childMinHeight, childNaturalHeight] children[i].get_preferred_height(-1);

@@ +92,3 @@
+        let x = box.x1;
+        for (let i = 0; i < children.length; i++) {
+            childBox.x1 = x;

This isn't an integer. 

Simple:

 childBox.x1 = Math.round(x);

Complex, to get an even spacing of itemSpacing:

    x = box.x1;
    prevChildBoxX1 = box.x1 - this._itemSpacing;

    for (<child>) {
         childBox.x1 = prevChildBoxX2 + this._itemSpacing;
         childBox.x2 = Math.round(x + this._childWidth);
         ....
         x += this._childWidth + this._itemSpacing;
         prevChildBoxX1 = childBox.x1;
    }

(In reply to comment #9)
> Review of attachment 162762 [details] [review]:
> 
> ::: js/ui/workspaceSwitcherPopup.js
> @@ +64,3 @@
> +        let width = 0;
> +        for (let i = 0; i < children.length; i++) {
> +            let [childMinHeight, childNaturalHeight] =
> children[i].get_preferred_height(-1);
> 
> The contract for Clutter actors isn't precisely defined. But I would consider
> it a good form to make sure that when the parent calls getPreferredWidth() then
> getPreferredHeight(), you call both get_preferred_width() and
> get_preferred_height() on the child, in either height-for-width or
> width-for-height order.
> 
> To keep things standard, I would replace this with:
> 
>   let [childMinWidth, childNaturalWidth] = 
> children[i].get_preferred_width(-1);
>   let [childMinHeight, childNaturalHeight] =
> children[i].get_preferred_height(childNaturalWidth);

OK

> 
> @@ +92,3 @@
> +        let x = box.x1;
> +        for (let i = 0; i < children.length; i++) {
> +            childBox.x1 = x;
> 
> This isn't an integer. 

Missed that one.

> Simple:
> 
>  childBox.x1 = Math.round(x);
> 
> Complex, to get an even spacing of itemSpacing:
> 
>     x = box.x1;
>     prevChildBoxX1 = box.x1 - this._itemSpacing;
> 
>     for (<child>) {
>          childBox.x1 = prevChildBoxX2 + this._itemSpacing;
>          childBox.x2 = Math.round(x + this._childWidth);
>          ....
>          x += this._childWidth + this._itemSpacing;
>          prevChildBoxX1 = childBox.x1;
>     }

OK, this looks better assuming you mean prevChildBoxX*2* otherwise it makes zero sense to me.

Created attachment 163119 [details] [review]
[workspaceSwitcher] Constrain to monitor size

*) Address review comments

Review of attachment 163119 [details] [review]:

Looks good, sorry about the X1/X2 confusion :-)

Attachment 163119 [details] pushed as 3fe7b13 - [workspaceSwitcher] Constrain to monitor size