Created attachment 145139 [details] [review]
* Clicking outside the boxes (app or window) should cancel the alt+tab mode

Created attachment 145140 [details] [review]
* The entire selection box/focus rectangle area should be active (clickable and sensitive to mouse hover)

Created attachment 145141 [details] [review]
* use const HOVER_TIME = 0; // milliseconds

Created attachment 145142 [details] [review]
* Mouse movements should not set focus on hover while keyboard navigation is occuring (or within a small delta afterwards)

Also need to disable hover until the mouse moves again, or else the
mouse could steal the focus back to a previous item even if you
weren't touching it any more.

Created attachment 145143 [details] [review]
* use const THUMBNAIL_POPUP_TIME = 500; // milliseconds

Created attachment 145154 [details] [review]
* Show down arrow indicator for unfocused apps at reduce (half?) opacity

Jon said on IRC that he probably meant intensity, not opacity

Created attachment 145155 [details] [review]
* Don't show previews for apps with one window on hover or automatically

amended by jon on irc to a generic "don't show thumbnails on hover"

Created attachment 145156 [details] [review]
* Don't automatically move focus down into the window preview box on hover

Likewise, in the next-window-of-current-app hack, we keep the focus in
the app list, not the window list

Created attachment 145157 [details] [review]
* Tabbing should always move the focus rectangle one space to the next item. If the end of a window box is reached, return to the app box.  If the end of the app box is reached, return to the start.

Created attachment 145158 [details] [review]
* Mouse wheel should move focus rectangle.  Wheel up moves to previous item, down to the next item.  With the exception that it always descends into the window box if an app has more than one window.  If the app doesn't have multiple windows the wheel mov

That should be everythign except:

 * show the window icon, if it is different from the app icon, in the lower
right corner of the window preview

several of the patches interact oddly with the put-next-window-of-current-app-before-first-window-of-next-app hack, so be sure to try that out and make sure it's behaving as expected

Created attachment 145169 [details] [review]
keep outline highlight on app switcher when thumbnails are focused

the outline is not quite right; it's opaque rather than translucent,
because I couldn't get it to work right translucently

Created attachment 145170 [details] [review]
tweak scroll behavior

Created attachment 145177 [details] [review]
Fix same-app-next-window special case

Created attachment 145184 [details] [review]
[AppSwitcher] misc refinements

If you read the list above first, it's pretty obvious what's going on in
this patch. It's mostly just the squashed version of the earlier patches,
but with some additional comments added.

(Still missing the icons-on-thumbnails part.)

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

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

Looking much better.  A few comments.

 * Mouse selection should only change when crossing into a new app or window target (ie. not just on any motion)
 * The window box should not appear or disappear abruptly.  It should quickly fade in and out.  Right now it is pretty abrupt.
 * When using the mouse wheel and moving from an app with multiple windows to another app with multiple windows the window box should slide and morph into the new shape.
 * Windows/Tabs should have the title underneath displayed in the same way as the app name

Also looks like we still have contention between mouse and keyboard navigation.  To demonostrate the problem, move your mouse from right to left across the app box while you are tabbing through it.

(In reply to comment #18)
> Also looks like we still have contention between mouse and keyboard navigation.
>  To demonostrate the problem, move your mouse from right to left across the app
> box while you are tabbing through it.

Right, I think I'd mentioned that on IRC. The problem is that either (a) you set the disable-mouse-after-keyboard timeout to something low, in which case it triggers when you don't want it, or (b) you set it to something high, and then you have to sit around and wait when you actually *do* want to use pointer selection. There does not seem to be any happy medium. (Try playing around with MOUSE_SELECTION_TIME_THRESHOLD.)

Colin and I had talked about maybe changing it to be a delay after you *stop* moving the mouse, but that could be very confusing if you only realized that the mouse worked sometimes, and not other times, and so you'd probably end up repeatedly fiddling with it to try to get it to "wake up", thereby preventing it from actually happening.

Created attachment 145550 [details] [review]
* The window box should not appear or disappear abruptly.  It should quickly fade in and out.

Created attachment 145551 [details] [review]
* Mouse selection should only change when crossing into a new app or window target (ie. not just on any motion)

My comments before about mouse-switching were not quite right; there
was a bug in the code, where the no-mouse-after-tab timeout wasn't
getting reset if you hit tab multiple times, so sometimes the mouse would
suddenly start working immediately after you hit Tab. That's fixed with
this

Created attachment 145552 [details] [review]
* Windows/Tabs should have the title underneath displayed in the same way as the app name

Created attachment 145567 [details] [review]
possible update for "misc refinements" patch against git

Naively updated the patch to apply and not throw after appicon changes.  Hard to live without these refinements :)

