GNOME Bugzilla – Bug 688913
Proposal to move the alternate-tab extension into core
Last modified: 2014-04-26 19:02:18 UTC
It's no big secret that there are a lot of complaints about the application switcher, so it might just make sense to offer a bit more flexibility in core itself. I actually came up with this approach quite a while ago, but didn't get myself to write the actual code (until now) - rather than adding a hidden option to swap out the existing switcher, a new 'switch-application' keybinding is added, which defaults to <alt>tab and triggers the existing switcher. The existing 'switch-windows' keybinding is then free to be used for a more traditional window switcher, which users can activate by setting up a keyboard shortcut in Settings - with that, users can even choose to use both switchers (e.g. using <alt>tab and <super>tab). (selfish as I am, I hope that this is a better solution for people that depend on the current workspace special-casing in the app-switcher and we can land bug 661156 without too much hurt)
Created attachment 229689 [details] [review] schemas: Add new 'switch-applications' keybinding GNOME Shell currently uses the 'switch-windows' keybinding for its application switcher, although it does not switch between windows but applications (duh!). Add a new dedicated keybinding instead, which will free 'switch-windows' to be used for window switching again. People unhappy with the Shell's alt-tab popup may then use it instead of (or in addition to!) the application-based switcher.
Created attachment 229690 [details] [review] Add 'switch-applications' keybinding Add an additional "switcher" keybinding for switching between applications rather than windows (like the existing 'switch-windows' and 'switch-group' bindings). The current implementation is only a stub, which is enough to be taken over by gnome-shell for its application-based alt-tab popup - if anyone feels the urge to actually implement an application switcher in mutter, be my guest ... :-)
Created attachment 229692 [details] [review] altTab: Use 'switch-applications' for app switcher The Shell's alt-tab popup is application-based, so using the 'switch-windows' keybinding for it never really made sense. Use the newly added 'switch-applications' keybinding instead.
Created attachment 229693 [details] [review] altTab: Minor code cleanup
Created attachment 229694 [details] [review] altTab: Use more generic names in AltTabPopup The alt-tab popup is currently strictly application-based, which is reflected in its property/method names - first level items are "apps", second level items "windows". However we are going to reintroduce a window-based switcher with windows as first level items, so to avoid the oddity of referring to those windows as "apps", use the more generic terms "item" and "subitem".
Created attachment 229695 [details] [review] altTab: Re-implement 'switch-windows' keybinding Now that we use the new 'switch-applications' keybinding for the application-based alt-tab popup, we can use the 'switch-windows' keybinding for a more traditional switcher. Based heavily on the alternate-tab extension from Giovanni Campagna.
In case anyone is wondering, first patch applies to gsettings-desktop-schemas, the second one to mutter, everything else is gnome-shell.
Review of attachment 229689 [details] [review]: How will this help / hurt migration? Those who assigned 'switch-windows' to a custom key binding may get screwed, right?
Review of attachment 229690 [details] [review]: WTF count is now 1. No. ::: src/core/display.c @@ +4657,3 @@ } #define IN_TAB_CHAIN(w,t) (((t) == META_TAB_LIST_NORMAL && META_WINDOW_IN_NORMAL_TAB_CHAIN (w)) \ This macro needs to be shot in the head and replaced with a function with a switch statement. ::: src/core/window-private.h @@ +609,3 @@ (((w)->input || (w)->take_focus ) && META_WINDOW_IN_NORMAL_TAB_CHAIN_TYPE (w) && (!(w)->skip_taskbar)) +#define META_WINDOW_IN_APPLICATION_TAB_CHAIN(w) \ + FALSE /* not implemented */ OK, WTF
Review of attachment 229692 [details] [review]: Sure.
Review of attachment 229693 [details] [review]: How did this ever originally pass review?
Review of attachment 229694 [details] [review]: ::: js/ui/altTab.js @@ +193,1 @@ + this._icons = this._switcher.icons; Not a fan of the name "icons". Given how we have this._currentItem, might we have this._items that we're switching between?
Review of attachment 229695 [details] [review]: ::: js/ui/altTab.js @@ +37,3 @@ +}; + + Extra whitespace. @@ +77,3 @@ this._initialDelayTimeoutId = 0; + this._applicationBased = true; eek @@ +179,3 @@ + let settings = new Gio.Settings({ schema: 'org.gnome.shell.window-switcher' }); + if (!settings.get_boolean('current-workspace-only')) { + // This is roughly what meta_display_get_tab_list does, except Implement this in mutter, not in here. Notes below are extras. @@ +182,3 @@ + // that it doesn't filter on workspace + // See in particular src/core/window-private.h for the filters + windows = global.get_window_actors().map(function(actor) { Not a fan of using the window actor list instead of the list of non-transient windows that mutter already keeps @@ +187,3 @@ + return !win.is_override_redirect() && + win.get_window_type() != Meta.WindowType.DESKTOP && + win.get_window_type() != Meta.WindowType.DOCK; Skip taskbar hint doesn't seem to be handled? @@ +197,3 @@ + + // Filter away attached modal dialogs (switch to their parents instead) + return windows.filter(function(win) { return !win.is_attached_dialog(); }); Doesn't get_tab_list do this automatically? Shouldn't this only be the case for the get_window_actors case? @@ +202,2 @@ show : function(backward, binding, mask) { + this._applicationBased = (binding != 'switch-windows' && Given that the way this works, we create a new AltTabPopup, and then immediately call show with some arguments, it might make sense to move those some arguments into the constructor or something similar, rather than having the constructor fill in a bunch of bogus values. @@ +370,3 @@ else this._select(this._currentItem, this._previousSubitem()); + } else if (this._applicationBased) { I'd prefer this as: } else { if (this._applicationBased) { // ... } else { // ... } } @@ +385,3 @@ else this._select(this._currentItem, this._nextSubitem()); + } else if (this._applicationBased) { ditto @@ +1206,3 @@ + let settings = new Gio.Settings({ schema: 'org.gnome.shell.window-switcher' }); + switch (settings.get_enum('app-icon-mode')) { + case AppIconMode.THUMBNAIL_ONLY: I'm not sure this is any of our current styles for switch statements. Mind looking around and seeing exactly how inconsistent we are? @@ +1214,3 @@ + x_align: Clutter.ActorAlign.CENTER, + y_align: Clutter.ActorAlign.CENTER, + // usual hack for the usual bug in ClutterBinLayout... this is like an inside joke at this point @@ +1223,3 @@ + scale = Math.min(1.0, 128 / width, 128 / height); + size = 128; + this.clone = new Clutter.Clone({ source: windowTexture, refactor out into createClone() method or use flags or group switch statements or something @@ +1246,3 @@ + if (this.app) { + this.appIcon = this.app.create_icon_texture(size); + this.appIcon.x_expand = this.appIcon.y_expand = true; lol
I don't have a problem with this in general, but can we please review the design before it lands? I've tried the alternative alt-tab extension. The experience isn't good enough for the core.
(In reply to comment #14) > I don't have a problem with this in general, but can we please review the > design before it lands? I've tried the alternative alt-tab extension. The > experience isn't good enough for the core. I agree. Instead of popping up asking you to choose a mode, this provides: * A dedicated "switch windows" keybinding which switches windows, not applications * A GSettings key to determine what to display in the switcher: app icons, window thumbnails, or both. * A GSettings key about whether to care about windows on other workspaces or not.
(In reply to comment #15) > I agree. Instead of popping up asking you to choose a mode [...] To be fair, I tried the extension before copy+pasting it, and it doesn't do that anymore. Nowadays it's pretty much a partly copy of AltTabPopup + the hunks > 3-4 lines in the last patch.
(In reply to comment #14) > I don't have a problem with this in general, but can we please review the > design before it lands? Oh, and of course we can do that - if we don't include it in core like this, it'll be part of the fallback-extensions set, so one way or another it'll need design review anyway :-)
So, what is this waiting for, design review ?
Comment on attachment 229693 [details] [review] altTab: Minor code cleanup Attachment 229693 [details] pushed as cb08bd2 - altTab: Minor code cleanup
Created attachment 230550 [details] [review] display: Make workspace parameter to get_tab_list() optional Currently meta_display_get_tab_list() will only return windows on a single workspace. Make the workspace parameter optional to allow requesting windows from all workspaces.
Created attachment 230552 [details] [review] altTab: Use 'switch-applications' for app switcher Rebased onto bug 689528
Created attachment 230553 [details] [review] altTab: Rename AltTabPopup to AppSwitcherPopup We will add support for a window-based popup in addition to the current application-based one, so use a more descriptive name.
Created attachment 230554 [details] [review] altTab: Re-implement 'switch-windows' keybinding (In reply to comment #13) > Doesn't get_tab_list do this automatically? Shouldn't this only be the case for > the get_window_actors case? No, because the extension uses TabList.NORMAL_ALL instead of TabList.NORMAL - no idea why, the latter should be just fine. > @@ +202,2 @@ > show : function(backward, binding, mask) { > + this._applicationBased = (binding != 'switch-windows' && > > Given that the way this works, we create a new AltTabPopup, and then > immediately call show with some arguments, it might make sense to move those > some arguments into the constructor or something similar, rather than having > the constructor fill in a bunch of bogus values. Actually, with Rui's work in bug 689528 it is easier to just use separate classes altogether ... > I'm not sure this is any of our current styles for switch statements. Mind > looking around and seeing exactly how inconsistent we are? Heh, the first patch version was just the extension code copy-pasted into altTab.js - at least I removed the most blatant cases of tabs/spaces mixups ;-)
Review of attachment 230550 [details] [review]: Eek. We moved away from MRU for other things, but I sort of left it in the alt-tab code out of laziness more than anything. Bonus points if you make it use the workspace stacking order rather than MRU. ::: src/core/display.c @@ +4802,3 @@ + GSList *w; + + // Yay for mixing GList and GSList in the API We should probably fix that.
(In reply to comment #9) > Review of attachment 229690 [details] [review]: > > WTF count is now 1. No. So before I'm going to address this - am I understanding you correctly that you want the new keybinding added *without* a mutter patch? Or are we using different definitions of "rejected"?
(In reply to comment #8) > How will this help / hurt migration? Those who assigned 'switch-windows' to a > custom key binding may get screwed, right? Yes. The only way I see around this is leaving 'switch-windows' as-is (switching windows in metacity/mutter and applications in gnome-shell) and add a new keybinding for the window-based switcher in gnome-shell ('switch-windows-no-really-i-mean-it-this-time' - ugh)
(In reply to comment #24) > Eek. We moved away from MRU for other things, but I sort of left it in the > alt-tab code out of laziness more than anything. Bonus points if you make it > use the workspace stacking order rather than MRU. Sure, but not really related to either this patch or this bug report. Not a priority I'd say :-)
(In reply to comment #25) > (In reply to comment #9) > > Review of attachment 229690 [details] [review] [details]: > > > > WTF count is now 1. No. > > So before I'm going to address this - am I understanding you correctly that you > want the new keybinding added *without* a mutter patch? Or are we using > different definitions of "rejected"? I mean "this patch is so bad I am not allowing it in this form". Maybe we should have some common definitions of what the review statuses are supposed to mean, because I just use a sliding scale based on quality: * ================ * ================ * ================ * rejected needs_work reviewed accepted-commit_now
(In reply to comment #28) > I mean "this patch is so bad I am not allowing it in this form". Ah, so we *are* using different definitions - for me: "not in this form" == "needs-work" "not in any form" == "rejected"
(In reply to comment #29) > (In reply to comment #28) > > I mean "this patch is so bad I am not allowing it in this form". > > Ah, so we *are* using different definitions - for me: > > "not in this form" == "needs-work" > "not in any form" == "rejected" Yeah that has been my understanding as well.
(In reply to comment #27) > (In reply to comment #24) > > Eek. We moved away from MRU for other things, but I sort of left it in the > > alt-tab code out of laziness more than anything. Bonus points if you make it > > use the workspace stacking order rather than MRU. > > Sure, but not really related to either this patch or this bug report. Not a > priority I'd say :-) Right, but you're sort of making it worse, since you're mixing the two and doing a cmp on the user time, which we don't want to use.
(In reply to comment #31) > Right, but you're sort of making it worse, since you're mixing the two and > doing a cmp on the user time, which we don't want to use. Uhm - it's kinda hard to avoid? Or did we change the stack order to be independent from workspaces at some point?
Yep. It swung back again to be not based on MRU lists. http://git.gnome.org/browse/mutter/commit/?id=a3bf9b01aa7019798924b618160fcb184e096a3c
(In reply to comment #23) > Created an attachment (id=230554) [details] [review] > altTab: Re-implement 'switch-windows' keybinding > > (In reply to comment #13) > > Doesn't get_tab_list do this automatically? Shouldn't this only be the case for > > the get_window_actors case? > > No, because the extension uses TabList.NORMAL_ALL instead of TabList.NORMAL - > no idea why, the latter should be just fine. Just checked. The regular alt-tab code does too. Was an bugfix because we didn't take the MRU of modal dialogs into account: see bug #667552 . If you can come up with a better solution, please do.
(In reply to comment #33) > Yep. It swung back again to be not based on MRU lists. > > http://git.gnome.org/browse/mutter/commit/?id=a3bf9b01aa7019798924b618160fcb184e096a3c What I mean is: just getting the list of windows from the stack will group them by workspace, which is not what we want in the popup, so I don't see how we can avoid sorting by user time.
(I'm only referring to the case of listing windows from all workspaces of course, the other case is just fine)
Hm. The server does keep a global stacking order, but I'm not sure that's what we want. Yeah, maybe sorting by global MRU is the lesser of all evils.
Review of attachment 230550 [details] [review]: ::: src/core/display.c @@ +4806,3 @@ + mru_list = g_list_prepend (mru_list, w->data); + mru_list = g_list_sort (mru_list, mru_cmp); + } else mru_list = workspace->mru_list; This lets us skip the statements below. @@ +4848,3 @@ + if (workspace == NULL) + return tab_list; Leaks mru_list.
Review of attachment 230553 [details] [review]: Sure.
Review of attachment 230552 [details] [review]: I think the regression issue is kind-of a big deal, but I'd prefer to get someone else's input
Review of attachment 230554 [details] [review]: ::: js/ui/altTab.js @@ +33,3 @@ +}; + + extra whitespace @@ +36,1 @@ const AppSwitcherPopup = new Lang.Class({ Regular app switcher needs to use WindowIcon, otherwise the app-icon-mode won't have any effect. @@ +712,3 @@ + vertical: true }); + this.icon = null; + this._iconBin = new St.Widget({ layout_manager: new Clutter.BinLayout() }); Not a bin. ::: js/ui/windowManager.js @@ +637,3 @@ + this._workspaceSwitcherPopup.destroy(); + + let tabPopup = new AltTab.WindowSwitcherPopup(); I wonder what happens if both popups happen at once. That should probably be handled globally, as I can imagine it can happen for CtrlAltTab as well, and for the new input method switcher. We should probably have a Main.currentSwitcher or something.
(In reply to comment #41) > @@ +36,1 @@ > const AppSwitcherPopup = new Lang.Class({ > > Regular app switcher needs to use WindowIcon, otherwise the app-icon-mode won't > have any effect. How does app-icon-mode make sense for the application-based switcher?
Sorry, really out of it today. I started off by thinking that the code that creates the window clones and app icons should be shared, and then decided to up it to "share WindowIcon", even though that makes no sense.
Created attachment 230596 [details] [review] display: Make workspace parameter to get_tab_list() optional Fix leaks (both one noted in the review + the GSList that went unnoticed), switch to a more concise style
Review of attachment 230596 [details] [review]: The style changes make this impossible to review sanely. Keep those separate for now until we get a "-w" mode in Splinter, I guess.
Created attachment 230600 [details] [review] display: Make workspace parameter to get_tab_list() optional Reattaching with -w
Review of attachment 230600 [details] [review]: ::: src/core/display.c @@ +4833,2 @@ { + l_window=w->data; Obviously needs to be MetaWindow *l_window ... here - sorry, it's getting late here
I guess I'd still prefer a style fixup patch afterwards.
Created attachment 230601 [details] [review] display: Clean up meta_display_get_tab_list()
Created attachment 230602 [details] [review] display: Make workspace parameter to get_tab_list() optional
Review of attachment 230601 [details] [review]: Of course.
Review of attachment 230602 [details] [review]: Works for me. ::: src/core/display.c @@ +4799,3 @@ + else + { + mru_list = g_list_copy (workspace->mru_list); Hoping to prevent the copy, but I guess it's not that big of a deal. You can do it somewhat cleanly like: GSList *free_mru_list = NULL; if (workspace == NULL) { free_mru_list = mru_list; } /* ... */ g_slist_free (free_mru_list); but with better names. @@ +4830,3 @@ + * other workspaces that demand attention + */ + for (w = windows; workspace && w; w = w->next) This is kind of sneaky. I'd prefer an explicit if (workspace) { }
Created attachment 230603 [details] mockup I've taken another look at the alternative alt-tab extension. It's generally OK, although there are two things that I think should be improved: 1. Titles are displayed below every window, and they frequently get ellipsised. GNOME 2 solved this problem the same way that Windows did (and still does) - it only shows the title of the focused window. I'd recommend doing the same here. 2. The application icons are way too big, and the positioning looks a bit off to me. 48x48px would be a more appropriate size. A mockup is attached.
Created attachment 230610 [details] Screenshot of updated switcher First shot of updating the switcher - there's a bit more padding above/below the label, OK anyway?
Looks good to me. We might want to introduce more horizontal padding between the window thumbnails, too.
Created attachment 230635 [details] [review] display: Make workspace parameter to get_tab_list() optional Updated according to review
Created attachment 230636 [details] [review] altTab: Re-implement 'switch-windows' keybinding Updated according to mockup and review
Created attachment 230637 [details] [review] display: Make workspace parameter to get_tab_list() optional Fix typo
Review of attachment 230637 [details] [review]: Getting closer. ::: src/core/display.c @@ +4841,3 @@ + } + else + g_list_free (mru_list); Yeah, I was hoping to do it without depending on the state of the "workspace" variable. I don't like it being in the same if statement. It's unrelated.
Review of attachment 230636 [details] [review]: ::: js/ui/altTab.js @@ +390,3 @@ + + this._switcherList = new WindowList(windows); + this._items = this._switcherList.icons; "items = icons;" seems wrong. @@ +709,3 @@ + this.window = window; + + this.actor = new St.BoxLayout({ style_class: 'alt-tab-app', 'alt-tab-item' @@ +714,3 @@ + + this.actor.add(this._icon, { x_fill: false, y_fill: false } ); + this.label = new St.Label({ text: window.get_title() }); Unused? @@ +748,3 @@ + }, + + _createWindowClone: function(window) { Are you going to make these used for the main switcher as well, or? We don't have to set_size in here. @@ +813,3 @@ + childBox.y2 = box.y2; + childBox.y1 = childBox.y2 - this._label.height; + this._label.allocate(childBox, flags); We're just poorly reimplementing a BoxLayout here at this point, aren't we. ::: js/ui/windowManager.js @@ +633,3 @@ + _startWindowSwitcher : function(display, screen, window, binding) { + /* prevent a corner case where both popups show up at once */ As said in the last review, I'd like this to be handled centrally.
Created attachment 230664 [details] [review] display: Make workspace parameter to get_tab_list() optional Currently meta_display_get_tab_list() will only return windows on a single workspace. Make the workspace parameter optional to allow requesting windows from all workspaces.
(In reply to comment #59) > I don't like it being in the same if statement. It's unrelated. No, it's not - mru_list's memory is owned by the workspace if specified or owned by ourselves if not. I can only see two ways around this, one is to always allocate memory for mru_list (previous patch version), or to use different variables, which makes everything more verbose ...
(In reply to comment #60) > + this.actor.add(this._icon, { x_fill: false, y_fill: false } ); > + this.label = new St.Label({ text: window.get_title() }); > > Unused? No. > + > + _createWindowClone: function(window) { > > Are you going to make these used for the main switcher as well, or? > > We don't have to set_size in here. Sigh. This is getting extremely unrelated to the scope of this bug. > + _startWindowSwitcher : function(display, screen, window, binding) { > + /* prevent a corner case where both popups show up at once */ > > As said in the last review, I'd like this to be handled centrally. Is this actually more than a gut feeling? All the switcher popups are KeybindingMode.NONE, which means that once they are up, *all* keybindings (including any "competing" switchers) are blocked.
(In reply to comment #60) > + this._switcherList = new WindowList(windows); > + this._items = this._switcherList.icons; > > "items = icons;" seems wrong. Oh, and just for the record: you considered the exact same line "a-c-n" in bug 689528 ;-)
(In reply to comment #63) > (In reply to comment #60) > > + this.actor.add(this._icon, { x_fill: false, y_fill: false } ); > > + this.label = new St.Label({ text: window.get_title() }); > > > > Unused? > > No. Ah, it's used as the label_actor. Didn't see that. A rename to "a11yLabel" might be worthwhile. > > + > > + _createWindowClone: function(window) { > > > > Are you going to make these used for the main switcher as well, or? > > > > We don't have to set_size in here. > > Sigh. This is getting extremely unrelated to the scope of this bug. We have time to do it right. I'd rather not have the same code duplicated throughout, when we can easily stuff it into a shared function. > Is this actually more than a gut feeling? All the switcher popups are > KeybindingMode.NONE, which means that once they are up, *all* keybindings > (including any "competing" switchers) are blocked. I keep forgetting that KeybindingMode.NONE means "all keybindings are blocked", I guess.
(In reply to comment #65) > Ah, it's used as the label_actor. Didn't see that. A rename to "a11yLabel" > might be worthwhile. It's used both as label_actor and to get the text of the currently selected window.
Created attachment 230668 [details] [review] altTab: Factor out thumbnail creation
Created attachment 230669 [details] [review] altTab: Re-implement 'switch-windows' keybinding (In reply to comment #60) > @@ +709,3 @@ > + this.actor = new St.BoxLayout({ style_class: 'alt-tab-app', > > 'alt-tab-item' We don't actually use that style class at the moment, but if we did, I guess we'd want it to be shared with the app switcher, so I'll leave it as-is. > + childBox.y2 = box.y2; > + childBox.y1 = childBox.y2 - this._label.height; > + this._label.allocate(childBox, flags); > > We're just poorly reimplementing a BoxLayout here at this point, aren't we. Not really. In this case, the actor contains four children, three of them aligned horizontally at the top and one stretching all the bottom. Or in other words, if the parent class used a BoxLayout here, it would be horizontal and we'd need to jump through more hoops to implement the behavior in the mockup. > ::: js/ui/windowManager.js > + _startWindowSwitcher : function(display, screen, window, binding) { > + /* prevent a corner case where both popups show up at once */ > > As said in the last review, I'd like this to be handled centrally. As mentioned, the switcher popups block all other keybindings, so I'm leaving this as-is for now.
Review of attachment 230668 [details] [review]: What about app icons? Not worth it?
Review of attachment 230669 [details] [review]: Minor whitespace fixes, otherwise fine. ::: js/ui/altTab.js @@ +735,3 @@ + case AppIconMode.THUMBNAIL_ONLY: + size = WINDOW_PREVIEW_SIZE; + this._icon.add_actor(_createWindowClone(mutterWindow, Shouldn't need wrapping now. @@ +741,3 @@ + case AppIconMode.BOTH: + size = WINDOW_PREVIEW_SIZE; + this._icon.add_actor(_createWindowClone(mutterWindow, ditto
Review of attachment 230664 [details] [review]: I'd prefer the old patch.
The app switcher uses a plain app.create_icon_texture(), so I'd say no.
(In reply to comment #71) > I'd prefer the old patch. Which one? This is the 7th iteration.
(In reply to comment #73) > (In reply to comment #71) > > I'd prefer the old patch. > > Which one? This is the 7th iteration. The one that does the: if (workspace) ... else g_list_free (mru_list); I'm not the biggest fan, and I think the free_mru_list strategy would work really well, but you seem reluctant.
(In reply to comment #74) > I'm not the biggest fan, and I think the free_mru_list strategy would work > really well, but you seem reluctant. Uh? In what way does this differ from the current patch (except that I used a variable name of "mru_list" instead of "free_mru_list")?
In the current patch, you do the "mru_list ? mru_list : workspace->mru_list" inline. I'm imagining that mru_list is set to an MRU list to traverse, and free_mru_list is set to a list to free. In the workspace == NULL case, mru_list and free_mru_list point to the newly built list, and in the otherwise case, mru_list is set to the workspace MRU list, but free_mru_list is set to NULL.
Created attachment 230675 [details] [review] display: Make workspace parameter to get_tab_list() optional Fine, here's your bikeshed!
Review of attachment 230675 [details] [review]: Yay.
Review of attachment 230668 [details] [review]: OK.
Some quick feedback from trying this: when I have a single window, the one switcher pops up and shows me that one window, he other one just doesn't do anything, which makes it feel like it is broken.
Created attachment 230678 [details] [review] altTab: Re-implement 'switch-windows' keybinding Interesting. This patch should fix the inconsistency.
Review of attachment 230678 [details] [review]: Two minor fixes, otherwise looks good. ::: js/ui/altTab.js @@ +3,2 @@ const Clutter = imports.gi.Clutter; +const Gdk = imports.gi.Gdk; Unused import. @@ +4,3 @@ +const Gdk = imports.gi.Gdk; +const Gio = imports.gi.Gio; +const Gtk = imports.gi.Gtk; Unused import.
Created attachment 230685 [details] [review] Add 'switch-applications' keybinding Make the new keybinding an alias of 'switch-windows' instead of a stub.
Review of attachment 230685 [details] [review]: Sure.
Created attachment 230687 [details] [review] Add 'switch-windows-real' keybinding GNOME Shell uses the existing 'switch-windows' keybindings to switch between applications; in order to introduce a more traditional window switcher as well, add an additional "switcher" keybinding. As Mutter always switches between windows anyway, make it an alias to the normal switcher when run standalone. This is a slight variation of the "Add 'switch-applications' keybinding" patch; it leaves the 'switch-windows' keybindings alone and adds an additional one for the new window switcher, which means we won't interfere with existing settings (at the cost of some confusion when using dconf-editor/gsettings). If we go that route, the gsettings-desktop-schemas and "Use 'switch-applications' for app switcher" patches are obsolete and the window-switcher one needs some trivial adjustments.
Given that repurposing the switch-windows binding to do what it says would only affect people who changed the default, I'd say: - Change the summary for switch-windows to match its name - Add a switch-apps keybinding - Switch the default between the 2 keys - Repeat above steps for the backwards version - Change the convert file to move the conversion from switch-windows to switch-apps with a comment - Mention it in the release notes
Created attachment 230778 [details] [review] schemas: Add new 'switch-applications' keybinding Also update the convert files as requested by hadess on IRC
Review of attachment 230778 [details] [review]: Rest looks good. ::: schemas/wm-schemas.convert @@ +44,3 @@ switch-group = /apps/metacity/global_keybindings/switch_group switch-group-backward = /apps/metacity/global_keybindings/switch_group_backward +switch-applications = /apps/metacity/global_keybindings/switch_windows Missing comment here.
Attachment 230601 [details] pushed as 3797eca - display: Clean up meta_display_get_tab_list() Attachment 230675 [details] pushed as 8703dac - display: Make workspace parameter to get_tab_list() optional
Comment on attachment 230778 [details] [review] schemas: Add new 'switch-applications' keybinding Attachment 230778 [details] pushed as 0f7a29d - schemas: Add new 'switch-applications' keybinding
Comment on attachment 230685 [details] [review] Add 'switch-applications' keybinding Attachment 230685 [details] pushed as 2282326 - Add 'switch-applications' keybinding
Attachment 230552 [details] pushed as 2fb1d70 - altTab: Use 'switch-applications' for app switcher Attachment 230553 [details] pushed as aba4672 - altTab: Rename AltTabPopup to AppSwitcherPopup Attachment 230668 [details] pushed as 525d3c2 - altTab: Factor out thumbnail creation Attachment 230678 [details] pushed as e725f8a - altTab: Re-implement 'switch-windows' keybinding
*** Bug 656651 has been marked as a duplicate of this bug. ***