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

Refactor core keyboard bindings into an external json file #2131

Merged
merged 9 commits into from
Dec 5, 2012

Conversation

jasonsanjose
Copy link
Member

Defines a new top level prefs folder that contains a keyboard.json file. This file was generated from the default keybindings in Brackets. KeyBindingManager loads this json file and queries it for default bindings based on the command ID.

@peterflynn
Copy link
Member

Thinking about the ultimate goal of user-configurable bindings, it seems like two important things we'd need to handle are: upgrading to a later version of Brackets; and shortcuts related to extensions.

I think we might be able to kill both birds with one stone if we tweaked it as follows:

  • JSON file in the install dir is recast as a "defaults" file that the user doesn't edit.
  • Extensions provide their own JSON file? (Or we assume that all calls to addBinding() during app startup represent "defaults", while later calls are treated as temporary programmatic changes... but that seems mildly sketchy).
  • JSON file in the per-user prefs dir acts as an overlay (set of overrides) atop the defaults. These settings could override both core bindings and extensions' bindings.

For the purposes of this pull request, I imagine we could punt on all of that. Except I'd suggest we name the folder something like "config" instead of "prefs" to avoid confusion.

@ghost ghost assigned njx Nov 16, 2012
@jasonsanjose
Copy link
Member Author

I like all those ideas. :) My idea was to make the first step to externalize the settings. Baby steps.

@peterflynn
Copy link
Member

Yep, definitely agree. What do you think about the folder rename?

@jasonsanjose
Copy link
Member Author

I think maybe defaults/keyboard.json instead of config/keyboard.json.

@peterflynn
Copy link
Member

That sounds good to me.

@njx
Copy link
Contributor

njx commented Nov 19, 2012

One general question: is there a reason why the JSON file has each keyboard shortcut map to an object with a single field (keybindings) instead of just mapping directly to a string or array?

@njx
Copy link
Contributor

njx commented Nov 19, 2012

Also agree that we should change the folder name to "defaults".

}

var defaultBinding = KeyboardPrefs[commandID];
keyBindings = keyBindings || (defaultBinding ? defaultBinding.keyBindings : undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this refactoring, it seems a little weird that you now have to call addBinding() with a null/undefined keyBindings in order to get the default keybinding applied (in other words, the default keybinding happens as a side-effect of the associated menu item being added, or of someone explicitly calling addBinding() on the command). It would make more sense to just have some separate place during initialization where we load all the default keybindings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I've changed this to instead add the default bindings when a command is registered.

@njx
Copy link
Contributor

njx commented Nov 19, 2012

Initial review done--a few general comments. I didn't review the extraction of each keybinding--do you think it would be valuable for me to do that?

@jasonsanjose
Copy link
Member Author

Waiting for sprint 18 before merging this in.

@jasonsanjose
Copy link
Member Author

Changes pushed. Ready for review after sprint 17.

@jasonsanjose
Copy link
Member Author

@njx merged sprint 17. Ready for review.

@njx
Copy link
Contributor

njx commented Dec 5, 2012

Looks good, 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.

3 participants