With Dan's latest patch the mouse/keyboard interaction is greatly improved.

There is one bug with the mouse focus.  If I hover the mouse long enough for the window box to pop up, then move outside the app icon but not onto another app, the window box is redisplayed (hidden+shown) when I move back over the same app icon.

Also, it seems like we've regressed on using alt-tab to switch between two windows of the same app.

Hopefully these aren't bugs I introduced in my patch.

Created attachment 145610 [details] [review]
[AppSwitcher] misc refinements

Rebased and re-squashed and ready for review.

Review of attachment 145610 [details] [review]:

Patch mostly looks good and feels good trying it out. Few comments on the patch and on the behavior; most serious problem I see is that the the switch-to-window-of-the-same-app behavior seems to be broken - if the last window was from the same app, the first alt-tab does nothing.

::: data/theme/gnome-shell.css
@@ +189,3 @@
+.switcher-list StLabel {
+    font: 12px sans-serif;
+    color: white;

I would generally avoid this type of construction, because it means that every time the CSS matcher hits a StLabel it needs to check up through the entire hierarchy to see if any of the parents have the .switcher-list class. Better to give a specific classname to the thing you want to style.

But actually, you probably can just do:

.switch-list {
   font: 12px sans-serif;
   color: white;
}

Since font and color are inherited properties.

::: js/ui/altTab.js
@@ +28,3 @@
 const POPUP_LIST_SPACING = 8;
 
+const MOUSE_SELECTION_TIME_THRESHOLD = 500; // milliseconds

What's "threshold" about this? the name is a bit weird. And a comment here explaining the algorithm (mouse is ignored for half a second after a keystroke?) could be useful.

@@ +32,3 @@
 const THUMBNAIL_SIZE = 256;
+const THUMBNAIL_POPUP_TIME = 500; // milliseconds
+const THUMBNAIL_FADE_TIME = 0.2; // seconds

Wonder if we should get into the habit of writing timeout_add(POPUP_TIME * 1000); - not suggesting a change here just thinking out loud.

@@ +197,3 @@
     },
 
