Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

KeyBindingManager no longer warns / prevents binding collisions #3032

Closed
peterflynn opened this issue Mar 4, 2013 · 5 comments
Closed

KeyBindingManager no longer warns / prevents binding collisions #3032

peterflynn opened this issue Mar 4, 2013 · 5 comments

Comments

@peterflynn
Copy link
Member

Place the following code in an extension:

KeyBindingManager.addBinding(Commands.FILE_OPEN_FOLDER, "Ctrl-E");

Result: Quick Edit binding is silently overwritten. No console warnings.

Expected: We used to print warning messages and ignore the attempt at overwriting the binding. KeyBindingManager._addBinding() still contains several bits of code that seem like they should do this, but the logic has gotten pretty convoluted so I'm not sure in which cases they'll run anymore.

@peterflynn
Copy link
Member Author

Here's the original repro case, slightly trickier, that I found in the Clojure extension:

    KeyBindingManager.addBinding(Commands.FILE_OPEN_FOLDER, [
        {
            "key": "Ctrl-E"
        },
        {
            "key": "Cmd-E",
            "platform": "mac"
        }
    ]);

@ghost ghost assigned dangoor Mar 6, 2013
@njx
Copy link

njx commented Mar 6, 2013

Reviewed. Low priority to @dangoor

jasonsanjose added a commit that referenced this issue Mar 11, 2013
Fixed: #3032 KeyBindingManager no longer warns / prevents binding collisions
@ghost ghost assigned peterflynn Mar 11, 2013
@jasonsanjose
Copy link
Member

FBNC @peterflynn. We still allow platform-specific overrides to clobber a generic binding, but not the other way around. @WebsiteDeveloper's fix will show errors in the case you've described here as well when clobbering a generic binding with another generic binding.

@peterflynn
Copy link
Member Author

We certainly do warn now, though not always at the right times -- see #4265 for more up to date discussion. Closing this one.

@redmunds
Copy link
Contributor

redmunds commented Sep 5, 2013

Verified that this is fixed in pull request #5066.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants