GNOME Bugzilla – Bug 590563
make altTab app-based
Last modified: 2009-10-07 18:08:51 UTC
The design says alt-tab should tab between applications.
Pasting more details from the spec: Some of the differences from the existing GNOME switcher should include: • Align this switcher with the layout norms and visual style of the shell - Ex: bigger icons • Fast switching to any running application regardless of workspace. • Show one icon per application, but provide a way to switch to any specific window. • Add a way to preview or improve the usefulness of selected window highlighting • Allow mouse selection ◦ Two handed use is more efficient ◦ Avoids the tab cycling problem Basic control • The Alt-Tab keystroke calls up the switcher • All Active applications in all workspaces shown • Applications are ordered by time of last use. • Single icon per application, regardless of number of windows. Users can optionally select specific windows for multi-window apps (see more below) • One application is always selected • Per current behavior, releasing Alt key is how the user selects the application to switch to. Clicking is not required. Using the mouse to set focus Using the mouse is a second, more direct way to set the focus in addition to repeatedly pressing the Tab key. Users are frequently annoyed with tabbing when they tab too far. Employing the mouse, which is likely to already be in their hand, lets the user quickly select arbitrarily from the application list. Multiple Windows • Click and hold to reveal the window(s) for the selected application (note: need a keyboard equivalent like space or down arrow) • We could use the same window list as in the overview design. And, like the overview, show a preview for each window as you hover over it. Open Questions: • If a window to reveal is in a different workspace than the current one, quickly switch to that workspace first. • If an application has windows in multiple workspaces, switch to the multiple workspaces view as needed. Same effect as in the shell. • Perhaps we can even show a hint to use alt-tab when we detect that the user is going in and out of the shell to switch windows in rapid succession?
(In reply to comment #1) > • Show one icon per application, but provide a way to switch to any specific > window. > • Add a way to preview or improve the usefulness of selected window > highlighting When you switch to an application, does it bring all windows for that application forward, or only the most-recently-used one? Presumably the preview/highlighting should show the same thing. > Using the mouse to set focus Mouse selection needs to occur only after you actually move the mouse while the switcher is active; you don't want the initial selection to be changed just because the pointer was near the center of the screen when the user hit Alt-Tab. > Multiple Windows > • Click and hold to reveal the window(s) for the selected application > (note: need a > keyboard equivalent like space or down arrow) OS X uses Alt-Tab (with a pop-up switcher window) to switch between applications, and Alt-` (without a pop-up) to switch between windows within an app. (I am pretty sure that the window binding actually changes based on keyboard layout, to always be "Alt plus the key above Tab".) Anyway, we could copy this, with a twist, and say that Alt-` normally is cycle-windows-within-app, Alt-Tab brings up the app switcher, and Alt-` inside the app switcher pops up the window pop-up and cycles through it. (Annoyingly, Alt-` in the OS X app switcher is equivalent to Alt-Shift-Tab. > • If a window to reveal is in a different workspace than the current > one, quickly > switch to that workspace first. That seems like it may cause whiplash. > • If an application has windows in multiple workspaces, switch to the > multiple > workspaces view as needed. Same effect as in the shell. It might be cleaner to just always switch to the overview, at least if there are multiple workspaces.
(In reply to comment #2) > (In reply to comment #1) > > • Show one icon per application, but provide a way to switch to any specific > > window. > > • Add a way to preview or improve the usefulness of selected window > > highlighting > > When you switch to an application, does it bring all windows for that > application forward, or only the most-recently-used one? Presumably the > preview/highlighting should show the same thing. Most recent. > > Using the mouse to set focus > > Mouse selection needs to occur only after you actually move the mouse while the > switcher is active; you don't want the initial selection to be changed just > because the pointer was near the center of the screen when the user hit > Alt-Tab. Yup. > > Multiple Windows > > • Click and hold to reveal the window(s) for the selected application > > (note: need a > > keyboard equivalent like space or down arrow) > > OS X uses Alt-Tab (with a pop-up switcher window) to switch between > applications, and Alt-` (without a pop-up) to switch between windows within an > app. (I am pretty sure that the window binding actually changes based on > keyboard layout, to always be "Alt plus the key above Tab".) > > Anyway, we could copy this, with a twist, and say that Alt-` normally is > cycle-windows-within-app, Alt-Tab brings up the app switcher, and Alt-` inside > the app switcher pops up the window pop-up and cycles through it. Nice idea. > > • If a window to reveal is in a different workspace than the current > > one, quickly > > switch to that workspace first. > > That seems like it may cause whiplash. I think that was kind of a random idea from Jeremy that I copied from the wiki. I don't favor it. It should come out of the spec I think. > > • If an application has windows in multiple workspaces, switch to the > > multiple > > workspaces view as needed. Same effect as in the shell. > > It might be cleaner to just always switch to the overview, at least if there > are multiple workspaces. I'm not sure about showing the windows as we move through the list at all. As Owen has pointed out a few times there is a competition between focusing on the list and the windows as they are revealed. We should probably stay focused on the list of apps and have a way to descend into a list of open windows from them - basically like we also want to do for the application launchers in the overview. Perhaps as you have mentioned above.
Implementation-related to this bug; I'd like ShellAppMonitor to be able to fake up "applications" if we don't know of one for a window. This would actually be based on code in ShellAppSystem, see the last patch in bug 590211. Then the function shell_app_monitor_get_window_app would never fail, you'd get a fake app. Would need to think a bit about the "id" for a fake window, i.e. what does shell_app_monitor_get_running_app_ids return. I'd like to see ShellAppMonitor hold some of the logic for say application ordering in the switcher, though we'd need to figure out how that divides up with window ordering and the mutter stacking.
(In reply to comment #1) > [...] > • Fast switching to any running application regardless of workspace. This would practically render workspaces useless, they are used to separate apps from each other (based on task or whatever metric the user is using), mixing the apps cross workspace like that makes them useless. If the window switching tools ignore the fact that there are multiple workspaces, why bother using them at all?
My most common workspace use is: Workspace 1 Workspace 2 Pile of maximized windows gimp Or: Workspace 1 Workspace 2 Pile of maximized windows spatial nautilus browsing So I'm using workspaces to deal with the fact that it's hard to mix maximized windows with applications that assume you can get to the desktop or require working with lots of small windows.
(In reply to comment #6) > My most common workspace use is: > > Workspace 1 Workspace 2 > Pile of maximized windows gimp > > Or: > > Workspace 1 Workspace 2 > Pile of maximized windows spatial nautilus browsing > > So I'm using workspaces to deal with the fact that it's hard to mix maximized > windows with applications that assume you can get to the desktop or require > working with lots of small windows. So when you are working with the spatial nautilus windows and alt-tab to switch to a window, wouldn't the "pile of maximized windows" from workspace 1 be just in your way? (in the "mix all workspaces in the alt-tab window" world). When I want to switch to an app on a different workspace, well I switch to that workspace. But while working on one workspace I don't really care what is on the other one. (well if I did there would be no point in working with more than one workspace, other than dealing with apps like gimp)
about to attach some patches so owen can grab them easily for a demo, but not that this code isn't finalized yet. known issues: - the cycle-windows keybinding doesn't work, either at the top level, or inside the alt-tab dialog - windows aren't highlighted when you hover over them in the pop-up menu - a lot of the old UI bits haven't been changed; it still uses the old "indicator" rectangle, and it still puts apps into a grid, rather than a row, if there are enough of them
Created attachment 143346 [details] [review] Remove taskbar-highlighting support Gets rid of the code in altTab.js for highlighting pieces of the taskbar when alt-tabbing to a minimized window
Created attachment 143347 [details] [review] Initial port of Alt-Tab switcher to custom_handler interface
Created attachment 143348 [details] [review] Use AppIcon in the Application Switcher
Created attachment 143349 [details] [review] Make BaseWellItem a subclass of AppIcon
Created attachment 143350 [details] [review] Move WellMenu to AppIconMenu
Created attachment 143351 [details] [review] Make Alt-Tab work by apps rather than windows, and implement window popup
the two patches not marked needs-work (remove taskbar-highlighting support, and make BaseWellItem a subclass of AppIcon) need better commit messages, but are otherwise ready for review
Comment on attachment 143346 [details] [review] Remove taskbar-highlighting support Maybe call it _lightbox to avoid the gestapo? Also it's unclear to me when you're hiding/showing it now; does that come in a future patch?
(In reply to comment #16) > Also it's unclear to me when you're hiding/showing it now; does that come in a > future patch? It has the same lifecycle as the popup; it's shown in AltTabPopup.show() and destroyed in AltTabPopup.destroy(). (The shows/hides that were removed in this patch were just for switching between full-screen-overlay mode and full-screen-minus-a-hole overlay mode)
Comment on attachment 143347 [details] [review] Initial port of Alt-Tab switcher to custom_handler interface >+function AltTabPopup(apps) { >+ return this._init(apps); > } Would be nicer to be dynamic; have it look up apps internally and watch for changes. > AltTabPopup.prototype = { >- _init : function() { >+ _init : function(apps) { > this.actor = new Big.Box({ background_color : POPUP_BG_COLOR, > corner_radius: POPUP_GRID_SPACING, > padding: POPUP_GRID_SPACING, > spacing: POPUP_GRID_SPACING, >- orientation: Big.BoxOrientation.VERTICAL }); >- >+ orientation: Big.BoxOrientation.VERTICAL, >+ reactive: true }); >+ >+ Main.pushModal(this.actor); Remember to change this to "if (!Main.pushModal..." after my patch lands which I'll do very shortly. >+ this._overlay.connect('button-release-event', function() { log('clicked on overlay'); }); >+ >+ // Fill in the windows >+ let appMonitor = Shell.AppMonitor.get_default(); Tangent - what do you think about adding these (and esp ShellTextureCache) to the global scope? >+ this._windows = []; >+ for (let i = 0; i < apps.length; i++) { >+ let appWindows = appMonitor.get_windows_for_app(apps[i].get_id()); >+ this._windows = this._windows.concat(appWindows); >+ } So this patch is sort of an in-between state where we still display windows, we're just looking them up from the app monitor and are starting to hook up some keybindings? > let pixbuf = item.metaWindow.icon; > item.icon = new Clutter.Texture({ width: POPUP_ICON_SIZE, > height: POPUP_ICON_SIZE, >- keep_aspect_ratio: true }); >+ keep_aspect_ratio: true, >+ reactive: true }); > Shell.clutter_texture_set_from_pixbuf(item.icon, pixbuf); Note you can use shell_texture_cache_bind_pixbuf_property for this (see usage in appDisplay). >@@ -112,6 +136,16 @@ AltTabPopup.prototype = { > item.n = this._items.length; > this._items.push(item); > >+ item.icon.connect('enter-event', >+ Lang.bind(this, function () { >+ this.select(item.n); >+ })); >+ item.icon.connect('button-press-event', >+ Lang.bind(this, function (actor, event) { >+ this.select(item.n); >+ this.finish(true, event.get_time()); >+ })); This may be hard to do, but you might consider switching to a ShellButtonBox for items, since otherwise you can get issues with handling things on button press, and then the release gets passed through to some underlying unrelated actor; say part of the alt-tab popup is over the dash and there's a button-release-event handler there. > > destroy : function() { > this.actor.destroy(); > this._overlay.destroy(); >+ >+ global.stage.disconnect(this._keyPressEventId); >+ global.stage.disconnect(this._keyReleaseEventId); >+ }, Rather than calling this explicitly, maybe have it be a signal handler on 'destroy' of the actor for more reliability? (In the ideal future of course we have gjs subclassing). >+ shellwm.takeover_keybinding('switch_windows'); >+ shellwm.connect('keybinding::switch_windows', Lang.bind(this, this._doAppSwitch)); >+ shellwm.takeover_keybinding('cycle_group'); >+ shellwm.connect('keybinding::cycle_group', Lang.bind(this, this._doWindowSwitch)); I'll leave the keybinding stuff to Owen =) >+ let tabPopup = new AltTab.AltTabPopup(apps); Would it make sense to have the popup be persistent rather than created each time?
Comment on attachment 143348 [details] [review] Use AppIcon in the Application Switcher >From 4ef0b2b64a3f15fe178337680152db583c3dd968 Mon Sep 17 00:00:00 2001 >From: Dan Winship <danw@gnome.org> >Date: Thu, 13 Aug 2009 12:52:50 -0400 >Subject: [PATCH] Use AppIcon in the Application Switcher > >https://bugzilla.gnome.org/show_bug.cgi?id=590563 >--- > js/ui/altTab.js | 72 ++++++++++++++++------------------------------------- > js/ui/appIcon.js | 4 ++- > 2 files changed, 25 insertions(+), 51 deletions(-) > >diff --git a/js/ui/altTab.js b/js/ui/altTab.js >index 9742b2b..cb8fb40 100644 >--- a/js/ui/altTab.js >+++ b/js/ui/altTab.js >@@ -3,10 +3,12 @@ > const Big = imports.gi.Big; > const Clutter = imports.gi.Clutter; > const Lang = imports.lang; >+const Mainloop = imports.mainloop; > const Meta = imports.gi.Meta; > const Pango = imports.gi.Pango; > const Shell = imports.gi.Shell; > >+const AppIcon = imports.ui.appIcon; > const Main = imports.ui.main; > const Tweener = imports.ui.tweener; > >@@ -20,9 +22,7 @@ POPUP_TRANSPARENT.from_pixel(0x00000000); > const POPUP_INDICATOR_WIDTH = 4; > const POPUP_GRID_SPACING = 8; > const POPUP_ICON_SIZE = 48; >-const POPUP_NUM_COLUMNS = 5; >- >-const POPUP_LABEL_MAX_WIDTH = POPUP_NUM_COLUMNS * (POPUP_ICON_SIZE + POPUP_GRID_SPACING); >+const POPUP_NUM_COLUMNS = 10; > > const OVERLAY_COLOR = new Clutter.Color(); > OVERLAY_COLOR.from_pixel(0x00000044); >@@ -57,26 +57,13 @@ AltTabPopup.prototype = { > gcenterbox.append(this._grid, Big.BoxPackFlags.NONE); > this.actor.append(gcenterbox, Big.BoxPackFlags.NONE); > >- // Selected-window label >- this._label = new Clutter.Text({ font_name: "Sans 16px", >- ellipsize: Pango.EllipsizeMode.END }); >- >- let labelbox = new Big.Box({ background_color: POPUP_INDICATOR_COLOR, >- corner_radius: POPUP_GRID_SPACING / 2, >- padding: POPUP_GRID_SPACING / 2 }); >- labelbox.append(this._label, Big.BoxPackFlags.NONE); >- let lcenterbox = new Big.Box({ orientation: Big.BoxOrientation.HORIZONTAL, >- x_align: Big.BoxAlignment.CENTER, >- width: POPUP_LABEL_MAX_WIDTH + POPUP_GRID_SPACING }); >- lcenterbox.append(labelbox, Big.BoxPackFlags.NONE); >- this.actor.append(lcenterbox, Big.BoxPackFlags.NONE); >- > // Indicator around selected icon > this._indicator = new Big.Rectangle({ border_width: POPUP_INDICATOR_WIDTH, > corner_radius: POPUP_INDICATOR_WIDTH / 2, > border_color: POPUP_INDICATOR_COLOR, > color: POPUP_TRANSPARENT }); > this.actor.append(this._indicator, Big.BoxPackFlags.FIXED); >+ this._indicator.hide(); > > this._items = []; > this._toplevels = global.window_group.get_children(); >@@ -106,21 +93,18 @@ AltTabPopup.prototype = { > this._windows.sort(function(w1, w2) { return w2.get_user_time() - w1.get_user_time(); }); > > for (let i = 0; i < this._windows.length; i++) >- this._addWindow(this._windows[i]); >+ this._addWindow(this._windows[i], appMonitor.get_window_app(_this.windows[i])); > > this._show(); > }, > >- _addWindow : function(metaWin) { >+ _addWindow : function(metaWin, app) { > let item = { metaWindow: metaWin, > window: metaWin.get_compositor_private() }; > >- let pixbuf = item.metaWindow.icon; >- item.icon = new Clutter.Texture({ width: POPUP_ICON_SIZE, >- height: POPUP_ICON_SIZE, >- keep_aspect_ratio: true, >- reactive: true }); >- Shell.clutter_texture_set_from_pixbuf(item.icon, pixbuf); >+ let appIcon = new AppIcon.AppIcon(app); >+ item.icon = appIcon.actor; >+ item.icon.reactive = true; > > item.box = new Big.Box({ padding: POPUP_INDICATOR_WIDTH * 2 }); > item.box.append(item.icon, Big.BoxPackFlags.NONE); >@@ -169,6 +153,11 @@ AltTabPopup.prototype = { > this.actor.y = Math.floor((global.screen_height - this.actor.height) / 2); > > this.select(0); >+ this._idleSelectId = Mainloop.idle_add(Lang.bind(this, >+ function () { >+ this.select(this._selected.n); >+ return false; >+ })); > }, > > _keyPressEvent : function(actor, event) { >@@ -219,6 +208,9 @@ AltTabPopup.prototype = { > }, > > destroy : function() { >+ if (this._idleSelectId) >+ Mainloop.source_remove(this._idleSelectId); >+ > this.actor.destroy(); > this._overlay.destroy(); > >@@ -227,15 +219,14 @@ AltTabPopup.prototype = { > }, > > next : function() { >- if (!this._selected || >- this._selected.n == this._windows.length - 1) >+ if (this._selected.n == this._windows.length - 1) > this.select(0); > else > this.select(this._selected.n + 1); > }, > > previous : function() { >- if (!this._selected || this._selected.n == 0) >+ if (this._selected.n == 0) > this.select(this._windows.length - 1); > else > this.select(this._selected.n - 1); >@@ -245,11 +236,6 @@ AltTabPopup.prototype = { > if (this._selected) { > // Unselect previous > >- if (this._allocationChangedId) { >- this._selected.box.disconnect(this._allocationChangedId); >- delete this._allocationChangedId; >- } >- > if (this._selected.above) > this._selected.window.raise(this._selected.above); > else >@@ -261,11 +247,6 @@ AltTabPopup.prototype = { > this._selected = item; > > if (this._selected) { >- this._label.set_size(-1, -1); >- this._label.text = this._selected.metaWindow.title; >- if (this._label.width > POPUP_LABEL_MAX_WIDTH) >- this._label.width = POPUP_LABEL_MAX_WIDTH; >- > // Figure out this._selected.box's coordinates in terms of > // this.actor > let bx = this._selected.box.x, by = this._selected.box.y; >@@ -276,7 +257,7 @@ AltTabPopup.prototype = { > actor = actor.get_parent(); > } > >- if (changed) { >+ if (changed && this._indicator.x != 0) { > Tweener.addTween(this._indicator, > { x: bx, > y: by, >@@ -290,22 +271,13 @@ AltTabPopup.prototype = { > this._indicator.set_size(this._selected.box.width, > this._selected.box.height); > } >- this._indicator.show(); >+ if (bx != 0) >+ this._indicator.show(); > > if (this._overlay.visible) > this._selected.window.raise(this._overlay); >- >- this._allocationChangedId = >- this._selected.box.connect('notify::allocation', >- Lang.bind(this, this._allocationChanged)); > } else { >- this._label.text = ""; > this._indicator.hide(); > } >- }, >- >- _allocationChanged : function() { >- if (this._selected) >- this.select(this._selected.n); > } > }; >diff --git a/js/ui/appIcon.js b/js/ui/appIcon.js >index 5ca1cfd..1cd2eca 100644 >--- a/js/ui/appIcon.js >+++ b/js/ui/appIcon.js >@@ -37,7 +37,9 @@ AppIcon.prototype = { > > let iconBox = new Big.Box({ orientation: Big.BoxOrientation.VERTICAL, > x_align: Big.BoxAlignment.CENTER, >- y_align: Big.BoxAlignment.CENTER }); >+ y_align: Big.BoxAlignment.CENTER, >+ width: APP_ICON_SIZE, >+ height: APP_ICON_SIZE }); > this.icon = appInfo.create_icon_texture(APP_ICON_SIZE); > iconBox.append(this.icon, Big.BoxPackFlags.NONE); > >-- >1.6.2.5
Comment on attachment 143348 [details] [review] Use AppIcon in the Application Switcher > }; >diff --git a/js/ui/appIcon.js b/js/ui/appIcon.js >index 5ca1cfd..1cd2eca 100644 >--- a/js/ui/appIcon.js >+++ b/js/ui/appIcon.js >@@ -37,7 +37,9 @@ AppIcon.prototype = { > > let iconBox = new Big.Box({ orientation: Big.BoxOrientation.VERTICAL, > x_align: Big.BoxAlignment.CENTER, >- y_align: Big.BoxAlignment.CENTER }); >+ y_align: Big.BoxAlignment.CENTER, >+ width: APP_ICON_SIZE, >+ height: APP_ICON_SIZE }); > this.icon = appInfo.create_icon_texture(APP_ICON_SIZE); > iconBox.append(this.icon, Big.BoxPackFlags.NONE); Should be split out into its own patch or maybe merged into another more related one. Otherwise this looks like we're moving along well.
Comment on attachment 143349 [details] [review] Make BaseWellItem a subclass of AppIcon >From fd41c656e4402babb6eca9928309eaf760bb1bcb Mon Sep 17 00:00:00 2001 >From: Dan Winship <danw@gnome.org> >Date: Fri, 11 Sep 2009 17:13:50 -0400 >Subject: [PATCH] Make BaseWellItem a subclass of AppIcon Sorry for misunderstanding the requirements initially btw; the specs for the well menu were nontrivial and I had trouble implementing that and keeping in my head what alt-tab's needs were. >- getDragActor: function(stageX, stageY) { >- return this.icon.getDragActor(stageX, stageY); >+ getDragActor: function() { >+ return this.createDragActor(); > }, > > // Returns the original icon that is being used as a source for the cloned texture > // that represents the item as it is being dragged. > getDragActorSource: function() { >- return this.icon.getDragActorSource(); >+ return this.actor; > } If we're inheriting now, both of these can just be deleted, no?
Comment on attachment 143350 [details] [review] Move WellMenu to AppIconMenu >- this.actor.connect('button-press-event', Lang.bind(this, this._onButtonPress)); >- this.actor.connect('notify::hover', Lang.bind(this, this._onHoverChanged)); > this.actor.connect('activate', Lang.bind(this, this._onActivate)); > }, > > _onActivate: function (actor, event) { > let modifiers = event.get_state(); > >- if (this._menuTimeoutId > 0) { >- Mainloop.source_remove(this._menuTimeoutId); >- this._menuTimeoutId = 0; >- } >- (note to self to look for where this stuff has moved to) >+// Constants for specifying pop-up menu behavior >+const MENU_NONE = 0; >+const MENU_ON_RIGHT = 1; >+const MENU_BELOW = 2; enums come out better like this: var MenuStyle = { NONE: 0, ON_RIGHT: 1, BELOW: 2 }; >+ if (menuType) { >+ this.windows = Shell.AppMonitor.get_default().get_windows_for_app(appInfo.get_id()); >+ for (let i = 0; i < this.windows.length; i++) { >+ this.windows[i].connect('notify::user-time', Lang.bind(this, this._resortWindows)); >+ } >+ this._resortWindows(); >+ >+ this.actor.connect('button-press-event', Lang.bind(this, this._updateMenuOnButtonPress)); >+ this.actor.connect('notify::hover', Lang.bind(this, this._updateMenuOnHoverChanged)); >+ this.actor.connect('activate', Lang.bind(this, this._updateMenuOnActivate)); >+ } else >+ this.windows = []; Why is all this conditional on menuType being "true"? I thought it was an enum? >+ }, >+ >+ _updateMenuOnActivate: function(actor, event) { >+ if (this._menuTimeoutId) { >+ Mainloop.source_remove(this._menuTimeoutId); >+ delete this._menuTimeoutId; Would prefer "if (this._menuTimeoutId > 0)" together with "this._menuTimeoutId = 0;". The Spidermonkey JIT does compute "shapes" for objects with a fixed set of properties and types. >+AppIconMenu.prototype = { Assuming not much else changed here aside from the move; if so can you note it?
Comment on attachment 143351 [details] [review] Make Alt-Tab work by apps rather than windows, and implement window popup Looks fine.
(In reply to comment #18) > (From update of attachment 143347 [details] [review]) > > >+function AltTabPopup(apps) { > >+ return this._init(apps); > > } > > Would be nicer to be dynamic; have it look up apps internally and watch for > changes. I'm not sure you'd want it to change on the fly if a new application finished starting while the popup was visible (the existing metacity one does not). Also, if there are no apps running, we don't want to create the popup, so the caller needs to call get_current_apps anyway... > >+ let appMonitor = Shell.AppMonitor.get_default(); > > Tangent - what do you think about adding these (and esp ShellTextureCache) to > the global scope? Makes sense to me. Or at least as properties on global? > So this patch is sort of an in-between state where we still display windows, > we're just looking them up from the app monitor and are starting to hook up > some keybindings? Yes. Also, you missed the part where most of the patches here were labelled "needs-work" and there was a comment saying to not bother reviewing them yet. :-) > This may be hard to do, but you might consider switching to a ShellButtonBox > for items, since otherwise you can get issues with handling things on button > press, and then the release gets passed through to some underlying unrelated > actor; say part of the alt-tab popup is over the dash and there's a > button-release-event handler there. Hm... should that happen at the AppIcon level? > Rather than calling this explicitly, maybe have it be a signal handler on > 'destroy' of the actor for more reliability? OK. To some extent this is another artifact of the patches not being done yet; there are places where it hasn't been modified very much from the original AltTabPopup but it should be. > >+ let tabPopup = new AltTab.AltTabPopup(apps); > > Would it make sense to have the popup be persistent rather than created each > time? I don't think so; I don't think it takes much time to create, and if it persisted then it would have to watch the appmonitor signals to track apps and stuff, which would make it more complicated. (In reply to comment #21) > If we're inheriting now, both of these can just be deleted, no? DND is a feature of BaseWellItem, not AppIcon, so it seemed to make more sense to have them at the BaseWellItem level (In reply to comment #22) > >+// Constants for specifying pop-up menu behavior > >+const MENU_NONE = 0; > >+const MENU_ON_RIGHT = 1; > >+const MENU_BELOW = 2; > > enums come out better like this: > > var MenuStyle = { NONE: 0, ON_RIGHT: 1, BELOW: 2 }; ah, nice > Why is all this conditional on menuType being "true"? I thought it was an > enum? it's conditional on menuType being "truthy", which ON_RIGHT and BELOW are, and NONE and "undefined" aren't. > >+ if (this._menuTimeoutId) { > >+ Mainloop.source_remove(this._menuTimeoutId); > >+ delete this._menuTimeoutId; > > Would prefer "if (this._menuTimeoutId > 0)" together with "this._menuTimeoutId > = 0;". The Spidermonkey JIT does compute "shapes" for objects with a fixed set > of properties and types. Not sure exactly what you mean by the JIT comment or what that implies for our execution speed... My feeling was that the presence/absence of a property is a clearer way to express the idea of the presence/absence of a timeout than having a property which sometimes has a special value meaning "no timeout". But I'm happy to switch if people would rather stick with the more glib-in-C-like way. > >+AppIconMenu.prototype = { > > Assuming not much else changed here aside from the move; if so can you note it? _findWindowCloneForActor -> _findMetaWindowForActor and related changes. Also, it supports both MENU_ON_RIGHT and MENU_BELOW. I'm not really happy with the use of ClutterGravity as a type for telling shell_draw_box_pointer which way to point but it seemed silly to use GtkArrowType and I didn't feel like defining my own type...
(In reply to comment #24) > I'm not sure you'd want it to change on the fly if a new application finished > starting while the popup was visible (the existing metacity one does not). > Also, if there are no apps running, we don't want to create the popup, so the > caller needs to call get_current_apps anyway... Fair enough. > > > This may be hard to do, but you might consider switching to a ShellButtonBox > > for items, since otherwise you can get issues with handling things on button > > press, and then the release gets passed through to some underlying unrelated > > actor; say part of the alt-tab popup is over the dash and there's a > > button-release-event handler there. > > Hm... should that happen at the AppIcon level? Actually you've made the AppIcon a ShellButtonBox now, so maybe whatever you're doing you can just connect to 'activate'? > Not sure exactly what you mean by the JIT comment or what that implies for our > execution speed... Well we don't really have any kind of numbers for performance and I haven't tried turning on the JIT yet (it turns out to just be a flag you pass when creating a JSContext), but: > My feeling was that the presence/absence of a property is a clearer way to > express the idea of the presence/absence of a timeout than having a property > which sometimes has a special value meaning "no timeout". But I'm happy to > switch if people would rather stick with the more glib-in-C-like way. I'd much prefer if, performance reasons aside, just for code maintainability we behaved as if Lang.seal(this); was called at the end of every _init function by default. In fact we might want to consider doing so (once it exists, bug 593320).
Comment on attachment 143346 [details] [review] Remove taskbar-highlighting support didn't rename overlay to lightbox cause it seems like "lightbox" would have to refer to either the overall effect, or just the highlighted window, but not just the... dark box. anyway, we need a lightbox.js anyway so this will eventually be ported to that
Created attachment 143451 [details] [review] Make sure small icons still get full-sized AppIcon actors
Created attachment 143452 [details] [review] Remove redundant window array from AppIcon
Created attachment 143453 [details] [review] Make BaseWellItem a subclass of AppIcon
Created attachment 143454 [details] [review] Genericize WellMenu a bit Rather than directly interacting with the overview, emit signals and let WellItem do the overview interactions.
Created attachment 143455 [details] [review] Move WellMenu to AppIconMenu
Created attachment 143456 [details] [review] Allow AppIcon menu to appear either to the right of or below the icon
Created attachment 143706 [details] [review] Add Main.activateWindow() to remove some duplication
Created attachment 143708 [details] [review] Initial port of Alt-Tab switcher to custom_handler interface So this patch: - Intercepts the "switch_windows" (aka Alt-Tab) keybinding from mutter - Use Main.pushModal() to grab the keyboard and mouse - Watches for Tab keypresses and Alt keyreleases and DTRT (A later patch will make altTab.js also watch for "`" keypresses when the switcher is up.) If the user has bound "switch_windows" to something other than Alt-Tab, then he will have to press that key to get into the switcher, and then use Tab to continue switching from within the switcher. (And if the original keybinding didn't involve the Alt key, then he'll have to press and release Alt to get out of it.) One alternative, and the way meta_keybindings_set_custom_handler() is supposed to work, would be for us to use meta_display_begin_grab_op() rather than Main.pushModal(), in which case mutter would continue to send us additional "switch_windows" events as the user continued to press Alt-Tab (or whatever their keybinding was), and would send some other event when the user released all the relevant modifier keys. But if we use mutter's grab mechanism, then mutter will release the grab when it thinks the user has done something non-alt-tab-popup-like, such as pressing the mouse button, or typing Alt-`. So we'd have to add hacks to gnome_shell_plugin_xevent_filter() to keep those events from reaching mutter, which means we'd be handling the Alt-` binding on our own anyway. I'm still not sure what the best solution here is.
Created attachment 143710 [details] [review] Make Alt-Tab use AppIcon and switch applications rather than windows Notably different from the earlier patch is that we now use the existing AppIcon border rather than hovering a separate indictor actor over the icons and tweening it between them. Besides being simpler, getting rid of the tweening also looks a lot better when we get to the "mouse selection in the alt-tab popup" patch later. But I had to make the border wider for it to look good. Maybe we should have a bigger border in the well too?
Created attachment 143711 [details] [review] Implement window cycling This makes Alt-` work inside the Alt-Tab popup (though not outside of it). Might want to split the AppIcon parts out separately?
Created attachment 143712 [details] [review] Implement pointer selection in Alt-Tab
Comment on attachment 143451 [details] [review] Make sure small icons still get full-sized AppIcon actors Yep. Consider prefixing with [AppIcon].
Comment on attachment 143453 [details] [review] Make BaseWellItem a subclass of AppIcon Looks good.
Comment on attachment 143454 [details] [review] Genericize WellMenu a bit > >- // Whether or not we successfully picked a window >- this.didActivateWindow = false; Would prefer to see this converted to _didActivateWindow instead of removed.
Comment on attachment 143455 [details] [review] Move WellMenu to AppIconMenu I assume any notable changes here were broken out into the previous patch.
Comment on attachment 143456 [details] [review] Allow AppIcon menu to appear either to the right of or below the icon > >- if (hasMenu) { >+ if (menuType) { I'd prefer an explicit menuType == MenuType.NONE here rather than relying on truthy but that could just be me. >+ if (this._type == MenuType.ON_RIGHT) { >+ childBox.x1 = 0; >+ childBox.x2 = APPICON_MENU_ARROW_SIZE; >+ childBox.y1 = Math.floor((height / 2) - (APPICON_MENU_ARROW_SIZE / 2)); >+ childBox.y2 = childBox.y1 + APPICON_MENU_ARROW_SIZE; >+ this._arrow.allocate(childBox, flags); >+ >+ /* overlap by one pixel to hide the border */ >+ childBox.x1 = APPICON_MENU_ARROW_SIZE - 1; This is my bug originally but that "- 1" should be " - APPICON_BORDER_WIDTH" (i think that's the right variable name now, without actually looking at the previous patches)
Comment on attachment 143706 [details] [review] Add Main.activateWindow() to remove some duplication >From 010e651a5744f1a7b1b5a877a856f07c382546e2 Mon Sep 17 00:00:00 2001 >From: Dan Winship <danw@gnome.org> >Date: Mon, 21 Sep 2009 16:29:37 -0400 >Subject: [PATCH] Add Main.activateWindow() to remove some duplication > >--- > js/ui/main.js | 15 +++++++++++++++ > js/ui/workspaces.js | 21 +++++---------------- > 2 files changed, 20 insertions(+), 16 deletions(-) > >diff --git a/js/ui/main.js b/js/ui/main.js >index 62eea85..ccc0b64 100644 >--- a/js/ui/main.js >+++ b/js/ui/main.js >@@ -299,3 +299,18 @@ function createAppLaunchContext() { > > return context; > } >+ >+function activateWindow(window, time) { Needs a doc comment. >+ let activeWorkspaceNum = global.screen.get_active_workspace_index(); >+ let windowWorkspaceNum = window.get_workspace().index(); >+ >+ if (!time) >+ time = Clutter.get_current_event_time(); From an earlier discussion we determined it'd be best to use global.screen.get_display().get_current_time() rather than the Clutter time, since the latter is only accurate inside the X event filter.
Comment on attachment 143708 [details] [review] Initial port of Alt-Tab switcher to custom_handler interface > >+ if (!Main.pushModal(this.actor)) >+ return false; >+ this.actor.connect('destroy', Lang.bind(this, function() { Main.popModal(this.actor); })); Hm, I'd expect this to have been: this.actor.connect('destroy', Lang.bind(this, this._onDestroy); > destroy : function() { this._onDestroy: function () { Main.popModal(this.actor); ... other destroy stuff And then whenever you want alt-tab to go away you call actor.destroy(). You could wrap it with a close() method if that feels cleaner. What I'm getting at though is if later we add some sort of generic popup/dialog/modality infrastructure it seems not unlikely that its "close" method would destroy the actor, but it wouldn't know to call your JS destroy(). Leaving the keybinding stuff to Owen as before.
(In reply to comment #43) > >+ let activeWorkspaceNum = global.screen.get_active_workspace_index(); > >+ let windowWorkspaceNum = window.get_workspace().index(); > >+ > >+ if (!time) > >+ time = Clutter.get_current_event_time(); > > From an earlier discussion we determined it'd be best to use > global.screen.get_display().get_current_time() rather than the Clutter time, > since the latter is only accurate inside the X event filter. Atually, this changed when I added event queing and compression to Clutter. global.screen.get_display().get_current_time() will now return CurrentTime inside button press handlers, etc. I think we need to actually have a convenience function somewhere that tries both.
Comment on attachment 143710 [details] [review] Make Alt-Tab use AppIcon and switch applications rather than windows Looks fine overall, but I'm not really familiar with the altTab.js code.
Comment on attachment 143711 [details] [review] Implement window cycling >+ popdown: function() { >+ this._windowContainer.popdown(); >+ this.emit('popup', false); >+ this.actor.hide(); >+ }, Should we also reset this._highlightedItem here? Otherwise looks OK.
Comment on attachment 143712 [details] [review] Implement pointer selection in Alt-Tab >From 50bff3ec213153f221a4a1179d426de6cff290fc Mon Sep 17 00:00:00 2001 >From: Dan Winship <danw@gnome.org> >Date: Mon, 21 Sep 2009 13:44:42 -0400 >Subject: [PATCH] Implement pointer selection in Alt-Tab > >--- > js/ui/altTab.js | 30 ++++++++++++++++++++++++++++++ > 1 files changed, 30 insertions(+), 0 deletions(-) > >diff --git a/js/ui/altTab.js b/js/ui/altTab.js >index 74fca37..cbd5cb7 100644 >--- a/js/ui/altTab.js >+++ b/js/ui/altTab.js >@@ -73,6 +73,8 @@ AltTabPopup.prototype = { > appIcon.connect('menu-popped-up', Lang.bind(this, this._menuPoppedUp)); > appIcon.connect('menu-popped-down', Lang.bind(this, this._menuPoppedDown)); > >+ appIcon.actor.connect('enter-event', Lang.bind(this, this._iconEntered)); >+ > // FIXME? > appIcon.actor.border = 2; > >@@ -101,6 +103,10 @@ AltTabPopup.prototype = { > this._keyPressEventId = global.stage.connect('key-press-event', Lang.bind(this, this._keyPressEvent)); > this._keyReleaseEventId = global.stage.connect('key-release-event', Lang.bind(this, this._keyReleaseEvent)); > >+ this._motionEventId = this.actor.connect('motion-event', Lang.bind(this, this._mouseMoved)); >+ this._mouseActive = false; >+ this._mouseMovement = 0; >+ > // Contruct the AppIcons, sort by time, add to the popup > let icons = []; > for (let i = 0; i < apps.length; i++) >@@ -172,6 +178,30 @@ AltTabPopup.prototype = { > this._highlightWindow(window); > }, > >+ _mouseMoved : function(actor, event) { >+ if (++this._mouseMovement < 3) >+ return; Should be a constant; would it make sense to use say the gtk dnd threshold? >+ >+ this.actor.disconnect(this._motionEventId); >+ this._mouseActive = true; >+ >+ let [x, y] = event.get_coords(); >+ actor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y); Hmm? Why isn't this event.get_source()?
(In reply to comment #40) > (From update of attachment 143454 [details] [review]) > > > >- // Whether or not we successfully picked a window > >- this.didActivateWindow = false; > > Would prefer to see this converted to _didActivateWindow instead of removed. WellMenu no longer cares about whether or not a window got activated though. So "didActivateWindow" gets removed from WellMenu, and RunningWellItem gains a "_didActivateWindow" to replace it. (In reply to comment #41) > (From update of attachment 143455 [details] [review]) > I assume any notable changes here were broken out into the previous patch. indeed (In reply to comment #42) > (From update of attachment 143456 [details] [review]) > > > >- if (hasMenu) { > >+ if (menuType) { > > I'd prefer an explicit menuType == MenuType.NONE here rather than relying on > truthy but that could just be me. as discussed on IRC, part of the goal was to have the argument be optional; both MenuType.NONE and undefined would compare as false here. But I changed it to de-optionalify it in the constructor: this._init(appInfo, menuType || MenuType.NONE); > This is my bug originally but that "- 1" should be " - APPICON_BORDER_WIDTH" fixed squashed and pushed these three patches (which were really a single "move menu from wellitem to appicon" patch but split into three pieces for ease of reviewing)
Created attachment 143801 [details] [review] Add Main.currentTime(), to abstract the different current-event-time methods I did Clutter.get_current_event_time() first because it doesn't require dereferencing as many properties, so it's faster, and probably correct in most of the places we'd be calling this from. We could also change various other methods to make their time arguments optional and just use Main.currentTime() when it's not passed?
(In reply to comment #49) > (In reply to comment #40) > > (From update of attachment 143454 [details] [review] [details]) > > > > > >- // Whether or not we successfully picked a window > > >- this.didActivateWindow = false; > > > > Would prefer to see this converted to _didActivateWindow instead of removed. > > WellMenu no longer cares about whether or not a window got activated though. So > "didActivateWindow" gets removed from WellMenu, and RunningWellItem gains a > "_didActivateWindow" to replace it. I meant I didn't see a _didActivateWindow initialization in the _init function which I think we should have to avoid adding new properties after the object has been created.
Review of attachment 143801 [details] [review]: I think my main comment on this is that we aren't just "abstracting" the get time methods, we are adding a function that gives the best possible idea of "current time"... it's more correct than what we are doing currently. I think you need to reverse the tests, however, Looking into the Clutter sources, Clutter.get_event_time() returns the timestamp of the last event that Clutter started processing. context->last_event_time is never set back to zero; on the other hand Mutter properly resets the timestamp to CurrentTime after processing an event. (And an explanatory comment is then needed to explain why the ordering is as it is.) Otherwise the patch looks straightforward and good to me. I think we probably should just have a policy of not passing timestamps around in our code and always use this function; I've never really needed to pass a time that wasn't the current event time for anything.
Review of attachment 143708 [details] [review]: Didn't really look at the non-keybinding stuff here in detail, but I assume it's OK. The keybinding stuff looks good with some minor comments. ::: js/ui/altTab.js @@ -41,3 +41,4 @@ padding: POPUP_GRID_SPACING, spacing: POPUP_GRID_SPACING, - orientation: Big.BoxOrientation.VERTICAL }); + orientation: Big.BoxOrientation.VERTICAL, + reactive: true }); Unrelated to this patch, right? @@ +169,3 @@ + let backwards = (event.get_state() & Clutter.ModifierType.SHIFT_MASK); + + if (keysym == Clutter.Tab) Hmm, this is going to have to be dealt with better eventually. Basically display_get_keybinding() in mutter/src/core/keybindings.c will have to be revealed. But good for now. ::: src/shell-wm.c @@ +173,3 @@ g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0); + shell_wm_signals[KEYBINDING] = This signal is unobvious without looking at the emit code - I think it would be nice to have a signal doc comment here, describing the parameters and the detail. (and maybe even point out that the detail is exactly the keybinding names, and keybinding names have "_" as opposed to notify::prop-name where there's a "-"... that could save someone a lot of head scratching.) @@ +430,3 @@ + sbinding.keysym = binding->keysym; + sbinding.keycode = binding->keycode; + sbinding.modifiers = binding->mask | (event->xkey.state & ShiftMask); I think I understand why you are doing this (for "reverses" bindings like Alt-Tab, the shift isn't part of the binding but added by Mutter behind the scenes?), but definitely needs an explanatory comment. But then again, why would the JS code care about the keysym/keycode/modifiers here. Isn't the shift state *all* that matters?
(In reply to comment #44) > Hm, I'd expect this to have been: > > this.actor.connect('destroy', Lang.bind(this, this._onDestroy); ... > And then whenever you want alt-tab to go away you call actor.destroy(). You > could wrap it with a close() method if that feels cleaner. What I'm getting at > though is if later we add some sort of generic popup/dialog/modality > infrastructure it seems not unlikely that its "close" method would destroy the > actor, but it wouldn't know to call your JS destroy(). We've got JS destroy()s in a bunch of other places. OK, mostly written by me. Looks like we need another convention. Many of these could probably be replaced by _onDestroy handlers. But given that windowManager.js currently (in the patches) does: if (!tabPopup.show(initialSelection)) tabPopup.destroy(); it seems weird that it should have to say "tabPopup.actor.destroy()" in the second line. I suppose we could do if (tabPopup.setup(initialSelection)) tabPopup.actor.show(); else tabPopup.actor.destroy(); Though we have JS "show" methods in other objects (NOT mostly written by me) too. (In reply to comment #47) > (From update of attachment 143711 [details] [review]) > > >+ popdown: function() { > >+ this._windowContainer.popdown(); > >+ this.emit('popup', false); > >+ this.actor.hide(); > >+ }, > > Should we also reset this._highlightedItem here? It gets cleared in _redisplay(), so we don't actually need to, although we could if you think it would be clearer. > >+ _mouseMoved : function(actor, event) { > >+ if (++this._mouseMovement < 3) > >+ return; > > Should be a constant; would it make sense to use say the gtk dnd threshold? That's "8", which seems a bit high. I'll add a const. > >+ actor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y); > > Hmm? Why isn't this event.get_source()? oops
(In reply to comment #54) > (In reply to comment #44) > > Hm, I'd expect this to have been: > > > > this.actor.connect('destroy', Lang.bind(this, this._onDestroy); > ... > > And then whenever you want alt-tab to go away you call actor.destroy(). You > > could wrap it with a close() method if that feels cleaner. What I'm getting at > > though is if later we add some sort of generic popup/dialog/modality > > infrastructure it seems not unlikely that its "close" method would destroy the > > actor, but it wouldn't know to call your JS destroy(). > > We've got JS destroy()s in a bunch of other places. OK, mostly written by me. > Looks like we need another convention. It's definitely a case where if we could write is-a instead of has-a ClutterActor things would probably be nicer; but for now we could say have a function destroy () { this.actor.destroy() }. > Though we have JS "show" methods in other objects (NOT mostly written by me) > too. I've been calling my JS "show" equivalents "open" and "close" to keep the ClutterActor state separate from the JS object state.
Comment on attachment 143801 [details] [review] Add Main.currentTime(), to abstract the different current-event-time methods updated, commented, and committed
(In reply to comment #53) > + orientation: Big.BoxOrientation.VERTICAL, > + reactive: true }); > > Unrelated to this patch, right? No, since we're using that as the base of the grab, we need to make it reactive or else events don't always end up where we need them. > This signal is unobvious without looking at the emit code - I think it would be > nice to have a signal doc comment here done > But then again, why > would the JS code care about the keysym/keycode/modifiers here. Isn't the shift > state *all* that matters? right, that was cruft from an earlier state of the patches. gone now. (In reply to comment #55) > It's definitely a case where if we could write is-a instead of has-a > ClutterActor things would probably be nicer; but for now we could say have a > function destroy () { this.actor.destroy() }. I've done that for now. > I've been calling my JS "show" equivalents "open" and "close" to keep the > ClutterActor state separate from the JS object state. That works for dialog-like things, but not for delegate classes in general. I've left it "show" for now. We probably need to add all of our delegate class best practices to the style guide, and then go through and patch everything up to match that.
Created attachment 143939 [details] [review] [AppSwitcher] Keep track of the selected window for each app Two last patches before calling it good for now... Rather than selecting windows[0] each time the cycle returns to an app, select whatever window of that app was selected last time around.
Created attachment 143940 [details] [review] [AppSwitcher] Allow use of arrow keys in the popup
Review of attachment 143939 [details] [review]: Looks good
Shouldn't the mouse wheel also do the same as the up/down arrows? (Especially as currently the mouse can already be used select applications).
*** Bug 596534 has been marked as a duplicate of this bug. ***
Some comments in the form of proposed additions to the spec. Let me know what you think... * Never use more than one row of icons * All app icons must have the same width and height. Select the largest value from following list of icon size such that the application icons will fit onto on row: [96, 64, 48, 32, 22] * Use the same spacing and truncation rules as application icons in the application well. * Like icons in the application well, show the full application name (no ellipsis) when the application icon is focused. * Unlike icons in the application well, do not show the glow used to indicate running apps. It is somewhat redundant here. These are all running apps and it is fairly clear from the window list if there are multiple instances available. * Do not immediately display application windows when using Alt+Tab to cycle through the application list. The mere fact that app icons must be crossed over to focus the desired application does not mean that the user is interested in seeing a window for that application. * Do not immediately display application windows when crossing over application icons in the list with the mouse pointer. This is important because as the mouse pointer moves across the screen to target an application icon in the list it may cross other application icons. The fact that it passes over the top of other app icons does not mean that the user is interested in seeing a window of this application. In fact, showing a window in that case will very likely disrupt the user. * Only "select" or focus an app with the mouse by clicking or hovering. Otherwise casual or unintentional mouse movements interferes with keyboard navigation. * The overlay should be a box with rounded corners and the following properties: background-color: black at 80% opacity (#000000CC) border-width: 1px border-color: halfway between forground and background colors, half as opaque as the background color (#80808066) (the border color is important when viewing the overlay above a black background) * The overlay should be positioned centered vertically and horizontally on the primary monitor. * If an app has multiple windows, when you tab to it for focus an arrow appears centered beneath the app title, pointing down. If you click the down arrow key or if it stays focused for one second or so, thumbnails for all open windows appear in a horizontal row below the row of running apps. Now the user can mouse or keyboard to the focus the desired window and let go to get to it. If the list of windows does not fit on the screen we should automatically scroll horizontally as the user moves through the list. * Left and right arrow keys may be allowed to move between applications or windows in the list * Up and down arrow keys may be used to switch between the application list and window list * Special case for quickly switching between two windows of the same application: - Immediately show the Window list and * Minimized or hidden pplications should appear at the end of the list
oops the missing part of that sentence: - Immediately show the Window list with the next window selected
Created attachment 144254 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Following the idea expressed in bug 590563 by mccann ("Minimized or hidden pplications should appear at the end of the list"), we should also put applications that have no visible window in the current workspace at the end of the alt-tab window list.
Created attachment 144258 [details] [review] [AppSwitcher] Do not show the glow for icons in alt+tab dialog. Unlike icons in the application well, do not show the glow used to indicate running apps. It is somewhat redundant here. These are all running apps and it is fairly clear from the window list if there are multiple instances available, according to mccann.
(In reply to comment #63) > * Like icons in the application well, show the full application name (no > ellipsis) when the application icon is focused. (This is not actually currently true of the application well.) > * If an app has multiple windows, when you tab to it for focus an arrow > appears centered beneath the app title, pointing down. If you click the down > arrow key or if it stays focused for one second or so, thumbnails for all open > windows appear in a horizontal row below the row of running apps. So the window menu is now gone, replaced by this? Presumably if you click on the down-arrow it opens the window list immediately? What if you press-and-hold on the app icon? > Now the user > can mouse or keyboard to the focus the desired window and let go to get to it. > If the list of windows does not fit on the screen we should automatically > scroll horizontally as the user moves through the list. What size are the thumbnails? Are they all-the-same-size or all-the-same-scale? Do they get their own separate #000000CC/#80808066 box underneath the main appswitcher box, or does the main box expand to accomodate them, or are they not boxed? > * Left and right arrow keys may be allowed to move between applications or > windows in the list > > * Up and down arrow keys may be used to switch between the application list > and window list Does Alt-Tab always move to the next application, even if you're currently in the window list? And likewise Alt-` for next window? (Is that still wanted? Inside and outside the app switcher?) > * Special case for quickly switching between two windows of the same > application: > - Immediately show the Window list with the next window selected When? Is this what Owen suggested, that if the next-most-recently-used window is in the same app as the current window, then Alt-Tab should switch to that window, rather than switching to the next app?
Comment on attachment 144254 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Codewise, it does what it says. This needs a design ack before going in though. This patch mixes minimized and off-workspace windows together; another possibility would be to sort them separately, with either the minimized-on-this-workspace windows before visible-on-another-workspace windows, or vice-versa.
(In reply to comment #67) > (In reply to comment #63) > > * Like icons in the application well, show the full application name (no > > ellipsis) when the application icon is focused. > > (This is not actually currently true of the application well.) Yeah seems so. > > * If an app has multiple windows, when you tab to it for focus an arrow > > appears centered beneath the app title, pointing down. If you click the down > > arrow key or if it stays focused for one second or so, thumbnails for all open > > windows appear in a horizontal row below the row of running apps. > > So the window menu is now gone, replaced by this? Presumably if you click on > the down-arrow it opens the window list immediately? What if you press-and-hold > on the app icon? Yes, I kinda think of this as just the horizontal graphical version of the window list. Jeremy came up with this idea and I think it sounds good. > > Now the user > > can mouse or keyboard to the focus the desired window and let go to get to it. > > If the list of windows does not fit on the screen we should automatically > > scroll horizontally as the user moves through the list. > > What size are the thumbnails? Are they all-the-same-size or all-the-same-scale? > Do they get their own separate #000000CC/#80808066 box underneath the main > appswitcher box, or does the main box expand to accomodate them, or are they > not boxed? Jeremy would like to try around 256px - long axis scaled to fit. I'd like to try having a separate box with an arrow pointing up to the application icon. Selection/focus indicator for keynav should be similar to the app box one. > > * Left and right arrow keys may be allowed to move between applications or > > windows in the list > > > > * Up and down arrow keys may be used to switch between the application list > > and window list > > Does Alt-Tab always move to the next application, even if you're currently in > the window list? And likewise Alt-` for next window? (Is that still wanted? > Inside and outside the app switcher?) Yeah, I think I agree with Owen that losing the flip to last "thing" just because it happens to be the same app is probably not good. So, I think we need to have the initial state always be on the last used window. Alt+` is still interesting I think but probably secondary. > > * Special case for quickly switching between two windows of the same > > application: > > - Immediately show the Window list with the next window selected > > When? Is this what Owen suggested, that if the next-most-recently-used window > is in the same app as the current window, then Alt-Tab should switch to that > window, rather than switching to the next app? I'm thinking that we should only special case the initial display. After that tabbing moves through apps as normal.
Comment on attachment 144258 [details] [review] [AppSwitcher] Do not show the glow for icons in alt+tab dialog. Attachment 144258 [details] pushed as fee385b - [AppSwitcher] Do not show the glow for icons in alt+tab dialog.
Created attachment 144282 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Following the idea expressed in bug 590563 by mccann ("Minimized or hidden pplications should appear at the end of the list"), we should also put applications that have no visible window in the active workspace at the end of the alt-tab window list, after apps which have minimized windows in the active workspace.
Created attachment 144285 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Following the idea expressed in bug 590563 by mccann ("Minimized or hidden pplications should appear at the end of the list"), we should also put applications that have no visible window in the active workspace at the end of the alt-tab window list. This patch does so by displaying apps with a window on the active workspace, then a separator, then the other apps. Note that this doesn't work so well when there are more than four apps, due to the way apps are wrapped.
Created attachment 144312 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Following the idea expressed in bug 590563 by mccann ("Minimized or hidden pplications should appear at the end of the list"), we should also put applications that have no visible window in the active workspace at the end of the alt-tab window list. This patch does so by displaying apps with a window on the active workspace, then a separator, then the other apps. Note that this doesn't work so well when there are more than four apps, due to the way apps are wrapped.
Created attachment 144366 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. Following the idea expressed in bug 590563 by mccann ("Minimized or hidden pplications should appear at the end of the list"), we should also put applications that have no visible window in the active workspace at the end of the alt-tab window list, after apps which have minimized windows in the active workspace.
Created attachment 144367 [details] [review] [AppSwitcher] Drop the line wrapping code.
Created attachment 144368 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. This allows defining some custom policy for size allocation. Currently, the minimum width is always used, but it can be tweaked afterwards when a sizing policy has been defined.
Created attachment 144369 [details] [review] [AppSwitcher] Display a separator between apps on this workspace and others. This makes a visible distinction between the apps that only have minimized windows on the current workspace and the ones that have no window on the workspace.
Created attachment 144370 [details] [review] [AppSwitcher] Fix the sizing of the separator.
Created attachment 144372 [details] A screenshot of the updated alt-tab dialog, with container and separator
Comment on attachment 144367 [details] [review] [AppSwitcher] Drop the line wrapping code. actually, as mentioned on IRC, we should rename the references to "grid", and remove the comment about possibly using NbtkGrid
Comment on attachment 144368 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. what exactly does this do differently from using Big.Box?
(In reply to comment #81) > (From update of attachment 144368 [details] [review]) > what exactly does this do differently from using Big.Box? I might be wrong, but it seems you can't control allocation that easily using Big.Box, to make it so every icon has the same size.
Created attachment 144416 [details] [review] [AppSwitcher] Drop the line wrapping code. Also rename _grid into _appsBox as grid is not an appropriate word anymore for what this box is.
Created attachment 144425 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. This allows defining some custom policy for size allocation. Currently, the minimum width is always used, but it can be tweaked afterwards when a sizing policy has been defined.
Created attachment 144426 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. This allows defining some custom policy for size allocation. Currently, the minimum width is always used, but it can be tweaked afterwards when a sizing policy has been defined. Rebased over the renaming of _grid to _appsBox.
Created attachment 144428 [details] [review] [AppSwitcher] Display a separator between apps on this workspace and others. This makes a visible distinction between the apps that only have minimized windows on the current workspace and the ones that have no window on the workspace. Squashed with the sizing fix, and adapted to _grid/_appsBox renaming.
Comment on attachment 144416 [details] [review] [AppSwitcher] Drop the line wrapping code. > const POPUP_GRID_SPACING = 8; rename to POPUP_APPS_SPACING or something please > // Add it to the grid >- if (!this._gridRow || this._gridRow.get_children().length == POPUP_NUM_COLUMNS) { >- this._gridRow = new Big.Box({ spacing: POPUP_GRID_SPACING, >- orientation: Big.BoxOrientation.HORIZONTAL }); >- this._grid.append(this._gridRow, Big.BoxPackFlags.NONE); >- } >- this._gridRow.append(appIcon.actor, Big.BoxPackFlags.NONE); >+ this._appsBox.append(appIcon.actor, Big.BoxPackFlags.NONE); Note the comment there... you can probably just remove it. Feel free to commit with those changes.
Comment on attachment 144416 [details] [review] [AppSwitcher] Drop the line wrapping code. Attachment 144416 [details] pushed as 97df305 - [AppSwitcher] Drop the line wrapping code.
Created attachment 144445 [details] [review] [AppSwitcher] Select windows on current workspace by default. When using alt+tab to switch windows, we will pick a window on the active workspace as the default focused window for the application if the application has at least one window on the current workspace (that is, if it is on the left of the app switcher separator). This makes the behaviour of alt+tab more predictable for the user, as an user will expect alt+tab to switch to the window he can see right now rather than the one on the workspace he just left (presumably to do something else on the workspace he's currently on).
(In reply to comment #89) > This makes the behaviour of alt+tab more predictable for the user, as > an user will expect alt+tab to switch to the window he can see right now > rather than the one on the workspace he just left (presumably to do > something else on the workspace he's currently on). (Steve, please use “they” for a generic user instead of “he” – “he” implies that all users are male, which is incorrect and can alienate women. Thanks.)
Created attachment 144494 [details] [review] [AppSwitcher] Select next window from the current app on alt+tab This slightly changes the behaviour of the alt+tab window, this way: when using alt-tab on a workspace that contains two or more windows from the same window, the application selected when hitting alt+tab is the currently selected application, but the highlighted window is the next one. Intended goal is to make it easier to cycle around windows of the same application while not having to cycle through all the applications first.
Comment on attachment 143940 [details] [review] [AppSwitcher] Allow use of arrow keys in the popup LGTM
Comment on attachment 143940 [details] [review] [AppSwitcher] Allow use of arrow keys in the popup Attachment 143940 [details] pushed as c2af05f - [AppSwitcher] Allow use of arrow keys in the popup
Comment on attachment 144366 [details] [review] [AppSwitcher] Put apps with no window on current workspace at the end. just two nitpicks: >hidden pplications should appear at the end of the list"), we should typo (missing "a" in "applications") >+ // We intend to sort the application list so applications appears "appear" (no "s")
Comment on attachment 144426 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. >- this._appsBox = new Big.Box({ spacing: POPUP_GRID_SPACING, >- orientation: Big.BoxOrientation.HORIZONTAL }); >+ this._appsBox = new Shell.GenericContainer() >+ this._appsBox.spacing = POPUP_GRID_SPACING; we still have "GRID" spacing. wasn't that supposed to be renamed? Also, you should be able to do: this._appsBox = new Shell.GenericContainer({ spacing: POPUP_GRID_SPACING }); It would be good to have a comment somewhere in the code (either here or at the signal handlers) explaining why we're using GenericContainer. >+ let totalSpacing = this._appsBox.spacing * Math.abs(children.length - 1); That seems a little odd. Usually you'd expect the requested width to be 0 if there are no children. (_appsBoxAllocate has the same issue) >+ alloc.min_size = maxChildMin + 2; >+ alloc.nat_size = maxChildNat + 2; define a constant for that please >+ let childHeight = box.y2 - box.y1; that doesn't take into account the +2 above >+ let vSpacing = Math.abs(childHeight - childNat) / 2; again, abs() doesn't seem like the right thing here; that means that once the allocation gets too small, you'll start drawing the child lower and lower in the allocated slot, causing more and more of it to extend off the bottom
(In reply to comment #95) > we still have "GRID" spacing. wasn't that supposed to be renamed? I think I've attached the wrong patch... > Also, you should be able to do: > > this._appsBox = new Shell.GenericContainer({ spacing: POPUP_GRID_SPACING > }); It doesn't work, as "spacing" is not a property. > It would be good to have a comment somewhere in the code (either here or at the > signal handlers) explaining why we're using GenericContainer. Ok I'll do that. > >+ let totalSpacing = this._appsBox.spacing * Math.abs(children.length - 1); > > That seems a little odd. Usually you'd expect the requested width to be 0 if > there are no children. (_appsBoxAllocate has the same issue) I've removed all Math.abs... children.length is always >= 1 anyway as AltTab aborts when there is no app/window to show. > >+ alloc.min_size = maxChildMin + 2; > >+ alloc.nat_size = maxChildNat + 2; > > define a constant for that please Actually the +2 is a leftover of my debugging session. I'm removing it.
Created attachment 144551 [details] [review] [AppSwitcher] Use GenericContainer instead of BigBox. This allows defining some custom policy for size allocation. Currently, the minimum width is always used, but it can be tweaked afterwards when a sizing policy has been defined.
Created attachment 144553 [details] [review] [AppIcon] Consider workspace when sorting windows in menu. By sorting the windows in the current workspace first, we ensure that when using alt+tab to switch windows, we will pick a window on the active workspace as the default focused window for the application if the application has at least one window on the current workspace (that is, if it is on the left of the app switcher separator). This makes the behaviour of alt+tab more predictable for the user, as an user will expect alt+tab to switch to the window he/she can see right now rather than the one on the workspace he just left (presumably to do something else on the workspace he's currently on). PS. This obsoletes the previous patch "Select windows on current workspace by default." by taking another approach, which is IMO cleaner and less bug-prone (window order is always the same as what is displayed in the context menu).
Comment on attachment 144553 [details] [review] [AppIcon] Consider workspace when sorting windows in menu. This makes sense, but it means that AppIcon._redisplay() is now doing more work than it needs to be, because it was also splitting the windows into on-workspace and off-workspace, whereas now it can assume they're already sorted correctly, it just needs to notice where to put the separator in.
Attachment 144428 [details] pushed as 5f5266c - [AppSwitcher] Display a separator between apps on this workspace and others. Attachment 144551 [details] pushed as 68e8b14 - [AppSwitcher] Use GenericContainer instead of BigBox.
Created attachment 144590 [details] [review] [AppSwitcher] Update colors/border Switch from Big.Box to St.Bin and update styling per latest spec
Comment on attachment 144590 [details] [review] [AppSwitcher] Update colors/border Looks good (I don't know if we want to try and keep things roughly alphabetical in the CSS file - we might eventually want to split it up and use @import)
Comment on attachment 144590 [details] [review] [AppSwitcher] Update colors/border Attachment 144590 [details] pushed as e382da9 - [AppSwitcher] Update colors/border
The way this is done currently breaks—at least for me—the way I use my system. I often want to move between two windows in the same application (Firefox, Emacs). Before gnome-shell/mutter, I would do this by alt+tab-ing between them. Now, it seems I have to ALT+TAB, then ALT+SHIFT+TAB, then ALT+` (or ALT+TAB all the way around and then ALT+`). If Alt+` isn't going to be bound until the ALT+TAB window appears, than the ALT+TAB window should start on the current application, not the next most-recently-used application.
(In reply to comment #104) > If Alt+` isn't going to be bound until the ALT+TAB window appears, I believe the plan is for it to be bound always, it's just not implemented yet.
Created attachment 144607 [details] [review] [AppIcon] Consider workspace when sorting windows in menu. By sorting the windows in the current workspace first, we ensure that when using alt+tab to switch windows, we will pick a window on the active workspace as the default focused window for the application if the application has at least one window on the current workspace (that is, if it is on the left of the app switcher separator). This makes the behaviour of alt+tab more predictable for the user, as an user will expect alt+tab to switch to the window he/she can see right now rather than the one on the workspace he just left (presumably to do something else on the workspace he's currently on). This amendment reworks the menu layout code to take profit of the fact that windows are already ordered by "workspace commonness".
Created attachment 144744 [details] [review] Testing version of updated AppSwitcher. Don't review the code. This is just for trying out the new thumbnail-based window picker. It's not done yet. Mouse interaction is mostly not working, but keyboard interaction should be pretty much as described. More functionality and cleaner code to come later.
Created attachment 144745 [details] [review] Testing version of updated AppSwitcher. Don't review the code. The patch to sometimes pick the next window of the current app had a bug where it would *always* pick the next window of the current app, rather than only picking it if it had been used more recently than the first window of the next app. This fixes that. (git master is currently broken)
Hey Dan. Overall this is looking really good. After a quick try a few comments (probably all known to you already). * as mentioned mouse interaction isn't working in the window list * having a window selected without the window box having keyboard focus is a bit misleading. I guess that means that we may need to: - Make the window box have keyboard focus when it shows - Use the white box to indicate keyboard focus only - Have another way to associate the window box with the app icon. Either make the down arrow light up or have the window box have an arrow that points up. - Have keyboard focus go back to the app box when the user clicks the up arrow key * Downward pointing arrows are missing. * The way I imagined it anyway, was that if I click on an app with the mouse I'd go directly to it. Currently, I have to release alt+tab first. * On hover with the mouse we should show the window box (just like we have with the keyboard now) * (probably future) we should endeavor to smoothly transition the window box when we switch between apps * It is pretty tough to read text in the window thumbnail but I guess that isn't really fixable. Maybe we can try makeing the previews a bit bigger - especially since we're going to use larger app icons. The delay before showing the window box feels just about right to me.
(In reply to comment #109) > * Downward pointing arrows are missing. It implements what you said before: "If an app has multiple windows, when you tab to it for focus an arrow appears centered beneath the app title, pointing down." Would you rather have the arrows visible all the time? Oh, if you mean it's never visible then you need to rebuild; the arrow-drawing code is in C.
Ah OK. After rebuilding I see them now. Thanks.
(In reply to comment #108) > Created an attachment (id=144745) [details] > Testing version of updated AppSwitcher. Don't review the code. There is something disturbing visually which happens when you pause on an application using alt+tab: the window list appears with the first item highlighted, but if you use arrows, the white box that moves is the one in the app bar rather than the one in the window bar. IMHO, the box should have a darker border there. Also there is no reference to the window title anymore, which can make it quite hard to know which terminal window you are interested in, for instance. Maybe it could be mitigated by not removing the lightboxing effect that happened before (as windows appear in real size). There is also no indicator that a window is on another workspace (use a vertical separator there?) btw there is a sizing problem now when there is a separator in the app box: there is no space between the right side of the last icon and the right end of the window (missing padding)
(In reply to comment #108) Also, point-and-click navigation of the alt+tab dialog is broken by your patch.
Created attachment 144809 [details] [review] Updated squashed messy app switcher update mouse-based operation, focus indication, keynav should be improved
Created attachment 144881 [details] [review] [AppSwitcher] Use thumbnails instead of a window menu, and other UI changes So even though there's a bunch of code reused from the previous iteration of the appswitcher, it all gets moved around enough that the diff is pretty useless and it might be easier to just reread the patched file instead. Still not done: picking a smaller icon size when there are lots of apps, scrolling the thumbnails when there are lots of windows, outer #ffffff55 border on the overlays. However, please do not add any further requests to this bug; unless this patch should actually be rejected, please file all additional requests on top of it as new bugs, because this bug has gotten unwieldy.
Comment on attachment 144809 [details] [review] Updated squashed messy app switcher update also, the new patch depends on bug 597498
Review of attachment 144881 [details] [review]: Generally feels like a very accurate and attractive implementation of the design. Lots of small comments below about the code, but nothing big - it looks well structured. Bugs I'm seeing trying it out: - Throws an exception and gets stuck when Alt released too fast (discussed on IRC and noted below) - Application labels are sometimes fuzzy - Arrows are frequently fuzzy and sometimes mishappen - I didn't catch it in my other review but the misshapen problem is likely the floor in floor (width * 0.5) in shell-drawing.c. That makes the two sides of the arrow different lengths if the surface size is odd. I'm not actually seeing it here, but Jon was seeing it on his machine. Not sure wha the difference is. (Fuzzy problem is probably lack of rounding noted below) - When windows are of different heights, they are misaligned in the window switcher. Noted below but I don't know why. ::: js/ui/altTab.js @@ +17,2 @@ const POPUP_APPICON_BORDER_COLOR = new Clutter.Color(); POPUP_APPICON_BORDER_COLOR.from_pixel(0xffffffff); This is now the arrow color not the border color ? @@ +23,1 @@ const POPUP_APPS_BOX_SPACING = 8; Should APPS be dropped from this? Used for windows as well, right? @@ +84,3 @@ // https://bugzilla.gnome.org/show_bug.cgi?id=596695 for + // details.) So we check now. (Have to do this after updating + // selection.) As discussed on IRC, this seems to still be too early and throws because this._apps hasn't been set up yet, though it was surprising to me that it was even this late. @@ +91,3 @@ } + this._apps = this._appSwitcher.icons; Hmm, conceptually I'd like it better if this was this._appIcons a) it's confusing to have this be different than the local variable apps. b) I'm not crazy about using AppIcon as a model and accessing appIcon.windows. That's not immediately fixable, but calling this variable this.appIcons would be a step in the right direction. @@ +92,3 @@ + this._apps = this._appSwitcher.icons; + this._currentWindows = this._apps.map(function (app) { return 0; }); Needs a comment like '// all apps start out with window 0', because my reading was "why mapping apps to 0" and my assumption was that there was something wrong with the usage of map() not that it was intended. (would alternatively have been crystal clear if this._currentWindows was this._currentWindowsIndicies) @@ +98,3 @@ + if (!backward && this._apps[0].windows.length > 1) { + let curAppNextWindow = this._apps[0].windows[1]; + let nextAppWindow = this._apps[1].windows[0]; What if there is only one app? @@ +126,3 @@ + }, + _previousWindow : function() { + return (this._currentWindows[this._currentApp] - 1 + this._apps[this._currentApp].windows.length) % this._apps[this._currentApp].windows.length; The repetition of this pattern is hard to read, especally when the modulus is 'this._apps[this._currentApp].windows.length' - I wonder if we should just have misc.Utils.addWrapped(base, increment, total); [See also dash.js:_getIndexWrapped] (Also some argument for just having a separate utility function in each file where we need it ... if it's not *really* generic) @@ +141,3 @@ + else if (this._thumbnails) { + if (keysym == Clutter.Left || keysym == Clutter.a) + this._select(this._currentApp, this._previousWindow()); awsd - now there's a easter-egg nobody is going to find :-) From irc: <danw> oh, that was for debugging in xephyr, since the arrow keys don't work :). I think it's fine if it's left in, but a comment about why it was added would be useful if someone needs to remove it in the future. @@ +193,3 @@ + _mouseMoved : function(actor, event) { + if (++this._mouseMovement < POPUP_POINTER_SELECTION_THRESHOLD) + return; Any reason not to do this as a more conventional threshold based on distance moved? I'd worry about input devices that produce high-frequency jitter without moving much @@ +219,3 @@ + + if (this._thumbnails) + this._thumbnails.actor.destroy(); Looks to me like this._thumbnails is a child of this.actor, so this destroy() shouldn't be necessary. @@ +277,3 @@ + + let thumbnailCenter; + if (this._thumbnails.actor.width < this._appSwitcher.actor.width) { Hmm, this condition doesn't seem right to me. | screen width | A B C D E F G H apps 1 2 3 4 5 6 windows Note that actor.width is somewhat dubious when allocation is involved - it gives you the preferred width if the actor isn't allocated yet, the allocated width otherwise. I'm wonding if it would be better to replace altTab.actor with a ShellGenericContainer, that has no-op get_preferred_width/height methods, and a allocate() method that a) positions the app switcher and allocations it b) figures out where the thumbnails should go based on the allocated icon positions and the preferred size of the thumbnails container. Shouldn't be any more code. I don't really trust the mix of fixed and allocated positioning very much ... I don't think it's fully been thought through in clutter. @@ +283,3 @@ + // allocation yet, and the get_transformed_position() call + // will return 0,0. Calling clutter_actor_get_allocation_box() + // would force it to properly allocate itself, but we can't Actually, that won't work (or at least it's theoretically unsound) because get_allocation_box() only checks if the *actor* needs reallocation, but a parent might need reallocation when the actor doesn't. @@ +286,3 @@ + // call that because it's currently introspected wrong. So + // we use clutter_stage_get_actor_at_pos(), which will force + // a reallocation as a side effect. Ouch! @@ +379,3 @@ + + if (this._highlighted != -1) + this._items[this._highlighted].style_class = 'selected-item-box'; Not sure it helps here, but in cases like this I like toing 'item-box selected-item-box' so you don't have to duplicate CSS rules @@ +382,3 @@ + }, + + checkHover: function() { This function needs an explanatory comment about what it does (causes item-hovered to be emitted again) and why it exists (we ignore item-hovered signals until the user moves the mouse) @@ +452,3 @@ + for (let i = 0; i < this._items.length; i++) { + let [childMin, childNat] = this._items[i].get_preferred_height(forWidth); Passing in forWidth here doesn't make any sense - the individually items won't get that much width. I think you want to pass in _maxChildWidth() which is the width you are going to allocate the child at. @@ +482,1 @@ + let childWidth = Math.max(0, box.x2 - box.x1 - totalSpacing) / this._items.length; This needs to subtract out separatorWidth as well, right? @@ +489,2 @@ let [childMin, childNat] = children[i].get_preferred_height(childWidth); let vSpacing = (childHeight - childNat) / 2; This looks right to me, but experimentally this isn't working out - if children are of different height, they aren't properly centered vertically. @@ +505,3 @@ + x += this._list.spacing + separatorWidth; + } else { + // Something else; we don't allocate it Might be good to give an e.g. here - mention the arrows in the app switcher @@ +526,3 @@ + return new AppIcon.AppIcon({ appInfo: appInfo, + size: POPUP_APPICON_SIZE }); + }); Funky indentation @@ +530,1 @@ + // Contruct the AppIcons, sort by time, add to the popup 'Construct' - but they are already constructed above... @@ +532,3 @@ + let workspaceIcons = []; + let otherIcons = []; + for (let i = 0; i < icons.length; i++) { If you are going to loop here anyways I might just loop over apps, and skip the map @@ +559,3 @@ + + let arrowHeight = this.actor.get_theme_node().get_padding(St.Side.BOTTOM) / 3; + let arrowWidth = arrowHeight * 2; I think you need to pixel-round here @@ +565,3 @@ + for (let i = 0; i < this._items.length; i++) { + let itemBox = this._items[i].allocation; + childBox.x1 = itemBox.x1 + (itemBox.x2 - itemBox.x1 - arrowWidth) / 2; Definitely needs pixel-rounding @@ +592,3 @@ + POPUP_APPICON_BORDER_COLOR, + POPUP_APPICON_BORDER_COLOR); + })); Funky indentation
(In reply to comment #117) > - Arrows are frequently fuzzy and sometimes mishappen - I didn't catch it in > my other review but the misshapen problem is likely the floor in floor (width * > 0.5) in shell-drawing.c. That makes the two sides of the arrow different > lengths if the surface size is odd. Ah, I'd seen the misshapenness but not understood it. So we want to avoid both misshapenness *and* fuzziness? Meaning, I guess, that if the width is even, we have to subtract 1 from it to make it odd? (You said the other way above, but we want a 1-pixel-wide point, not 2-pixel-wide, so the total width has to be odd.) > - When windows are of different heights, they are misaligned in the window > switcher. Noted below but I don't know why. It seems that if you allocate the clone more height than it wants, then it bottom-aligns itself, so we just have to adjust its allocation. > @@ +98,3 @@ > + if (!backward && this._apps[0].windows.length > 1) { > + let curAppNextWindow = this._apps[0].windows[1]; > + let nextAppWindow = this._apps[1].windows[0]; > > What if there is only one app? Then we would have already bailed out; the switcher only comes up when there are at least two apps to switch between. Although now with the mixed app/window switching behavior that's probably wrong I guess. So should it still come up even if there's only one app with only one window? > + _previousWindow : function() { > + return (this._currentWindows[this._currentApp] - 1 + > this._apps[this._currentApp].windows.length) % > this._apps[this._currentApp].windows.length; > > The repetition of this pattern is hard to read yeah, that's why I broke it out into helper functions... > I wonder if we should just have > misc.Utils.addWrapped(base, increment, total) or just a Math.mod that does the right thing with negative numbers > + if (this._thumbnails.actor.width < this._appSwitcher.actor.width) { > > Hmm, this condition doesn't seem right to me. > > | screen width | > A B C D E F G H apps > 1 2 3 4 5 6 windows Yeah... but this looks bad too: | A B C D E | v 1 2 And of course, the window list could be so wide that it would go off the screen regardless of where it was centered. needs-work > @@ +283,3 @@ > + // allocation yet, and the get_transformed_position() call > + // will return 0,0. Calling clutter_actor_get_allocation_box() > + // would force it to properly allocate itself, but we can't > > Actually, that won't work (or at least it's theoretically unsound) because > get_allocation_box() only checks if the *actor* needs reallocation, but a > parent might need reallocation when the actor doesn't. The switcher never gets reallocated after its first allocation, so the two cases are equivalent here. > @@ +532,3 @@ > + let workspaceIcons = []; > + let otherIcons = []; > + for (let i = 0; i < icons.length; i++) { > > If you are going to loop here anyways I might just loop over apps, and skip the > map hm... yeah, i think the map made more sense in some intermediate stage of the work
(In reply to comment #118) > (In reply to comment #117) > > - Arrows are frequently fuzzy and sometimes mishappen - I didn't catch it in > > my other review but the misshapen problem is likely the floor in floor (width * > > 0.5) in shell-drawing.c. That makes the two sides of the arrow different > > lengths if the surface size is odd. > > Ah, I'd seen the misshapenness but not understood it. So we want to avoid both > misshapenness *and* fuzziness? Meaning, I guess, that if the width is even, we > have to subtract 1 from it to make it odd? (You said the other way above, but > we want a 1-pixel-wide point, not 2-pixel-wide, so the total width has to be > odd.) Well, three separate things: - Asymetrical arrows always will look bad and worse than either odd or even width arrows - Yes, I think odd width/height arrows likely look better because they have sharper points. Subtracting 1 to enforce oddness is a good idea. - Once we create the texture, we have to be sure to position it at an integer pixel position or we'll fuzz out what we drew > > - When windows are of different heights, they are misaligned in the window > > switcher. Noted below but I don't know why. > > It seems that if you allocate the clone more height than it wants, then it > bottom-aligns itself, so we just have to adjust its allocation. Still not quite catching on - it looked like the buttonbox was allocated its preferred height, and its preferred height should be the set height of the clone plus padding? But if you understand it, good enough. > > @@ +98,3 @@ > > + if (!backward && this._apps[0].windows.length > 1) { > > + let curAppNextWindow = this._apps[0].windows[1]; > > + let nextAppWindow = this._apps[1].windows[0]; > > > > What if there is only one app? > > Then we would have already bailed out; the switcher only comes up when there > are at least two apps to switch between. > > Although now with the mixed app/window switching behavior that's probably wrong I guess. Yeah. > So should it still come up even if there's only one app with only one window? No strong opinion. Might give the user better feedback about why nothing is happening. > > + if (this._thumbnails.actor.width < this._appSwitcher.actor.width) { > > > > Hmm, this condition doesn't seem right to me. > > > > | screen width | > > A B C D E F G H apps > > 1 2 3 4 5 6 windows > > Yeah... but this looks bad too: > > | A B C D E | > v > 1 2 > > And of course, the window list could be so wide that it would go off the screen > regardless of where it was centered. What I'd expect it centered, then pushed in. (Ignoring the too-wide-for-screen case.) > needs-work > > > @@ +283,3 @@ > > + // allocation yet, and the get_transformed_position() call > > + // will return 0,0. Calling clutter_actor_get_allocation_box() > > + // would force it to properly allocate itself, but we can't > > > > Actually, that won't work (or at least it's theoretically unsound) because > > get_allocation_box() only checks if the *actor* needs reallocation, but a > > parent might need reallocation when the actor doesn't. > > The switcher never gets reallocated after its first allocation, so the two > cases are equivalent here. yeah, just commenting that it might work here, but it's coincidental workingess, it's not really reliable either.
Created attachment 144962 [details] [review] Fixes for comments
(In reply to comment #117) > What if there is only one app? OK, now it implements one app the same as multiple apps. This involved changing the logic for when to automatically pop down the thumbnail list; there's a check to make sure it doesn't restart the thumbnail pop-down timer when the user does Up Arrow from the thumbnail list to close it; previously the code assumed that whenever we were re-selecting the same app, it was because of that case, but now if you type Alt-Tab when there's only one app, then it will reselect the same app because there isn't any other app to select. But it felt wrong to not start the timer in that case, so I had to split up those two cases. > + _mouseMoved : function(actor, event) { > + if (++this._mouseMovement < POPUP_POINTER_SELECTION_THRESHOLD) > + return; > > Any reason not to do this as a more conventional threshold based on distance > moved? I'd worry about input devices that produce high-frequency jitter without > moving much Well, that makes sense with DND where the goal is to move the object away from where it was originally anyway, but in this case if the user just jiggles the mouse a little bit and ends up right back where he started, we want that to count. OS X only requires a single pixel movement to trigger the mouse in this case, so... > I don't really trust the mix of fixed and allocated positioning very much ... I > don't think it's fully been thought through in clutter. I didn't fix any of this (or the thumbnails-going-offscreen) now. > Passing in forWidth here doesn't make any sense - the individually items won't > get that much width. I think you want to pass in _maxChildWidth() which is the > width you are going to allocate the child at. I changed it to -1, since none of our allocation actually makes use of height-for-width. > This looks right to me, but experimentally this isn't working out - if children > are of different height, they aren't properly centered vertically. It was setting y1 correctly but setting y2 to y1+maximum-thumbnail-height rather than y1+THIS-thumbnail-height, and then the thumbnail was drawing itself at the bottom of its allocation. > + let arrowHeight = > this.actor.get_theme_node().get_padding(St.Side.BOTTOM) / 3; > + let arrowWidth = arrowHeight * 2; > > I think you need to pixel-round here After much playing around with various possibilities, I ended up just pixel-aligning the arrow and using a transparent stroke, and it looks fine now. (Albeit with a 2-pixel-wide point. I was never able to get a symmetric arrow with a 1-pixel point.)
Review of attachment 144962 [details] [review]: Fixes look good to me. ::: js/ui/altTab.js @@ +32,3 @@ const HOVER_TIME = 500; // milliseconds +function mod(a, b) { Probably could use a brief comment like // wrap for a + b == -1 for those not familiar with the idiom.
Attachment 144881 [details] pushed as d7af6d4 - [AppSwitcher] Use thumbnails instead of a window menu, and other UI changes This bug is now FIXED. If additional changes/fixes/whatevers to the app switcher are needed, please file new bugs. Do not add comments here.
Great work Dan! Thanks for pushing hard on this.