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.

Sign up for GitHub

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 when pressing Shift + TAB in a TextEdit #3878

Closed
wants to merge 6 commits into from
Closed

Fix: panic when pressing Shift + TAB in a TextEdit #3878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b0b842f
Fix: panic when pressing Shift + TAB in a TextEdit
rustbasic Jan 24, 2024
3b8fe47
Update text_cursor_state.rs
rustbasic Jan 24, 2024
7812bbc
Merge branch 'emilk:master' into patch2
rustbasic Jan 25, 2024
a493bf4
Update text_cursor_state.rs
rustbasic Jan 26, 2024
edc227a
Merge branch 'emilk:master' into patch2
rustbasic Feb 5, 2024
a82763f
Merge branch 'emilk:master' into patch2
rustbasic Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions crates/egui/src/text_selection/text_cursor_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,18 +302,42 @@ pub fn find_line_start(text: &str, current_index: CCursor) -> CCursor {
// number of multi byte chars.
// We need to know the char index to be able to correctly set the cursor
// later.
let chars_count = text.chars().count();
let mut char_line_indexes: Vec<usize> = create_char_line_indexes(text);
let line_start_index = get_line_start_index(&mut char_line_indexes, current_index.index);

let position = text
.chars()
.rev()
.skip(chars_count - current_index.index)
Copy link
Owner

Choose a reason for hiding this comment

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

What was wrong with the old code?

Maybe it just needs to handle the char_count < current_index.index case?

Copy link
Contributor Author

@rustbasic rustbasic Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since UTF-8 characters use 1 to 4 bytes,
using rev() can cause a panic and terminate the program.
You can test it by copying various UTF-8 characters
and using them in multiline().

Copy link
Owner

Choose a reason for hiding this comment

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

.rev() doesn't panic: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1647ea03b9ad85c69d72ee2189b73fc0

Copy link
Contributor Author

@rustbasic rustbasic Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mix the UTF8 characters with English, fill the screen ( with them using multiline(), egui_demo_app, web demo ), and pressing Shift + TAB here and there and here and there.

Please let me know if you can't find the panic occurrence.
Let me explain it to you again.

Copy link
Contributor Author

@rustbasic rustbasic Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's sample TEXT.
pressing Shift + TAB here and there and here and there.

    ABC äö 日本語 ABC äö 日本語                                                         
        ABC äö 日本語 ABC äö 日本語
               ABC äö 日本語 ABC äö 日本語
    ABC äö 日本語 ABC äö 日本語                                                         
        ABC äö 日本語 ABC äö 日本語
               ABC äö 日本語 ABC äö 日本語
    ABC äö 日本語 ABC äö 日本語                                                         
        ABC äö 日本語 ABC äö 日本語
               ABC äö 日本語 ABC äö 日本語

Copy link
Contributor Author

@rustbasic rustbasic Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are two more instances of rev() in text_cursor_state.rs,
fortunately, those two do not cause panics
but instead incorrectly select a block where UTF8 characters exist.
I will register this as a separate issue later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilk
Have you noticed the panic occurring?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't have time for this right now. I believe you that here is a panic in egui, but I don't think the panic is in the call to .rev(), and so I think a much simpler solution to your problem can be found.

Run in debug mode and RUST_BACKTRACE=1 and paste the stack-trace to the actual panic.

.position(|x| x == '\n');
CCursor::new(line_start_index)
}

pub fn create_char_line_indexes(text: &str) -> Vec<usize> {
Copy link
Owner

Choose a reason for hiding this comment

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

If you use CCursor instead of usize, the code becomes self-documenting

Suggested change
pub fn create_char_line_indexes(text: &str) -> Vec<usize> {
pub fn create_char_line_indexes(text: &str) -> Vec<CCursor> {

Copy link
Contributor Author

@rustbasic rustbasic Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If CCursor is used instead of usize,
it seems handling functions might become difficult,
so I'm not sure if it's a good approach.
I will respect your final opinion.

By the way, the create_char_line_indexes() function is also used
in the next Pull Request related to TAB.

let mut line_indexes = vec![0];

match position {
Some(pos) => CCursor::new(current_index.index - pos),
None => CCursor::new(0),
let mut count = 0;
for c in text.chars() {
count += 1;
if c == '\n' {
line_indexes.push(count);
}
}
line_indexes.push(count);

line_indexes
}

pub fn get_line_start_index(line_indexes: &mut [usize], index: usize) -> usize {
let (_current_line, line_start) = get_line_and_start_index(line_indexes, index);
line_start
}

pub fn get_line_and_start_index(line_indexes: &mut [usize], index: usize) -> (usize, usize) {
let mut current_line = 1;
for (i, line_start) in line_indexes.iter().enumerate() {
current_line = i;
if *line_start > index {
break;
}
}

(current_line, line_indexes[current_line - 1])
}

pub fn byte_index_from_char_index(s: &str, char_index: usize) -> usize {
Expand Down
6 changes: 4 additions & 2 deletions crates/egui/src/widgets/text_edit/text_buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,12 @@ pub trait TextBuffer {
fn decrease_indentation(&mut self, ccursor: &mut CCursor) {
let line_start = find_line_start(self.as_str(), *ccursor);

let remove_len = if self.as_str()[line_start.index..].starts_with('\t') {
let remove_len = if self.as_str().chars().nth(line_start.index) == Some('\t') {
Some(1)
} else if self.as_str()[line_start.index..]
} else if self
.as_str()
.chars()
.skip(line_start.index)
.take(TAB_SIZE)
.all(|c| c == ' ')
{
Expand Down
Loading