Add support for activating and deactivating package-specific keymaps

[tmunro]

Adds support for activating and deactivating the keymaps for individual packages. Currently, there's no good way to quickly disable conflicting keymaps. Adding unset! entries for every binding in your own keymap is tedious and error-prone. This adds Package support for activating and deactivating keymaps, so you can easily choose which keymaps from which packages are important to you.

See atom/settings-view#610 for UI support.

Related: atom/atom-keymap#82

[bronson]

Extra newline

[tmunro]

Fixed!

[bronson]

Great feature! You thinking about adding UI in another PR or is this good enough?

[tmunro]

Yes! UI support is in atom/settings-view#610, forgot to put a link in when I added the other PR.

[maxbrunsfeld]

This feature makes a lot of sense. Thanks for taking the time to add it!

Right now, I can't tell if it works to add a package to the disabledKeymaps after it has been activated. Could you make sure that is tested? I would suggest just modifying your second test, to something like this:

describe "when a package's keymaps are disabled and re-enabled after it is activated", ->
  it "removes and re-adds the keymaps", ->
    element1 = $$ -> @div class: 'test-1'

    waitsForPromise ->
      atom.packages.activatePackage("package-with-keymaps-manifest")

    runs ->
      atom.config.set("core.disabledKeymaps", ['package-with-keymaps-manifest'])
      expect(atom.keymaps.findKeyBindings(keystrokes: 'ctrl-z', target: element1[0])).toHaveLength 0

      atom.config.set("core.disabledKeymaps", [])
      expect(atom.keymaps.findKeyBindings(keystrokes: 'ctrl-z', target: element1[0])[0].command).toBe 'keymap-1'

[tmunro]

@maxbrunsfeld Good point, I knew I was missing something in the specs. Added.

[maxbrunsfeld]

It would be nice to do this config-observing in the PackageManager. Performance-wise, it would mean there'd be one observer instead of one for each package, and it would also be more consistent with the way we observe disabledPackages. I think the code could be very similar to the code for observeDisabledPackages.

Would you mind refactoring this code in that way?

[tmunro]

Sure, that makes more sense, thanks for the suggestion.

Would you suggest splitting it into onDidChange for the observer and an initial config check for activation? I'm thinking that makes more sense too.

[maxbrunsfeld]

Would you suggest splitting it into onDidChange for the observer and an initial config check for activation?

I don't have a strong opinion on that; go with whichever way makes the most sense as you're writing the code.

[maxbrunsfeld]

You can remove the second if, since its condition is the inverse of the first if. How about this:

keymapIsDisabled = _.include(atom.config.get("core.disabledKeymaps") ? [], @name)
if keymapIsDisabled
  @deactivateKeymaps()
else
  @activateKeymaps()

[maxbrunsfeld]

I left some very minor additional feedback, and restarted the travis build because it failed for reasons unrelated to your change. Looks good!

[maxbrunsfeld]

Alright! 🚢 💸

[t9md]

Thanks @tmunro and nice work.

I want to put comment for setting name core.disabledKeymaps.
If we add another keymap disabling feature for specific keystroke(keystroke based disabling).
The name core.disabledKeymaps become ambiguous.

How about rename core.disabledKeymaps to core.keymapDisabledPackages?
Its also well sync current core.disabledPackages.

[maxbrunsfeld]

If we add another keymap disabling feature for specific keystroke(keystroke based disabling.

If you wanted to disable a specific keystroke, couldn't you just unset! in your own keymap?

[bronson]

If you wanted to disable a specific keystroke, couldn't you just unset! in your own keymap?

Yes, if you already have some Atom expertise. Even then, figuring out the right selectors gets tedious.

If it gets @t9md to start including more bindings in his packages then it's totally worth it. ;-)

[t9md]

I'm not so sure but I want some easy to use UI in settings-view to disable specific keymap by searching keystroke from Keybindings left menu on settings-view.

But anyway, even if we won't introduce keystroke based disabling UI, I want to vote core.keymapDisabledPackages since its store package list, current core.disabledKeymaps indicate its store keymaps but it actually don't.

[maxbrunsfeld]

I want to vote core.keymapDisabledPackages since its store package list.

Yeah, that's a good point @t9md. It would be good to include packages in the name, since the value is a list of package names.

How about core.packagesWithDisabledKeymaps?

/cc @atom/feedback

[t9md]

I like core.packagesWithKeymapDisabled if it goes with version ;)
But I think core.keymapDisabledPackages is simple and clear enough.

[bronson]

core.packagesWithoutKeymaps? Naming things is tough.

[t9md]

Not 'without', since its originally 'with' keymap and disabled intentionally by user, so keyword 'disable' should be included to name.. but I agree naming is tough and but very important.
I frequently hit the situation that I want to rename after release.

[bronson]

core.packagesNotKeymaps :)

Gotta say, I think core.packagesWithDisabledKeymaps is the best so far. Readable and unambiguous. core.keymapDisabledPackages is shorter but awkward. It's trying to make keymap-disabled an adjective.

[maxbrunsfeld]

Maybe packagesWithKeymapsDisabled makes it a bit more clear that all of the package's keymaps are disabled (not just some).

[t9md]

Maybe reason for I don't think keymapdisabkedPackages awkward is because I'm not English native. So I will agree as long as keyword packages and disable included. 😄

[bronson]

packagesWithKeymapsDisabled 👍 It sounds a little strange but it's more descriptive.

[t9md]

👍