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

I thought I'd just add that this bug is still there with dynamic workspaces on as well - the ThumbnailsBox does not slide out as far as it should the more workspaces you have, until eventually it does not slide out at all (whereas if dynamic workspaces is off, the ThumbnailsBox is always slid out, and again it does not slide out as much the more workspaces you have).

Created attachment 245157 [details] [review]
workspaceThumbnail: Fix erroneous allocation with many workspaces

The current code assumes that the thumbnail scale factor matches
the maximum thumbnail scale factor, which is not the case when
using many workspaces; use the real factor in that case.

Created attachment 245158 [details] [review]
overviewControls: Fix missing offset in allocation

The SliderLayout's allocate function assumes an x1 coordinate of 0
when calculating the x2 coordinate - fix that.

Review of attachment 245157 [details] [review]:

OK.

Created attachment 245162 [details] [review]
overviewControls: Fix offset for SlideLayouts on the left

Controls on the left side should be left aligned, while the current
code aligns them on the right. No one noticed, because the code
happens to work when the child fills the available width, and the
only case where that's not the case (workspace switcher in RTL locales
with many workspaces) was broken in other ways covering up this issue ...

Review of attachment 245162 [details] [review]:

OK.

Review of attachment 245158 [details] [review]:

OK.

Comment on attachment 245157 [details] [review]
workspaceThumbnail: Fix erroneous allocation with many workspaces

this._scale is reset each time the overview is shown, so the patch has some update issues when the switcher is expected to be partly-hidden.

Comment on attachment 245158 [details] [review]
overviewControls: Fix missing offset in allocation

This breaks the partly-hidden switcher horribly when using dynamic workspaces - I'll need to look into this more.

Comment on attachment 245162 [details] [review]
overviewControls: Fix offset for SlideLayouts on the left

This breaks the partly-hidden switcher horribly when using dynamic workspaces - I'll need to look into this more.

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

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

Have you looked more into this, Florian?

Yes, but so far I've only come up with a hack (hardcode scale to MAX_THUMBNAIL_SCALE in workspaceThumbnail.js:_getPreferredWidth()) to get it working in all circumstances.

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

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

(In reply to comment #15)
> Yes, but so far I've only come up with a hack (hardcode scale to
> MAX_THUMBNAIL_SCALE in workspaceThumbnail.js:_getPreferredWidth()) to get it
> working in all circumstances.

I didn't look at the code but if it "works in all circumstances" why don't we just use a fixed value there?

Created attachment 257074 [details] [review]
overviewControls: Correct the use of x2 in SlidingControl

The x2 here needs to be more than just the width; it needs to
be added onto the x1.

Created attachment 257075 [details] [review]
overviewControls: Clarify some code with a comment

translationX is sort of a bad name, since it confuses with the
actor's translation, which is used for sliding without allocation.

Created attachment 257076 [details] [review]
overviewControls: Don't try to align something sliding to the right

0 is already the correct value.

Created attachment 257077 [details] [review]
overviewControls: Don't use the child's preferred size to slide from

In order for the workspace thumbnails box to have the correct size,
we need to constrain the width of the thumbnails box to the height we're
given, instead of assuming an unlimited height.

Created attachment 257078 [details] [review]
workspaceThumbnail: Drop the _background hack

During the 3.6 cycle when we added the new search view, Cosimo
changed the way animating the windows in the overview works so
that rather than animate to the final position, we actually do
adjust the allocation of the parent a lot.

As the hack is no longer necessary, drop it.

I tested these patches on LTR and RTL layouts, in all scenarios I could think of, and it works for me. Please test. If it doesn't work for someone, please let me know.

I think the fix would be worth it for 3.10.1.

Review of attachment 257074 [details] [review]:

::: js/ui/overviewControls.js
@@ +68,3 @@
+        actorBox.x2 = actorBox.x1 + child.x_expand ? availWidth : natWidth;
+        actorBox.y1 = 0;
+        actorBox.y2 = actorBox.y1 + child.y_expand ? availHeight : natHeight;

Why the style change?
It's correct anyway

Review of attachment 257075 [details] [review]:

Ok

Review of attachment 257076 [details] [review]:

Yes

Review of attachment 257077 [details] [review]:

Does this change the behavior of the dash?

Review of attachment 257078 [details] [review]:

Why the hack is no longer necessary?

(In reply to comment #25)
> Why the style change?
> It's correct anyway

It's the style we use everywhere else, and I like that style more.

(In reply to comment #28)
> Does this change the behavior of the dash?

Not as far as I can tell, but more testing is always appreciated.

The Dash was already a vertical BoxLayout, so it already used a WIDTH_FOR_HEIGHT request mode, so this should be mostly equivalent.

(In reply to comment #29)
> Why the hack is no longer necessary?

I explain it in the commit message, but it changed here: https://git.gnome.org/browse/gnome-shell/commit/?id=3d8a87563d218c2d9e17b1898452c28f622feaa3

Beforehand, the workspaces and workspace thumbnails were in the viewSelector directly, as sort of a "split pane", and when we expanded the thumbnails out, we set the allocations directly to the final, told the fake window clones to animate, and used the background hack to look like it was expanding when it really had the full allocation by default.

Now that we use the SlidingControl framework, we actually do set the allocation at every step, so this is superfluous. Additionally, this code complicates how the allocation is interpreted, and caused the background to fly off in various directions while the thumbnails box allocation looked correct.

Review of attachment 257077 [details] [review]:

Ok, can't see regressions in testing.

Review of attachment 257078 [details] [review]:

It's not clear that the first paragraph of the commit message is why the hack is not necessary and not the hack itself, but the code is correct.

Attachment 257074 [details] pushed as 40cd92f - overviewControls: Correct the use of x2 in SlidingControl
Attachment 257075 [details] pushed as 3e6c8e6 - overviewControls: Clarify some code with a comment
Attachment 257076 [details] pushed as 58a8845 - overviewControls: Don't try to align something sliding to the right
Attachment 257077 [details] pushed as 89b9d07 - overviewControls: Don't use the child's preferred size to slide from
Attachment 257078 [details] pushed as ad043e0 - workspaceThumbnail: Drop the _background hack