-
Notifications
You must be signed in to change notification settings - Fork 281
Fix #1846 Remove key binding #644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| exports.CMD_ADD_TO_WORKINGSET_AND_OPEN = "cmd.addToWorkingSetAndOpen"; // DocumentCommandHandlers.js handleOpenDocumentInNewPane() | ||
|
|
||
| // NAVIGATE | ||
| exports.NAVIGATE_NEXT_DOC = "navigate.nextDoc"; // DocumentCommandHandlers.js handleGoNextDoc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do it this way, since this is only going to fix part of it. This file is defining a module with the default set of Commands. Elsewhere in the code, you'll see that there are users of this:
$ git grep NAVIGATE_NEXT_DOC
src/command/Commands.js: exports.NAVIGATE_NEXT_DOC = "navigate.nextDoc"; // DocumentCommandHandlers.js handleGoNextDoc()
src/command/Commands.js: exports.NAVIGATE_NEXT_DOC_LIST_ORDER = "navigate.nextDocListOrder"; // DocumentCommandHandlers.js handleGoNextDocListOrder()
src/command/DefaultMenus.js: menu.addMenuItem(Commands.NAVIGATE_NEXT_DOC);
src/command/DefaultMenus.js: menu.addMenuItem(Commands.NAVIGATE_NEXT_DOC_LIST_ORDER);
src/command/Menus.js: NAVIGATE_DOCUMENTS_COMMANDS: {sectionMarker: Commands.NAVIGATE_NEXT_DOC},
src/document/DocumentCommandHandlers.js: CommandManager.register(Strings.CMD_NEXT_DOC, Commands.NAVIGATE_NEXT_DOC, handleGoNextDoc);
src/document/DocumentCommandHandlers.js: CommandManager.register(Strings.CMD_NEXT_DOC_LIST_ORDER, Commands.NAVIGATE_NEXT_DOC_LIST_ORDER, handleGoNextDocListOrder);
src/extensions/default/NavigationAndHistory/main.js: KeyBindingManager.addBinding(Commands.NAVIGATE_NEXT_DOC, KeyboardPrefs[NEXT_IN_RECENT_FILES]);
test/perf/OpenFile-perf-files/brackets-concat.js: exports.NAVIGATE_NEXT_DOC = "navigate.nextDoc";
test/perf/OpenFile-perf-files/brackets-concat.js: CommandManager.register(Strings.CMD_NEXT_DOC, Commands.NAVIGATE_NEXT_DOC, handleGoNextDoc);
test/perf/OpenFile-perf-files/brackets-concat.js: KeyBindingManager.addBinding(Commands.NAVIGATE_NEXT_DOC, [{key: "Ctrl-Tab", platform: "win"},
test/perf/OpenFile-perf-files/brackets-concat.js: exports.NAVIGATE_NEXT_DOC = "navigate.nextDoc";
test/perf/OpenFile-perf-files/brackets-concat.js: CommandManager.register(Strings.CMD_NEXT_DOC, Commands.NAVIGATE_NEXT_DOC, handleGoNextDoc);
test/perf/OpenFile-perf-files/brackets-concat.js: KeyBindingManager.addBinding(Commands.NAVIGATE_NEXT_DOC, [{key: "Ctrl-Tab", platform: "win"},
So you'd have to deal with all the various call sites too. A better solution would be to just not add the keybinding.
Also, if you do this for NEXT, you should probably do it for PREV too.
|
This fix still isn't right. What happens if you just remove these lines? https://github.com/mozilla/brackets/blob/master/src/base-config/keyboard.json#L263-L272 Can you test that and see if it solves his issue? |
|
@humphd Sir, I made changes that fixes the key-binding error. I think the issue here was that on mac system shift+command was triggering windows + pageup/pagedown button. |
humphd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of problems to clean up here, let me know when those are fixed and I'll test and merge.
src/base-config/keyboard.json
Outdated
| { | ||
| "key" : "Ctrl-Alt-H" | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to remove this extra space you added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I made the changes in the latest commit.please review it
| exports.TOGGLE_ACTIVE_LINE = "view.toggleActiveLine"; // EditorOptionHandlers.js _getToggler() | ||
| exports.TOGGLE_WORD_WRAP = "view.toggleWordWrap"; // EditorOptionHandlers.js _getToggler() | ||
| exports.TOGGLE_ALLOW_JAVASCRIPT = "cmd.toggleAllowJavaScript"; // EditorOptionsHandlers.js _getToggler() | ||
| exports.TOGGLE_AUTO_UPDATE = "cmd.toggleAutoUpdate"; // EditorOptionsHandlers.js _getToggler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to restore this file to how it should be. Use git checkout master src/command/Commands.js to fix.
|
Merged, thanks a lot. @flukeout will be thrilled! |
* Remove key binding * Commit changes * indent fixes * Changes - that add new for next/prev page [OSX] * Remove key-binding next/prev * Indent changes * mend
attempts to fix issue
https://github.com/mozilla/thimble.mozilla.org/issues/1846#issuecomment-285478285