Fix prompt for multi-byte characters. #41
Conversation
src/prompt.rs
Outdated
| *self.value_mut() = self | ||
| .value() | ||
| .chars() | ||
| .take(self.position()) | ||
| .chain(std::iter::once(c).chain(self.value().chars().skip(self.position()))) | ||
| .collect(); |
There was a problem hiding this comment.
#The following is a bit easier to read (uses the Itertools::chain macro):
| *self.value_mut() = self | |
| .value() | |
| .chars() | |
| .take(self.position()) | |
| .chain(std::iter::once(c).chain(self.value().chars().skip(self.position()))) | |
| .collect(); | |
| *self.value_mut() = chain![ | |
| self.value().chars().take(self.position()), | |
| once(c), | |
| self.value().chars().skip(self.position()) | |
| ] | |
| .collect(); |
Also a small comment here about why this is necessary would be handy for a future reader to understand the intent.
src/text_state.rs
Outdated
| #[test] | ||
| fn multi_byte_characters() { | ||
| let mut test = TextState::new(); | ||
| test.push('Ö'); | ||
| test.push('a'); | ||
| } |
There was a problem hiding this comment.
Can you please add a couple of tests to cover insertion at the start, middle and end, and assert the expected values of each?
src/prompt.rs
Outdated
| @@ -114,23 +114,23 @@ pub trait State { | |||
|
|
|||
| fn delete(&mut self) { | |||
| let position = self.position(); | |||
| if position == self.value().len() { | |||
| if position == self.value().chars().count() { | |||
There was a problem hiding this comment.
What about extracting this to a function named len()?
|
Hi, thanks for your quick response! I addressed your review and rebased. Edit: During investigation, I just noticed they fail as well on the current main, so that's unrelated to my PR. |
Before this contribution, entering characters that consists of multiple bytes lead to undesired behaviour such as panics. This results from the position previous position handling inside the prompt. The String::len() returns the size of the string in bytes. Since a single unicode scalar value may consist of multiple bytes, this is not useful to determine the position inside the prompt. The panic occured in the State::push() method and resulted from the attempt to insert at a byte index that does not represent a character boundary. See: https://doc.rust-lang.org/stable/std/primitive.str.html#method.len https://doc.rust-lang.org/stable/std/string/struct.String.html#method.insert
I've been meaning to look at them but haven't done so yet: joshka/tui-widgets#9 |
Before this contribution, entering characters that consists of multiple bytes lead to undesired behaviour such as panics.
This results from the position previous position handling inside the prompt.
The String::len() returns the size of the string in bytes. Since a single unicode scalar value may consist of multiple bytes, this is not useful to determine the position inside the prompt. The panic occured in the State::push() method and resulted from the attempt to insert at a byte index that does not represent a character boundary.
See:
https://doc.rust-lang.org/stable/std/primitive.str.html#method.len https://doc.rust-lang.org/stable/std/string/struct.String.html#method.insert
Fixes #40