Allow toggling keyboard shortcuts

[wcerfgba]

What has Changed?

Closes #784

We introduce a calva.paredit.keybindingsEnabled config option and a
paredit.toggleKeybindingsEnabled command which allows toggling Paredit
keybindings on and off while preserving the current mode. The Paredit
keybindings are updated to only activate when the
paredit:keybindingsEnabled context key is true, in addition to the
existing conditions on the keymap. Finally, the Paredit StatusBar
component is updated to receive a KeyMapConfig object containing
keyMap and the new keybindingsEnabled boolean, and a new path is
added in the UI state management to handle this case as distinct from the
'none' keyMap case.

Inside the Paredit StatusBar component, I factored out the code for setting properties on the _toggleBarItem into a updateUIState fn, to indicate a separation of concerns between the keyMapConfig setter and updating the UI: previously the statements in the switch cases were updating the internal _keyMap value and also updating the _toggleBarItem. I have not touched visible setter.

Tested in VSCode 1.48.1 on Arch Linux.

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • If I am fixing the issue, I have used GitHub's fixes/closes syntax
• Created the issue I am fixing/addressing, if it was not present.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

[PEZ]

Hello. Thanks for this PR! I had to ponder this a bit to decide whether I thought it is a good idea or not. My default reaction to new settings is that they are bad. 😄 (Only half joking here, as @bpringe can tell you). Anyhow, I've decided that this might actually be a good thing to support: To quickly stove away some troublesome shortcuts and then switch them back back in again. I'll let @bpringe chime in as well.

Here are some thoughts:

I wonder about whether this really is a paredit thing, or rather a Calva in general thing. The problem is that some of Calva's shortcuts are overriding shortcuts that someone might be used to using for something else, right? That it is Calva Paredit that register most of those is due to the fact that Paredit registers the bulk of Calva's shortcuts, and that some of the Paredit shortcuts are of the ”simple” type, with high risk for conflict. So maybe we should name the option ”calva keyboard shortcuts enabled” (or some such) and let the command toggle them all?

Also the current name makes it sound a bit like you are enabling/disabling paredit, which you are not, just the shortcuts. (I have both glanced at the code changes and tried it, so I know that Paredit is still enabled when this option is disabling the shortcuts 😄).

When toggling the shortcuts off, the paredit button in the status bar shows this nicely. But I can't toggle it back on using that button, which seems a bit odd to me. But I also see how this button isn't used to disable the shortcuts, so it makes sense too. 😄 Your thoughts on this, @wcerfgba and @bpringe ?

As for the code changes, I think they look clean and also improves the code a bit, Kaizen style.

[wcerfgba]

Hi @PEZ , thanks for your detailed reply to my PR. 😃 I wrote this just because it is something I wanted and this solution solves my own use case, but I'm happy to work with you and others to revise this to make something that is more useful for other users. 😄

So maybe we should name the option ”calva keyboard shortcuts enabled” (or some such) and let the command toggle them all?

This sounds like a good idea. For me, I haven't found any other conflicts with any other features in Calva, the Paredit shortcuts are the big one for me, but this idea is more in line with the intention of the PR. I guess there are some edge cases where a user may want to still eg boot a REPL, evaluate a form, or format a form, even though they want to disable the Paredit keybindings, but I am not sure how common this would be, and I think it would make more sense to make all of the Calva shortcuts modal to start, and then more granular toggling of keybindings for specific features could be introduced if there were requests for this?

Also the current name makes it sound a bit like you are enabling/disabling paredit, which you are not, just the shortcuts.

Great spot, how about "Toggle Paredit Keyboard Shortcuts" or "Toggle Paredit Keybindings" (or "Toggle Calva Keyboard Shortcuts" or "Toggle Calva Keybindings" subject to earlier discussion on modality)?

When toggling the shortcuts off, the paredit button in the status bar shows this nicely. But I can't toggle it back on using that button, which seems a bit odd to me.

Yes, I did not want to overload or change the behaviour of this button in the PR, only provide a path in the code to render the UI differently for this case as distinct from the 'keyMap none' case, and make it somewhat obvious what is happening via the tooltip -- ideally we could colour the lambda differently or use a different symbol or something to indicate that the keyboard shortcuts are disabled. If we are going to change this to be a toggle for all keyboard shortcuts, we could add another separate toggle/indicator to the main statusbar component, to sit alongside the prettyPrintToggle?

As for the code changes, I think they look clean and also improves the code a bit, Kaizen style.

Thanks! 😺 If we decide to generalise this change to cover all keyboard shortcuts I can include a separate commit which keeps the extracted updateUIState fn in Paredit statusbar but preserves the original behaviour. 🤘

