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

Fix for Issue 4265: Mac: KeyBindingManager displays errant error message #4305

Merged
merged 4 commits into from
Jun 25, 2013

Conversation

WebsiteDeveloper
Copy link
Contributor

@ghost ghost assigned jasonsanjose Jun 21, 2013
@@ -554,6 +554,16 @@ define(function (require, exports, module) {
var keyBinding;
results = [];

keyBindings.sort(function (a, b) {
if(a.platform === brackets.platform) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert space after if

@jasonsanjose
Copy link
Member

@WebsiteDeveloper I tested this on mac and still saw the error. I believe it's because we still attempt to add all key bindings, not just the first one that matches. We might want to update addBinding at line 567 with

keyBindings.some(function addSingleBinding(keyBindingRequest) {
    // attempt to add keybinding
    keyBinding = _addBinding(commandID, keyBindingRequest, keyBindingRequest.platform);

    if (keyBinding) {
        results.push(keyBinding);
        return true;
    }

    return false;
});

Initial review complete

@WebsiteDeveloper
Copy link
Contributor Author

@jasonsanjose added it and it should fix the problem.

@@ -554,13 +554,26 @@ define(function (require, exports, module) {
var keyBinding;
results = [];

keyBindings.forEach(function addSingleBinding(keyBindingRequest) {
keyBindings.sort(function (a, b) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing the wrong shortcuts appear on mac (e.g. F3 for find next). I tried fixing the sort function and this seems to work. I think you have your sort logic backwards:

keyBindings.sort(function (a, b) {
    if (a.platform === brackets.platform) {
        // "a" is platform specific and matches
        return -1;
    } else if (b.platform === brackets.platform) {
        // "b" is platform specific and matches
        return 1;
    } else if (!a.platform && b.platform) {
        // "a" is generic and "b" is not matching
        return -1;
    } else if (!b.platform && a.platform) {
        // "b" is generic and "a" is not matching
        return 1;
    } else {
        return 0;
    }
});

@jasonsanjose
Copy link
Member

Reviewed again. Back to @WebsiteDeveloper

@WebsiteDeveloper
Copy link
Contributor Author

@jasonsanjose pushed the changes.
by the way what about the boss option? is it still necessary?

@jasonsanjose
Copy link
Member

@WebsiteDeveloper thanks for the reminder. I'll do that separately. Merging.

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

Successfully merging this pull request may close these issues.

None yet

2 participants