Created attachment 180468 [details] [review]
workspace-switcher: Switch to vertical orientation

With workspaces now being stacked vertically, the horizontal
indicators in the workspace switcher are rather odd. There are
some designs for an improved workspace switch animation, but
it may take a while to implement them, so for now just change
the orientation of the existing switcher.

Review of attachment 180468 [details] [review]:

Looks good; maybe a bit odd when it overlaps the panel with a lot of workspaces - perhaps it really should be fit into the work area not into the total monitor height - but if we're changing the visuals anyways, doesn't make sense to do the work on that.

(In reply to comment #2)
> Review of attachment 180468 [details] [review]:
> 
> Looks good; maybe a bit odd when it overlaps the panel with a lot of workspaces
> - perhaps it really should be fit into the work area not into the total monitor
> height

Yeah, I actually have that locally, but didn't update the patch here (basically subtracting Main.panel.actor.height from the availHeight in getPreferredHeight). Does that change sound OK or should I attach for another review?

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 180468 [details] [review] [details]:
> > 
> > Looks good; maybe a bit odd when it overlaps the panel with a lot of workspaces
> > - perhaps it really should be fit into the work area not into the total monitor
> > height
> 
> Yeah, I actually have that locally, but didn't update the patch here (basically
> subtracting Main.panel.actor.height from the availHeight in
> getPreferredHeight). Does that change sound OK or should I attach for another
> review?

That sounds OK, but how do you deal with getting it offset downwards by that amount?

Created attachment 180493 [details] [review]
workspace-switcher: Switch to vertical orientation

(In reply to comment #4)
> That sounds OK, but how do you deal with getting it offset downwards by that
> amount?

Right. In _position() I add Main.panel.actor.height/2 to the vertical container position. Anyway, attaching again.

Review of attachment 180493 [details] [review]:

::: js/ui/workspaceSwitcherPopup.js
@@ +127,3 @@
         let primary = global.get_primary_monitor();
         this._container.x = primary.x + Math.floor((primary.width - this._container.width) / 2);
+        this._container.y = primary.y + Math.floor((Main.panel.actor.height + primary.height - this._container.height) / 2);

Don't you want the Main.panel.actor.height outside the / 2?

(In reply to comment #6)
> +        this._container.y = primary.y + Math.floor((Main.panel.actor.height +
> primary.height - this._container.height) / 2);
> 
> Don't you want the Main.panel.actor.height outside the / 2?

Makes sense, but then we'd need to subtract it from primary.height as well:

y = primary.y + panelHeight + (primary.height - panelHeight - containerHeight) / 2

I guess it's a tad bit clearer than the more concise version above ...

(In reply to comment #7)
> (In reply to comment #6)
> > +        this._container.y = primary.y + Math.floor((Main.panel.actor.height +
> > primary.height - this._container.height) / 2);
> > 
> > Don't you want the Main.panel.actor.height outside the / 2?
> 
> Makes sense, but then we'd need to subtract it from primary.height as well:
> 
> y = primary.y + panelHeight + (primary.height - panelHeight - containerHeight)
> / 2
> 
> I guess it's a tad bit clearer than the more concise version above ...

Hmm, right, yeah, excessive simplification confused me. The above is clearer and probably worth a couple of extra cycles to write it that way. (I might also write

 ((primary.height - panelHeight) - containerHeight)

to make the grouping clear.)

Attachment 180493 [details] pushed as ef8c9e0 - workspace-switcher: Switch to vertical orientation

(In reply to comment #8)
> (I might also write
> 
>  ((primary.height - panelHeight) - containerHeight)
> 
> to make the grouping clear.)

Pushed with the above change.