Created attachment 154488 [details] [review]
[Overview] Use one button to toggle between single and grid view

Mmmh, this obviously needs ui review - I don't see that much benefit here, apart from gaining 24px of empty space / scrollbar.

On the other hand, there are some disadvantages IMO:

 - it breaks the symmetry of having two buttons on each side
   (yes, that's somewhat superficial, but it provides some balance
    and is easier on the eye)

 - especially the single-view button is completely cryptic if displayed
   stand-alone (and remember that shell defaults to grid view, so the
   first time the user clicks the (+) button, a little square will appear
   on the bottom left - right now, there's a highlighted little grid icon
   which somehow resembles the layout on screen beside that square, which
   hopefully makes it a lot easier to figure out what those controls do)

 If you try to forget for a moment that there are two toggle buttons in the source code, you may think of it more like a radio button group - the available options are all visible, but only one of them is active at any time. Instead of adding a special widget for customizable radio buttons (which would probably never be used anywhere else), it was implemented much more reasonably as toggle buttons with linked states - it's an implementation detail which should not determine the interface. I'm not saying that the current interface cannot be improved, but this looks like a stop backward to me.

Of course, IANAID (I am not an interface designer) ...

(In reply to comment #2)
> Mmmh, this obviously needs ui review - I don't see that much benefit here,
> apart from gaining 24px of empty space / scrollbar.

Sure that's why I added the ui-review keyword.
Well my idea is that you don't need a button that does nothing other than telling you what mode you are currently using (which is obvious anyway).

> On the other hand, there are some disadvantages IMO:
> 
>  - it breaks the symmetry of having two buttons on each side
>    (yes, that's somewhat superficial, but it provides some balance
>     and is easier on the eye)

Well the buttons on the right need a redesign anyway imho (+ is supposed to be a drop target but is way to small for that).

>  - especially the single-view button is completely cryptic if displayed
>    stand-alone (and remember that shell defaults to grid view, so the
>    first time the user clicks the (+) button, a little square will appear
>    on the bottom left - right now, there's a highlighted little grid icon
>    which somehow resembles the layout on screen beside that square, which
>    hopefully makes it a lot easier to figure out what those controls do)
> 
>  If you try to forget for a moment that there are two toggle buttons in the
> source code, you may think of it more like a radio button group - the available
> options are all visible, but only one of them is active at any time. Instead of
> adding a special widget for customizable radio buttons (which would probably
> never be used anywhere else), it was implemented much more reasonably as toggle
> buttons with linked states - it's an implementation detail which should not
> determine the interface. I'm not saying that the current interface cannot be
> improved, but this looks like a stop backward to me.

Well if we can do the same as we do with the (-) button when there is only workspace it I'd be fine with having two buttons (i.e make the other one insensitive) ... but the redundant one feels odd to me. (well it is a small detail which I can just ignore but ...)

> Of course, IANAID (I am not an interface designer) ...

Neither am I ;)

(In reply to comment #2)
>  - especially the single-view button is completely cryptic if displayed
>    stand-alone

Sure, but it's completely cryptic when displayed next to the grid-view button too :-). Especially since you can't actually tell which one is highlighted and which one isn't. (Notably, the "single" button is a solid color when it's selected, and has a white highlight around it when it's *not* selected, which doesn't make much sense.)

Probably the "single" button should show a little bit of a the workspaces on the left and right sides of the focused workspace, quickly fading to black. That doesn't match how it's actually displayed, but it's more evocative of the single-workspace layout than just showing a single workspace is.

Some valid points raised.  I agree that our single workspace icon is pretty poor.  So, I'm not sure this makes that mode much less mysterious.  Since we are defaulting to linear/single view I think having a toggle to grid makes sense.  I think we are in a process of de-emphasizing the grid view anyway.  In the longer term, I expect it will remain but perhaps as an expose of workspaces view - an uber-overview.  We'll see.

I am sensitive to the symmetry arguments but don't think we should be overly constrained by them.

That said, this seems like a nice change, in line with where we see this stuff going, and that I'm happy to try.

Created attachment 156371 [details] [review]
[Overview] Use one button to toggle between single and grid view

*) Rebase to master

Review of attachment 156371 [details] [review]:

Overall looks good - all comments are stylistic.

::: js/ui/workspacesView.js
@@ +1204,3 @@
             this.actor = new St.BoxLayout({ style_class: 'workspaces-bar' });
+        this._toggleViewButton = new St.Button({ style_class: "workspace-controls switch-single" });
+        // View switcher button

Double quotes should be used to mark translatable strings - use single quotes here!
I'd also prefer assigning the correct class from the start and not changing it eventually right after creation - the actor has not been added to a stage yet, so there are no performance implications.

@@ +1217,1 @@
+        this.actor.add(this._toggleViewButton, {'y-fill' : false, 'y-align' : St.Align.START});

I very much prefer y_fill: false (without quotes and underscore, no space before the colon)

@@ +1298,2 @@
         } else {
+            this._toggleViewButton.show();

Our style allows to leave out the braces in cases like this (both if-else parts have just one line).

Created attachment 156381 [details] [review]
[Overview] Use one button to toggle between single and grid view

*) Fixed style issues

Created attachment 156383 [details] [review]
[Overview] Use one button to toggle between single and grid view

Argh ... upload the working version

Created attachment 156385 [details] [review]
[Overview] Use one button to toggle between single and grid view

Review of attachment 156385 [details] [review]:

Looks good - I have one minor comment left, but feel free to go ahead anyway if you disagree ;)

::: js/ui/workspacesView.js
@@ +1178,3 @@
+            this._toggleViewButton.set_style_class_name('workspace-controls switch-mosaic');
+        if (WorkspacesViewType.SINGLE == view)
+

This code is duplicated below - not a big deal, but if we ever happen to rename the style classes, the change has to be done in two places, so it may be a good idea to move that code to a separate function.

(Also note that instead of using set_style_class_name() you can just assign to the style_class property - of course there's no benefit besides cutting down a little on the line length)

Created attachment 156453 [details] [review]
[Overview] Use one button to toggle between single and grid view

*) Avoid code duplication

Review of attachment 156453 [details] [review]:

Looks good.