fix(helix-term): Defer mutation of editor config to the application

[khutchens]

Description

This is a proposed fix for #13187.

Commits

There are 2 commits in this PR.

997da16: Add integration tests to reproduce multi-command key binding bug

This commit adds 4 integration tests, one for each of the following scenarios:

• Binding a key to a single :set command.
• Binding a key to multiple :set commands.
• Binding a key to a single :toggle command.
• Binding a key to multiple :toggle commands.

a4eefb1: Defer mutation of editor config to the application

This commit reworks how the :set and :toggle commands communicate config changes to the application. In the existing design, these commands do the following:

1. Load the current config.
2. Determine the path to the target field according to the provided key argument.
3. Mutate the loaded config according to the provided value argument.
4. Pass the new mutated config to the application.
5. The application then overwrites its entire config with the new config.

The bug arises when multiple commands are mapped to a single key. In this case, the commands are executed serially. When each command is executed it loads the config, mutates it, and passes the result to the application. Because all of the bound commands execute before the results from any of them are applied, they all load the same config rather than each command loading the result from the previous command. This means the result of each command gets clobbered by the command following it and only the final command result actually gets applied.

In the new design, the commands do the following:

1. Load the current config.
2. Determine the path to the target field according to the provided key argument.
3. Determine the new value that should be applied.
4. Pass the path and new value to the application.
5. The application then loads the config, mutates it, and stores the changes.

The key difference is that the command is only passing a path and value to the application, not an entire config. This avoids the command capturing stale values for fields that it's not actually operating on.

This does result in some duplicated work since both the command and application need to load the config, convert it to a serde_json::Value, etc. I think this is desirable because it keeps the behavior for each command in it's command handler. To deduplicate this work, we'd have to pass the raw arguments to the application, and then the command logic would need to move to the application as well. This logic is pretty trivial for :set but it's somewhat complex for :toggle. Moving that logic to the application seems like it would be confusing and generally less than ideal.

Testing

Integration Tests

I tested this by first adding a set of integration tests to reproduce the issue. Running these tests at 997da16 (prior to the fix) I get the following result:

running 4 tests
test test::commands::typed::test_toggle_config_single ... ok
test test::commands::typed::test_set_config_single ... ok
test test::commands::typed::test_set_config_multi ... FAILED
test test::commands::typed::test_toggle_config_multi ... FAILED

Running these tests again at a4eefb1 (with the fix) I get:

running 4 tests
test test::commands::typed::test_set_config_single ... ok
test test::commands::typed::test_toggle_config_single ... ok
test test::commands::typed::test_toggle_config_multi ... ok
test test::commands::typed::test_set_config_multi ... ok

Manual Testing

I also verified the behavior is as expected when running the new build interactively.

I have the following snippet in my config:

[keys.normal]
z.d = [
  ":set end-of-line-diagnostics hint",
  ":set inline-diagnostics.other-lines hint",
  ":set inline-diagnostics.cursor-line hint",
]
z.n = [
  ":set end-of-line-diagnostics disable",
  ":set inline-diagnostics.other-lines disable",
  ":set inline-diagnostics.cursor-line disable",
]

Prior to this fix (and the reason I found the original bug report) these two bindings would only change the inline-diagnostics.cursor-line setting. With the fix, all three settings are adjusted as expected in both cases.