Behavior choice: Helix keymap #338

Strackeror · 2024-07-02T12:07:10Z

Hey there !

So I've been maintaining a personal use fork of dance with helix keybindings for a while now, but I've been seeing some of the keybinds making their way into the original codebase (with plans for more in #323), so I thought I'd just go ahead and try to remake it in a way that could fit into the main project.

So, here is a draft for a 'behavior' choice, where a user could choose which base keymap to use from a single setting. This is currently just the basic normal and select mode keymaps, without any of the helix menus, but it's a good starting point to show how it would look like in practice.

Currently the handling in the commands file is pretty naive, with a lot of duplication for the select mode. And I'm not sure on the backward compatibility when adding a new condition to a lot of keybindings like that, so I'm very open to discussion.

71 · 2024-07-07T02:58:47Z

Hey there, and thanks for the PR! I haven't looked at everything, but I overall agree with the idea!

I don't want to let the perfect be the enemy of the good, but one desire that I had when thinking about letting users switch between Kakoune/Dance/Helix keybindings is supporting multiple keymaps (e.g. non-QWERTY, macOS). Given the large number of keybindings in Dance I would have preferred to change the user's keybindings.json file automatically or to distribute alternative keybindings as extensions, though nothing beats the "behavior" as introduced by your PR in terms of convenience, so for now I'm perfectly fine with keeping things as-is.

Anyway, it is still good to have a "library" of Helix-specific keybindings; even if the way these keybindings are chosen changes in the future, such a change would be easy to automate following this change. But for now I think I would prefer marking the "behavior" setting as "experimental".

I'm also wondering if there might be an overlap between custom modes and behaviors (e.g. maybe instead of behaviors, we could have helix/normal and kakoune/normal modes).

And I'm not sure on the backward compatibility when adding a new condition to a lot of keybindings like that, so I'm very open to discussion.

This is a good point, I will think about it as well.

Strackeror · 2024-07-08T08:09:42Z

I... actually didn't think of generating a new extension with just keybinding and settings. I'll explore that idea, it feels like it'd be a better way to organize and avoid bloating the package.json further.

I'm not a fan of editing keybindings.json, mostly because I like to keep the vscode config files somewhat simple, it can very easily become a mess to manage with settings sync, profiles, the different configuration levels... It's a big reason I kept maintaining an actual fork of dance instead of just config files :)

The more I think about it, the more I think just having 'namespaced' helix modes and keeping the current kakoune modes as default is probably the least likely to break anything, and also the most simple to handle. Makes it a bit harder to show it as 'experimental' though, since you'd just change the default mode.

Strackeror · 2024-07-10T12:31:02Z

Okay, so, turns out making a basic extension with a bunch of keybinds and setting overrides was pretty easy !

It seems to work pretty well, with a couple caveats :

The extension contribute setting for overriding default configurations can't merge or do anything complex, which leads to some duplication between the package builders. I didn't think it was worth factoring that duplication yet, but my mind could easily be changed.
To avoid having to ignore all the keybinds in the base extension, I went ahead with the namespaced modes idea. It's currently very basic and just some basic string replacement when switching modes and generating keybinds, so that could probably be improved, but i'm not sure on the exact way yet.

Apart from that, I like it a lot better as a starting point. It's a bit barebones and missing some helix behaviors, but it's probably a better idea to defer those to separate PRs.

Remove category field from package.json

71

Thanks a lot! Sorry again for the delay.

I haven't looked at everything though I did skim over the files (and left a few comments, though they're primarily nits / code style).

Seeing the helix: select commands inline like that while knowing that they won't "bloat" the extension is pretty cool!

I think I'm mostly worried about code duplication; I think that I'd like to get to a point where Dance is Dance "Core" + Dance "Kakoune" (or Dance "Helix"), so we would benefit from reusing the same code/assets/package.json build machinery for all extensions.

.vscode/launch.json

71 · 2024-07-18T13:44:54Z

package.build.ts

@@ -22,7 +22,7 @@ const builtinModesAreDeprecatedMessage =
  "Built-in modes are deprecated. Use `#dance.modes#` instead.";

 const modeNamePattern = {
-  pattern: /^[a-zA-Z]\w*$/.source,
+  pattern: /^[a-zA-Z]\w*\/?\w*$/.source,


Suggested change

pattern: /^[a-zA-Z]\w*\/?\w*$/.source,

pattern: /^[a-zA-Z]\w*(\/\w+)$/.source,

Wait, doesn't that regex force having a slash somewhere ? I think a ? is missing after the group maybe ?

package.build.ts

71 · 2024-07-18T13:46:13Z

meta.ts

@@ -655,6 +653,36 @@ function getKeybindings(module: Omit<Builder.ParsedModule, "keybindings">) {
  ].sort((a, b) => a.command.localeCompare(b.command));
 }

