Created attachment 174550 [details] [review]
linear-view: Remove the scrollbar

The scrollbar is the main culprit for cluttered controls in the
linear view - all its functionality is already provided by the
workspace indicators, so it is save to remove the scrollbar in
order to clean up the interface.

Created attachment 174551 [details] [review]
overview: Replace InfoBar with message tray notifications

The layout of recent mockups occupies the space previously reserved
for the info bar with the view selector. As the bar's purpose is
mainly to provide the user with feedback, it makes sense to use the
existing message tray facility instead of moving the bar elsewhere.

Created attachment 174552 [details] [review]
overview: Do not zoom the desktop background

While scaling the desktop background with the window previews represents
workspaces quite intuitively, the approach is not without problems.
As window previews in the overview behave quite differently to "real"
windows, the representation of workspaces as miniature versions of
"real" workspaces is flawed. The scaling also makes the transitions
to and from the overview much more visually expensive, without adding
much benefit.
Leaving the background in place provides more visual stability to the
transitions and emphasizes the distinctive behavior of elements in the
overview.

Created attachment 174553 [details] [review]
linear-view: Remove shadows when zoomed out

Overlaying inactive workspaces with a gradient to fade out the actors
does no longer work when re-using the normal desktop background. If
we keep the current DND behavior, we probably want to implement a real
fade effect - for now, just remove the visually disruptive shadows.

Created attachment 174554 [details] [review]
linear-view: Remove NewWorkspaceArea

As the button to add workspaces will move to the same position as
the new workspace drop area in drag mode, the latter is redundant
and can be removed.

Created attachment 174555 [details] [review]
dash: Reimplement the dash based on AppWell code

The new dash implementation is a single-column vertical sidebar,
whose items are scaled dynamically to fit the available height.
If the height is still exceeded after scaling down to a minimum
item size, excess items are cut off.
The now unused old dash implementation is renamed to OldDash, as
its code will be used as a base for the new view selector element.

Created attachment 174556 [details] [review]
dash: Move padding into the icon for Fittsability

With this change, the icons' reactive area extends to the screen
edge, making them good targets according to Fitts' law.

Created attachment 174557 [details] [review]
Use the old dash code to implement the view selector

The view selector is a tabbed interface with a search entry. Starting
a search switches focus to the results' tab, ending a search moves the
focus back to the previously selected tab. Activating a normal tab
while a search is active cancels the search.

Created attachment 174558 [details] [review]
view-selector: Add keyboard shortcut for view switching

As the view selector is a tabbed interface, use the default keyboard
shortcut of Ctrl-PageUp/PageDown of GtkNotebook for switching between
views.

Created attachment 174559 [details] [review]
Fake workspaces tab

Created attachment 174560 [details] [review]
all-app-view: Slight cleanup and style update

Being no longer an independent menu pane, the view's structure can
be simplified a bit. Update the style to fit into the view selector.

Created attachment 174561 [details] [review]
workspaces-view: Remove MosaicView

The new layout does no longer support view switching, so merge
GenericWorkspacesView and SingleView, and remove MosaicView.
Also rename or remove workspace properties and functions which
are now unused.
The grid will have a comeback with the new DND behavior.

Created attachment 174562 [details] [review]
workspaces-view: Swap workspace ordering for RTL locales

Make the first workspace the right-most one in RTL locales, as one
would expect. Update all dragging/scrolling functions to behave
correctly.

Created attachment 174563 [details] [review]
workspaces: Rework workspace controls for the view selector

As workspaces will appear as a particular view in the view selector,
merge WorkspacesControls and WorkspacesManager to control workspaces
and related controls, so that a single actor can be added to the
selector instead of positioning the elements from the overview.

Created attachment 174564 [details] [review]
overview: Add ViewSelector to the overview

Add the view selector and adjust the positioning of elements in the
overview. Unlike the old dash, the view selector is made public to
indicate that extensions may add additional views or search providers.

Created attachment 174565 [details] [review]
search-results: Change the default display to use iconGrid

Current mockups display all search results as icons as used by
application results, so change the default result display to use
iconGrid/BaseIcon. Remove the custom application results display,
as it is no longer needed.

Created attachment 174566 [details] [review]
workspaces: Change handling of window-drag signals

Delegate the emission of the window-drag-begin/window-drag-end
signals to overview functions, as done already for other items.
This will enable objects to react to those signals without having
access to the workspace objects / the workspaces view.

Created attachment 174567 [details] [review]
dash: Improve DND to dash and allow reordering

Show a positional indicator where a new favorite will be added and
make the favorites re-orderable. Also allow the removal of favorites
using drag-and-drop according to the mockups.

Created attachment 174568 [details] [review]
overview: Update animation

Update the animation on entering/leaving the overview to only zoom
the window previews and fade other elements.

Created attachment 174569 [details] [review]
workspace-indicators: Add hover indication

Scale up indicators on hover to hint at their clickability.

Review of attachment 174550 [details] [review]:

Looks perfect to me except a tiny comment typo:

::: js/ui/workspacesView.js
@@ +695,3 @@
         this._animating = false; // tweening
+        this._scrolling = false; // dragging desktop
+        this._animatingScroll = false; // programatically update the adjustment

updating

Review of attachment 174551 [details] [review]:

One comment about the behavior and a couple of minor style things.

::: js/ui/overview.js
@@ +79,3 @@
 let displayGridRowHeight = null;
 
