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

Prioritize platform-specific keybindings over generic bindings #2372

Merged
merged 4 commits into from
Dec 18, 2012
Merged

Prioritize platform-specific keybindings over generic bindings #2372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ea160b
remove generic bindings when a platform-specific binding is added later
jasonsanjose Dec 17, 2012
29fa637
update KeyBindingManager unit tests
jasonsanjose Dec 17, 2012
16d3366
remove typo in comment
jasonsanjose Dec 17, 2012
8c65d34
code review comments
jasonsanjose Dec 17, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 43 additions & 21 deletions src/command/KeyBindingManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,8 @@ define(function (require, exports, module) {
normalizedDisplay,
explicitPlatform = keyBinding.platform || platform,
targetPlatform = explicitPlatform || brackets.platform,
command;
command,
bindingsToDelete = [];

key = (keyBinding.key) || keyBinding;
if (brackets.platform === "mac" && explicitPlatform === undefined) {
Expand All @@ -376,8 +377,7 @@ define(function (require, exports, module) {
}

// for cross-platform compatibility
if (brackets.platform !== "mac" &&
brackets.platform !== "win") {
if (exports.useWindowsCompatibleBindings) {
if (explicitPlatform === "win") {
// windows-only key bindings are used as the default binding
// only if a default binding wasn't already defined
Expand All @@ -393,24 +393,6 @@ define(function (require, exports, module) {

// target this windows binding for the current platform
targetPlatform = brackets.platform;
} else if (!explicitPlatform || (explicitPlatform === brackets.platform)) {
// if adding a generic binding or a binding for the current
// platform, clobber any windows bindings that may have been
// installed
var existingBindings = _commandMap[commandID] || [],
bindingsToDelete = [];

// filter out windows-only bindings in _commandMap
existingBindings.forEach(function (binding) {
if (binding.explicitPlatform === "win") {
bindingsToDelete.push(binding);
}
});

// delete windows-only bindings in _keyMap
bindingsToDelete.forEach(function (binding) {
removeBinding(binding.key);
});
}
}

Expand All @@ -419,6 +401,33 @@ define(function (require, exports, module) {
return null;
}

// delete existing bindings when
// (1) replacing a windows-compatible binding with a generic or
// platform-specific binding
// (2) replacing a generic binding with a platform-specific binding
var existingBindings = _commandMap[commandID] || [],
isWindowsCompatible,
isReplaceGeneric;

existingBindings.forEach(function (binding) {
// remove windows-only bindings in _commandMap
isWindowsCompatible = exports.useWindowsCompatibleBindings &&
binding.explicitPlatform === "win";

// remove existing generic binding
isReplaceGeneric = !binding.explicitPlatform &&
explicitPlatform;

if (isWindowsCompatible || isReplaceGeneric) {
bindingsToDelete.push(binding);
}
});

// remove generic or windows-compatible bindigns
bindingsToDelete.forEach(function (binding) {
removeBinding(binding.key);
});

// optional display-friendly string (e.g. CMD-+ instead of CMD-=)
normalizedDisplay = (keyBinding.displayKey) ? normalizeKeyDescriptorString(keyBinding.displayKey) : normalized;

Expand Down Expand Up @@ -570,6 +579,9 @@ define(function (require, exports, module) {
},
true
);

exports.useWindowsCompatibleBindings = (brackets.platform !== "mac")
&& (brackets.platform !== "win");
}

$(CommandManager).on("commandRegistered", _handleCommandRegistered);
Expand All @@ -586,4 +598,14 @@ define(function (require, exports, module) {
exports.removeBinding = removeBinding;
exports.formatKeyDescriptor = formatKeyDescriptor;
exports.getKeyBindings = getKeyBindings;

/**
* Use windows-specific bindings if no other are found (e.g. Linux).
* Core Brackets modules that use key bindings should always define at
* least a generic keybinding that is applied for all platforms. This
* setting effectively creates a compatibility mode for third party
* extensions that define explicit key bindings for Windows and Mac, but
* not Linux.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a note here that the purpose of this is to make extensions backward-compatible and brackets core code should always have keybindings explicitly defined.

Is there a way to verify this in the code and write something in the console log?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to verify what? That we eventually get either (1) a generic key binding or (2) a key binding for the current platform? We could validate the default keyboard.json, but for clients calling addBinding directly we can't.

Copy link
Member Author

Choose a reason for hiding this comment

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

added comment

exports.useWindowsCompatibleBindings = false;
});
36 changes: 36 additions & 0 deletions test/spec/KeyBindingManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,13 @@ define(function (require, exports, module) {
});

it("should use windows key bindings on linux", function () {
var original = KeyBindingManager.useWindowsCompatibleBindings;

this.after(function () {
KeyBindingManager.useWindowsCompatibleBindings = original;
});

KeyBindingManager.useWindowsCompatibleBindings = true;
brackets.platform = "linux";

// create a windows-specific binding
Expand All @@ -225,6 +232,35 @@ define(function (require, exports, module) {

expect(KeyBindingManager.getKeymap()).toEqual(expected);
});

it("should use replace generic key bindings with platform-specific", function () {
var original = KeyBindingManager.useWindowsCompatibleBindings;

this.after(function () {
KeyBindingManager.useWindowsCompatibleBindings = original;
});

KeyBindingManager.useWindowsCompatibleBindings = true;
brackets.platform = "linux";

// create a generic binding
KeyBindingManager.addBinding("test.cmd", "Ctrl-A");

var expected = keyMap([
keyBinding("Ctrl-A", "test.cmd")
]);

expect(KeyBindingManager.getKeymap()).toEqual(expected);

// create a linux-only binding to replace the windows binding
KeyBindingManager.addBinding("test.cmd", "Ctrl-B", "linux");

expected = keyMap([
keyBinding("Ctrl-B", "test.cmd", null, "linux")
]);

expect(KeyBindingManager.getKeymap()).toEqual(expected);
});
});

describe("removeBinding", function () {
Expand Down