+export function generateIgnoredKeybinds(


nit: spelling

Suggested change

export function generateIgnoredKeybinds(

export function generateIgnoredKeybindings(

Could you also add a documentation string? By looking at the source code I can fairly easily know what it does, but the name itself isn't enough to understand what is returned.

meta.ts

extensions/helix/package.build.ts

71 · 2024-07-18T14:13:32Z

extensions/helix/package.build.ts

+        "": {
+          hiddenSelectionsIndicatorsDecoration: {
+            after: {
+              color: "$list.warningForeground",
+            },
+            backgroundColor: "$inputValidation.warningBackground",
+            borderColor: "$inputValidation.warningBorder",
+            borderStyle: "solid",
+            borderWidth: "1px",
+            isWholeLine: true,
+          },
+        },
+        "input": {
+          cursorStyle: "underline-thin",
+        },


If these two entries are omitted does Dance stop working normally?

From quick testing, it doesn't, I just copy pasted the base settings because I wasn't sure what to with those.

Strackeror · 2024-07-18T18:50:56Z

Thanks a lot! Sorry again for the delay.

I haven't looked at everything though I did skim over the files (and left a few comments, though they're primarily nits / code style).

Seeing the helix: select commands inline like that while knowing that they won't "bloat" the extension is pretty cool!

I think I'm mostly worried about code duplication; I think that I'd like to get to a point where Dance is Dance "Core" + Dance "Kakoune" (or Dance "Helix"), so we would benefit from reusing the same code/assets/package.json build machinery for all extensions.

Yah, no worries, there's no hurry, and this thing's become a sizeable PR 😅.

I'll have to think about how to deduplicate the package generating code, I'm not sure exactly which part would benefit from abstracting yet.
I'm frankly not sure what a 'Core' dance would include. Right now in the keybind definitions, "core" more of a catchall for helix+kakoune. Using it like that would result in this very weird and incomplete intersection of the two, and I don't think that's the ideal way to go.

71 · 2024-07-20T05:02:22Z

I'll have to think about how to deduplicate the package generating code

If you'd like I can take care of this; we could submit your PR soon (either to master or to some other branch, releasing the Helix extension as "pre-release" only or waiting to release it) and I'll look into deduplicating some of the stuff.

I'm frankly not sure what a 'Core' dance would include

For me, it would include all the Dance commands and API, but none of the keybindings. That way you would install the "Kakoune keybindings" or "Helix keybindings", both of which would depend on "Dance Core", and you would only get the keybindings you need.

With this model, I guess we would create a new extension "Dance Core", and migrate the current extension to be "Dance with Kakoune keybindings".

Right now in the keybind definitions, "core" more of a catchall for helix+kakoune.

"Dance Core" (the extension) would be different from core keybindings; core keybindings are more "common" or "shared" keybindings which would be included in both extensions.

Strackeror · 2024-07-20T10:45:34Z

If you'd like I can take care of this; we could submit your PR soon (either to master or to some other branch, releasing the Helix extension as "pre-release" only or waiting to release it) and I'll look into deduplicating some of the stuff.

I'd be okay with that, if that works for you ! It'll also allow me to submit the couple other helix-related PRs I'm working on :)

"Dance Core" (the extension) would be different from core keybindings; core keybindings are more "common" or "shared" keybindings which would be included in both extensions.

Ah I see, that makes a lot more sense when thinking of it this way. Might make sense to rename the 'core' category to avoid confusion in that case, but that's a small issue.

71

Sorry, I again did not much time to review until now.

I tried it out and it's nice! I opened a PR in Strackeror#1 to work on reusing some of the logic.

71 · 2024-08-16T14:29:38Z

src/commands/misc.ts

- * | Add the digit 9 to the counter | `9` (core: normal), `NumPad9` (core: normal) | `[".updateCount", { addDigits: 9 }]` |
+ * | Title                          | Keybinding                                                                                   | Command                              |
+ * | ------------------------------ | -------------------------------------------------------------------------------------------- | ------------------------------------ |
+ * | Add the digit 0 to the counter | `0` (core: normal), `NumPad0` (core: normal), `0` (helix: select), `NumPad0` (helix: select) | `[".updateCount", { addDigits: 0 }]` |


Does (core: normal, helix: select) not work? I don't remember if I implemented this, but I can see several keybindings in your PR (and even from before your PR) which could benefit from such a syntax.

As of right now it doesn't (I would have felt like quite an idiot if it did !). But it actually sounds like a good idea, so I'll see if I can implement that, it would help make things less verbose !

Well, I did it. It's separated by ; instead of , because otherwise the regex would conflict with the one for tags, but it seems to work quite well

src/api/modes.ts

package.build.ts

It's not consistent, but vscode sometimes gives me errors about the semver for the helix package not being valid, and indeed it seems leading zeroes are not valid semver, so switching to a tag to mark the prerelease

akdor1154 · 2024-11-05T02:30:23Z

hey guys, where's this sitting? any way an excited bystander can help get this over the line? :)

Strackeror force-pushed the choose-behavior branch from 41851a8 to cc565bc Compare July 3, 2024 23:04

Behavior choice: Basic helix keymap

a5c3e7d

Strackeror force-pushed the choose-behavior branch from cc565bc to a5c3e7d Compare July 3, 2024 23:07

Strackeror force-pushed the choose-behavior branch 2 times, most recently from 6c0c8bd to 1916ec6 Compare July 10, 2024 12:22

Strackeror marked this pull request as ready for review July 10, 2024 12:40

Behavior choice: Helix keymap as an extension

7baaa30

Strackeror force-pushed the choose-behavior branch from 1916ec6 to 7baaa30 Compare July 10, 2024 12:43

Strackeror added 4 commits July 15, 2024 15:43

Fix shift+c in helix modes

7e1a759

Remove category field from package.json

Fix missing editorTextFocus condition

8babb7f

Add % binding to helix

1b71b25

Fix binding: Shift+R

91bc9d6

71 reviewed Jul 18, 2024

View reviewed changes

Strackeror force-pushed the choose-behavior branch from 415cac9 to 438baf3 Compare July 18, 2024 16:50

PR Fixes

10e50a9

Strackeror force-pushed the choose-behavior branch from 438baf3 to 10e50a9 Compare July 18, 2024 16:53

Strackeror added 2 commits July 31, 2024 13:37

Fix typos and title, add toggle indices

a1f82a7

Add numpad bindings to select mode

38be042

71 reviewed Aug 16, 2024

View reviewed changes

Strackeror and others added 4 commits August 17, 2024 17:37

PR Fixes

0887662

Improve keybind parsing for less verbose definitions

47c2737

use symlinks

77882fd

Fix invalid pre-release version string

e908946

It's not consistent, but vscode sometimes gives me errors about the semver for the helix package not being valid, and indeed it seems leading zeroes are not valid semver, so switching to a tag to mark the prerelease

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Behavior choice: Helix keymap #338

Behavior choice: Helix keymap #338

Strackeror commented Jul 2, 2024 •

edited

Loading

71 commented Jul 7, 2024

Strackeror commented Jul 8, 2024 •

edited

Loading

Strackeror commented Jul 10, 2024 •

edited

Loading

71 left a comment

71 Jul 18, 2024

Strackeror Jul 18, 2024 •

edited

Loading

71 Jul 18, 2024

71 Jul 18, 2024

71 Jul 18, 2024

Strackeror Jul 18, 2024

Strackeror commented Jul 18, 2024

71 commented Jul 20, 2024 •

edited

Loading

Strackeror commented Jul 20, 2024

71 left a comment

71 Aug 16, 2024

Strackeror Aug 17, 2024

Strackeror Aug 17, 2024

akdor1154 commented Nov 5, 2024

	pattern: /^[a-zA-Z]\w\/?\w$/.source,
	pattern: /^[a-zA-Z]\w*(\/\w+)$/.source,

	export function generateIgnoredKeybinds(
	export function generateIgnoredKeybindings(

Behavior choice: Helix keymap #338

Are you sure you want to change the base?

Behavior choice: Helix keymap #338

Conversation

Strackeror commented Jul 2, 2024 • edited Loading

71 commented Jul 7, 2024

Strackeror commented Jul 8, 2024 • edited Loading

Strackeror commented Jul 10, 2024 • edited Loading

71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strackeror Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strackeror commented Jul 18, 2024

71 commented Jul 20, 2024 • edited Loading

Strackeror commented Jul 20, 2024

71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akdor1154 commented Nov 5, 2024

Strackeror commented Jul 2, 2024 •

edited

Loading

Strackeror commented Jul 8, 2024 •

edited

Loading

Strackeror commented Jul 10, 2024 •

edited

Loading

Strackeror Jul 18, 2024 •

edited

Loading

71 commented Jul 20, 2024 •

edited

Loading