bobjunga
commented
4 years ago Sorry about all the extraneous commits. It took me a while to find 'npm run lint' (I did try a lot of 'apm ..' commands)
I would clean it up but I dont see a way to change the branch once a PR is created and I would think rebasing the existing branch would be bad.
If you point me to instructions I would be happy to clean it up.
Description of the Change
In PackageKeymapView::updateKeyBindingView in the loop that creates the table element with rows for each of the packages keybings, I added a block that checks to see if the keybinding is currently in atom.keymaps.keyBindings[] and adds decorators to the row elements if it is not.
Decorators on Disabled Keybinding rows:
The result is ...
I added two specs -- one to test that enabled keybindings do not have the class decorators and another to test that a disabled keybinding does. I did two specs so that I could reuse an existing fixture which seemed less intrusive.
Alternate Designs
This just makes the package keymap section of settings view tell the current truth about the state of the package's keybindings. I can not think of any alternatives.
Benefits
There are already packages like 'disable-keybindings' (8k downloads) that allow the user selectively disable just some of the keybindings that a package provides but the settings view does not reflect the correct state when this happens. It decreases confusion when the user has a more accurate picture of the system state.
Possible Drawbacks
I think that this change is safe because it does not introduce any new behavior itself (you may support or be against the idea of disabling keybindings selectively at runtime) but if for any reason a keybinding is not active, this allows the user to see that its not active.
The user may be left wondering why the keybinding is disabled but addressing that would be a step further. Compared to the settings view making it look like all the package's keybindings are enabled when they are not, the user is better off.
We could go further and add a KeymapManager::remove(reason, filterCallback) API. Then the PackageKeymapView::updateKeyBindingView code could display the reason a keybinding is disabled. That would provide a path to good citizenship for the disable-keybindings and similar packages that modify atom.keymap.keyBindings directly when really they should not be doing that.
LMK if you would like me to add that to this PR.
Applicable Issues
None known. (BTW, I was not sure if I should go straight to a PR or if I should have created a feature request issue first. LMK)