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

fix: Recalculate completion when going through prompt history #3193

Merged
merged 5 commits into from
Aug 31, 2022
Merged

fix: Recalculate completion when going through prompt history #3193

merged 5 commits into from
Aug 31, 2022

Conversation

Frojdholm
Copy link
Contributor

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.

@sudormrfbin sudormrfbin linked an issue Jul 26, 2022 that may be closed by this pull request
It should not be possible to update the line without also updating the
completion since the completion holds an index into the line.
with_line was the last function where recalculate completion had to be
done manually. This function now also recalculates the completion so
that it's impossible to forget.
@Frojdholm
Copy link
Contributor Author

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.

Copy link
Contributor

@aikomastboom aikomastboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Frojdholm Thank you for this solution.

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

I am new to Rust.. not to programming.. and these changes look solid to me.

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.

great start on testing (for regression ;))

my 2cnts.

Keeping the selection index when the completion has been recalculated
doesn't make sense. This clears the selection automatically, removing
most needs to manually clear it.
@Frojdholm
Copy link
Contributor Author

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.

Copy link
Contributor Author

@Frojdholm Frojdholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the whole prompt state is very interconnected (line, completion list and selection for example), care must be taken when modifying it so as not to make it go out of sync.

With these changes I think the public API of Prompt should be safe such that the internal state can't go out of sync by calling any public methods.

let cursor = line.len();
self.line = line;
self.cursor = cursor;
self.recalculate_completion(editor);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reasonable approach here would be to clear the completion list instead of recalculating it, since this function is likely to be used initializing a new prompt and the consumer might not want completions to be calculated instantly.

@the-mikedavis the-mikedavis added this to the 22.08 milestone Aug 3, 2022
@@ -466,12 +466,12 @@ impl<T: Item> Picker<T> {
.map(|(index, _score)| &self.options[*index])
}

pub fn save_filter(&mut self, cx: &Context) {
pub fn save_filter(&mut self, cx: &mut Context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't need &mut?

@archseer archseer merged commit 4c9f144 into helix-editor:master Aug 31, 2022
@Frojdholm Frojdholm deleted the fix-panic-in-prompt branch August 31, 2022 16:58
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
…editor#3193)

* fix: Recalculate completion when going through prompt history

* Update completion when the prompt line is changed

It should not be possible to update the line without also updating the
completion since the completion holds an index into the line.

* Fix Prompt::with_line recalculate completion

with_line was the last function where recalculate completion had to be
done manually. This function now also recalculates the completion so
that it's impossible to forget.

* Exit selection when recalculating completion

Keeping the selection index when the completion has been recalculated
doesn't make sense. This clears the selection automatically, removing
most needs to manually clear it.

* Remove &mut on save_filter

Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
jdrst pushed a commit to jdrst/helix that referenced this pull request Sep 13, 2022
…editor#3193)

* fix: Recalculate completion when going through prompt history

* Update completion when the prompt line is changed

It should not be possible to update the line without also updating the
completion since the completion holds an index into the line.

* Fix Prompt::with_line recalculate completion

with_line was the last function where recalculate completion had to be
done manually. This function now also recalculates the completion so
that it's impossible to forget.

* Exit selection when recalculating completion

Keeping the selection index when the completion has been recalculated
doesn't make sense. This clears the selection automatically, removing
most needs to manually clear it.

* Remove &mut on save_filter

Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
herkhinah pushed a commit to herkhinah/helix that referenced this pull request Dec 11, 2022
…editor#3193)

* fix: Recalculate completion when going through prompt history

* Update completion when the prompt line is changed

It should not be possible to update the line without also updating the
completion since the completion holds an index into the line.

* Fix Prompt::with_line recalculate completion

with_line was the last function where recalculate completion had to be
done manually. This function now also recalculates the completion so
that it's impossible to forget.

* Exit selection when recalculating completion

Keeping the selection index when the completion has been recalculated
doesn't make sense. This clears the selection automatically, removing
most needs to manually clear it.

* Remove &mut on save_filter

Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panicked at 'assertion failed: self.is_char_boundary(n)'
4 participants