Created attachment 134657 [details]
screenshot

screenshot showing the current spacing

The significant amount of space between each window is one problem but we may also want to look at improving the layout strategy as well.

Looking at this and referenced paper:
http://www.blainebell.org/SpaceManager/

Using *all* available space would look bad imho, but using *more* space would be nice.

However, there's one special case where I would love to have the preview take the whole workspace: workspaces with only a single maximized or fullscreen window. This would not only make it easier to see what happens in that workspace (bigger preview) but also give a more accurate representation of that workspace (no wallpaper visible) which I think could potentially make it easier to find a give workspace (user knows to look for a fullscreen app, there's only one workspace with a fullscreen app...)

I'd vote for a customizable version (with a default padding set to the coolest looking one). :) Where one can set it even to overlap windows in an overview mode if he tends to use too much windows in one workspace.

Needs more designing work done. And what about allowing user to switch window selection mode to a bookshelf (or what it's called ?) mode and other Compiz-like things ?

I definitely agree that this space should be used smarter, so that the windows are much larger.

Though GNOME Shell has changed a lot since this was filed, it's still a problem - actually it's probably worse, now. The area used to display open windows in the Activities view seems to be forced to be the same shape as the desktop, even though I can't think of a reason why this has to be the case, and it seems to makes no sense. For instance, on my system - a dual-head desktop - this means the area used to display open windows in the Activities view is a rectangle that's much longer than it is tall (because the desktop is the shape of two 20" monitors next to each other - 3360 x 1050) with a huge 'letterbox' of unused black space surrounding it. Even within this tiny area, a large amount of space is wasted. The upshot is that even though these are live views, they're far too small to actually be able to make out any details of what's going on in any window, and even so small that sometimes it's hard to tell at a glance which window is which.

The display of open windows should use all the space available, regardless whether it's a different shape from the desktop, and should use an efficient algorithm (see above citations) to use as much of that space as possible.

Note I'm using GNOME Shell 2.31.5, latest available in Fedora 14 - sorry if this has already been improved since then, but if so, it would be a good idea to update this bug report :)

Attaching a screenshot to illustrate the problem.

Created attachment 173348 [details]
inefficient use of space in the window selector (2.31.5)

Still the case with GNOME Shell 2.91.3. Screenshot attached. See allllll the wasted space in the overview mode on a dual screen system: the right hand screen is completely unused, and lots of space in the left hand screen is wasted too.

Created attachment 175681 [details]
wasted space in 2.91.3 (dual head)

Created attachment 190578 [details]
Modified screenshot that demonstrates the proposed change

This change would be a huge improvement. The attached image clearly demonstrates how much easier it would be to pick the desired window if it were implemented.

Created attachment 190579 [details]
Screenshot of the current state

Just for comparison - here's the current situation with 5 open windows.

Created attachment 190599 [details]
Modified screenshot that demonstrates the proposed change

I've tweaked the modded screenshot to increase the padding between windows and raise the windows.

Also note that the windows are always arranged in a grid here - this is easier to parse and makes the layout much more predictable.

Created attachment 190650 [details]
Padding specification for the window thumbnail layout

After some experimentation, I've come up with what seems like a sane spec for the padding between window thumbnails.

 * 38px horizontal between the left-most thumbnail and dash and right-most thumbnail and window picker

 * 24px horizontal between thumbnails

 * 24px vertical between thumbnails

See the attached image for details.

Created attachment 190652 [details]
Mockup - window thumbnails with excess x space

Note that window thumbnails should be horizontally centered in cases where there is excess horizontal space. This is much nicer than pushing the thumbnails to the left and right edges.

Alternatively, it might be worth avoiding thumbnail layouts with excess horizontal space altogether.

(In reply to comment #13)
> Created an attachment (id=190650) [details]
> Padding specification for the window thumbnail layout

...

>  * 24px vertical between thumbnails

Doesn't look like you show that off very well in the screenshot: do you mean "24px" between each "slot", where the slot is determined based on the largest window and the number of windows?

Created attachment 190693 [details]
Revised padding spec

(In reply to comment #15)
> (In reply to comment #13)
> > Created an attachment (id=190650) [details] [details]
> > Padding specification for the window thumbnail layout
> 
> ...
> 
> >  * 24px vertical between thumbnails
> 
> Doesn't look like you show that off very well in the screenshot:

Agreed - apologies for that.

> do you mean
> "24px" between each "slot", where the slot is determined based on the largest
> window and the number of windows?

Yes. See the attached mockup, which uses the new window picker appearance that is being worked on in bug 650254 - it's much clearer.

Created attachment 190694 [details]
Grid layout logic mockup

(In reply to comment #14)
> Created an attachment (id=190652) [details]
...
> Alternatively, it might be worth avoiding thumbnail layouts with excess
> horizontal space altogether.

Having done some more experimentation, this is my preferred solution - a grid that always has enough rows to ensure that all the horizontal space is occupied.

(In reply to comment #17)
> Having done some more experimentation, this is my preferred solution - a grid
> that always has enough rows to ensure that all the horizontal space is
> occupied.

By rows, I mean columns, of course.

Created attachment 190786 [details]
Revised grid layout logic mockup

The attached mockup is the same as before, except window titles and preview images are centre aligned.

Would it be worth adjusting the logic for portrait-oriented monitors? In that case, keeping the vertical space fully occupied is more important than the horizontal space, right?

I've been working on examining the various options for laying out a grid of windows. The result is too large to add as an attachment so I'm having to link to it:

http://dl.dropbox.com/u/5031519/grid-layout-options.png

In choosing a layout option, we need to decide on the balance we want between grid stability on the one hand and thumbnail size on the other. As the mockup shows, there are multiple points along the continuum from one to the other.

A more stable grid is potentially easier to read since it helps the user know where to look to start scanning the grid. It comes at the cost of diminished thumbnail sizes, however.

Note that these mockups aren't meant to address the question of where each window gets inserted into the grid.

We used a version of thumbnail space optimised in moblin netbook which worked moderately well until you had irregularly shaped windows. I'd like to see some work looking at those (calculator and empathy buddy list are two offenders) as they can make some of these layouts look very silly.

Certainly we'd be well against the total stability option which didn't have a good experience for 'one app on a workspace'.

(In reply to comment #21)

I've discussed these ideas with Jon and we both agreed that the current approach is probably OK for the time being. I'm still kinda interested in the grid approach, but we can fix this bug without having to adjust the layout so radically.

Removing the ui-review keyword.

Comment on attachment 190693 [details]
Revised padding spec

Here's a new set of guidelines: http://dl.dropbox.com/u/5031519/layout-spec.png

A few things to note:

1) Window thumbnails are pushed up so that they occupy the top of the available space. There are a few reasons for this:

 * It ensures that the top row of windows is at eye level. This is more comfortable.

 * It means that there is a clear relationship with the 'window' label

 * It's always aligned with the top of the dash

2) The thumbnails are tightly packed, so that the padding between each 'cell' is always the same.

3) Avoids bug 646704 by always scaling the thumbnail down.

4) As with the previous spec, padding between window 'cells' is 24px.

Adding the ui-review keyword - I'd like a thumbs up/down on this.

Forgot the keyword.

I just uploaded a revised version of the layout: http://dl.dropbox.com/u/5031519/layout-spec.png

This adjusts the vertical positioning of the window thumbnails. When these do not occupy the whole of the available vertical space they don't push right up to the top. Instead, the top of the window 'cell' sits 38px below the absolute upper limit (indicated by a white dashed line on the mockup).

This seems optimal in terms of ensuring that the thumbnails are at eye level. It also seems to look the best.

The Dropbox links are 404s. Can you attach them to the bug?

I confirm this bug.
on a netbook it's very very hard to choose your window when 5 windows or more are displayed in overview

I absolutely confirm this bug.
on a netbook it's very very hard to choose your window when 5 windows or more are displayed in overview. Actually I have to zoom.

(In reply to comment #27)
> The Dropbox links are 404s. Can you attach them to the bug?

Only if they are less than 1MB. :(

Here's some new guidance based on discussions I've had with Jon and Jimmac:

http://dl.dropbox.com/u/5031519/window-picker-guidance.png

@Allan Day

I really like the mockup you have, it seems like it would really help

Created attachment 205788 [details] [review]
WIP implementation

Here's a proof of concept implementation of Allan Day's proposal.

A few notes:
- only tested on my laptop (1366x768 resolution)
- I couldn't build gnome-shell from git yet, so this patch was created for Debian version of gnome-shell (3.2.1)
- I didn't use the window caption/button sizes in the layout computations yet.
- I'm far from being a JS expert, so code critics are highly welcomed :)

Let me know what you think.

Review of attachment 205788 [details] [review]:

This looks very similar to the patch that I had, before I abandoned it for more important things. Good to see that I was on the right track :)

I didn't inspect the algorithm too heavily, here. This is a first pass review.

::: js/ui/workspace.js
@@ +1003,3 @@
+        let slots = this._computeAllWindowSlots(clones);
+        // windows should be ordered before selecting the layout...
+        //clones = this._orderWindowsByMotionAndStartup(clones, slots);

Don't comment out lines of code, just remove them. In the final edition of this patch, you should be able to remove a ton of code that you're not calling. You can remove _orderWindowsByMotionAndStartup in the final edition of this patch.

@@ +1020,3 @@
                 continue;
 
+            let [x, y, scale] = slot; //this._computeWindowLayout(metaWindow, slot);

Again, you can remove _computeWindowLayout in the final revision of this patch.

@@ +1461,3 @@
+    // returns the width/height for non-grid aligned rows
+    _computeRowsSizesNonAligned: function(windows, rowCount, columnCount, scale, rowSpacing, columnSpacing) {
+        let rowsSizeX = new Array(rowCount);

Don't use "new Array".

@@ +1472,3 @@
+
+            let s = windows[i].metaWindow.get_outer_rect();
+            rowsSizeX[row] += s.width * scale + columnSpacing;

Isn't this going to add an extra columnSpacing?

@@ +1473,3 @@
+            let s = windows[i].metaWindow.get_outer_rect();
+            rowsSizeX[row] += s.width * scale + columnSpacing;
+            rowsSizeY[row] = Math.max(rowsSizeY[row], s.height * scale);

Is there anything wrong with windows[i].width / windows[i].height? Transferring a boxed from C and getting a property on a boxed is expensive in comparison to just getting property on an object.

@@ +1477,3 @@
+        let rowsSize = new Array(rowCount);
+        for (let i = 0; i < rowCount; i++) {
+            rowsSize[i] = [rowsSizeX[i] - columnSpacing, rowsSizeY[i]];

Oh, I see, you remove the excess here. I'm curious why you use two arrays, and then merge it with one. Instead of two arrays, how about:

    rowsSize.push({ width: 0, height: 0 });

when you create a row, and:

    rowsSize[row].width += windows[i].width * scale + columnSpacing;
    rowsSize[row].height += Math.max(rowsSize[row].height, s.height * scale);

and then:

    // We added excess columnSpacing to simplify the loop above. Compensate here.
    for (let i = 0; i < rowCount; i ++)
        rowsSize[row].width -= columnSpacing;

@@ +1536,3 @@
+                let rowHeight = 0;
+                for (let j = 0; j < c && (i * c + j)<totalWindows; j++) {
+                    let rect = windows[i * c + j].metaWindow.get_outer_rect();

I'd just add 'let windowIdx = 0' before the loop, and increment it at the bottom of the loop. Additionally, I'd do:

    if (windowIdx > windows.length)
        break;

@@ +1553,3 @@
+            // thumbnails should be less than 70% of the original size
+            let newScale = Math.min(
+                Math.min(horizontalScale , verticalScale), 0.7);

This should be a constant at the top: MAX_THUMBNAIL_SCALE

@@ +1557,3 @@
+
+            let scaleWeight = 0.5;
+            let ratioScore = 0.5;

These need to be constants at the top of the file.

@@ +1558,3 @@
+            let scaleWeight = 0.5;
+            let ratioScore = 0.5;
+            let score = scale * scaleWeight + Math.abs(1 / (workspaceRatio - ratio)) * ratioScore;

Was this meant to be "newScale * scaleWeight + ..."?

@@ +1566,3 @@
+                bestScore = score;
+            } else
+                break;

Style nit: if either side of the branch has braces, both do.

@@ +1577,3 @@
+        let rowCount = layout[0];
+        let columnCount = layout[1];
+        let scale = layout[2];

Bah. Destructuring assignment is cooler.

    let [rowCount, columnCount, scale] = layout;

@@ +1588,3 @@
+            let col = i - row * columnCount;
+
+            let x = rowOrigins[row][0];

There's a reason I wanted objects with properties rather than arrays. "rowOrigins[row].x + rowSizes[row].width" is much clearer than "rowOrigins[row][0] + rowSizes[row][0]".

@@ +1595,3 @@
+            let y = rowOrigins[row][1] +
+                rowSizes[row][1] -
+                windows[i].metaWindow.get_outer_rect().height * scale;

Don't wrap lines like this.

@@ +1600,3 @@
+                 [this._x +  x,
+                 this._y +  y,
+                 scale]);

Don't wrap lines like this. "slots.push([this._x + x, this._y + y, scale]);" is fine.

@@ +1611,3 @@
+        let rowCount = layout[0];
+        let columnCount = layout[1];
+        let scale = layout[2];

Destructuring assignment!

@@ +1629,3 @@
+        scale = Math.min(scale, (this._height - (rowCount - 1) * rowSpacing) / totalHeight);
+
+        let gridSize = [maxWidth * scale, maxHeight * scale];

"cellSize" is probably a better word here.

@@ +1646,3 @@
+                col * (gridSize[0] + columnSpacing) +
+                gridSize[0] * 0.5 -
+                windows[i].metaWindow.get_outer_rect().width * scale * 0.5;

I'd rather see this as:

    let x = origin.x + col * (cellSize.width + columnSpacing);
    // Center in cell.
    x += (cellSize.width - windows[i].width * scale) / 2;

@@ +1652,3 @@
+                row * (gridSize[1] + rowSpacing) +
+                gridSize[1] -
+                windows[i].metaWindow.get_outer_rect().height * scale;

And this:

    let y = origin.y + row * (cellSize.height + rowSpacing);
    // Bottom-align.
    y += cellSize.height - windows[i].height * scale;

@@ +1666,3 @@
+        let totalWindows = windows.length;
+        const rowSpacing = 30;
+        const columnSpacing = 10;

These should probably go in CSS somewhere.

@@ +1672,3 @@
+
+        // if more than 2 rows -> use grid layout
+        if (layout[0] <= 2) {

I thought the max for unaligned was three rows, not two?

(In reply to comment #33)
> Review of attachment 205788 [details] [review]:

Thanks, I've added my comments inline.


> Don't comment out lines of code, just remove them. 

Ok will do, i didn't remove them at first to have a lighter patch.


> +        let rowsSizeX = new Array(rowCount);
> 
> Don't use "new Array".

I tried to avoid that, but I wasn't sure what was the best way to express what I need: an array with 'rowCount' elements, each one initialized to [0, 0]

> @@ +1473,3 @@
> +            let s = windows[i].metaWindow.get_outer_rect();
> +            rowsSizeX[row] += s.width * scale + columnSpacing;
> +            rowsSizeY[row] = Math.max(rowsSizeY[row], s.height * scale);
> 
> Is there anything wrong with windows[i].width / windows[i].height? 

Hum... I only grepped for width/height and found the 'metaWindow.get_outer_rect'  way to retrieve it.
Of course I'll change that :-)

> @@ +1477,3 @@
> +        let rowsSize = new Array(rowCount);
> +        for (let i = 0; i < rowCount; i++) {
> +            rowsSize[i] = [rowsSizeX[i] - columnSpacing, rowsSizeY[i]];
> 
> Oh, I see, you remove the excess here. I'm curious why you use two arrays, and
> then merge it with one. Instead of two arrays, how about:
> 
>     rowsSize.push({ width: 0, height: 0 });

Ahh, really nice. Thanks for the tip.

> 
> I'd just add 'let windowIdx = 0' before the loop, and increment it at the
> bottom of the loop. Additionally, I'd do:
> 
>     if (windowIdx > windows.length)
>         break;

Indeed, should be more readable

> @@ +1558,3 @@
> Was this meant to be "newScale * scaleWeight + ..."?

Yes

> 
> @@ +1577,3 @@
> +        let rowCount = layout[0];
> +        let columnCount = layout[1];
> +        let scale = layout[2];
> 
> Bah. Destructuring assignment is cooler.
> 
>     let [rowCount, columnCount, scale] = layout;

Yes this looks much nicer.

> 
> @@ +1588,3 @@
> There's a reason I wanted objects with properties rather than arrays.
> "rowOrigins[row].x + rowSizes[row].width" is much clearer than
> "rowOrigins[row][0] + rowSizes[row][0]".

I'm not sure how to do that (maybe like the 'ScaledPoint' prototype at the top of the file ?). I'll look into JS doc.

> 
> @@ +1666,3 @@
> +        let totalWindows = windows.length;
> +        const rowSpacing = 30;
> +        const columnSpacing = 10;
> 
> These should probably go in CSS somewhere.

Me too.
Also, maybe they should proportion of workspace size, instead of fixed size ?

> 
> @@ +1672,3 @@
> +
> +        // if more than 2 rows -> use grid layout
> +        if (layout[0] <= 2) {
> 
> I thought the max for unaligned was three rows, not two?

"Only align thumbnails to grid if there are more than two rows" says the mockup.

I'll attach an updated patch soon.

(In reply to comment #34)
> (In reply to comment #33)
> > Review of attachment 205788 [details] [review] [details]:
> > @@ +1588,3 @@
> > There's a reason I wanted objects with properties rather than arrays.
> > "rowOrigins[row].x + rowSizes[row].width" is much clearer than
> > "rowOrigins[row][0] + rowSizes[row][0]".
> 
> I'm not sure how to do that (maybe like the 'ScaledPoint' prototype at the top
> of the file ?). I'll look into JS doc.
> 

If you do what I said (build { width: 0, height: 0 } instead of [0, 0]), you should be all set. Of course, do the same for rowOrigins as well: { x: 0, y: 0 } instead of [0, 0].

Created attachment 205925 [details] [review]
2nd version

Changes:
- modifications following Jasper's comments
- new simpler formula for deciding wheither a new row should be added: both scale and ratio should be improved, otherwise it stops trying to add a new row

(In reply to comment #36)
> Created an attachment (id=205925) [details] [review]
> 2nd version
> 
> Changes:
> - modifications following Jasper's comments
> - new simpler formula for deciding wheither a new row should be added: both
> scale and ratio should be improved, otherwise it stops trying to add a new row

Thanks for working on this, Pierre-Eric! I've tried your patch and it seems to be heading in the right direction. A few initial comments:

 * There should be more padding between the thumbnails

 * It doesn't handle updates well - you should be able to add windows without the layout completely updating. Currently adding windows results in the layout being reorganised as well as clipped thumbnails.

Created attachment 206013 [details] [review]
Third version

Thanks Allan for your comments.
Here's a new slightly updated version of the patch :
- horizontal padding between cells set to 24px
- window order changed from :
123      123
456  ->  654
789      987

I think it's a bit better, because it avoids diagonal moves (for instance, W7 going to take W6's old place, if you closed W5).

What do you all think ?

Created attachment 206018 [details]
screenshot of patch in action

I've tried the new patch (screenshot attached). Feedback:

 * There needs to be vertical padding between thumbnails too

 * It seems that the 'boundary' in which the thumbnails are being placed is taller than the actual space available. This is causing the layout to be misaligned and thumbnails to be clipped at the top and bottom.

I used 30px for the vertical and horizontal padding in the mockups.

Created attachment 206027 [details] [review]
Fourth version!

Ok here's a new patch.

> I've tried the new patch (screenshot attached). 

Weird. Using the same windows (4 fullscreen, 2 not-fs), the layout choosen on my laptops uses only 2 rows (which looks better). I've fixed a bug, maybe this will fix this problem.

> Feedback:
>  * There needs to be vertical padding between thumbnails too

Ok. That's a point I was planning to improve. 
So vertical spacing between rows should be '30px + window caption height' ?
For now I've changed it to '46px', but it'll require a proper handling.

> 
>  * It seems that the 'boundary' in which the thumbnails are being placed is
> taller than the actual space available. This is causing the layout to be
> misaligned and thumbnails to be clipped at the top and bottom.

I just found a few bugs in vertical placement. Let me know if you still experience this issue.

Created attachment 206034 [details]
2 screenshots

The new version is better but still has some issues.

 * The thumbnails are still placed above the viewing area in some cases (seems to be mostly when there are 3-4 windows) 

 * I'm seeing some overshoot on the right hand side too.

 * There are some occassional odd layouts when adding windows, which I've seen when adding a 7th window within the overview (see the top screenshot that's attached)

 * There are also some odd misalignment issues when removing windows from the overview (see the bottom screenshot that's attached)

 * When the workspace switcher slides in the thumbnails scale down when there isn't any need for them to (since they are not using the full width)

Created attachment 206130 [details] [review]
Fifth version

One more version, it's amazing how many bugs and typos I can produce within so few lines of code :)

Here's the changelog :
- lot of bugs fixed
- the area used to display windows thumbnails is reduced before trying to scale windows. This is done to reserve some place at the top for the 'close' button, and at the bottom for titles of bottom row windows
- when not using a linear layout: I added an extra step, allowing to use non-equal column counts per row. For instance: 1 fullscreen app on the top row, 3 gnome-calc on the bottom row.

Regarding comment 41:
" * The thumbnails are still placed above the viewing area in some cases (seems
to be mostly when there are 3-4 windows) " => should be fixed

" * I'm seeing some overshoot on the right hand side too." => I don't have this problem. But I have the workspace switcher always displayed (not sure why), so maybe this explains the difference

" * There are some occassional odd layouts when adding windows, which I've seen
when adding a 7th window within the overview (see the top screenshot that's
attached)" : I think I've reproduced this one, but it did not seems to be a layout issue. Looked like a bug in window animations, but I'm not sure

" * There are also some odd misalignment issues when removing windows from the
overview (see the bottom screenshot that's attached)": this is the expected behavior. 2 rows -> non grid aligned. As your top-left window is not a fullscreen one, the whole row is smaller than the bottom one.

" * When the workspace switcher slides in the thumbnails scale down when there
isn't any need for them to (since they are not using the full width)" : see above, I need to figure out why my workspace switcher is always shown.

(In reply to comment #42)
> Created an attachment (id=206130) [details] [review]

Wow, this is starting to look really nice (from a design perspective, anyway!) The layout logic seems to be there now.

I did see a few glitches with new thumbnails overlapping existing ones when they were added within the overview. Can't reproduce now though.

One thing - the thumbnails shouldn't be scaled in a linear fashion. It should probably be a log (?) of their original size - we want to convey that the windows are smaller without letting them (a) look silly and (b) not be recognisable.

The other thing that is missing is having the window captions extend wider than the windows themselves [1]. Is that possible?

[1] http://dl.dropbox.com/u/5031519/window-picker-guidance.png

> One thing - the thumbnails shouldn't be scaled in a linear fashion. It should
> probably be a log (?) of their original size - we want to convey that the
> windows are smaller without letting them (a) look silly and (b) not be
> recognisable.

Ok I'll try this.

> 
> The other thing that is missing is having the window captions extend wider than
> the windows themselves [1]. Is that possible?

I have something working for grid layout (max window caption size = cell width).
For non-grid layout, it's a bit more tricky... 
By the way, on your mockup, when using non-aligned layout, you used a different column spacing for each row. Is this needed, and if so, what kind of formula should be used ?

Regarding comment #41 issue regarding the "workspace switcher slides in". The problem comes from the calculation of available area. On my laptop, when workspace switcher is hidden, the area is 1200x600, when the switcher is visible, the area is 1000x500.
The reduced size (with aspect ratio preserved) lead to a reduced size for all windows.

(In reply to comment #44)
...
> By the way, on your mockup, when using non-aligned layout, you used a different
> column spacing for each row. Is this needed, and if so, what kind of formula
> should be used ?

When it is non-aligned, treat the thumbnail + caption as the unit, with the standard padding between each one.

 <- thumbnail ->                  <- thumbnail ->

<-   caption   -> <- padding -> <-    caption    ->

> Regarding comment #41 issue regarding the "workspace switcher slides in". The
> problem comes from the calculation of available area. On my laptop, when
> workspace switcher is hidden, the area is 1200x600, when the switcher is
> visible, the area is 1000x500.
> The reduced size (with aspect ratio preserved) lead to a reduced size for all
> windows.

Sounds like a bug to me. The height of the available area clearly doesn't shrink when the slider comes in. :)

At some point we need to rebase this patch on top of master. I don't suppose you've got a working build, do you?

(In reply to comment #45)
> (In reply to comment #44)
> ...
> > By the way, on your mockup, when using non-aligned layout, you used a different
> > column spacing for each row. Is this needed, and if so, what kind of formula
> > should be used ?
> 
> When it is non-aligned, treat the thumbnail + caption as the unit, with the
> standard padding between each one.
> 
>  <- thumbnail ->                  <- thumbnail ->
> 
> <-   caption   -> <- padding -> <-    caption    ->

This is hard to do for various reasons, so can we drop it from the initial land of the patch?

(In reply to comment #46)
... 
> > When it is non-aligned, treat the thumbnail + caption as the unit, with the
> > standard padding between each one.
> > 
> >  <- thumbnail ->                  <- thumbnail ->
> > 
> > <-   caption   -> <- padding -> <-    caption    ->
> 
> This is hard to do for various reasons, so can we drop it from the initial land
> of the patch?

Sure, the patch is an improvement in and of itself.

Created attachment 206289 [details] [review]
Sixth version

This version includes only 1 change: in grid layout, caption width is now limited by cell size (not window size).

I tried a few things around the 'log scale' stuff but with no good results. If anyone has an idea on how this should work, let me know :)

Also, this patch has removed the existing behavior regarding window ordering (which where sorted by least motion, then by creation time). I'm not sure how much this is a problem (and sorting by least motion would not be an easy thing I believe).

Last, I managed to build gnome-shell using jhbuild, but it won't start because of dbus/config errors:
Details -  1: GetIOR failed: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Method "GetIOR" with signature "" on interface "org.gnome.GConf" doesn't exist

I'd like to be able to rebase/test on master, though if someone want to do it, I'm fine with that :-)

Created attachment 206291 [details] [review]
Small patch to fix the comment 45 issue

Here's another small patch: it simply use the same height for the workspace view, wheither the workspace switch is in/out.
Also I think the whole workspace view should be sightly moved to the left, for better centering - but again I'm testing on an old gnome-shell so maybe this is not an issue on master.

(In reply to comment #48)
> Last, I managed to build gnome-shell using jhbuild, but it won't start because
> of dbus/config errors:
> Details -  1: GetIOR failed:
> GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Method "GetIOR" with
> signature "" on interface "org.gnome.GConf" doesn't exist

This means that you have a system gconfd configured to use ORBIT while you have a libgconf that's looking for DBus. Try adding:

    module_autogenargs['gconf'] = '--enable-orbit'

to your ~/.jhbuildrc. If that doesn't work, try it the other way around with '--disable-orbit'. I can never remember these things.

Created attachment 206300 [details] [review]
v7

7th version :
  -> rebased on top of master (thanks magcius and fmuellner for help with jhbuild)
  -> fixed (I think), the erratic issue where a newly added window had a wrong position.

Nice job (didn't take a look at the code yet though)! Some observations though (Allan, please shout if you disagree):

 - I think we need some minimum spacing between window thumbnails
   and workspace switcher - with the current patch I've seen the
   thumbnails touch the switcher occasionally (which not only looks
   bad imo, but it also means that the close button is cut off)

 - I think we need more vertical spacing between rows - right now,
   if we show the close button for a window in a lower row it overlaps
   with the title caption of a window of the row above; too crowded
   for my taste ...

 - with two maximized windows and a nonmaximized smaller one (let's
   call it "terminal" ;-), I've seen two different placements (and no
   idea why I get one or the other):
     (1) one maximized window + terminal on top, the other maximized
         window below
     (2) one maximized window on top, the other one + terminal below
   I'm not really sure why, but (1) feels a lot more balanced than (2)
   to me

(In reply to comment #52)
>  - I think we need more vertical spacing between rows - right now,
>    if we show the close button for a window in a lower row it overlaps
>    with the title caption of a window of the row above; too crowded
>    for my taste ...

In fact, the caption may end up closer to a window below than to the window it actually belongs to - that can't be right, can it?

(In reply to comment #52)
> Nice job (didn't take a look at the code yet though)! Some observations though
> (Allan, please shout if you disagree):
> 
>  - I think we need some minimum spacing between window thumbnails
>    and workspace switcher - with the current patch I've seen the
>    thumbnails touch the switcher occasionally (which not only looks
>    bad imo, but it also means that the close button is cut off)

Yes I agree. But I didn't find where the workspace's (x, y, width, height) values came from. Any idea ? I think the workspace could be centered between the workspace-switcher (on or off) and the thing-at-the-left-with-the-app-icons-in-it.


> 
>  - I think we need more vertical spacing between rows - right now,
>    if we show the close button for a window in a lower row it overlaps
>    with the title caption of a window of the row above; too crowded
>    for my taste ...

The vertical padding was enough with my old gnome-shell, but with git version it's too small. Here are the values used:
        const rowSpacing = 45;
        const columnSpacing = 30;

> 
>  - with two maximized windows and a nonmaximized smaller one (let's
>    call it "terminal" ;-), I've seen two different placements (and no
>    idea why I get one or the other):
>      (1) one maximized window + terminal on top, the other maximized
>          window below
>      (2) one maximized window on top, the other one + terminal below
>    I'm not really sure why, but (1) feels a lot more balanced than (2)
>    to me

I've noticed this one two. I vote for (1) too (and will verify the code)

Review of attachment 206300 [details] [review]:

Getting better. I went over the algorithm and it made sense and I couldn't find anything majorly wrong with the approach. Most of the stuff here are easy cosmetic fixes.

::: js/ui/workspace.js
@@ +842,3 @@
             } else {
+				// cancel any active tweens (otherwise they might override our changes)
+				Tweener.removeTweens(clone.actor);

Wrong indentation.

@@ +1246,3 @@
 
+    // returns the width/height for non-grid aligned rows
+    _computeRowsSizesNonAligned: function(windows, rowCount, scale, rowSpacing, columnSpacing, assignment) {

_computeRowSizesNonGridAligned

@@ +1249,3 @@
+    	let rowsSize = [];
+
+        // We'll add excess columnSpacing to simplify the next loop above. Compensate here.

If that's my comment verbatim, I'm terrible... try:

    // We add extra column spacing in the loop below, so compensate by offsetting here.

@@ +1265,3 @@
+
+    // returns origin point for non-grid aligned rows
+    _computeRowsOriginNonAligned: function(rowsSize, rowCount, rowSpacing, columnSpacing, availArea) {

Any reason you can't merge this with _computeRowSizesNonGridAligned?

@@ +1275,1 @@
+		let baseLine = (availArea.height - h) * 0.5;

Wrong indentation.

@@ +1275,3 @@
+		let baseLine = (availArea.height - h) * 0.5;
+        for (let i = 0; i < rowCount; i++) {
+            let _y = baseLine;

Use x, y, not _x, _y. (Brings me back to Flash days...)

@@ +1276,3 @@
+        for (let i = 0; i < rowCount; i++) {
+            let _y = baseLine;
+            for (let j=0; j < i; j++) {

j = 0

@@ +1287,3 @@
     },
 
+    _computeWindowsSlotNonAligned: function(windows, layout, rowSpacing, columnSpacing, availArea, assignment) {

_computeWindowSlotsNonGridAligned, assignments

@@ +1294,3 @@
+
+        let rowSizes = this._computeRowsSizesNonAligned(windows, rowCount, scale, rowSpacing, columnSpacing, assignment);
+

Extra line.

@@ +1307,3 @@
+            }
+
+            // odd row index -> from right to left

A little more explanation here. Maybe:

    // For odd row indexes, order windows from right to left.
    // This zig-zag window ordering is meant to minimize movement
    // when adding/removing windows, so a window moving between rows
    // doesn't jump across the screen.

@@ +1309,3 @@
+            // odd row index -> from right to left
+            if (row % 2) {
+            	x = availArea.width - x - windows[i].actor.width * scale;

I don't understand why we're subtracting the actor's width here...

@@ +1316,3 @@
+            let overlay = this._windowOverlays[i];
+            if (overlay) {
+                overlay._maxTitleWidth = windows[i].actor.width * scale;

Don't access/set another object's private property.

@@ +1337,3 @@
+        }
+
+        // adjust scale to fit everyone on a grid (could have been done in computeRowColumnCountAndScale...)

"could have been done in..." is a useless comment. Why wasn't it done there? Laziness? Do it there if it's better.

@@ +1339,3 @@
+        // adjust scale to fit everyone on a grid (could have been done in computeRowColumnCountAndScale...)
+        let totalWidth = maxWidth * columnCount;
+        let totalHeight = maxHeight * rowCount;

I talked with Allan about this earlier and he said that the "maximum size" shouldn't be the maximum of all the windows, but instead the size of a maximized window.

I'm not sure if there's an easy way to get the size of what a maximized window would be, actually, so this is fine for now.

@@ +1349,3 @@
+        let oY = (availArea.height - (cellSize.height * rowCount + (rowCount - 1) * rowSpacing)) * 0.5;
+
+        let origin = { x: oX, y: oY };

let origin = { x: availArea.width - cellSize.width * columnCount + columnSpacing * (columnCount - 1),
                   y: availArea.height - cellSize.height * rowCount + rowSpacing * (rowCount - 1) };

@@ +1359,3 @@
+            x += (cellSize.width - windows[i].actor.width * scale) * 0.5;
+
+            if (row % 2) {

// See comment in _computeWindowSlotsNonAligned

@@ +1394,3 @@
+                let diff = (rowWidth + w) / idealRowWidth;
+                // either new width is < idealWidth or new width is nearer from idealWidth then oldWidth
+                if (diff <= 1 || (Math.abs(1 - diff) < Math.abs(1 - oldDiff)) || (i == rowCount-1)) {

rowCount - 1

@@ +1395,3 @@
+                // either new width is < idealWidth or new width is nearer from idealWidth then oldWidth
+                if (diff <= 1 || (Math.abs(1 - diff) < Math.abs(1 - oldDiff)) || (i == rowCount-1)) {
+                    assignment.push( { row: i, column: col });

Extra space.

@@ +1410,3 @@
+        for (let i = 0; i < windows.length; i++) {
+            let r = Math.floor(i / columnCount);
+            assignment.push( { row: r,  column: i - r * columnCount });

Extra space.

@@ +1427,3 @@
+        let scale = 0;
+        let ratio = 0;
+        let availAreaForWindows = [];

= null;

@@ +1428,3 @@
+        let ratio = 0;
+        let availAreaForWindows = [];
+        let finalAssign;

assignments

@@ +1441,3 @@
+
+        while (true) {
+            let r = rowCount + 1;

newRowCount

@@ +1442,3 @@
+        while (true) {
+            let r = rowCount + 1;
+            let c = Math.ceil(totalWindows / r);

newColumnCount

@@ +1444,3 @@
+            let c = Math.ceil(totalWindows / r);
+
+            // if adding a new row does not increase column count just stop

"does not decrease"

@@ +1451,3 @@
+
+            // assign windows to row/column
+            let assignment = (r <= 2) ? this._assignWindowsToRowAndColumnNonAlignedLayout(windows, r, c) : this._assignWindowsToRowAndColumnGridAlignedLayout(windows, r, c);

newAssignments

@@ +1484,3 @@
+            availAreaForWindows = { x: this._x, y: this._y, width: this._width, height: this._height };
+            // reserve vertical space
+            availAreaForWindows.y +=  topRowWinMaxButtonHeight;

Extra space.

@@ +1487,3 @@
+            availAreaForWindows.height -= topRowWinMaxButtonHeight + bottomRowWinMaxTitleHeight;
+
+            let horizontalScale = (availAreaForWindows.width - columnSpacing * (c-1)) / maxRowWidth;

(newColumnCount - 1)

@@ +1493,3 @@
+            let newScale = Math.min(Math.min(horizontalScale , verticalScale), MAX_THUMBNAIL_SCALE);
+
+            let ratioFormula = Math.abs(maxRowWidth / totalRowsHeight - workspaceRatio);

newRatio

@@ +1514,3 @@
+        let totalWindows = windows.length;
+        const rowSpacing = 45;
+        const columnSpacing = 30;

Again, these should be from CSS. Look into StWidget.get_theme_node and StThemeNode.get_length.

Review of attachment 206291 [details] [review]:

This is obviously just a quick hack patch.

(In reply to comment #54)
> (In reply to comment #52)
> >  - I think we need some minimum spacing between window thumbnails
> >    and workspace switcher - with the current patch I've seen the
> >    thumbnails touch the switcher occasionally (which not only looks
> >    bad imo, but it also means that the close button is cut off)
> 
> Yes I agree. But I didn't find where the workspace's (x, y, width, height)
> values came from. Any idea?

[workspacesView.js] workspacesDisplay => [workspacesView.js] workspacesView.setGeometry() => workspace.setGeometry()


> [...] the thing-at-the-left-with-the-app-icons-in-it.

"Dash" :-)


> >  - I think we need more vertical spacing between rows - right now,
> >    if we show the close button for a window in a lower row it overlaps
> >    with the title caption of a window of the row above; too crowded
> >    for my taste ...
> 
> The vertical padding was enough with my old gnome-shell, but with git version
> it's too small. Here are the values used:
>         const rowSpacing = 45;
>         const columnSpacing = 30;

Ah, yes - the text size of the labels was increased recently. So if I understand you correctly, "rowSpacing" is the space between the bottom of window previews in one row and the (maximum) top of window previews in the row below? And the spacing is supposed to be big enough to fit the caption? If so, we should be able to do better[0]. Also note that the universal access menu (the little man in the top bar) contains a "Large Text" item which works as advertised, so the labels may even change size at runtime (and indeed, it makes the problem even worse).


> >  - with two maximized windows and a nonmaximized smaller one (let's
> >    call it "terminal" ;-), I've seen two different placements (and no
> >    idea why I get one or the other):
> >      (1) one maximized window + terminal on top, the other maximized
> >          window below
> >      (2) one maximized window on top, the other one + terminal below
> >    I'm not really sure why, but (1) feels a lot more balanced than (2)
> >    to me
> 
> I've noticed this one two. I vote for (1) too (and will verify the code)

It looks to me like it's related to MRU, i.e. if one of the maximized windows is the most-recently-used window, I end up with (1) and with (2) when the terminal has focus (haven't looked at the code in detail to confirm that observation)


[0] window previews and captions are in different groups because we used to
    scale the workspace actor (with the window previews in it), and didn't
    want the scale to affect the (fixed size) captions. It looks like the
    workspaces are no longer scaled themselves, so it may be easier to use
    a BoxLayout for every window which holds the (scaled down) window preview
    and the (unscaled) caption; the height of one row would than be from the
    (maximum possible) top of window previews to the bottom of the caption;
    ensuring that the spacing between rows (e.g. from the bottom of the caption
    to the top of the previews of the row below) is bigger than the spacing
    between preview and caption (expressed by the 'spacing' CSS property) is
    then trivial ...

Review of attachment 206300 [details] [review]:

::: js/ui/workspace.js
@@ +34,3 @@
 const BUTTON_LAYOUT_KEY = 'button-layout';
 
+const MAX_THUMBNAIL_SCALE = 0.7;

Just a drive-by comment: this constant already exists in master (as WINDOW_CLONE_MAXIMUM_SCALE) - you should either use that or remove the original one (note though that so far the term "thumbnail" is used in the workspace switcher code, but not to refer to the window previews)

Thanks for the ongoing work on this! In terms of getting the patch landed, the priorities seem to be:

 * Padding to the left and right of the window picker area - ie. the space between the thumbnails and the dash on the one side and the workspace selector on the other. This needs to be even, and a minimum amount of padding needs to be enforced.

 * Vertical padding between thumbnails - they're too close at the moment. 

 * Thumbnail scaling - either needs to be logarithmic (or something else that isn't linear) or needs to be dropped for now

The caption width and how we handle the workspace switcher sliding in can wait.

Another nit:
I'm not entirely sold on throwing out the minimum-window-movement algorithm completely. I've just seen the following:

Starting with three windows

  (A)     (B)

       (C)


after closing window (C) I end up with

   (B)      (A)

The crossing of (A) and (B) is pointless and makes the animation way too heavy for my taste (in particular as it is a regression from the current behavior) ...


(oh, and please don't get discouraged by all those comments - it's awesome to see you work on this!)

(In reply to comment #60)
> Another nit:
> I'm not entirely sold on throwing out the minimum-window-movement algorithm
> completely. I've just seen the following:

Since slots are no longer fixed, we'd have to try every permutation of windows to find the minimum distance. With 5 windows, that's 120 tries.

> Starting with three windows
> 
>   (A)     (B)
> 
>        (C)
> 
> 
> after closing window (C) I end up with
> 
>    (B)      (A)
> 
> The crossing of (A) and (B) is pointless and makes the animation way too heavy
> for my taste (in particular as it is a regression from the current behavior)
> ...

I don't understand why that would happen -- we are still sorting clones based on some form of stable ordering (area of window, metacity's "stable order"). (A) and (B) should never switch places if we are doing that, since we're still packing windows based on a predetermined order.

> (oh, and please don't get discouraged by all those comments - it's awesome to
> see you work on this!)

(In reply to comment #61)
> I don't understand why that would happen -- we are still sorting clones based
> on some form of stable ordering (area of window, metacity's "stable order").
> (A) and (B) should never switch places if we are doing that, since we're still
> packing windows based on a predetermined order.

I can't give a sequence to reproduce (yet?) - most of the time the algorithm behaves correctly, but window crossing does happen occasionally.

(In reply to comment #55 and comment #58)
> Review of attachment 206300 [details] [review]:

Thanks for your reviews. I've cleaned up the patch according to your comments.

(In reply to comment #56)
"This is obviously just a quick hack patch." 
Yes: it was the quickest way to test the change.
And no: it does exactly what I wanted: do not maintain a fixed aspect ratio when the workspace switcher slides in.
Do you think there is better place/way to do this ?

> 
> [workspacesView.js] workspacesDisplay => [workspacesView.js]
> workspacesView.setGeometry() => workspace.setGeometry()
> 

Yes I saw that.
I think the answer I was looking for is: the workspace view is added to added as a tab to the viewSelector :-)
Now I *think* the positionning of the ws view inside the view selector is still not clear. It comes from css (style_class: 'view-tab-page' maybe) ?

> 
> 
> [0] window previews and captions are in different groups because we used to
>     scale the workspace actor (with the window previews in it), and didn't
>     want the scale to affect the (fixed size) captions. It looks like the
>     workspaces are no longer scaled themselves, so it may be easier to use
>     a BoxLayout for every window which holds the (scaled down) window preview
>     and the (unscaled) caption; the height of one row would than be from the
>     (maximum possible) top of window previews to the bottom of the caption;
>     ensuring that the spacing between rows (e.g. from the bottom of the caption
>     to the top of the previews of the row below) is bigger than the spacing
>     between preview and caption (expressed by the 'spacing' CSS property) is
>     then trivial ...

Hmm... 
Or define 2 row spacing css properties: normal-caption-text-row-spacing and big-caption-text-row-spacing instead of my hard coded 'rowSpacing = 45' thing :-) ?

(In reply to comment #59)
> 
>  * Padding to the left and right of the window picker area - ie. the space
> between the thumbnails and the dash on the one side and the workspace selector
> on the other. This needs to be even, and a minimum amount of padding needs to
> be enforced.

We want to evenly space objects from different levels of the view hierarchy:
ViewSelector: Dash
 \-- Workspaces View: WS Switcher
     \-- Workspace
To evenly space Dash<->WS and WS<->switcher, the workspace view needs to know the Dash size/position... Or I am completely missing something.

> 
>  * Vertical padding between thumbnails - they're too close at the moment. 

See above comment.

> 
>  * Thumbnail scaling - either needs to be logarithmic (or something else that
> isn't linear) or needs to be dropped for now

I'm still annoyed with this one... I'm still searching a good looking formula. 
Any proposal ?

(In reply to comment #64)
> Yes I saw that.
> I think the answer I was looking for is: the workspace view is added to added
> as a tab to the viewSelector :-)
> Now I *think* the positionning of the ws view inside the view selector is still
> not clear. It comes from css (style_class: 'view-tab-page' maybe) ?

Nope (except for some spacing/padding values). See _relayout() in overview.js.


> Hmm... 
> Or define 2 row spacing css properties: normal-caption-text-row-spacing and
> big-caption-text-row-spacing instead of my hard coded 'rowSpacing = 45' thing
> :-) ?

No, that is not an option. "Large Text" is what is directly exposed in the shell UI. In the Universal Access System Settings panel, the setting has four possible values ("Small", "Normal", "Large", "Larger"), but the underlying preference is actually a double.

You *can* use a CSS property like this if you specify it in 'em' instead of 'px', but the distance "from the bottom of one element to the top of the second-next element" is hardly intuitive ;-)

Created attachment 206547 [details] [review]
Ugly new version

Ok, here's a new version. !! WARNING: it's in ugly shape atm !!

It contains 2 fixes proposal:
 [0] horizontal centering of the workspace view (WSV) (with even spacing on both sides)
 [1] vertical row spacing is bigger (and works with any size of text)

I'm posting it before trying to clean it up to see how you feel with the technical choices.

[0]: I removed 'DASH_SPLIT_FRACTION' constant. Now the WSV is placed at x = dash width + spacing. It's width = monitor_width - x - control_width - spacing. I didn't manage to retrieve the 'spacing' property from CSS (gnome-shell was unhappy and crashed...), so for now I've hardcoded the 12px value.

[1]: for vertical spacing I tried a simpler solution, based on 1 assumption: all caption height are equal. With this precondition, the rowSpacing used equals row spacing + text height (see _computeAllWindowSlots)

What do you think ?

(In reply to comment #67)
> Created an attachment (id=206547) [details] [review]
...
> What do you think ?

Wow, couple of big changes here. The even padding on each side is great (although I think we want a little more than you have here), and the wider labels work really well. The vertical row padding looks good too.

The main issues I can see right now - close buttons get clipped on the right hand side, and the windows can get rearranged when the workspace switcher slides in. Also, the thumbnail captions get very narrow when there are 2 rows or less.

Created attachment 206664 [details] [review]
WIP patch++

Ok, here's another version: it's an incremental upate of the previous one.

Changes:
- I modified the code to get rid of Clutter warning when changing viewSelector pos/size
- left/right padding increased from 12 to 30
- I've slightly changed the layout algorithm but it's still not good enough. Will need some more tuning.
- the clipping rectangle is a bit bigger than total windows width, to avoid cutting the close button

Created attachment 206726 [details] [review]
workspacesView: Don't clip the overview to allocation anymore

This is a relic from the early days, when we displayed each workspace
separately at once in the overview. It hasn't been necessary since
the overview relayout.

Review of attachment 206664 [details] [review]:

Getting better. Allan, can you comment on the behaviour of this patch?

::: js/ui/workspace.js
@@ +35,3 @@
 
+const LAYOUT_SCALE_WEIGTH = 1;
+const LAYOUT_RATIO_WEIGTH = 0.1;

WEIGHT, not WEIGTH

@@ +1249,3 @@
+    	// return (3 / (3 + i)) * scale;
+    	// return (1 - i * 0.8) * scale;
+    	return scale;

Allan, can you modify this patch to uncomment out some of the lines and describe which window scale methods you like?

@@ +1264,3 @@
+            let col = assignment[i].column;
+
+			let s = this._computeWindowScale(windows[i], scale);

Indentation issues.

@@ +1313,3 @@
+            }
+
+			let s = this._computeWindowScale(windows[i], scale);

Indentation.

@@ +1321,3 @@
+            if (row % 2) {
+                // x is the left side of the windows. As we negate it, it now represents the right border of the window.
+                // So we substract scaled window width. Example with 1 window:

"subtract"

@@ +1521,3 @@
+            let newRatio = Math.abs(maxRowWidth / totalRowsHeight - workspaceRatio);
+
+			// log(newRowCount + "x" + newColumnCount + ": " + newScale + ", " + newRatio + " -> ");

Remove this for your final patch.

@@ +1529,3 @@
+				} else {
+					// keep new layout if scale gain outweight aspect ratio loss
+					// log("\t cas 1 : " + ((newScale - scale) * LAYOUT_SCALE_WEIGTH) + " >?> " + ((newRatio - ratio) * LAYOUT_RATIO_WEIGTH));

Remove.

@@ +1534,3 @@
+			} else {
+				if (newRatio > ratio) {
+					// worst scale & ratio -> baad layout

"bad"

@@ +1537,3 @@
+					betterLayout = false;
+				} else {
+					// log("\t cas 2 : " + ((ratio - newRatio) * LAYOUT_RATIO_WEIGTH) + " >?> " + ((scale - newScale) * LAYOUT_SCALE_WEIGTH));

Remove.

::: js/ui/workspacesView.js
@@ +170,3 @@
+        // with the actor visible (e.g: dash width change with overview visible)
+        this.actor.set_clip(this._clipX, this._clipY,
+                                    this._clipWidth, this._clipHeight);

Indentation.

@@ +860,2 @@
         let clipWidth = width - controlsVisible;
+        let clipHeight = fullHeight;

If you want to do this for real, you need to modify the code. As an example, below, the code does "fullHeight - clipHeight", which is now 0.

@@ +865,3 @@
+        // spacing is substracted after clip calculation, otherwise
+        // right window close button may be clipped
+        width = width - spacing;

No. Use something like the patch I just attached.

(In reply to comment #70)
> Created an attachment (id=206726) [details] [review]
> workspacesView: Don't clip the overview to allocation anymore
> 
> It hasn't been necessary since the overview relayout.

If by "not necessary" you mean that the clip does not make a visual difference, than you are wrong - have a close look at the view titles / search entry during a workspace switch (in particular swipe scrolling).

However if the visual change is intended and the designers agree, the code looks good (but you should update the commit message in that case).

Created attachment 207007 [details] [review]
New version

Here's a new version.
Changes:
- added a new css id (#window-clone) holding spacing properties. Maybe the naming isn't very good?
- fixed the custom scale function - using: f(x) =  4 / (3+x), with x € [0, 1] being window width / screen width
- fixed indentation issues

Still not done:
- clipping issue mentionned by Jasper in comment #71
- private property access for caption max width (comment #55)

Created attachment 207152 [details] [review]
New version

Here's my latest experimental patch for this bug :-)
After this one, I'll stop trying new things, and instead try to get at least 'something' merged.

Changes:
- I tried to prevent relayout when the workspace switcher slides in. It's really simple: a preserve-layout flag is set when a layout is choosen, and resetted on certain events (add window, remove window). Then we're asked to layout windows and the flag is set, the same layout is reused, potentially with a different scale. If this solution isn't good, I'll revert my earlier modification regarding workspace geometry change on workspace switcher event.
- CSS naming changes.
- private access to window caption property
- probably tons of indentation issues (my fault for using a poor text editor I guess...).

Also, this layout thing is getting bigger and bigger, maybe it could be split in another js file (layoutHelper.js?) ?

Review of attachment 207152 [details] [review]:

The resulting placement looks good the algorithm looks overly complicated though.
It is not really obvious what it is doing (and why it does it this way).
There are lots of style issues in there (whitespaces and indention). 
The commit message is missing a body and add the bug reference at the bottom of (full url).

::: js/ui/overview.js
@@ +195,2 @@
         this._workspacesDisplay = new WorkspacesView.WorkspacesDisplay();
+        

Useless whitespace.

@@ +211,2 @@
         // TODO - recalculate everything when desktop size changes
+        

Useless whitespace.

@@ +223,3 @@
+        	// This is deferred to avoid Clutter-WARNING like this one:
+        	// Clutter-WARNING **: The actor 'ClutterGroup' is currently inside an allocation cycle; calling clutter_actor_queue_relayout() is not recommended
+			// Not sure if it's a good idea though

It is at least that is the solution we use all over the place to deal with that so just remove the comment.

@@ +225,3 @@
+			// Not sure if it's a good idea though
+			Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,
+            function () {

Broken indentation.

@@ +539,3 @@
+        let viewHeight = contentHeight - 2 * this._spacing;
+        let viewY = contentY + this._spacing;
+        let viewX = rtl ? 0 : dashWidth + this._spacing;

You can't just assume 0 here the primary monitor might be at a position != 0 so use primary.x instead (same for calculating the y values).

@@ +611,3 @@
+        // change its size.
+        this._dash.actor.show();
+        // At this step, dash has its final size, we can safely update view 

clutter_actor_show is not synchronous, it will just set a flag and call clutter_actor_queue_redraw on the the parent.

::: js/ui/workspace.js
@@ +422,3 @@
         this._parentActor = parentActor;
         this._hidden = false;
+        this._allowedTitleWidth = windowClone.actor.width;

Allowed sounds weird ... maxTitleWidth would make more sense and fits better with its usage.

@@ +1253,3 @@
 
+    _computeWindowScale: function(window, scale) {
+    	 let i = 0;

Indentation is wrong here. Should be 4 spaces.

@@ +1258,3 @@
+        else
+    	    i = window.actor.height / Main.layoutManager.primaryMonitor.height;
+        //return (Math.log(1 + i) / i) * scale;

Remove this comments.

@@ +1259,3 @@
+    	    i = window.actor.height / Main.layoutManager.primaryMonitor.height;
+        //return (Math.log(1 + i) / i) * scale;
+    	 return (2 / (1 + i)) * scale;

This needs a comment because it is not immediately obvious why its done this way.

@@ +1266,2 @@
+    // returns the width/height for non-grid aligned rows
+    _computeRowSizesNonGridAligned: function(windows, rowCount, scale, rowSpacing, columnSpacing, assignment) {

assignment is kind of non descriptive.

@@ +1285,3 @@
 
+    // returns origin point for non-grid aligned rows
+    _computeRowOriginsNonGridAligned: function(rowsSize, rowCount, rowSpacing, columnSpacing, availArea) {

s/origin/position/

@@ +1310,3 @@
         let slots = [];
+
+        let totalWindows = windows.length;

s/totalWindows/windowCount/

@@ +1336,3 @@
+                // |...|WIN|...|
+                //     ^x  ^-x
+                x = availArea.width - x - windows[i].actor.width * s;

I don't get this ... why don't you just add the new scaled with to x ?

@@ +1351,3 @@
+    },
+
+    _computeWindowsSlotGridAligned: function(windows, layout, rowSpacing, columnSpacing, availArea, assignment) {

Hmmm ... so end up having lots of complicated _computeWindowFoo functions that hardly share any code. You should try to factor out common stuff into separate functions.

@@ +1415,3 @@
+            for (; windowIdx < windows.length; windowIdx++) {
+                let w = windows[windowIdx].actor.width;
+                let oldDiff = rowWidth / idealRowWidth;

That isn't a difference so calling it "diff" is kind of odd. (actually its a ratio).

@@ +1440,3 @@
+    
+    _computeMaxRowWidthTotalHeightAvailableAreaForLayout: function(rowCount, columnCount, windows, rowSpacing, columnSpacing, assignments, overlayChromeCaptionHeight, windowsOverlayHeight) {
+        let totalWindows = windows.length;

s/totalWindows/windowCount/

@@ +1482,3 @@
+        let node = this.actor.get_theme_node();
+        availAreaForWindows.width -= node.get_length('spacing');
+        

To much whitespace.

@@ +1501,3 @@
+
+    // Returns a triplet [row count, column count, scale]
+    // We look for the largest scale that allows us to fit the largest row/tollest column on the workspace.

tollest ? ;)

@@ +1503,3 @@
+    // We look for the largest scale that allows us to fit the largest row/tollest column on the workspace.
+    _computeRowColumnCountScaleAndAvailArea: function(windows, rowSpacing, columnSpacing, overlayChromeCaptionHeight, windowsOverlayHeight) {
+        let monitor = Main.layoutManager.primaryMonitor;

Your not using this.

@@ +1504,3 @@
+    _computeRowColumnCountScaleAndAvailArea: function(windows, rowSpacing, columnSpacing, overlayChromeCaptionHeight, windowsOverlayHeight) {
+        let monitor = Main.layoutManager.primaryMonitor;
+        let totalWindows = windows.length;

windowCount

@@ +1536,3 @@
+            newRatio = Math.abs(newRatio - area.width / area.height);
+
+			let betterLayout = false;

Broken indentation.

@@ +1538,3 @@
+			let betterLayout = false;
+			if (newScale >= scale) {
+				if (newRatio <= ratio) {

Broken indentation.

@@ +1542,3 @@
+					betterLayout = true;
+				} else {
+					// keep new layout if scale gain outweights aspect ratio loss

Broken indentation.

@@ +1547,3 @@
+			} else {
+				if (newRatio > ratio) {
+					// worst scale & ratio -> worse layout

Broken indentation.

@@ +1550,3 @@
+					betterLayout = false;
+				} else {
+					// keep new layout if aspect ratio gain outweights scale loss

Broken indentation.

@@ +1551,3 @@
+				} else {
+					// keep new layout if aspect ratio gain outweights scale loss
+					betterLayout = ((ratio - newRatio) * LAYOUT_RATIO_WEIGHT > (scale*scale - newScale*newScale) * LAYOUT_SCALE_WEIGHT);

Missing whitespaces.

@@ +1572,3 @@
+    // return [x,y,scale] for each window
+    _computeAllWindowSlots: function(windows) {
+        let totalWindows = windows.length;

windowCount

@@ +1574,3 @@
+        let totalWindows = windows.length;
+        let node = this.actor.get_theme_node();
+        const columnSpacing = node.get_length('-horizontal-spacing');

Why const ?

::: js/ui/workspacesView.js
@@ +848,3 @@
         let width = fullWidth;
         let height = fullHeight;
+		let spacing = 30; // TODO: read from CSS (overview: spacing)

Indentation broken.

Created attachment 207436 [details]
screenshot of the patch in action

(In reply to comment #74)
> Created an attachment (id=207152) [details] [review]
> New version
> 
> Here's my latest experimental patch for this bug :-)
> After this one, I'll stop trying new things, and instead try to get at least
> 'something' merged.
...

Hey Eric, I've tried your patch. The behaviour is excellent. I see that you have got the thumbnail sizing right now, I can also set the correct padding between thumbnails using the theme, which is great. This is almost there from a design point of view.

Two minor issues I noticed:

 * There seems to be slightly different padding on the left and right side of the picker area (the gap between the picker and the ws switcher is larger than on the other side)

 * When the thumbnails are being displayed in a grid, the grid is ordered left to right rather than right to left (see the attachment).

(In reply to comment #76)
>  * When the thumbnails are being displayed in a grid, the grid is ordered left
> to right rather than right to left (see the attachment).

This is intentional and useful behavior. This is so that if you close a window early in the grid, the former starts of the rows don't cross the entire screen to get to the ends of the previous rows. Snaking like this to prevent movement is a good solution.

Review of attachment 206726 [details] [review]:

Are you sure this is right:

 - When dragging on the background to switch workspaces
 - When animating workspace changes?

Aren't we counting on clipping to avoid windows going up under the search entry and panel?

(In reply to comment #78)
> Aren't we counting on clipping to avoid windows going up under the search entry
> and panel?

I'd say yes, see comment #72

Review of attachment 206726 [details] [review]:

(marking rejected to get it off of the patch review list)

Review of attachment 206726 [details] [review]:

(marking rejected to get it off of the patch review list)

(In reply to comment #74)
> Created an attachment (id=207152) [details] [review]
> New version
> 
> Here's my latest experimental patch for this bug :-)
> After this one, I'll stop trying new things, and instead try to get at least
> 'something' merged.
> 
> Changes:
> - I tried to prevent relayout when the workspace switcher slides in. It's
> really simple: a preserve-layout flag is set when a layout is choosen, and
> resetted on certain events (add window, remove window). Then we're asked to
> layout windows and the flag is set, the same layout is reused, potentially with
> a different scale. If this solution isn't good, I'll revert my earlier
> modification regarding workspace geometry change on workspace switcher event.
> - CSS naming changes.
> - private access to window caption property
> - probably tons of indentation issues (my fault for using a poor text editor I
> guess...).
> 
> Also, this layout thing is getting bigger and bigger, maybe it could be split
> in another js file (layoutHelper.js?) ?

Sure, I'm fine with that.

Pierre, are you ever going to come back to this?

Created attachment 220742 [details] [review]
workspace: Reindent

Created attachment 220743 [details] [review]
workspace: Position windows only when needed

Right now, when entering the overview, we compute the window slots about
four or five times, from scratch each time. Move to a queued system where
extraneous calls to positionWindows don't matter.

Created attachment 220744 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 220745 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
too see my workspaces.

Created attachment 220746 [details] [review]
workspacesView: Add some spacing between the workspaces and thumbs

Make it the same padding as between the overview and dash, too.

Created attachment 220747 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.

Created attachment 220748 [details] [review]
workspace: Have a stable window order

Created attachment 220749 [details] [review]
workspace: Refactor code a bit

I don't have much free time at the moment, so thanks a lot for cleaning up the patches and trying to get them merged Jasper.

(In reply to comment #91)
> I don't have much free time at the moment, so thanks a lot for cleaning up the
> patches and trying to get them merged Jasper.

Thanks again for your work Pierre-Eric!

(In reply to comment #90)
> Created an attachment (id=220749) [details] [review]
> workspace: Refactor code a bit

I tried out these patches. On the whole the behaviour is excellent, although there are a few improvements required:

 * When there are only two windows, they are stacked vertically. This feels wrong (even if it is the most efficient layout in terms of window size).
 * Likewise, when I have 6 windows open, I get a 3x2 grid, when it feels like I should get 2x3
 * There needs to be more vertical padding between thumbnails (should be the same as the horizontal padding).
 * The padding between the thumbnails and dash on the left and the workspace switcher on the right is still a bit out - it should be even.

(In reply to comment #93)
> (In reply to comment #90)
> > Created an attachment (id=220749) [details] [review] [details] [review]
> > workspace: Refactor code a bit
> 
> I tried out these patches. On the whole the behaviour is excellent, although
> there are a few improvements required:
> 
>  * When there are only two windows, they are stacked vertically. This feels
> wrong (even if it is the most efficient layout in terms of window size).
>  * Likewise, when I have 6 windows open, I get a 3x2 grid, when it feels like I
> should get 2x3

This is because you have workspaces open. There used to be some code that kept the aspect ratio correct. It was removed so that it would use the entire height of the view. Which makes the code go "oh, OK, I'll use the full height". Note that there should be some code in there that determines whether the new layout is "better" because it matches the aspect ratio of the actual area.

>  * There needs to be more vertical padding between thumbnails (should be the
> same as the horizontal padding).

Fiddle around with the size.

>  * The padding between the thumbnails and dash on the left and the workspace
> switcher on the right is still a bit out - it should be even.

Sigh. I tried for a long time to get this right, but the problem is that the overview is a large pile of hacks. I intentionally stepped away from this, because both interns are changing it all around, and I didn't want stubbed toes.

(In reply to comment #94)
>
> Fiddle around with the size.

By which I mean "fiddle around with the CSS" -- there's -horizontal-spacing and -vertical-spacing. Find some values that look right (hint: making them the same does not look "right")

Created attachment 220825 [details] [review]
workspace: Reindent

Created attachment 220826 [details] [review]
workspace: Position windows only when needed

Right now, when entering the overview, we compute the window slots about
four or five times, from scratch each time. Move to a queued system where
extraneous calls to positionWindows don't matter.

Created attachment 220827 [details] [review]
workspacesView: Don't conform to aspect ratio

We want the window picker to use the full height of the area it's given.

Created attachment 220828 [details] [review]
workspace: Make sure to hide other workspace when returning from the overview

Created attachment 220829 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 220830 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.

Created attachment 220831 [details] [review]
workspacesView: Add some spacing between the workspaces and thumbs

Make it the same padding as between the overview and dash, too.

Created attachment 220832 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.

Created attachment 220833 [details] [review]
workspace: Have a stable window order

Created attachment 220834 [details] [review]
workspace: Refactor code a bit

Created attachment 220923 [details]
Before and after screenshots with 9 windows open

When testing these patches, I noticed a sub-optimal layout with 9 windows. You can see that the patches result in two rows of thumbnails, rather than a 3x3 grid.

Created attachment 221486 [details] [review]
workspace: Reindent

Fix some of the issues that Allan was talking about, and rebase.

Created attachment 221487 [details] [review]
workspace: Position windows only when needed

Right now, when entering the overview, we compute the window slots about
four or five times, from scratch each time. Move to a queued system where
extraneous calls to positionWindows don't matter.

Created attachment 221488 [details] [review]
workspacesView: Don't conform to aspect ratio

We want the window picker to use the full height of the area it's given.

Created attachment 221489 [details] [review]
workspace: Make sure to hide other workspace when returning from the overview

Created attachment 221490 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 221491 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.

Created attachment 221492 [details] [review]
workspacesView: Add some spacing between the workspaces and thumbs

Make it the same padding as between the overview and dash, too.

Created attachment 221493 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.

Created attachment 221494 [details] [review]
workspace: Have a stable window order

Created attachment 221495 [details] [review]
workspace: Refactor code a bit

I've been rebasing the code on the wip/rewindow branch. Please try it out there.

Review of attachment 221486 [details] [review]:

Sure.

Review of attachment 221487 [details] [review]:

Makes sense.

Attachment 221486 [details] pushed as 4ff0697 - workspace: Reindent
Attachment 221487 [details] pushed as 3776c50 - workspace: Position windows only when needed

This is looking really good to me. A few small issues:

 * When there are only two windows, they are sometimes stacked on top of each other, rather than being placed side-by-side. Even if the vertical arrangement is more efficient, I'd still recommend a side-by-side layout for two windows (assuming that the screen is landscape). It just feels wrong to have them on top of each other.

 * The minimum amount of vertical padding needs to be increased.

 * I'm sure I've seen vertical padding grow unecessarily, but I won't quibble over that. ;)

Another place where these patches could be refined is in the allocation of the space to the thumbnail group as a whole. Having discussed this with Jasper, I have some guidance:

 * Dynamically allocate the maximum amount of screen width to the window thumbnails, so that space is reallocated as both the dash and workspace switcher change size.

 * In cases where the group of window thumbnails requires less than the available screen width, attempt to horizontally center it in the middle of the screen. This will ensure that it aligns with the search bar and clock above.

Created attachment 222006 [details] [review]
workspacesView: Don't conform to aspect ratio

We want the window picker to use the full height of the area it's given.

Created attachment 222008 [details] [review]
workspace: Make sure to hide other workspace when returning from the overview

Created attachment 222009 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 222010 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.

Created attachment 222011 [details] [review]
workspace: Have a stable window order

Created attachment 222012 [details] [review]
workspace: Refactor code a bit

Created attachment 222013 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.

Created attachment 222014 [details] [review]
workspacesView: Refactor thumb zooming out code

Created attachment 222015 [details] [review]
workspacesView: Add some more spacing between window and workspace thumbs

The attached patch stack is the one I want to try to land today. Florian, please review.

There's two patches that are a bit iffy:

 * overview: Reduce space between window picker and dash

This one would be helped immensely by "overview as boxlayouts". I still need to finish that patch (which requires investigation), so it may not land in time for this landing. I hope the hacky solution is alright until we can land it for real.

 * workspacesView: Add some more spacing between window and workspace thumbs

This one is simply accessing Main.overview._spacing directly. This is a quick fix to make the property public (and a previous patch did that until I had to rebase on top of the mode switch kill)

Review of attachment 222011 [details] [review]:

Squash this with the main patch

Review of attachment 222012 [details] [review]:

*shrug* sure, why not

Review of attachment 222013 [details] [review]:

Mmmh, okay

::: js/ui/overview.js
@@ +493,3 @@
         let searchY = contentY + this._spacing;
 
+        let dashWidth = DASH_MAX_SIZE;

DASH_MAX_WIDTH would be a slightly better name

Review of attachment 222015 [details] [review]:

::: js/ui/workspacesView.js
@@ +872,2 @@
         let widthAdjust = this._zoomOut ? controlsNatural : controlsVisible;
+        widthAdjust += Main.overview._spacing;

_spacing is private!

Review of attachment 221492 [details] [review]:

Not sure whether you want to land this patch or the two separate ones ...

::: js/ui/overview.js
@@ +122,3 @@
         global.overlay_group.add_actor(this._desktopFade);
 
+        this.spacing = 0;

Mmmh, not really a fan - given that
  - there are plans to move the workspace
    thumbnails into the overview
  - the left/right padding is only the
    same iff the dash takes up the entire
    width
I'd rather leave this private and use a separate style property for the windows/workspaces spacing for now

Review of attachment 222010 [details] [review]:

::: js/ui/workspace.js
@@ +1538,3 @@
+    _rectEqual: function(one, two) {
+        if (one == two)
+            return false;

false is completely unexpected here - if anything relies on that behavior, this needs a comment

Review of attachment 222009 [details] [review]:

Couple of nitpicks

::: js/ui/workspace.js
@@ +35,3 @@
 
+const LAYOUT_SCALE_WEIGHT = 1;
+const LAYOUT_RATIO_WEIGHT = 0.01;

Those could use some explanation

@@ +718,3 @@
+
+    computeWindowSlots: function(layout, area) {
+        this._computeRowSizes(layout);

A dummy implementation of this (and computeLayout()) would be nice for documentation

@@ +736,3 @@
+        for (let i = 0; i < rows.length; i++) {
+            let row = rows[i];
+            row.y += baseY;

I'd just use baseY directly in the y computation below instead of iterating over all rows again

@@ +1475,3 @@
 
+    _isBetterLayout: function(oldLayout, newLayout) {
+        if (oldLayout.scale === undefined || newLayout.scale === undefined)

Huh? If newLayout.scale === undefined, it is *better*?

@@ +1516,3 @@
+            let layout = { strategy: strategy, numRows: numRows, numColumns: numColumns };
+            strategy.computeLayout(windows, layout);
+            strategy.computeScaleAndRatio(layout, area);

Not really happy with the mix of public and strategy-internal properties in layout, but well ...

@@ +1518,3 @@
+            strategy.computeScaleAndRatio(layout, area);
+
+            if (this._isBetterLayout(lastLayout, layout) || (numRows == 0)) {

When is numRows 0?
(also: no braces)

@@ +1546,3 @@
+                let [closeButtonHeight, captionHeight] = overlay.chromeHeights();
+                maxCloseButtonHeight = Math.max(maxCloseButtonHeight, closeButtonHeight);
+                maxCaptionHeight = Math.max(maxCaptionHeight, captionHeight);

You could just do

  if (overlay) {
      [closeButtonHeight, captionHeight] = overlay.chromeHeights();
      break;
  }

(and use those instead of the maxFooHeight variables) - the values are the same for all overlays

Review of attachment 222008 [details] [review]:

It shouldn't come surprising that I prefer the fix in bug 682002 :-)

In any case, "hide other workspace" is not quite the same as destroying them ...

Review of attachment 222006 [details] [review]:

To be honest, I don't see any visual difference between what you remove here and what you re-add later (and I did a side-by-side comparison with screenshots). If you must change the method for avoiding a relayout, it makes sense in my opinion to merge both patches.

Review of attachment 222014 [details] [review]:

Sure

Review of attachment 222006 [details] [review]:

re-add later? Where?

This isn't part of avoiding a relayout, either. Did you review the wrong patch?

Review of attachment 222008 [details] [review]:

If you want to fix this one, feel free.

Review of attachment 222009 [details] [review]:

::: js/ui/workspace.js
@@ +1516,3 @@
+            let layout = { strategy: strategy, numRows: numRows, numColumns: numColumns };
+            strategy.computeLayout(windows, layout);
+            strategy.computeScaleAndRatio(layout, area);

Would you prefer that I had a "strategyData" struct inside the layout?

Maybe I should document this, but the point of not saving data inside the strategy is so that the next commit, which re-runs only a part of the strategy, is as simple as possible. Otherwise, we would have to "reset" the strategy between runs.

Review of attachment 222010 [details] [review]:

::: js/ui/workspace.js
@@ +1538,3 @@
+    _rectEqual: function(one, two) {
+        if (one == two)
+            return false;

Nice catch. We will never hit this, because we always build a new rectangle. I just threw it in there because why not.

Review of attachment 222015 [details] [review]:

Should I make it public, or?

(I saw your comments before. I was assuming we could get "overview as boxlayouts" in before this. How do you feel about landing this with a comment/commit message saying that this is a hack?)

Created attachment 222086 [details] [review]
workspacesView: Don't conform to aspect ratio

We want the window picker to use the full height of the area it's given.

Created attachment 222088 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 222089 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.

Created attachment 222090 [details] [review]
workspace: Refactor code a bit

Created attachment 222091 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.

Created attachment 222092 [details] [review]
workspacesView: Refactor thumb zooming out code

Created attachment 222093 [details] [review]
workspacesView: Add some more spacing between window and workspace thumbs

Attachment 222090 [details] pushed as 6851c54 - workspace: Refactor code a bit
Attachment 222092 [details] pushed as efc128e - workspacesView: Refactor thumb zooming out code


Pushed these two previously ACN fixes.

Created attachment 222276 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>



Whoops. I completely forgot about the dummy methods for documentation. Added those.

Created attachment 222277 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.



Rebased onto master, and made this commit a bit less messy by squashing
some parts into previous commits.

Created attachment 222278 [details] [review]
overview: Reduce space between window picker and dash

Do this in a hacky way by hardcoding this. When mode switch or whatever
lands, hopefully we'll fix the overview layout code up.



These two are hacks and are designed to be hacks. If we don't want
to land them, OK with you.

Created attachment 222279 [details] [review]
workspacesView: Add some more spacing between window and workspace thumbs

I'm fine with rewriting this to make Main.overview._spacing public if you
are fine with this strategy.

(In reply to comment #142)
> Review of attachment 222006 [details] [review]:
> 
> re-add later? Where?
> 
> This isn't part of avoiding a relayout, either.

Not explicitly, but by keeping the aspect ratio when popping out the workspace switcher, the algorithm picks the same layout (using a smaller scale for windows of course). Your later patch explicitly keeps the old layout, but still adjusts the windows' scales.
As mentioned, if I compare your branch with a local branch where I remove the two patches in question, I'm unable to spot a visual difference. So while the change still makes sense to me now that we have an explicit layout that is passed around, the two patches are clearly related imho.

(In reply to comment #158)
> I'm fine with rewriting this to make Main.overview._spacing public if you
> are fine with this strategy.

I was looking into parts of the search stuff, as I suspect that this will allow us to avoid the issue altogether. As mentioned there though, I see that more like 3.8 material, so we'd need to go with what we have here ...

(In reply to comment #155)
> Created an attachment (id=222276) [details] [review]
> workspace: Use all available space for windows in window selector

Looks like this one has regressed (on origin/wip/rewindow)? Two windows are consistently stashed vertically for me ...


> Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Btw, how heavily? Does it make sense to leave the original attribution?

In reply to comment #161)
> Looks like this one has regressed (on origin/wip/rewindow)? Two windows are
> consistently stashed vertically for me ...

There's a lot of factors to balance. It probably identifies that putting the windows vertically stacked would give your windows a better scale, compared to the one-row layout. You can debug it if you want, but we should probably just hardcode that case, or pump up the aspect ratio rate a bunch. Would you mind playing with it for me? My laptop stacks them horizontally.

> > Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>
> 
> Btw, how heavily? Does it make sense to leave the original attribution?

A lot of the algorithms and techniques are the same.(In reply to comment #159)
> (In reply to comment #142)
> > Review of attachment 222006 [details] [review] [details]:
> > 
> > re-add later? Where?
> > 
> > This isn't part of avoiding a relayout, either.
> 
> Not explicitly, but by keeping the aspect ratio when popping out the workspace
> switcher, the algorithm picks the same layout (using a smaller scale for
> windows of course). Your later patch explicitly keeps the old layout, but still
> adjusts the windows' scales.
> As mentioned, if I compare your branch with a local branch where I remove the
> two patches in question, I'm unable to spot a visual difference. So while the
> change still makes sense to me now that we have an explicit layout that is
> passed around, the two patches are clearly related imho.

It's a subtle change, but if you have a few windows that, and then try to zoom in the workspace switcher, it won't adjust the scale of the windows if they fit, but just slide them over. The layout doesn't change at all, and the scale doesn't, either.

(In reply to comment #162)
> > As mentioned, if I compare your branch with a local branch where I remove the
> > two patches in question, I'm unable to spot a visual difference. So while the
> > change still makes sense to me now that we have an explicit layout that is
> > passed around, the two patches are clearly related imho.
> 
> It's a subtle change, but if you have a few windows that, and then try to zoom
> in the workspace switcher, it won't adjust the scale of the windows if they
> fit, but just slide them over.

Mmh, okay - I haven't seen it, but I'll take your word for it. As mentioned before, I don't mind the change itself - all I'm really saying is that rather than something like:

 - Break stable layouts when showing workspace switcher
 - Big changes to layout
 - Make layouts stable when showing workspace switcher

I'd prefer something like:

 - Big changes to layout
 - Make stable layouts when showing workspace switcher more explicit

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

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

Just putting the aspect ratio patch after the layout changes should do that, and that's a very trivial rebase (I don't think there's any merge conflicts at all).

If that's all you want, can I get a review on the new patches?

Ping?

It doesn't look like comment #161 has been addressed, so needs-work.

(In reply to comment #168)
> It doesn't look like comment #161 has been addressed, so needs-work.

As I've said before, the algorithm has figured out that putting windows on top is a better use of space. Do we want to hard-code the two-window case, which would be unoptimal for portrait-style monitors? Can you play with LAYOUT_RATIO_WEIGHT until you find something that looks good?

(In reply to comment #169)
> (In reply to comment #168)
> > It doesn't look like comment #161 has been addressed, so needs-work.
> 
> Do we want to hard-code the two-window case, which would be unoptimal 
> for portrait-style monitors?

I assume we want a rule that columns >= rows for landscape (possibly the reverse for portrait orientation), not a special rule for the two-window case (note the 2-3 vs 3-2 item in comment #93) - Allan?

That's what LAYOUT_RATIO_WEIGHT does. It will try to minimize the difference between the screen's aspect ratio and the layout's aspect ratio. Try fiddling around with it.

(In reply to comment #171)
> That's what LAYOUT_RATIO_WEIGHT does. It will try to minimize the difference
> between the screen's aspect ratio and the layout's aspect ratio.

No, it's slightly different. Consider the following extreme case of two windows, each filling the full screen width and half the screen height - a vertical layout is perfect in terms of matching the screen ratio, but it still violates the columns >= rows rule.


> Try fiddling around with it.

That means it'll work for my resolution and the windows I happen to test with. If we always want that behavior (Allan?), we should have an explicit rule.

Oh, if you want an explicit "aspect ratio of the grid is the same as the aspect ration of the monitor", that's going to knock out a lot of good optimal solutions. Imagine tall, skinny windows (XChat, a few Empathy chat windows, GIMP toolbox, buddy list, whatever) in a grid formation. That's taller than it is wide, because the windows are skinny. Forcing the aspect ratio would decrease the scale, most likely punting it to be all one row, and make the top/bottom unused.

> (In reply to comment #171)
> > That's what LAYOUT_RATIO_WEIGHT does. It will try to minimize the difference
> > between the screen's aspect ratio and the layout's aspect ratio.
> 
> No, it's slightly different. Consider the following extreme case of two
> windows, each filling the full screen width and half the screen height - a
> vertical layout is perfect in terms of matching the screen ratio, but it still
> violates the columns >= rows rule.

IMO, that's a more optimal layout.

Also, it's not "columns >= rows" (in that case, there are 2 rows, and 1 column, so it doesn't violate it. I assume you got your rows and columns mixed up), it's "width of the grid vs. height of the grid", in pixels. For one/two rows, it cares about each individual window's size, scaled, along with constant spacing between the windows.

For three+ rows, yes, the grid size is based on the number of windows, and the size of the maximum window.

(In reply to comment #173)
> Oh, if you want an explicit "aspect ratio of the grid is the same as the aspect
> ration of the monitor", that's going to knock out a lot of good optimal
> solutions.

No, that's explicitly *not* what I was saying. The problem I would like to see addressed is basically an optical illusion, where a horizontal layout that scores good appears vertical to the eye. So tweaking the layout ratio is probably going to make it worse, as it favors the technically better layout over the one that is *perceived* better.


> > No, it's slightly different. Consider the following extreme case of two
> > windows, each filling the full screen width and half the screen height - a
> > vertical layout is perfect in terms of matching the screen ratio, but it still
> > violates the columns >= rows rule.
> 
> IMO, that's a more optimal layout.

Yes, in this extreme case that's certainly better (both regarding width/height ratio and perceived orientation). I'm not sure we actually need to get an optimal result for a corner case like this, but we could certainly add a weight for the "columns >= rows" rule as well.


> Also, it's not "columns >= rows" (in that case, there are 2 rows, and 1 column,
> so it doesn't violate it. I assume you got your rows and columns mixed up),

Unless 1 >= 2 === true, I'm not the one that got rows and columns mixed up ;-)


> it's "width of the grid vs. height of the grid", in pixels.

No, that's not what I'm talking about - that part is implemented and generally works, with the exception of some corner cases. I'm talking about "maximum number of items stacked horizontally" vs "maximum number of items stacked vertically", independent from any pixel values.

So Florian, where did we land on this with our last IRC conversation, and what next?

(In reply to comment #174)
> Yes, in this extreme case that's certainly better (both regarding width/height
> ratio and perceived orientation). I'm not sure we actually need to get an
> optimal result for a corner case like this, but we could certainly add a weight
> for the "columns >= rows" rule as well.

I don't know how you can weight a boolean like this. I know we discussed this on IRC, but I forgot.

Created attachment 226209 [details] [review]
workspace: Replace "ratio" with "space"

Through testing, we've found that the percentage of available space
is a better indicator than the difference in aspect ratio, which
is subtle and not what the eye sees.

This is another patch that eventually should be squashed; just attaching for quick feedback. Review should be normal, as usual.

Review of attachment 226209 [details] [review]:

Looks reasonable enough to land (at least now we have a couple of months to fix eventual issues), so squash away and I'll do a final review before rolling 3.7.1 (I'm assuming that the other patches are merely rebased)

Review of attachment 226209 [details] [review]:

Not sure what you want me to do. Do you want me to squash and attach the final patch here, or squash and push?

Squash and attach.

Created attachment 227067 [details] [review]
workspace: Use all available space for windows in window selector

Change the layout strategy to be more like the mockups. With less than
two rows of windows, we try to fit every window in a non-aligned situation;
with more than three rows of windows, we try to fit every window in an
aligned situation.

Based heavily on a patch from Pierre-Eric Pelloux-Prayer <pelloux@gmail.com>

Created attachment 227068 [details] [review]
workspace: Don't relayout windows when zooming workspace thumbnails

It looks ugly and busy to have windows shuffle around when I just wanted
to look at my workspaces.

Did you actually squash anything? In particular, did you address comment #163?

I squashed the "ratio => space" patch into the main patch. Comment #163 involves simply changing the patch order, as far as I can see it.

I've already done the reordering locally. Would you like me to reattach the entire existing patch stack, without changes, so the correct order is in git bz apply?

No, comment #163 refers to squashing the first and last patch (as in the current order).

That does not make any sense to me. The intent of the first patch is not to retain stable layouts, but to use all of the available space.

The fixed layout was what kept the layout stable, so the last patch fixes a problem introduced by the first one.

Created attachment 227074 [details] [review]
workspacesView: Don't conform to aspect ratio

We want the window picker to use the full width and height of the area
it's given, so that it can utilize all of it. This means that the area
when zooming out the thumbnails vs. zooming in the thumbnails may be
different. To prevent thumbnails from being arranged differently in this
case, save the existing layout of the workspace thumbnails and only
invalidate it when we need to, like when the number of windows changes.



Is this something like what you want?

Review of attachment 222278 [details] [review]:

"When mode switch or whatever lands" - mode switch or whatever has landed, please update the message accordingly.

Review of attachment 222279 [details] [review]:

Hackish, but ok for now

Review of attachment 227067 [details] [review]:

Good to push with the style glitch fixed

::: js/ui/workspace.js
@@ +1559,3 @@
+            strategy.computeScaleAndSpace(layout);
+
+            if (this._isBetterLayout(lastLayout, layout)) {

No braces

Review of attachment 227074 [details] [review]:

(In reply to comment #189)
> Is this something like what you want?

The subject doesn't make much sense for a combined patch, either fix that or use the separate patches. Minor style nit pointed out below:

::: js/ui/workspace.js
@@ +1613,3 @@
         area.height -= closeButtonHeight;
 
+        if (!this._currentLayout) {

No braces

Review of attachment 227074 [details] [review]:

What doesn't make sense for a combined patch? Please either suggest an alternate commit message, or tell me to keep the patches separate.

I was thinking of something along the suggestion in comment #163; if you want the no-aspect-ratio/more-space aspect in the subject, just go with the separate patches.

(In reply to comment #195)
> I was thinking of something along the suggestion in comment #163; if you want
> the no-aspect-ratio/more-space aspect in the subject, just go with the separate
> patches.

Can you review the separate patches, then?

Uhm - those would be the same, no? Should be good with the style glitch fixed.

Attachment 222278 [details] pushed as 3ffeeac - overview: Reduce space between window picker and dash
Attachment 222279 [details] pushed as 96556eb - workspacesView: Add some more spacing between window and workspace thumbs
Attachment 227067 [details] pushed as d9b46b4 - workspace: Use all available space for windows in window selector
Attachment 227074 [details] pushed as d7929a2 - workspacesView: Don't conform to aspect ratio


Fixed up and pushed. Thanks!

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

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

I think it is not fixed at all, see:

9 windows:
original:
http://postimage.org/image/kptcxff3t/
edited:
http://postimage.org/image/v46bc7tul/

8 windows:
original:
http://postimage.org/image/i7gwbtacb/
edited:
http://postimage.org/image/53nex3a3h/

But I don't know if it is the intended behaviour

The second one is certainly intended, as we explicitly require some amount of padding.

The first one isn't intended necessarily, but I can't find a better layout than that.

Jasper,

Yes, I was thinking in that and I can't find a better solution too...for the grid layout I guess you make the better solution.

Maybe the problem is using a grid layout, don't you think?

(In reply to comment #203)
> Maybe the problem is using a grid layout, don't you think?

It could potentially get quite messy, but we could certainly try it.

Another thing that would help here would be to give more vertical space to the window selector. We already have a bug filed about hiding the message tray in the overview. We could try hiding the search bar too.

I'm not finding anything in this Activities mode. 

You're shuffling windows all over the place and it's totally illogical for me to find anything. I have to run my eyes through every window to find what I'm looking for. 

I have a separate bug:

https://bugzilla.gnome.org/show_bug.cgi?id=771234