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 panic in fuzzy-select when using non-ASCII characters #245

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Fix panic in fuzzy-select when using non-ASCII characters #245

merged 3 commits into from
Sep 12, 2023

Conversation

stormshield-kg
Copy link
Contributor

@stormshield-kg stormshield-kg commented Feb 22, 2023

Fixes #181.

Use the str::char_indices() to keep valid byte offsets in memory.

Also add handling of the Delete key.

@Gordon01
Copy link
Contributor

Gordon01 commented Sep 5, 2023

I tested this PR and it indeed fixes the problem with non-ASCII characters.

However, I believe that format_fuzzy_select_prompt() may be the only one that needs to be fixed to correctly support UTF-8.
Specifically replace indexing with .chars() method as described here: https://stackoverflow.com/questions/48642342/how-to-get-the-last-character-of-a-str

Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR

@pksunkara pksunkara requested a review from Gordon01 September 6, 2023 21:44
@Gordon01
Copy link
Contributor

Gordon01 commented Sep 8, 2023

I believe we don't need an optimization for search_term indexes.
It's small (will fit into the L1 cache) so recalculating it in a loop will not affect performance much.

Details

diff --git a/src/prompts/fuzzy_select.rs b/src/prompts/fuzzy_select.rs
index 39824be..3f83947 100644
--- a/src/prompts/fuzzy_select.rs
+++ b/src/prompts/fuzzy_select.rs
@@ -195,18 +195,9 @@ impl FuzzySelect<'_> {
 
     fn _interact_on(self, term: &Term, allow_quit: bool) -> Result<Option<usize>> {
         // Place cursor at the end of the search term
-        let mut bytes_position = self.initial_text.len();
+        let mut cursor = self.initial_text.chars().count();
         let mut search_term = self.initial_text.to_owned();
 
-        let mut position_indices = search_term
-            .char_indices()
-            .map(|(index, _)| index)
-            .collect::<Vec<_>>();
-
-        position_indices.push(search_term.len());
-
-        let mut char_index = position_indices.len() - 1;
-
         let mut render = TermThemeRenderer::new(term, self.theme);
         let mut sel = self.default;
 
@@ -233,8 +224,14 @@ impl FuzzySelect<'_> {
         let mut vim_mode = false;
 
         loop {
+            let mut byte_indices = search_term
+                .char_indices()
+                .map(|(index, _)| index)
+                .collect::<Vec<_>>();
+            byte_indices.push(search_term.len());
+
             render.clear()?;
-            render.fuzzy_select_prompt(self.prompt.as_str(), &search_term, bytes_position)?;
+            render.fuzzy_select_prompt(self.prompt.as_str(), &search_term, byte_indices[cursor])?;
 
             // Maps all items to a tuple of item and its match score.
             let mut filtered_list = self
@@ -313,16 +310,14 @@ impl FuzzySelect<'_> {
                     }
                     term.flush()?;
                 }
-                (Key::ArrowLeft, _, _) | (Key::Char('h'), _, true) if char_index > 0 => {
-                    char_index -= 1;
-                    bytes_position = position_indices[char_index];
+                (Key::ArrowLeft, _, _) | (Key::Char('h'), _, true) if cursor > 0 => {
+                    cursor -= 1;
                     term.flush()?;
                 }
                 (Key::ArrowRight, _, _) | (Key::Char('l'), _, true)
-                    if char_index < position_indices.len() - 1 =>
+                    if cursor < byte_indices.len() - 1 =>
                 {
-                    char_index += 1;
-                    bytes_position = position_indices[char_index];
+                    cursor += 1;
                     term.flush()?;
                 }
                 (Key::Enter, Some(sel), _) if !filtered_list.is_empty() => {
@@ -342,44 +337,18 @@ impl FuzzySelect<'_> {
                     term.show_cursor()?;
                     return Ok(sel_string_pos_in_items);
                 }
-                (Key::Backspace, _, _) if char_index > 0 => {
-                    let old_position = bytes_position;
-
-                    char_index -= 1;
-                    bytes_position = position_indices[char_index];
-
-                    let char_len = old_position - bytes_position;
-                    position_indices.remove(char_index);
-                    for index in &mut position_indices[char_index..] {
-                        *index -= char_len;
-                    }
-
-                    search_term.replace_range(bytes_position..old_position, "");
+                (Key::Backspace, _, _) if cursor > 0 => {
+                    cursor -= 1;
+                    search_term.remove(byte_indices[cursor]);
                     term.flush()?;
                 }
-                (Key::Del, _, _) if char_index < position_indices.len() - 1 => {
-                    let char_len = position_indices[char_index + 1] - bytes_position;
-                    position_indices.remove(char_index + 1);
-
-                    for index in &mut position_indices[char_index + 1..] {
-                        *index -= char_len;
-                    }
-
-                    search_term.replace_range(bytes_position..bytes_position + char_len, "");
+                (Key::Del, _, _) if cursor < byte_indices.len() - 1 => {
+                    search_term.remove(byte_indices[cursor]);
                     term.flush()?;
                 }
                 (Key::Char(chr), _, _) if !chr.is_ascii_control() => {
-                    let char_len = chr.len_utf8();
-                    let old_position = bytes_position;
-                    bytes_position += char_len;
-                    position_indices.insert(char_index, old_position);
-
-                    char_index += 1;
-                    for index in &mut position_indices[char_index..] {
-                        *index += char_len;
-                    }
-
-                    search_term.insert(old_position, chr);
+                    search_term.insert(byte_indices[cursor], chr);
+                    cursor += 1;
                     term.flush()?;
                     sel = Some(0);
                     starting_row = 0;

Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

Please consider the attached diff.

Also, please update the changelog.

src/prompts/fuzzy_select.rs Outdated Show resolved Hide resolved
src/theme/colorful.rs Outdated Show resolved Hide resolved
@Gordon01
Copy link
Contributor

Gordon01 commented Sep 8, 2023

Just realized that Input builds a Vec of chars. That's also a way for code simplification. https://github.com/console-rs/dialoguer/blob/master/src/prompts/input.rs#L321

@Gordon01
Copy link
Contributor

Gordon01 commented Sep 9, 2023

Let's add Delete key handling to Input too, since it also lacks this functionality.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@pksunkara please allow CI to run

Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

LGTM

src/prompts/fuzzy_select.rs Show resolved Hide resolved
src/prompts/fuzzy_select.rs Show resolved Hide resolved
src/theme/mod.rs Show resolved Hide resolved
@pksunkara pksunkara merged commit ac365d8 into console-rs:master Sep 12, 2023
7 checks passed
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.

Fuzzy Select: Special characters lead to panic
3 participants