Created attachment 178167 [details] [review]
Add a way to switch among windows of the same application

of course, if you don't have a US keyboard, you'll have to use a different key once you get into the switcher...

(In reply to comment #2)
> of course, if you don't have a US keyboard, you'll have to use a different key
> once you get into the switcher...

This means that the whole AltTabPopup._keyPressEvent() function must be rewritten to not rely on clutter's key-press-event and instead do several Main.wm.setKeybindingHandler() and create the respective handlers ?

no, setKeybindingHandler() only affects what happens when there is no keyboard grab.

The fix is that rather than doing:

    if (keysym == Clutter.grave)
        this._select(this._currentApp, this._nextWindow());

it needs to do

    let keyCode = event.get_key_code();
    let modifierState = Shell.get_event_state(event);
    let display = global.screen.get_display();
    let action = display.get_keybinding_action(keyCode, modifierState);

    if (action == Meta.KeyBinding.Action.CYCLE_WINDOWS)
        this._select(this._currentApp, this._nextWindow());

etc

Created attachment 178334 [details] [review]
Add a way to switch among windows of the same application

(In reply to comment #5)

This patch should work better and be more elegant as you say. I've left open the question of handling SWITCH_WINDOWS/SWITCH_WINDOWS_BACKWARD instead of keysym == Clutter.Tab. Should I do that too? If yes, then what about Clutter.Escape, Clutter.Left, and so on?

the Clutter.Tab part should be switched to use keybinding_action. The rest (arrows, esc) is not configurable so there'd be no action to switch them to

Created attachment 178340 [details] [review]
altTab: use switch_group and switch_windows keybinding actions

Use metacity's keybinding actions switch_group and swicth_windows to "change
window" and "change application" respectively.

To test this you need:

$ gconftool-2 -s /apps/metacity/global_keybindings/switch_windows_backward -t string "<Shift><Alt>Tab"
$ gconftool-2 -s /apps/metacity/global_keybindings/switch_group -t string
"<Alt>Above_Tab"
$ gconftool-2 -s /apps/metacity/global_keybindings/switch_group_backward -t
string "<Shift><Alt>Above_Tab"

Created attachment 178418 [details] [review]
altTab: use switch_group and switch_windows keybinding actions

Use metacity's keybinding actions switch_group and switch_windows for "change
application window" and "change application" respectively.

Ok, now I think we're following metacity's behavior more closely by handling the shift key being pressed to reverse the meaning of the action. This way only the switch_group gconf key must be set to get the whole experience.

Created attachment 178526 [details] [review]
altTab: use switch_group and switch_windows keybinding actions

Use metacity's keybinding actions switch_group and switch_windows for "change
application window" and "change application" respectively.

I prefer this last version for 2 reasons:

1. It simplifies the code.

2. I personally find it useful to jump quickly to another application with a Tab press while a window thumbnail is selected instead of using the Up arrow or having to Tab through the whole thumbnail list until the last one to then select the following app.

Comments?

Created attachment 179311 [details] [review]
altTab: use cycle_group and switch_windows keybinding actions

Use metacity's keybinding actions cycle_group and switch_windows for "change
application window" and "change application" respectively.

Comment on attachment 179311 [details] [review]
altTab: use cycle_group and switch_windows keybinding actions

So, the current behavior of Tab being sometimes switch-app and sometimes switch-window is by design, and if you want to change it, you need to talk to the designers and get an OK on that. Otherwise, revert it back so that it works the way it did before. (You should still check for Meta.KeyBindingAction.SWITCH_WINDOWS rather than Clutter.Tab, just have it mean different things in the two different cases.)

>+                this._select (0, this._appIcons[0].cachedWindows.length - 1);

there should be no space between function name and "("

>+        } else if (action == Meta.KeyBindingAction.SWITCH_WINDOWS) {
>+            this._select(shift ? this._previousApp() : this._nextApp());
>+        } else if (action == Meta.KeyBindingAction.SWITCH_WINDOWS_BACKWARD) {
>+            this._select(shift ? this._nextApp() : this._previousApp());

this is redundant. Either get_keybinding_action() takes shift into account for you (so you don't need to check it yourself) or it doesn't (so you don't need to handle SWITCH_WINDOWS_BACKWARD).

(In reply to comment #15)
> (From update of attachment 179311 [details] [review])
> So, the current behavior of Tab being sometimes switch-app and sometimes
> switch-window is by design, and if you want to change it, you need to talk to
> the designers and get an OK on that.

OK, I'm adding the ui-review keyword.

> Otherwise, revert it back so that it works
> the way it did before. (You should still check for
> Meta.KeyBindingAction.SWITCH_WINDOWS rather than Clutter.Tab, just have it mean
> different things in the two different cases.)

I'm not checking for Clutter.Tab anywhere in this latest patch. Am I missing something?

> 
> >+                this._select (0, this._appIcons[0].cachedWindows.length - 1);
> 
> there should be no space between function name and "("

Sure, will fix.

> 
> >+        } else if (action == Meta.KeyBindingAction.SWITCH_WINDOWS) {
> >+            this._select(shift ? this._previousApp() : this._nextApp());
> >+        } else if (action == Meta.KeyBindingAction.SWITCH_WINDOWS_BACKWARD) {
> >+            this._select(shift ? this._nextApp() : this._previousApp());
> 
> this is redundant. Either get_keybinding_action() takes shift into account for
> you (so you don't need to check it yourself) or it doesn't (so you don't need
> to handle SWITCH_WINDOWS_BACKWARD).

Well, this is how metacity does it too. Check mutter's src/core/keybindings.c:process_tab_grab()

I agree it sucks because we already have both a switch_windows and a switch_windows_reversed. But if we are to use those then the gconf default for switch_windows_reversed has to be changed from disabled to <Shift><Alt>Tab. Analogously for cycle_group[_reversed].

Regarding the design question, my opinion is that the current behavior of Tab iterating through apps or windows depending on the context isn't much predictable and thus prone to misuses and frustration. So, now that we have a nice way to know about the key above tab, I think it works better if we do only one thing on each key since it's more predictable.

Created attachment 179614 [details] [review]
altTab: enable the cycle_group keybinding action

Allows the user to bring up the Alt+Tab popup with the current application's
window thumbnails selected.

Created attachment 179615 [details] [review]
altTab: enable the cycle_group keybinding action

Allows the user to bring up the Alt+Tab popup with the current application's
window thumbnails selected.

Created attachment 179616 [details] [review]
altTab: use keybinding actions instead of clutter keysym comparison

Use keybinding actions instead of clutter keysym comparisons to select
next/prev application and next/prev window.

Sorry for the bug spam. I'm still learning to use git-bz.

Attachment 179616 [details] should actually be first and doesn't change behavior. Although, to handle the shift case, I think something like:

diff --git a/src/include/all-keybindings.h b/src/include/all-keybindings.h
index 643a123..16db7ac 100644
--- a/src/include/all-keybindings.h
+++ b/src/include/all-keybindings.h
@@ -156,7 +156,7 @@ keybind (switch_windows,            handle_switch,        META_TAB_LIST_NORMAL,
          BINDING_REVERSES,       "<Alt>Tab",
         _("Move between windows, using a popup window"))
 keybind (switch_windows_backward,  handle_switch,        META_TAB_LIST_NORMAL,
-         REVERSES_AND_REVERSED,  NULL,
+         REVERSES_AND_REVERSED,  "<Shift><Alt>Tab",
         _("Move backward between windows, using a popup window"))
 keybind (switch_panels,             handle_switch,        META_TAB_LIST_DOCKS,
          BINDING_REVERSES,       "<Control><Alt>Tab",
@@ -170,7 +170,7 @@ keybind (cycle_group,               handle_cycle,         META_TAB_LIST_GROUP,
         BINDING_REVERSES,        "<Alt>Above_Tab",
         _("Move between windows of an application immediately"))
 keybind (cycle_group_backward,     handle_cycle,         META_TAB_LIST_GROUP,
-        REVERSES_AND_REVERSED,   NULL,
+        REVERSES_AND_REVERSED,   "<Shift><Alt>Above_Tab",
         _("Move backward between windows of an application immediately"))
 keybind (cycle_windows,             handle_cycle,         META_TAB_LIST_NORMAL,
         BINDING_REVERSES,        "<Alt>Escape",

should be committed to mutter. Else, something like I had in the previous patch must be added on top of this to implement the BINDING_REVERSES semantics like metacity does.

The second patch (attachment 179615 [details] [review]) just adds the possibility of triggering the window switcher without having to press Alt+Tab first.

The behavior changing part I'll leave out for now until we have design input and I can then open a new bug for it.

(assigning to Dan for continued review of the keybinding handling portions)

Comment on attachment 179615 [details] [review]
altTab: enable the cycle_group keybinding action

Hm... this is odd. It's using the cycle_group keybinding, but the behavior is that of switch_group. ("cycle" = switch immediately, "switch" = select via pop-up window).

Assuming we're going to keep this UI, we should probably move the binding in metacity/mutter to switch_group, and then update this patch to use that.

Beyond that, the only problem I see is that if the app only has a single window, then Alt-Above_Tab should probably be a no-op, just like Alt-Tab is if you only have a single application open.

Comment on attachment 179616 [details] [review]
altTab: use keybinding actions instead of clutter keysym comparison

this is good, subject to the same cycle-vs-switch issue as the other bug

(In reply to comment #23)
> (From update of attachment 179615 [details] [review])
> Hm... this is odd. It's using the cycle_group keybinding, but the behavior is
> that of switch_group. ("cycle" = switch immediately, "switch" = select via
> pop-up window).

Right, on first patches I submitted to this bug I was actually using the switch_* keybindings, but then I changed it to cycle_* since that's what Owen chose as default for Alt+Above_Tab in the mutter patch:

http://git.gnome.org/browse/mutter/commit/?id=4ea00e102b6afe25e2b84f9def2f44da1b4953c6

I don't really care either way.

What about the BINDING_REVERSES semantics?

And, btw, currently, in a jhbuild environment where you already have a system metacity installed the user must configure the keybindings in gconf for them to work. How should that be handled? Oh, and I'm assuming that mutter's all-keybindings.h defaults are actually used if metacity's gconf keys aren't installed, right?

(In reply to comment #25)
> Right, on first patches I submitted to this bug I was actually using the
> switch_* keybindings, but then I changed it to cycle_* since that's what Owen
> chose as default for Alt+Above_Tab in the mutter patch:

Right. He assumed we were going to do the UI OS X style, where Alt+Above_Tab switched windows instantly, which corresponds to cycle_group in metacity. But what you implemented uses the pop-up switcher, which would be switch_group. So, if we're keeping this UI, we should change metacity/mutter to put the binding on switch_group.

Owen, want to try this patch out and see if you like the behavior?

> What about the BINDING_REVERSES semantics?

It's a mutter bug, but not in the place you pointed out. meta_display_get_keybinding_action() ought to be noticing the BINDING_REVERSES flag (or rather, the flag that that flag gets turned into later, IIRC), and returning the correct reversed keybinding when Shift is down. (That's the way it works in other places in mutter.)

> And, btw, currently, in a jhbuild environment where you already have a system
> metacity installed the user must configure the keybindings in gconf for them to
> work. How should that be handled?

Not sure. I'd suggested adding metacity to gnome-shell's jhbuild before.

Trying this out, the good thing about it is that it handles windows on other workspaces well (also minimized/hidden windows)

The bad thing is that if you have an application with a bunch of very similar looking windows - like a bunch of terminals, the small previews arent' a very good way to tell them apart - and if they were raised as a group and all visible to you it's a bit silly that you are staring at tiny little thumbnails to try and figure out which is which instead of just waiting until the window you want to focus - that you can just look at - is focused.

I guess my inclination is that we should go with this change (and the accompanying mutter/metacity changes to the key bindings) so that we have something that is consistent and complete - so we don't have to solve:

 - Windows on other workspaces
 - Maybe hidden windows
 - Getting rid of the weird indicator rectangle that Mutter is drawing currently on cycle_group

Some other way, though I'm not really sold on this being the best possible user interface.

Created attachment 179917 [details] [review]
altTab: enable the switch_group keybinding action

Allows the user to bring up the Alt+Tab popup with the current application's
window thumbnails selected.

Created attachment 179918 [details] [review]
altTab: use keybinding actions instead of clutter keysym comparison

Use keybinding actions instead of clutter keysym comparisons to select
next/prev application and next/prev window.

Created attachment 179919 [details] [review]
altTab: enable the switch_group keybinding action

Allows the user to bring up the Alt+Tab popup with the current application's
window thumbnails selected.

Created attachment 179920 [details] [review]
all-keybindings.h: change <Alt>Above_Tab from cycle_group to switch_group

Comment on attachment 179918 [details] [review]
altTab: use keybinding actions instead of clutter keysym comparison

>+        let shift = event_state & Clutter.ModifierType.SHIFT_MASK;

call it "backwards" instead of "shift"; it will make things clearer below

>-        if (keysym == Clutter.grave)
>+        if (action == Meta.KeyBindingAction.SWITCH_GROUP && !shift)
>             this._select(this._currentApp, this._nextWindow());
>-        else if (keysym == Clutter.asciitilde)
>+        else if (action == Meta.KeyBindingAction.SWITCH_GROUP && shift)
>             this._select(this._currentApp, this._previousWindow());

I think it would be clearer to squash these. Eg,

    if (action == Meta.KeyBindingAction.SWITCH_GROUP)
        this._select(this._currentApp, backwards ? this._previousWindow() : this._nextWindow());

>+            if (action == Meta.KeyBindingAction.SWITCH_WINDOWS && !shift) {
>                 if (this._currentWindow == this._appIcons[this._currentApp].cachedWindows.length - 1)

and in this case, "if (action == Meta.KeyBindingAction.SWITCH_WINDOWS)" first, and then "if (backwards)" inside it.

Comment on attachment 179919 [details] [review]
altTab: enable the switch_group keybinding action

this is good to commit once the other stuff is in. (Do you have git bits or do you need us to commit it?)

Comment on attachment 179920 [details] [review]
all-keybindings.h: change <Alt>Above_Tab from cycle_group to switch_group

Owen, what do we do here wrt metacity? Do we need to discuss this change with them, or just commit it?

Just commit it, but make sure to put cycle-group back to the old alt-f6 or whaver it was keybinding (haven't looked at the patch to see if it does that)

Created attachment 180607 [details] [review]
all-keybindings.h: change <Alt>Above_Tab from cycle_group to switch_group

Created attachment 180608 [details] [review]
altTab: use keybinding actions instead of clutter keysym comparison

Use keybinding actions instead of clutter keysym comparisons to select
next/prev application and next/prev window.

Attachment 179919 [details] pushed as a047132 - altTab: enable the switch_group keybinding action
Attachment 180608 [details] pushed as 1bc8052 - altTab: use keybinding actions instead of clutter keysym comparison

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