Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support macro-style keybindings #4709

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Support macro-style keybindings #4709

merged 3 commits into from
Aug 9, 2024

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Nov 11, 2022

Macro key-sequences can be used to implement custom keybinds for commands that require some sort of input like t or mi:

[keys.normal]
D = "@t<ret>d"
W = "@miw"

To implement them, we parse a sequence of KeyEvents the same way that we parse macro input, then execute them by setting up a callback which passes the KeyEvents through the compositor.

Closes #1383

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements A-keymap Area: Keymap and keybindings S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 11, 2022
@the-mikedavis

This comment was marked as outdated.

@the-mikedavis the-mikedavis marked this pull request as draft November 11, 2022 02:50
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-review Status: Awaiting review from a maintainer. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 11, 2022
@the-mikedavis the-mikedavis marked this pull request as ready for review November 11, 2022 13:39
@Omnikar
Copy link
Contributor

Omnikar commented Nov 11, 2022

The issue of static commands being executed immediately while macros are executed by the compositor afterwards doesn't seem to be resolved?
For example, given

"D" = ["find_till_char", "@<ret>", "delete_selection"]

the expected behaviour upon pressing D would be deleting to the end of the line (specifically selecting to the end of the line and then deleting), but the actual behaviour is deleting the current selection, then selecting to the end of the line. This is because delete_selection runs before the <ret> key is processed.

@the-mikedavis
Copy link
Member Author

Ah nice catch, thanks for the good example! That's a tricky case. It's partially related to #4013.

We may want to support macro keybinds initially by disallowing them in sequences: supporting macro keybinds is straightforward if we ignore sequences.

I'll push a change to fix that case (and #4013) but I feel like it might be a bit hacky: I'm adding a fake Event for the compositor to handle so that it can handle sequences that use callbacks in the correct order.

@archseer
Copy link
Member

There's also #4244

@Omnikar
Copy link
Contributor

Omnikar commented Nov 12, 2022

So the expected behaviour is happening now, good 👍. However, one small bug I've found: take the D command example. When it's executed, we get the desired behaviour of deleting to the end of the line, but a phantom D is rendered in the bottom right corner until the next keypress.

@the-mikedavis
Copy link
Member Author

There's also #4244

It looks like #4244 has a similar concept for pending commands. It fixes the problem for on-next-key callbacks but not the compositor callbacks used by macros (cx.callback).

a phantom D is rendered in the bottom right corner until the next keypress

Whoops, forgot to clear the pseudo-pending 😅. Should be fixed in the latest

@the-mikedavis the-mikedavis marked this pull request as draft November 18, 2022 01:25
@the-mikedavis the-mikedavis marked this pull request as ready for review November 18, 2022 01:49
@tmke8
Copy link

tmke8 commented Nov 22, 2022

Maybe this isn't easily doable but wouldn't it be nicer if instead of

[keys.normal]
D = "@t<ret>d"
W = "@miw"

you could write

[keys.normal]
D = ["@find_next_char <ret>", "delete_selection"]
W = "@select_textobject_inner w"

(or with some symbol other than @) because that way there doesn't need to be an existing keymapping for find_next_char and select_textobject_inner? (And also the resulting config would be a bit more robust to other changes in your keymap.)

@the-mikedavis
Copy link
Member Author

That style is also possible with this branch:

[keys.normal]
D = ["find_till_char", "@<ret>", "delete_selection"]
W = ["select_textobject_inner", "@w"]

@the-mikedavis the-mikedavis marked this pull request as draft November 27, 2022 21:18
@tmke8
Copy link

tmke8 commented Dec 7, 2022

Do you still want to get this merged? I saw you marked it as draft.

@the-mikedavis
Copy link
Member Author

There's a regression for long lists of regular commands where we don't re-render after executing the commands. The changes around that are a bit hacky anyway so I need to think about how to do this nicely (and without regressions)

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 7, 2022
@the-mikedavis the-mikedavis added this to the 24.03 milestone Mar 23, 2024
This is a temporary limitation because of the way that command sequences
are executed. Each command is currently executed back-to-back
synchronously, but macros are by design queued up for the compositor.
So macros mixed into a command sequence will behave undesirably: they
will be executed after the rest of the static and/or typable commands
in the sequence.

This is pending a larger refactor of how we handle commands.
<https://redirect.github.com/helix-editor/helix/issues/5555> has
further details and <https://redirect.github.com/helix-editor/helix/issues/4508>
discusses a similar problem faced by the command palette.
@llakala
Copy link

llakala commented Nov 18, 2024

Ah nice catch, thanks for the good example! That's a tricky case. It's partially related to #4013.

We may want to support macro keybinds initially by disallowing them in sequences: supporting macro keybinds is straightforward if we ignore sequences.

I'll push a change to fix that case (and #4013) but I feel like it might be a bit hacky: I'm adding a fake Event for the compositor to handle so that it can handle sequences that use callbacks in the correct order.

Running into this now. Any chance of a follow-up PR to get macro keybinds working in sequences?

@KiaraGrouwstra
Copy link
Contributor

could we get a new release for this feature? 🙏

@the-mikedavis
Copy link
Member Author

yeah the hope is to cut a new release either later this month or in early January as time allows

@ThinkChaos
Copy link

ThinkChaos commented Jan 5, 2025

@llakala I made an issue for tracking: #12420 - Support macros in keybinding sequences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-keymap Area: Keymap and keybindings C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet