Created attachment 155915 [details] [review]
Add connect and disconnect functions for appSwitcher so that extension etc. can disconnect the default one and attach their own.

Created attachment 155916 [details] [review]
Same patch as above, with proper spacing

Review of attachment 155916 [details] [review]:

This patch looks OK except your commit message is way too long.

I'd say "[windowManager] Allow extensions to more easily override alt-tab".

Created attachment 155926 [details] [review]
Updated patch with better comment

Thanks for the feedback!  I made the comment extra-descriptive the first time because I wasn't sure how much was wanted, I figured its better to err on the side of more description than not enough.  How's this?

Review of attachment 155926 [details] [review]:

Verbose commit messages are good, but the first line in git is special - it serves the same purpose as Subject: does in email.  Try "git shortlog 2.28.0.." for example.

Anything more verbose goes after; you might say "This allows extensions to override it more easily." for example.

I'll go ahead and fix up the one comment below and commit.

::: js/ui/windowManager.js
@@ +298,3 @@
+        this._switchWindowsId = this._shellwm.connect('keybinding::switch_windows', Lang.bind(this, this._startAppSwitcher));
+        this._shellwm.takeover_keybinding('switch_windows');
+    _connectAppSwitcher : function() {

Actually something I missed before (that I usually check for religiously =) )

Please set:

this._switchWindowsId = 0;

after disconnecting.

Created attachment 155971 [details] [review]
Set switchWindowsId = 0 when we disconnect.

Ok, this one should be good. Thanks again for the feedback.

Comment on attachment 155971 [details] [review]
Set switchWindowsId = 0 when we disconnect.

>-        let shellwm = global.window_manager;
>+        let shellwm =  global.window_manager;

fix that.

>+    _disconnectAppSwitcher : function() {

underscore-prefixed means "private", meaning that extensions shouldn't be fiddling with it. So if extensions *should* be fiddling with it, then it shouldn't have an underscore.

A cleaner way of doing this (and allowing overriding the other keybindings as well) would be to add a method to WindowManager something like this:

    setKeybindingHandler: function(keybinding, handler) {
        if (this._handlers[keybinding])
            this._shellwm.disconnect(this._handlers[keybinding]);
        else
            this._shellwm.takeover_keybinding(keybinding);

        this._handlers[keybinding] =
            this._shellwm.connect('keybinding::' + keybinding, handler);
    }

(and fix _init() to initialize this._handlers, and to use setKeybindingHandler to set up the initial alt-tab/workspace handlers).

Then your extension could just call

    Main.wm.setKeybindingHandler('switch_windows', myAltTabFunction);

and WindowManager would disconnect the old handler and connect the new one

Created attachment 156242 [details] [review]
Use the advice given by dan above

Thanks for the advice Dan.  I like this design, and obviously it works with all the other keybindings as well so extensions can replace any of them they may want.  Let me know how this looks.  Thanks.

(In reply to comment #7)
> (From update of attachment 155971 [details] [review])
> >-        let shellwm = global.window_manager;
> >+        let shellwm =  global.window_manager;
> 
> fix that.
> 
> >+    _disconnectAppSwitcher : function() {
> 
> underscore-prefixed means "private", meaning that extensions shouldn't be
> fiddling with it. So if extensions *should* be fiddling with it, then it
> shouldn't have an underscore.

I don't think we are going to necessarily draw a clear line for extensions; a lot of the power of the Firefox model is how deeply they can poke into internals, while at the same time, bearing the cost of doing so.

However I'm also all in favor of explicit API for extensions.

Review of attachment 156242 [details] [review]:

One comment; to save you the round trip I've done the change myself and committed this patch.

::: js/ui/windowManager.js
@@ +25,1 @@
+        this._handlers = [];

Would prefer this variable to be called _keyBindingHandlers.  More verbose, yes, but on the other hand it's a lot clearer it isn't used for other things.