+function Source() {

It doesn't seem readable to me that an Overview.Source is a message try source "Source" is too generic to be used outside the context of the message tray and mean "message tray source" - it would be better if this was MessageSource. Hmm, but all are other subclasses are just called Source. So this is OK for consistency.

@@ +88,3 @@
     _init: function() {
+        MessageTray.Source.prototype._init.call(this,
+                                                "System Information");

It doesn't make sense to me to have an icon called "System Information" that disappears a few seconds after the message slides back in. Either we should keep around an icon with the text "Favorite Removed" until the user exits the overview. Or we should not have an icon in the lower left at all and *just* have the notification slideout. Since either option likely requires message tray changes, not a blocker for landing the branch, but you should file a bug about it if not fixing it immediately.

@@ +90,3 @@
+                                                "System Information");
+        this._setSummaryIcon(this.createNotificationIcon());
+        this.connect('clicked', Lang.bind(this, this.destroy));

Judging from other Source subclasess, you should override _notificationClicked rather than connecting directly to 'clicked'.

@@ +149,3 @@
+        if (notification == null)
+            notification = new MessageTray.Notification(this._source, text,
+                                                        null, null);

The approach windowAttentionHandler.js: takes for calling Notification() is to pass only three arguments when there are no optional parameters. which strikes me as a little cleaner than passing null. (Passing {} would also be clean .. but then would create an object for no reason.)

Review of attachment 174552 [details] [review]:

Most of this looks quite reasonable. A few moderately substantive comments.

::: js/ui/overview.js
@@ +172,3 @@
 Overview.prototype = {
     _init : function() {
+        this._desktop = new St.Bin();

I think this._desktopFade might be a more descriptive name

@@ +469,3 @@
 
+        if (!this._desktop.child)
+            this._desktop.child = this._getDesktopClone();

So, things are going to break if the user toggles Nautilus off, or the desktop window is any other way removed or replaced? We'll hold on to a clone of the old actor - which will display nothing, display junk, or even segfault if asked to paint after being destroyed. (Hopefully not segfault, but we often free things in dispose() and then not check before using them somewhere else.) You either need to get a new clone each time you need one or connect to ::destroy and destroy the clone along with the source. Connecting to ::destroy would have the advantage of working in the corner case where the desktop window goes away *while* we are animating.

@@ +528,3 @@
         this._hideInProgress = true;
+
+        let active = global.screen.get_active_workspace_index();

You don't use this variable

::: js/ui/workspace.js
@@ +304,3 @@
         let background = new Clutter.Clone({ source: Main.background.source });
         this.actor.add_actor(background);
+        background.hide();

We add a hidden clone of the background which we never show? It looks like St.Group() includes hidden actors in the size, so maybe you are doing this to make the transparent group the size of the desktop, but I would definitely recommend explicit sizing instead - if we are doing it for Workspace, we can easily propagate to DesktopClone.

DesktopClone is also a pretty weird name for something that doesn't draw the desktop any more. Is there any reason you can't just make Workspace reactive instead?

It's also somewhat suspect to me to have this actor in Workspace._windows - I can't see any sense in which it is a window.

Review of attachment 174553 [details] [review]:

There definitely needs to be some visual work to come up with the right representation of workspaces grouping during DND - it's a bit mysterious right. But certainly the shadows don't fit in with anything we are doing. Looks good except for a leftover constant.

::: js/ui/workspacesView.js
@@ -638,2 @@
     _init: function(width, height, x, y, workspaces) {
-        let shadowWidth = Math.ceil(global.screen_width * WORKSPACE_SHADOW_SCALE);

The constant is no longer used - should be removed

Review of attachment 174554 [details] [review]:

Looks good to me

(In reply to comment #21)
> Review of attachment 174550 [details] [review]:
> 
> Looks perfect to me except a tiny comment typo:

Fixed locally.

Created attachment 174616 [details] [review]
overview: Replace InfoBar with message tray notifications

(In reply to comment #22)
> Review of attachment 174551 [details] [review]:
> It doesn't make sense to me to have an icon called "System Information" that
> disappears a few seconds after the message slides back in. Either we should
> keep around an icon with the text "Favorite Removed" until the user exits the
> overview. Or we should not have an icon in the lower left at all and *just*
> have the notification slideout. Since either option likely requires message
> tray changes, not a blocker for landing the branch, but you should file a bug
> about it if not fixing it immediately.

Needs design input, so I didn't address this yet. Another question would be if ShellInfo should be left in overview.js or moved to main.js - I know hadess was interested in using it for Bluetooth error messages ...

Created attachment 174617 [details] [review]
overview: Do not zoom the desktop background

(In reply to comment #23)
> Review of attachment 174552 [details] [review]:
> 
> Most of this looks quite reasonable. A few moderately substantive comments.
> 
> I think this._desktopFade might be a more descriptive name

Done.


> Connecting to ::destroy would have the advantage of working in the corner
> case where the desktop window goes away *while* we are animating.

Done.

> We add a hidden clone of the background which we never show? It looks like
> St.Group() includes hidden actors in the size, so maybe you are doing this to
> make the transparent group the size of the desktop, but I would definitely
> recommend explicit sizing instead - if we are doing it for Workspace, we can
> easily propagate to DesktopClone.

The idea was to leave it hidden in case I'd need it for grid DND; not a good idea, so I removed DesktopClone entirely.

Review of attachment 174555 [details] [review]:

In general, the code looks fine to me. There's one bug I observed, and a couple of larger issues that need to get fixed, but not necessarily before landing the branch.

::: data/theme/gnome-shell.css
@@ +567,2 @@
 .app-well-app.running {
+    text-shadow: black 0px 2px 2px;

There's some bug here where the text shadow only sometimes shows up on running apps - they have no shadow then you prelight them and move back away and they have a shadow.

::: js/ui/dash.js
@@ +997,3 @@
+        if (children.length == 0) {
+            this._placeholderText = new St.Label({ text: _("Drag here to add favorites") });
+            this._box.add_actor(this._placeholderText);

Isn't this going to explode out horizontally and go over the windows or something? Maybe the dash needs to just have a minimum height no matter how few things are in it - so if you have 0 icon or 1 icon or 2 icon, it's always at least 3 icons high and 1 icon width wide. Or maybe the min height is just 1? Or maybe we just hide the dash if there are no apps and no favorites and say you have to start an app or do a right-click add-to-favorites. Any of those are possible.

@@ +1005,3 @@
+
+                for (let j = 0; j < children.length; j++)
+                    children[j]._delegate.icon.setIconSize(iconSizes[i]);

This probably needs some improvement. We're doing a *LOT* of work every time we add an icon when there are a lot of icons by setting every icon to all the sizes successively. We'll actually load and cache all the icons at all the sizes, among other things.

The first observation I'd make is that given N children and a current icon size of P pixels, the height we need at Q is 'current_size + (Q - P ) * N

So there's really no reason to loop and set sizes here - we should be able to figure out things and only change the size if we need to change the size. Which would be a pretty huge efficiency win.

But actually, the current behavior is pretty ugly - you get

 [tiny icon]
 Fo..
 [huge space]

So probably as soon as we drop below 48 we should drop the text, and we should start reducing the padding. So that means my approach above doesn't really work in the long term. We probably need logic that actually extracts out the relevant CSS style properties and figures out how big everything should be without actually allocating anything. (If necessary, the code could keep around StThemeNodes for the actor and for an icon child at different sizes.) I don't think that should block landing the branch, so I'd say:

 - File a bug to improve the code and the appearance
 - Either take my suggestion above or leave it as is, at your choice

::: js/ui/overview.js
@@ +312,3 @@
         if (rtl) {
+            this._dash.actor.set_position(primary.width - displayGridColumnWidth - WORKSPACE_GRID_PADDING / 2,
+                                          this._workspacesY);

Looks wrong but fixed in a later patch (overview: Add ViewSelector to the overview), fine for an intermediate step.

(In reply to comment #24)
> ::: js/ui/workspacesView.js
> @@ -638,2 @@
>      _init: function(width, height, x, y, workspaces) {
> -        let shadowWidth = Math.ceil(global.screen_width *
> WORKSPACE_SHADOW_SCALE);
> 
> The constant is no longer used - should be removed

It is still used in NewWorkspaceArea - squashed locally with the next patch which removes it.

(In reply to comment #29)
> So probably as soon as we drop below 48 we should drop the text, and we should
> start reducing the padding. So that means my approach above doesn't really work
> in the long term.

Yeah, improving the dash's appearance is definitively on my TODO list. I agree with dropping the text below some threshold, and for the padding my idea was to assign :large :medium :small pseudo classes to the dash.

Also it would be need if I can get size changes to animate nicely instead of "jumping" from one size to another ...

Review of attachment 174556 [details] [review]:

Looks good to me. (Was confused for a second about how it worked, then remembered we supported unequal sized borders for allocation, we just don't when we draw them.)

Review of attachment 174557 [details] [review]:

Mostly minor comments. More major:

 - Think the implementation of the search view should not be in viewselector.js
 - Grumpy an undocumented combination of deferred idles and clutter constraints

::: js/ui/viewSelector.js
@@ +88,3 @@
+        // make sure the entry gets a key-focus-out signal and sets the hint
+        global.stage.set_key_focus(this.entry);
+        global.stage.set_key_focus(null);

Can you break this change out. (focusing the entry first) It's hacky and doesn't belong in big bunch of code that is being moved between files unchanged in the same commit.

@@ +170,3 @@
+Signals.addSignalMethods(SearchEntry.prototype);
+
+function SearchResult(provider, metaInfo, terms) {

Should this really be in viewSelector.js? It seems like something that doesn't have anything to do with view selection at all.

@@ +178,3 @@
+        this.provider = provider;
+        this.metaInfo = metaInfo;
+        this.actor = new St.Clickable({ style_class: 'dash-search-result',

Somewhat odd css class naming now that search results aren't in the dash

@@ +299,3 @@
+        this._searchSystem = searchSystem;
+
+        this.actor = new St.Bin({ name: 'SearchResults',

Was an example earlier in an earlier patch that I didn't catch (Dash), but our convention from this other than 1 or 2 exceptions seems to clearly be #searchResults not #SearchResults.

@@ +308,3 @@
+        let scrollView = new St.ScrollView({ x_fill: true,
+                                             y_fill: false,
+                                             vshadows: true });

As noted on IRC ... not sure if having scrolling here is workable - we really want to show a few results from all providers and try to squeeze all providers on screen. If there are more providers than fit on screen, I think the user is going to just keep typing and not scroll down. Or we could even go to a two-column layout. In no way blocking - a comment for the future.

@@ +535,3 @@
+        ViewTab.prototype._init.call(this, 'search', pageActor);
+        this.title.destroy();
+        this.title = entryActor;

Umm, wow. OK. I might have gone with a common base class, but should work.

@@ +566,3 @@
+
+        this._contentArea = new Shell.GenericContainer();
+        this._contentArea.connect('allocate', Lang.bind(this,

Not sure it's legitimate to have a container that allocates children without ever asking them for a size. (if ignoring that size.) [GTK+ it wasn't OK, not sure about clutter]. Can you use ShellStack here, or do we *need* to ignore the size of the children?

@@ +583,3 @@
+        this._activeTab = null;
+
+        let workId = Main.initializeDeferredWork(this.actor,

A) needs a comment explaining why you are doing it this way
B) The work system is all about batching up repeated tasks. (And does this even do anything without a call to queueDeferredWork()? To just do something later, you should use Meta.later_add() directly.

@@ +593,3 @@
+        this.constraintY = new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.Y });
+        this.constraintHeight = new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.HEIGHT });
+        this._constraintOffsetId = Main.initializeDeferredWork(this.actor,

Again, I'd only use the deferred work system for batching. Here you do call queueDeferredWork, but its' at a point divorced from where you are define what the deferred work does it and the result is well, hard to understand. You are doing something with constraints to do something and you have to do it in some unusual order. That's about as much as I can figure out given a lack of comments. Constraints to me seem like they should never be deferred/idled. You are telling clutter "Make this true when you lay things out".

@@ +793,3 @@
+        this._switchDefaultTab();
+
+        this._searchEntry.show();

Unclear why the search entry is being shown and hidden here and the other tabs aren't (the search entry show hide stuff is there in the old code and not commented either.)

(In reply to comment #22)
> It doesn't make sense to me to have an icon called "System Information" that
> disappears a few seconds after the message slides back in. Either we should
> keep around an icon with the text "Favorite Removed" until the user exits the
> overview. Or we should not have an icon in the lower left at all and *just*
> have the notification slideout. Since either option likely requires message
> tray changes, not a blocker for landing the branch, but you should file a bug
> about it if not fixing it immediately.

I talked with Jon about this:
 <mccann> it should be "transient" but perhaps with a longer timeout
 <mccann> we need transient support in the tray btw

So I'll need support for transient notifications (bug 633412) to fix this.

Review of attachment 174558 [details] [review]:

Looks fine to me

Review of attachment 174559 [details] [review]:

If we're going to leave puzzles in the source code, this one is reasonably transparent.

Review of attachment 174560 [details] [review]:

Looks fine

Review of attachment 174561 [details] [review]:

I think just removing the code is the right thing to do - if we add gridding back in some form for DND, we can still steal some of this code if it's useful, but it's going to be easier to review as "from scratch" rather than a 
revival of unused and likely broken code.

I'm just going to assume basically that this moves code around and deletes code and doesn't add anything questionable :-) Basic structure and the stuff outside of workspace.js looks good to me. And the linear view seems to work fine still (except for UI weirdness from removal of all indication of where one workspace begins and another ends)

Review of attachment 174562 [details] [review]:

OK, except for some minor suggestions about reducing if () complexity.

::: js/ui/workspacesView.js
@@ +531,3 @@
                 if ((this._lastMotionTime > 0 && this._lastMotionTime > event.get_time() - 2) || noStop) {
+                    if (St.Widget.get_default_direction() == St.TextDirection.RTL) {
+                        if (stageX < this._dragStartX && activate > 0)

OK like this but might be cleaner with something like:

 let difference = startX < this._dragStartX ? -1 : 1;
 if (rtl)
    difference *= -1;
 activate += difference;
 if (activate >= 0 && activate < last)

@@ +914,3 @@
+        if (direction == Clutter.ScrollDirection.DOWN) {
+            if (rtl && current > 0)
+                activate--;

Same comment above about reducing number of cases with a *= -1;

Review of attachment 174563 [details] [review]:

Most comments here are about missing comments - I think both using GenericContainer and using idles/laters/deferrals are things that are obscure enough that they generally should get an overview "here's what we are trying to do here" comments.

::: js/ui/workspacesView.js
@@ +808,3 @@
+        this._switchWorkspaceNotifyId =
+            global.window_manager.connect('switch-workspace', function() {
+                Main.queueDeferredWork(workId);

I'm not sure why this is being done deferred. It does save some work if we switch workspaces a bunch of times without returning to the main loop, but it doesn't seem to save *that* much work.

@@ +824,3 @@
+        // Don't display a single indicator
+        if (children.length == 1)
+            return;

This leaves the single workspace indicator allocated at some random size and place. Think it would be better to just show/hide the indicators as appropriate in updateWorkspaces.

@@ +831,3 @@
+
+        let childBox = new Clutter.ActorBox();
+        childBox.x1 = Math.floor((availWidth - natWidth) / 2);

Not really sure I understand the point of this GenericContainer - should be commented. The two things I see here:

 A) Centering
 B) Setting a 0-minimum width (what's that about?)

Don't seem like they should need custom layout.

@@ +948,3 @@
+                               Lang.bind(this, this._onHoverChanged));
+
+        let workId = Main.initializeDeferredWork(this.actor,

A) later_add() since this isn't batching B) comment why. Or maybe we should do:

 overview = new Overview.Overview()
 overview.create_controls()

So the controls can just connect to signals on Main.overview without jumping through hoops.

@@ +954,3 @@
+                Main.overview.connect('item-drag-end',
+                                      Lang.bind(this, this.popIn));
+                this._controls.x = this._poppedInX();

Doesn't look like this gets updated properly on changes to the monitor setup?

@@ +972,1 @@
+    _allocate: function(actor, box, flags) {

would like to see a comment about the purpose of the usage of GenericContainer. (Looks like this is to get the child to have a manually positioned x coordinate but be automaticlaly sized vertically and then clip it so it doesn't extend outside the monitor?)

@@ +983,3 @@
+            this.popOut();
+        else
+            this.popIn();

Hmm, little concerned about multiple calls to popIn/popOut interacting weirdly, but not worth extra code.

Review of attachment 174564 [details] [review]:

Looks good, some comments about the details and a couple of minor bugs.

::: data/theme/gnome-shell.css
@@ +251,3 @@
 /* Overview */
 
+#Overview {

Earlier comments about class names.

::: js/ui/appDisplay.js
@@ -158,2 @@
         this._appView = new ViewByCategories();
-        this._appView.connect('launching', Lang.bind(this, this.close));

Don't seem to have much to do with what was mentioned in the commit body (Dont' really care if it's split out - can just extend the message to say something about removing functionality no longer in use)

::: js/ui/overview.js
@@ +28,3 @@
+// We split the screen vertically between the side panel and the view
+// selector.
+const SIDE_PANEL_SPLIT_FRACTION = 0.1;

Hmm, but the view selector takes the entire top of the screen - aren't you splitting between the view selector and the view contents? If the dash hash the same height as the view contents, that seems like just a "side effect"

@@ +135,3 @@
+                let node = this._group.get_theme_node();
+                let [has_spacing, spacing] = node.lookup_length('spacing',
+                                                    false);

Looks like wasn't updated for the change to lookup_length()

@@ +430,3 @@
+        // Reset the overview actor's scale/position, so that other elements
+        // can calculate their position correctly the next time the overview
+        // is shown

Why does the scale/position of the overview matter? Are things working in stage coordinates?

(Just a question - I'm a little concerned we might be doing a lot layout when we show the overview:

 - Query a size, allocate the stage
 - Add an actor, queue a relayout
 - Query a size agaain, allocate the stage again
 - ...
 - Set the scale to start the tween
 - Lay out the stage again for the initial frame

But the only way to investigate that is to actually chart the number of stage layouts we do between triggering the overview and painting the first frame. spending work before that.

::: js/ui/workspacesView.js
@@ +1090,3 @@
+        let [x, y] = this._workspacesBin.get_transformed_position();
+        x = Math.floor(x + Math.abs(binWidth - width) / 2);
+        y = Math.floor(y + Math.abs(binHeight - height) / 2);

If this is why you set the scale when showing the overview, shouldn't be hard to work around. E.g., use clutter_actor_apply_relative_transform_to_point()

@@ +1092,3 @@
+        y = Math.floor(y + Math.abs(binHeight - height) / 2);
+
+        log("(" + x + ", " + y + ") : " + width + "x" + height);

Left-over debug spew

Review of attachment 174565 [details] [review]:

Looks good to me.

::: js/ui/viewSelector.js
@@ +18,3 @@
 const Tweener = imports.ui.tweener;
 
+const MAX_SEARCH_RESULTS_ROWS = 2;

Hmm, wonder if this should be 1. 2 feels like a lot of items on my laptop and huge number on a big monitor. But different on a netbook, of course. (On the other hand, netbook is where having to scroll to get to search providers is a problem.) 2 is fine for now.

@@ +186,3 @@
         let content = provider.createResultActor(metaInfo, terms);
         if (content == null) {
+            content = new St.Bin({ style_class: 'dash-search-result-content',

As noted on other patches, dash- class names are leftovers and a little confusing

Review of attachment 174566 [details] [review]:

Good cleanup

Review of attachment 174567 [details] [review]:

Looks This all looks good to me except some niggles. DND has been very reliably for me in testing, so I'm not sure what people are running into who have reported unreliability.

::: js/ui/appFavorites.js
@@ +116,3 @@
+        let pos = -1;
+        for (let i = 0; i < ids.length; i++)
+            if (ids[i] == appId) {

I'd use Array.indexOf() ... it's even standard in Ecmascript 5.

::: js/ui/dash.js
@@ +62,3 @@
+        let id = app.get_id();
+
+        Mainloop.idle_add(Lang.bind(this, function () {

Generally should always use Meta.later_add() ... idle_add() is not guaranteed to happen at any particular point in time ... if glxgears is running and we are constantly repainting, the icon might not get removed.

@@ +115,3 @@
+    show: function() {
+        this._itemDragBeginId = Main.overview.connect('item-drag-begin',
+            Lang.bind(this, this._onDragBegin));

OK, guess this obsoletes some of my earlier comments about doing these connections deferred.

@@ +340,3 @@
         }
 
+        Mainloop.idle_add(Lang.bind(this, function () {

Same comment about idles

Review of attachment 174568 [details] [review]:

Basically looks great to me.

::: js/ui/overview.js
@@ +378,3 @@
         // transition is used in show().
+        this.workspaces.actor.hide();
+        this.workspaces.actor.reparent(this._zoomContainer);

Out of curiousity, why do we need the separate zoomContainer and the reparenting as opposed to just zooming this.workspaces.actor?

@@ -440,3 @@
-        // Reset the overview actor's scale/position, so that other elements
-        // can calculate their position correctly the next time the overview
-        // is shown

OK, you can ignore my comment about this on the other bug

Review of attachment 174569 [details] [review]:

::: js/ui/workspacesView.js
@@ +823,3 @@
         childBox.x2 = childBox.x1 + natWidth;
+        childBox.y1 = Math.floor((availHeight - natHeight) / 2);
+        childBox.y2 = childBox.y2 + natHeight;

Maybe a little odd that horizontally the padding between icons is taken out of the spacing specified in the CSS but vertically you add padding automatically instead of in the CSS. But fine this way.

Review of attachment 174616 [details] [review]:

Looks good

Review of attachment 174617 [details] [review]:

Looks good

Created attachment 174781 [details] [review]
search-display: Move SearchResults to a separate file

With the new layout, search results will be displayed in an independent
view like window previews, applications and possible future additions;
it does not make much sense keeping it with the switching logic, so move
the code to its own file.

Also remove the dash-prefix from the relevant style classes.

According to comment 33.

Created attachment 174782 [details] [review]
Use the old dash code to implement the view selector

(In reply to comment #33)
> ::: js/ui/viewSelector.js
> @@ +88,3 @@
> +        // make sure the entry gets a key-focus-out signal and sets the hint
> +        global.stage.set_key_focus(this.entry);
> +        global.stage.set_key_focus(null);
> 
> Can you break this change out. (focusing the entry first) It's hacky and
> doesn't belong in big bunch of code that is being moved between files unchanged
> in the same commit.

Done.


> Should this really be in viewSelector.js? It seems like something that doesn't
> have anything to do with view selection at all.
> [...]
> Somewhat odd css class naming now that search results aren't in the dash

See previous patch.


> Was an example earlier in an earlier patch that I didn't catch (Dash), but our
> convention from this other than 1 or 2 exceptions seems to clearly be
> #searchResults not #SearchResults.

Updated the entire branch locally.


> As noted on IRC ... not sure if having scrolling here is workable - we really
> want to show a few results from all providers and try to squeeze all providers
> on screen. If there are more providers than fit on screen, I think the user is
> going to just keep typing and not scroll down. Or we could even go to a
> two-column layout. In no way blocking - a comment for the future.

This comment still applies.


> Umm, wow. OK. I might have gone with a common base class, but should work.

Done. Ignore the nonsense SearchTab prototype, it will make sense in the follow-up patch.


> Not sure it's legitimate to have a container that allocates children without
> ever asking them for a size. (if ignoring that size.) [GTK+ it wasn't OK, not
> sure about clutter]. Can you use ShellStack here, or do we *need* to ignore the
> size of the children?

Oh - I was unaware of ShellStack - changed. I'd say it is generally not OK to allocate children without querying their size, it just works in this case because the container is set to expand when added to the parent BoxLayout.


> A) needs a comment explaining why you are doing it this way
> B) The work system is all about batching up repeated tasks. (And does this even
> do anything without a call to queueDeferredWork()? To just do something later,
> you should use Meta.later_add() directly.

Done to defer the signal connections until Main.overview actually exists. Changed to connect/disconnect on show/hide instead.


> Again, I'd only use the deferred work system for batching. Here you do call
> queueDeferredWork, but its' at a point divorced from where you are define what
> the deferred work does it and the result is well, hard to understand. You are
> doing something with constraints to do something and you have to do it in some
> unusual order. That's about as much as I can figure out given a lack of
> comments. Constraints to me seem like they should never be deferred/idled. You
> are telling clutter "Make this true when you lay things out".

Here the reason for using deferred work was to avoid clutter warnings ("Actor bla is inside an allocation cycle"). Does no longer happen with current clutter, so removed (the deferred work, not the constraints).
Added a (hopefully helpful) comment about the reason to setup constraints.


> Unclear why the search entry is being shown and hidden here and the other tabs
> aren't (the search entry show hide stuff is there in the old code and not
> commented either.)

It connects/disconnects a signal handler to move key focus to the entry when starting to type - the designers didn't want the entry to have focus unless explicitly clicked or by using find-as-you-type. Added a comment here, though the follow-up patch will clean this up a little too.

Created attachment 174783 [details] [review]
view-selector: Move search logic into SearchTab

The view selector should only deal with view switching, so move the
logic to deal with search (find-as-you-type, cancelling a search,
navigating/activating results) into the SearchTab.

Created attachment 174789 [details] [review]
workspaces-view: Swap workspace ordering for RTL locales

> OK like this but might be cleaner with something like:
> 
>  let difference = startX < this._dragStartX ? -1 : 1;
>  if (rtl)
>     difference *= -1;

Done.

(In reply to comment #42)
> 
> +const MAX_SEARCH_RESULTS_ROWS = 2;
> 
> Hmm, wonder if this should be 1. 2 feels like a lot of items on my laptop and
> huge number on a big monitor. But different on a netbook, of course. (On the
> other hand, netbook is where having to scroll to get to search providers is a
> problem.) 2 is fine for now.

OK, left at 2.

 
> @@ +186,3 @@
>          let content = provider.createResultActor(metaInfo, terms);
>          if (content == null) {
> +            content = new St.Bin({ style_class: 'dash-search-result-content',
> 
> As noted on other patches, dash- class names are leftovers and a little
> confusing

Fixed locally throughout the branch.

(In reply to comment #41)
> ::: js/ui/appDisplay.js
> @@ -158,2 @@
>          this._appView = new ViewByCategories();
> -        this._appView.connect('launching', Lang.bind(this, this.close));
> 
> Don't seem to have much to do with what was mentioned in the commit body (Dont'
> really care if it's split out - can just extend the message to say something
> about removing functionality no longer in use)

Squashed with "app-display: Slight cleanup and style update" using the following commit message:
    Being no longer an independent menu pane, both the toggle() and
    close() functions are no longer needed, and the view's structure
    can be simplified a bit.
    Also update the style to fit into the view selector.


> ::: js/ui/overview.js
> @@ +28,3 @@
> +// We split the screen vertically between the side panel and the view
> +// selector.
> +const SIDE_PANEL_SPLIT_FRACTION = 0.1;
> 
> Hmm, but the view selector takes the entire top of the screen - aren't you
> splitting between the view selector and the view contents? If the dash has the
> same height as the view contents, that seems like just a "side effect"

Not sure if I understand that comment. Thinking in rows, the layout is as follows:

 0) Panel
 1) Spacing (from CSS)
 2) Dash (fixed width of 10% primary width) + spacing + ViewSelector
 3) Spacing
 4) MessageTray

So the dash would align with the view selector, if its vertical position weren't "contraint" to the view contents. The same applies for the height. That said, using a screen-relative size for the dash was not really smart, as the dash's "content" width is limited by its maximum icon size (e.g. a fixed pixel value) ...

For now, I only changed the constant's name to DASH_SPLIT_FRACTION.


> @@ +135,3 @@
> +                let node = this._group.get_theme_node();
> +                let [has_spacing, spacing] = node.lookup_length('spacing',
> +                                                    false);
> 
> Looks like wasn't updated for the change to lookup_length()

Hmm, changed to
  let spacing = node.get_length('spacing');
  if (this._spacing != spacing) {
    this._spacing = spacing;
    this.relayout();
  }
which looks more correct ...


> @@ +430,3 @@
> +        // Reset the overview actor's scale/position, so that other elements
> +        // can calculate their position correctly the next time the overview
> +        // is shown
> 
> Why does the scale/position of the overview matter? Are things working in stage
> coordinates?
> [...]
> If this is why you set the scale when showing the overview, shouldn't be hard
> to work around. E.g., use clutter_actor_apply_relative_transform_to_point()

Indeed that was the reason. The proposed change works, but as the scaling/positioning is removed in a later commit anyway I left it as-is.


> Left-over debug spew

Removed, though it still marks a bug: the values are different when first entering the overview causing the workspaces view to overlap the indicators. I don't think it is a blocker, but help would be appreciated ...

Created attachment 174803 [details] [review]
overview: Adjust for background_actor change

The mutter background actor is not redrawn while the overview is
active, as the overview actors actually covers it entirely (though
using a translucent color).
while at it, remove the obsolete _backOver actor.

OK to squash with the "no background zoom" patch?

Created attachment 174806 [details] [review]
overview: Adjust for background_actor change

Hmprf, made an error when rebasing the patch. This one works.

Review of attachment 174781 [details] [review]:

Looks good

Review of attachment 174806 [details] [review]:

Code looks good to me, commit message needs some improvement. As mentioned on IRC the cause for the breakage is that the normal background actor is now inside the WindowGroup which is hidden when showing the overview, so there's nothing behind the overview.

(In reply to comment #58)
> Code looks good to me, commit message needs some improvement.

I merged the commit with the no-background-zoom patch with the following comment:
+        // The actual global.background_actor is inside global.window_group,
+        // which is hidden when displaying the overview, so we display a clone.

Review of attachment 174789 [details] [review]:

::: js/ui/workspacesView.js
@@ +538,3 @@
                 if ((this._lastMotionTime > 0 && this._lastMotionTime > event.get_time() - 2) || noStop) {
+                    if (difference < 0 && activate > 0 ||
+                        difference > 0 && activate < last)

I'd suggest always using parentheses when mixing && and || ... I certainly don't bother to remember which has higher priority.

I actually also find this rather difficult to figure out as compared to:

 if (activate + difference >= 0 && activate + difference < last)

@@ +917,3 @@
+        if (difference < 0 && activate > 0 ||
+            difference > 0 && activate < last)
+            activate += difference;

Same comments

Review of attachment 174782 [details] [review]:

Generally good

::: js/ui/viewSelector.js
@@ +292,3 @@
+        // accessing the properties.
+        this.constraintY = new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.Y });
+        this.constraintHeight = new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.HEIGHT });

Comment is indeed useful and explanatory - makes a lot more sense to me now. Name seems a little awkward - I would expect either:

 constrainY [probably better]
or:
 yConstraint

@@ +365,3 @@
+
+        this.constraintY.set_source(tab.page);
+        this.constraintHeight.set_source(tab.page);

Why do why constrain to tab.page and not to _PageArea? It doesn't seem we'd ever want the dash to move when you switched pages?

Review of attachment 174783 [details] [review]:

Looks good

Review of attachment 174789 [details] [review]:

::: js/ui/workspacesView.js
@@ +538,3 @@
                 if ((this._lastMotionTime > 0 && this._lastMotionTime > event.get_time() - 2) || noStop) {
+                    if (difference < 0 && activate > 0 ||
+                        difference > 0 && activate < last)

Of course for my suggestion:

- if (activate + difference >= 0 && activate + difference < last)
+ if (activate + difference >= 0 && activate + difference <= last)

(In reply to comment #59)
> (In reply to comment #58)
> > Code looks good to me, commit message needs some improvement.
> 
> I merged the commit with the no-background-zoom patch with the following
> comment:
> +        // The actual global.background_actor is inside global.window_group,
> +        // which is hidden when displaying the overview, so we display a
> clone.

Sounds good.

Created attachment 174820 [details] [review]
Fix stacking when leaving the overview for removal of desktop clone

Small fix patch, could be merged wth the patch that removes the desktop
clone from the workspace or left separate.

Created attachment 174860 [details] [review]
overview: Fix ordering of desktopFade and background actors

Another little fixup patch.

Review of attachment 174820 [details] [review]:

Woops. Good catch.

Review of attachment 174860 [details] [review]:

Duh. How could I miss that?

Created attachment 175076 [details] [review]
workspaces: Rework workspace controls for the view selector

(In reply to comment #40)
> I'm not sure why this is being done deferred. It does save some work if we
> switch workspaces a bunch of times without returning to the main loop, but it
> doesn't seem to save *that* much work.

Done.


> @@ +824,3 @@
> +        // Don't display a single indicator
> +        if (children.length == 1)
> +            return;
> 
> This leaves the single workspace indicator allocated at some random size and
> place. Think it would be better to just show/hide the indicators as appropriate
> in updateWorkspaces.

Hmm, yeah -

> Not really sure I understand the point of this GenericContainer - should be
> commented. The two things I see here:
> 
>  A) Centering
>  B) Setting a 0-minimum width (what's that about?)
> 
> Don't seem like they should need custom layout.

And, more importantly, allocating the box' height even if it is not displayed. It is pretty ugly, but I don't see a compelling alternative while we fake the workspace size/position to look like it's placed inside the container (rather than actually placing it inside the container).

I hope the comment is understandable.

> Or maybe we should do:
> 
>  overview = new Overview.Overview()
>  overview.create_controls()

I added show/hide methods called from WorkspacesDisplay. Seemed cleaner to me than going through the overview directly.


> Doesn't look like this gets updated properly on changes to the monitor setup?

Right. I didn't fix that one explicitly, but the position is now set each time when entering the overview. And as far as I can tell, the user is taken out of the overview when the screen size changes, so ...


> would like to see a comment about the purpose of the usage of GenericContainer.
> (Looks like this is to get the child to have a manually positioned x coordinate
> but be automaticlaly sized vertically and then clip it so it doesn't extend
> outside the monitor?)

And to keep the width of the actor constant, so the workspaces don't overlap the controls. Note that in the latest mockups, the controls push the workspaces aside when the controls pop out - this requires some refactoring of the workspaces code though.

> Hmm, little concerned about multiple calls to popIn/popOut interacting weirdly,
> but not worth extra code.

Hmm, both animations tween the same properties - if I recall correctly, the previous tween is simply overwritten in that case. (It works at least in the case where popIn() is called before popOut() finishes (or vice versa), which should cover the vast majority of cases).

Created attachment 175077 [details] [review]
overview: Add ViewSelector to the overview

(In reply to comment #41)
> @@ +430,3 @@
> +        // Reset the overview actor's scale/position, so that other elements
> +        // can calculate their position correctly the next time the overview
> +        // is shown
> 
> Why does the scale/position of the overview matter? Are things working in stage
> coordinates?
> 
> (Just a question - I'm a little concerned we might be doing a lot layout when
> we show the overview

Left as-is, as changed in a later patch.

Owen: I squashed both your fixes into the background patch, so marking obsolete.

(In reply to comment #60)
> I actually also find this rather difficult to figure out as compared to:
> 
>  if (activate + difference >= 0 && activate + difference < last)

Done.

(In reply to comment #45)
> Out of curiousity, why do we need the separate zoomContainer and the
> reparenting as opposed to just zooming this.workspaces.actor?

I did that first and ran into some positioning issues. Those appear to have been fixed when addressing the dual monitor issues, so I changed it back.

(In reply to comment #61)d useful and explanatory - makes a lot more sense to me now.
> Name seems a little awkward - I would expect either:
> 
>  constrainY [probably better]
> or:
>  yConstraint

Done (the former).


> Why do why constrain to tab.page and not to _PageArea? It doesn't seem we'd
> ever want the dash to move when you switched pages?

Done.

I also removed some left-over CSS (workspace shadows, new-workspace area) and squashed in the appropriate commits. Doesn't seem worth re-uploading for review though.

Review of attachment 175076 [details] [review]:

Generally looks good.

::: js/ui/workspacesView.js
@@ +826,3 @@
+        // Don't display a single indicator
+        if (children.length == 1)
+            return;

No really, this is bad - not allocating your children leaves them in an inconsistent state. It's not a mustfix for landing the branch, but it is a mustfix. I guess you can't hide it because then the box would change size? shell_generic_container_set_skip_paint() will allow you easily to make this._box not paint or pick when there is one child.

@@ +951,1 @@
+    show: function() {

Not completely sure if it's tasteful to have show() and hide() methods that don't actually show and hide actor but instead do processing when an ancestor of this.actor() is shown or hidden. It's not really as much "show()" as "onOverviewShown". But OK.

(In reply to comment #76)
> shell_generic_container_set_skip_paint() will allow you easily to make
> this._box not paint or pick when there is one child.

Updated locally.

 
> Not completely sure if it's tasteful to have show() and hide() methods that
> don't actually show and hide actor but instead do processing when an ancestor
> of this.actor() is shown or hidden. It's not really as much "show()" as
> "onOverviewShown". But OK.

Yeah. On the other hand, onOverviewShown sounds like it is a handler for the overview's 'shown' signal - obviously not possible, as the only reason to do the signal connections at a later point is the overview not having been initialized yet when running the constructor ...

Review of attachment 175077 [details] [review]:

Didn't have any comments on this previously other than one you said "Left as-is, as changed in a later patch." - quick scan it still seems reasonable to me.

Comment on attachment 174550 [details] [review]
linear-view: Remove the scrollbar

Attachment 174550 [details] pushed as 8d47a15 - linear-view: Remove the scrollbar

Comment on attachment 174616 [details] [review]
overview: Replace InfoBar with message tray notifications

Attachment 174616 [details] pushed as 1ea488b - overview: Replace InfoBar with message tray notifications

Comment on attachment 174617 [details] [review]
overview: Do not zoom the desktop background

Attachment 174617 [details] pushed as f24e567 - overview: Do not zoom the desktop background

Comment on attachment 174553 [details] [review]
linear-view: Remove shadows when zoomed out

Attachment 174553 [details] pushed as e06b608 - linear-view: Remove shadows when zoomed out

Comment on attachment 174554 [details] [review]
linear-view: Remove NewWorkspaceArea

Attachment 174554 [details] pushed as 2d2ac5b - linear-view: Remove NewWorkspaceArea

Comment on attachment 174555 [details] [review]
dash: Reimplement the dash based on AppWell code

Attachment 174555 [details] pushed as 3e4f744 - dash: Reimplement the dash based on AppWell code

Comment on attachment 174556 [details] [review]
dash: Move padding into the icon for Fittsability

Attachment 174556 [details] pushed as 26225f0 - dash: Move padding into the icon for Fittsability

Comment on attachment 174781 [details] [review]
search-display: Move SearchResults to a separate file

Attachment 174781 [details] pushed as e6bb06a - search-display: Move SearchResults to a separate file

Comment on attachment 174782 [details] [review]
Use the old dash code to implement the view selector

Attachment 174782 [details] pushed as 48fff0e - Use the old dash code to implement the view selector

Comment on attachment 174783 [details] [review]
view-selector: Move search logic into SearchTab

Attachment 174783 [details] pushed as 688a315 - view-selector: Move search logic into SearchTab

Comment on attachment 174558 [details] [review]
view-selector: Add keyboard shortcut for view switching

Attachment 174558 [details] pushed as ffd7eae - view-selector: Add keyboard shortcut for view switching

Comment on attachment 174559 [details] [review]
Fake workspaces tab

Attachment 174559 [details] pushed as 1a77acf - Fake workspaces tab

Comment on attachment 174560 [details] [review]
all-app-view: Slight cleanup and style update

Attachment 174560 [details] pushed as 1eb6dfe1b83 app-display: Slight cleanup and style update.

Comment on attachment 174561 [details] [review]
workspaces-view: Remove MosaicView

Attachment 174561 [details] pushed as 0942f50 - workspaces-view: Remove MosaicView

Comment on attachment 174789 [details] [review]
workspaces-view: Swap workspace ordering for RTL locales

Attachment 174789 [details] pushed as 6c5c3be - workspaces-view: Swap workspace ordering for RTL locales

Comment on attachment 175076 [details] [review]
workspaces: Rework workspace controls for the view selector

Attachment 175076 [details] pushed as 7811632 - workspaces: Rework workspace controls for the view selector

Comment on attachment 175077 [details] [review]
overview: Add ViewSelector to the overview

Attachment 175077 [details] pushed as d5d7d8a - overview: Add ViewSelector to the overview

Comment on attachment 174565 [details] [review]
search-results: Change the default display to use iconGrid

Attachment 174565 [details] pushed as search-display: Change the default display to use iconGrid

Comment on attachment 174566 [details] [review]
workspaces: Change handling of window-drag signals

Attachment 174566 [details] pushed as 5fef918 - workspaces: Change handling of window-drag signals

Comment on attachment 174567 [details] [review]
dash: Improve DND to dash and allow reordering

Attachment 174567 [details] pushed as b59daac - dash: Improve DND to dash and allow reordering

Comment on attachment 174568 [details] [review]
overview: Update animation

Attachment 174568 [details] pushed as e2e11b1 - overview: Update animation

Comment on attachment 174569 [details] [review]
workspace-indicators: Add hover indication

Attachment 174569 [details] pushed as 6f9ede5 - workspace-indicators: Add hover indication