[bpringe]

Just a note here, the current docs are now in the calva-docs repo, and should be updated there. We need to see about removing these old docs so as not to cause confusion.

[wcerfgba]

Noted, I will remove this change from the PR. I can submit another PR if you like which deletes all the docs and updates the PR template to point people towards calva-docs instead if you'd like?

[bpringe]

Thanks for taking the time to make this PR. It's well done. I do agree with @PEZ that it should reflect that it's disabling/enabling the keybindings and not paredit itself. I also think it's best to keep it at just the paredit shortcuts, so the user can keep them off if desired and still do things like jack-in and evaluate forms.

Is it best to have a toggle button for this in the status bar? I wonder the frequency at which users would actually need to toggle it. If infrequent, then it makes sense to just have it as a checkbox in settings. If you find yourself wanting to toggle it often, then the status bar button makes sense.

[wcerfgba]

I do agree with @PEZ that it should reflect that it's disabling/enabling the keybindings and not paredit itself.

Roger, will update. 👍

I also think it's best to keep it at just the paredit shortcuts, so the user can keep them off if desired and still do things like jack-in and evaluate forms.

Interesting. I am happy to leave it as it is, since this is fine for my workflow (and means I don't have to change the PR too much 😛 ) so long as you are both happy with it from a philosophical and UX perspective. 😃

Is it best to have a toggle button for this in the status bar? I wonder the frequency at which users would actually need to toggle it. If infrequent, then it makes sense to just have it as a checkbox in settings. If you find yourself wanting to toggle it often, then the status bar button makes sense.

Personally I want to toggle it often and easily, so I would use the keybinding I assigned to the command, rather than clicking a statusbar button, but I think it is nice to provide a visual indication of the modality -- cf vi with no indication of insert/command mode and vim where it indicates --INSERT-- in the statusbar. 😃

[PEZ]

I also think it's best to keep it at just the paredit shortcuts, so the user can keep them off if desired and still do things like jack-in and evaluate forms.

Interesting. I am happy to leave it as it is, since this is fine for my workflow (and means I don't have to change the PR too much 😛 ) so long as you are both happy with it from a philosophical and UX perspective. 😃

Yeah interesting. 😄 If I understand the use case correctly this is to very temporarily move shadowing shortcuts away, while the user does a few things and then they would be switched back again. The thing is, we don't know which shortcuts are in the way. Maybe some extension uses ctrl+enter or alt+enter for something? Then moving just the paredit shortcuts away wouldn't do the trick. All or none, is my take on it.

Is it best to have a toggle button for this in the status bar? I wonder the frequency at which users would actually need to toggle it. If infrequent, then it makes sense to just have it as a checkbox in settings. If you find yourself wanting to toggle it often, then the status bar button makes sense.

Personally I want to toggle it often and easily, so I would use the keybinding I assigned to the command, rather than clicking a statusbar button, but I think it is nice to provide a visual indication of the modality -- cf vi with no indication of insert/command mode and vim where it indicates --INSERT-- in the statusbar. 😃

I think this is not a very common need. Giving it a place of its own in the status bar is a bit too much. A keyboard shortcut is perfect. It was just that the paredit mode icon reflected the disablement that got me to want to use it to toggle back. I think it just leaving that icon displaying the keymap info is OK. Grey it out, but keep it toggling between keymaps.

[bpringe]

The thing is, we don't know which shortcuts are in the way. Maybe some extension uses ctrl+enter or alt+enter for something? Then moving just the paredit shortcuts away wouldn't do the trick. All or none, is my take on it.

I see your point here, and I agree. All Calva shortcuts or nothing is best. @wcerfgba Sorry this increases your work for this PR, but I think it's best overall for users of Calva.

Giving it a place of its own in the status bar is a bit too much. A keyboard shortcut is perfect. It was just that the paredit mode icon reflected the disablement that got me to want to use it to toggle back. I think it just leaving that icon displaying the keymap info is OK. Grey it out, but keep it toggling between keymaps.

I agree here too. Do you think it would also be wise to add something to the hover tooltip? Like now it might say Toggle to original mode but it could also say Toggle to original mode (paredit shortcuts are off), or something when they're off, and maybe also say ... are on when on. Or maybe this gray/color thing should only be mentioned in the docs. I'm not sure.

[bpringe]

As per the discussion. Let's make this toggle all Calva shortcuts, and not just for paredit. Again, thanks for your work on this.

[wcerfgba]

Closing in favour of #796 , I changed the branch name on my fork. 👍