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

Fixed: #3032 KeyBindingManager no longer warns / prevents binding collisions #3081

Merged
merged 2 commits into from
Mar 11, 2013

Conversation

WebsiteDeveloper
Copy link
Contributor

Fix for #3032

@ghost ghost assigned jasonsanjose Mar 11, 2013
@jasonsanjose
Copy link
Member

@WebsiteDeveloper are you on win or mac? On your branch, I get the following console error on mac:

Cannot assign Cmd-G to navigate.gotoLine. It is already assigned to edit.findNext 

This works out to be okay. Cmd-G is assigned to findNext, it isn't removed, and gotoLine is assigned Cmd-G.

if (existing) {
if (!existing.explicitPlatform) {
Copy link
Member

Choose a reason for hiding this comment

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

In #3032, what happens is that the extension tries to replace a cross-platform Ctrl-E/Cmd-E binding with another cross-platform Ctrl-E/Cmd-# binding. I think a more correct fix might be to check the new binding for an explicit platform and only override it in that case:

if (!existing.explicitPlatform && existingPlatform) {
  // remove the the generic binding to replace with this new platform-specific binding
  removeBinding(normalized)
} else { 
  console.error(...);
  return null;
}

@jasonsanjose
Copy link
Member

Initial review complete

@WebsiteDeveloper
Copy link
Contributor Author

@jasonsanjose i adressed your remark

@jasonsanjose
Copy link
Member

Merging

jasonsanjose added a commit that referenced this pull request Mar 11, 2013
Fixed: #3032 KeyBindingManager no longer warns / prevents binding collisions
@jasonsanjose jasonsanjose merged commit ecc0583 into adobe:master Mar 11, 2013
@lkcampbell
Copy link
Contributor

FYI, I just double checked the KeyBindingManager unit tests because we had two merges on KeyBindingManager.js fairly close to each other. I wasn't getting an error in my branch but I am getting an error now. I don't know if it is because of my change or @WebsiteDeveloper 's change or a combination of both, but it is there now.

@WebsiteDeveloper
Copy link
Contributor Author

@lkcampbell it happens actually because of my changes, but jasonsanjose already set up a pull to fix this

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.

3 participants