helix icon indicating copy to clipboard operation
helix copied to clipboard

fix: Recalculate completion when going through prompt history

Open Frojdholm opened this issue 2 years ago • 2 comments

Fixes a panic related to completion when using the arrow keys to navigate through the prompt history.

Minimal reproduction steps for the panic

<launch helix>
:asdf<Enter> <- Anything that is shorter than what you will be trying to complete next works.
:theme d <- Note that triggering completion is not necessary since it's recalculated on each character entry.
<up arrow>
<tab>

It should not be possible to modify the prompt line without also updating completion list. Most functions modifying the line update the completions. Prompt::insert_str and Prompt::change_history does not which allows this panic to happen. This means that the <c-s> shortcut probably also can create a panic and there are also possible panics in the DAP prompt as well (helix-term/src/commands/dap.rs).

This PR fixes the immediate problem, but I think I real solution should force update the completion list when updating the prompt line in some centralized way. Another solution might be to tie the completion list more tightly to the prompt line itself so that it can be recalculated if needed when completing.

Frojdholm avatar Jul 25 '22 22:07 Frojdholm

With the last two commits it's no longer possible to call a function that modifies the prompt line without also updating the completion.

The only direct calls to recalculate the completion now happen when you want to have a completion with an empty line. I think this makes sense since you might not always want to have the completion list.

Frojdholm avatar Jul 28 '22 19:07 Frojdholm

I've done some manual testing locally and it has not crashed on me yet.

Great!

I also noticed the pairing of exit_selection and recalculate_completion and am wondering if an extra (private) function to join them is warranted to keep the code short. But as the order may or may not be of importance (reversed in clear function) this may just add noise, especially since this is local to this module.

I looked into it. exit_selection only clears an index into the completion list so order here shouldn't matter. I've added a call to exit_selection in recalculate_completion since it doesn't make sense to keep the old index when the list has changed.

In the future you could try to match the previous selection into the new list if possible, but I don't think the extra complexity would be worth it.

Frojdholm avatar Jul 31 '22 11:07 Frojdholm