+    _onScroll : function(actor, event) {

When you have widows open for an app, the behavior where the first click takes you to the app and the next descends feels weird to me, especially since it doesn't reverse properly - if you pass an app and scroll back up, then the first click takes you to the app click and it takes another to get back where were before.

It seems to me that it might feel better if the first click immediately descended when there were windows and apps with multiple windows never are a scroll destination in themselves. (There's the initial state before you start scrolling - in that case the first click would descend to that apps windows.)

This also means that it takes N clicks to pass an app with N windows always, instead of 1 click for an app with 1 window and N + 1 otherwise.

@@ -191,2 +234,3 @@
     _appActivated : function(appSwitcher, n) {
-        Main.activateWindow(this._appIcons[n].cachedWindows[this._currentWindows[n]]);
+        let window = (n == this._currentApp) ? this._currentWindow : 0;
+        Main.activateWindow(this._appIcons[n].cachedWindows[window]);

I'm a little unclear how a non-current-app could be activated

@@ +318,3 @@
+     * If @app and @window are specified and @forceAppFocus is %true,
+     * then @app will be highlighted, and @window outlined, and the
+     * app list will have the keyboard focus.

As a naming thing not quite sure what keyboard focus is here; maybe better as "forceAppHighlight" or something like that? 

The behavior doesn't seem to be as described - if I have two windows from the same app as current and last used, then alt-tab doesn't outline @window when popped up (and releasing immediately leaves the focus on the current window, and doesn't go to the next.)

@@ +340,1 @@
             this._appSwitcher.highlight(app);

Could just write:

 this._appSwitcher.highlight(app, this._thumbnailsFocused);

?

@@ +345,3 @@
+            this._currentWindow = window;
+            if (forceAppFocus && window == 0)
+                this._thumbnails.highlight(-1);

Hmmm, is this the right visual behavior? Seems a bit odd to have things act differently if it's window 0 as oppposed to some other window.

@@ +660,3 @@
+    // highlighted; their redraw handler will use the right color
+    // based on this._curApp; we just need to do a queue_relayout() to
+    // force it to redraw. (queue_redraw() doesn't work.)

Either should explain why - which is that ShellDrawingArea only redoes the cairo drawing on allocate - or add a queue_redraw handler to ShellDrawingArea and fix it (this probably means setting a dirty flag and doing the actual redraw signal emission out of paint before chaining up, so not 100% trivial, so maybe the comment and perhaps a bug filed is better.)

@@ +743,3 @@
+
+            let name = new St.Label({ text: windows[i].get_title() });
+            // We don't currently support text-align?

No ? necessary. I'm not 100% sure we want to support it - it's a bit like expand and other "layout" that we don't CSS style.

(In reply to comment #26)
> most serious problem I see is that the the
> switch-to-window-of-the-same-app behavior seems to be broken - if the last
> window was from the same app, the first alt-tab does nothing.

That was bug 598389.

> +const MOUSE_SELECTION_TIME_THRESHOLD = 500; // milliseconds
> 
> What's "threshold" about this?

It made more sense where there was a MOUSE_SELECTION_MOVEMENT_THRESHOLD too. I'll change it.

> +    _onScroll : function(actor, event) {

I agree with your comments. Jon?

> +        let window = (n == this._currentApp) ? this._currentWindow : 0;
> +        Main.activateWindow(this._appIcons[n].cachedWindows[window]);
> 
> I'm a little unclear how a non-current-app could be activated

If you click on it while !mouseActive. That's less likely now than it was in earlier states of the patch, but it's still possible. (Eg, immediately after typing Tab, you click an icon that isn't the highlighted one. Or else, you type Alt-Tab and then click on the icon under the pointer without leaving and re-entering it first.)

> As a naming thing not quite sure what keyboard focus is here; maybe better as
> "forceAppHighlight" or something like that? 

"keyboard focus" means which list the arrow keys control; when "forceAppFocus" is true, we keep the focus on the app list even though the thumbnail list is visible. I'll clarify.

> The behavior doesn't seem to be as described - if I have two windows from the
> same app as current and last used, then alt-tab doesn't outline @window when
> popped up (and releasing immediately leaves the focus on the current window,
> and doesn't go to the next.)

It seems to work fine for me; the wrong selection on keyrelease is bug 598389 again. The absence of highlight may be related.

> @@ +345,3 @@
> +            this._currentWindow = window;
> +            if (forceAppFocus && window == 0)
> +                this._thumbnails.highlight(-1);
> 
> Hmmm, is this the right visual behavior? Seems a bit odd to have things act
> differently if it's window 0 as oppposed to some other window.

I'm overloading variable meanings... "(forceAppFocus && window == 0)" is only true in the pop-up-the-thumbnails-after-a-timeout case, in which case we don't want to select anything. I'll figure something out to make it clearer.

> +    // force it to redraw. (queue_redraw() doesn't work.)
> 
> Either should explain why - which is that ShellDrawingArea only redoes the
> cairo drawing on allocate

I assumed that was implied. I'll update the comment.

Created attachment 145797 [details] [review]
address owen's comments

doh, I'd meant to attach this Friday afternoon but then got distracted by
https://bugzilla.redhat.com/show_bug.cgi?id=529463.

This does not change the scroll-wheel behavior, though we can do that later.
Also, I changed the highlight/outline behavior so that when the thumbnails
pop up, the first window is outlined to indicate that it's the one that
would be selected, for consistency with the case where the second window is
outlined instead.

Yeah the scroll wheel thing is a little odd.  I'm not sure the current forward behavior is that bad but it certainly feels a little odd that the reverse behaves differently.  It is probably fine to skip right into the window box when on the first scroll tick (right now we also stop on the app).  However, this might be a tweak that we can do after landing...

Review of attachment 145797 [details] [review]:

Looks good to me.

::: js/ui/altTab.js
@@ +347,3 @@
                 this._createThumbnails();
             this._currentWindow = window;
+            this._thumbnails.highlight(window, forceAppFocus);

I really miss Python keyword arguments in cases like this, it's just so much more readable to have justOutline=forceAppFocus.

Attachment 145610 [details] pushed as 8fd00ac - [AppSwitcher] misc refinements