Make Windows specific keybindings usable on Linux #2000
Conversation
While keeping Win and Mac specific keybindings separate.
| @@ -314,7 +314,7 @@ define(function (require, exports, module) { | |||
| command; | |||
|
|
|||
| // skip if this binding doesn't match the current platform | |||
| if (targetPlatform !== brackets.platform) { | |||
| if ( (targetPlatform === "mac") ^ (brackets.platform === "mac")) { | |||
There was a problem hiding this comment.
Typo: you meant to have one "mac" and one "win", but forgot to update your second copy.
There was a problem hiding this comment.
No, I think this is as intended. Note that the next line is a return statement.
There was a problem hiding this comment.
By the way, the eXclusive OR might not look obvious, but I felt it was less misleading than a not-equal-to operator.
There was a problem hiding this comment.
Then how does is work on Windows?
JSLint won't allow the ^ operator. Also you also need to remove the extra space before the first condition.
|
The expression evaluates to true if only one of the pair (targetPlatform vs brackets.platform) is a mac. It doesn't check for win, simply because Windows is not mac. Same goes for Linux, it's a not mac platform. Essentially, this lets Linux ride along with Windows, and doesn't disturb the original objective of mac keybindings being set on a windows system, or vice versa I don't think we need to add keybindings separate for Linux and Windows, seeing as they're keyboard-compatible with each other. And adding a Linux keybinding for every command that has platform-specific bindings at the moment would be a pain |
|
Nice Logic! Essentially, this change makes all unknown platforms including unix to use Windows key bindings.That said, this may break Brackets unit tests as we're using some imaginary platforms in our unit tests. @jasonsanjose should we merge this first even if it breaks our unit tests? |
|
@RaymondLim we should make any unit tests updates with this pull request. |
ghost
assigned
|
Do the unit tests use keybindings? I have seen excerpts from only one unit test till now and it explicitly invoked the command. I haven't seen all the tests yet, though. Any pointers to where they may be would be helpful. |
|
Debug > Run Tests. In that window, choose KeyBindingManager. |
|
I see that the virtual platform names in the tests start with test. This suggests we can either check for this prefix before skipping; or if there is another flag brackets uses internally to signal being tested, we could use that to disable permissive keybinding behaviour |
|
Are the tests passing or failing? I'd prefer modifying to tests if needed. |
|
@jasonsanjose You mean modifying this code, or modifying the tests? The latter might be too much work, I think. Also, is there any brackets.isBeingTested or similar flag? |
|
It's failing. I agree with @jasonsanjose that this has to be fixed in tests and not in the code, but I don't know how exactly we have to update the tests. The tests have no knowledge of all the key bindings generated from the code and they're just checking what they added in. |
|
I mean modifying the tests. No, we don't have a flag in the production code to indicate unit tests. |
|
Another way to approach this might be to make addBinding() automatically expand platform:"win" into a pair of "win"+"linux" (at least for now). This makes it a little more explicit what's going on, and also leaves open the ability to distinguish between more than just two platforms (e.g. you could still add Linux-only bindings too). I think that would also avoid breaking the unit tests. |
|
@pflynn that seems to be a good solution. On Mon, Nov 5, 2012 at 12:03 PM, Peter Flynn [email protected]:
|
| keyBinding = _addBinding(commandID, keyBindingRequest, keyBindingRequest.platform); | ||
|
|
||
| if (keyBinding) { | ||
| results.push(keyBinding); | ||
| } | ||
| if(keyBindingRequest.platform && keyBindingRequest.platform == "win") { |
There was a problem hiding this comment.
To pass JSLint, you need a space after if and you need to use === instead of ==
|
Hi @pritambaral I've created another pull request (#2331) based on this one that fixes the JSLint error as well as adding some comments. The commit history is preserved, so you still get the credit you deserve. I'm closing this one and will get #2331 reviewed. |
While keeping Win and Mac specific keybindings separate.