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

panicked in Line::truncated method due to byte index not being a char boundary #1032

Closed
ymgyt opened this issue Apr 15, 2024 · 11 comments · Fixed by #1089 · May be fixed by #1055
Closed

panicked in Line::truncated method due to byte index not being a char boundary #1032

ymgyt opened this issue Apr 15, 2024 · 11 comments · Fixed by #1089 · May be fixed by #1055
Labels
bug Something isn't working

Comments

@ymgyt
Copy link
Sponsor

ymgyt commented Apr 15, 2024

Description

panic occured at ratatui-0.26.2/src/text/line.rs:477:59
byte index 51 is not a char boundary; it is inside 'で' (bytes 49..52) of 🦀 RFC8628 OAuth 2.0 Device Authorization GrantでCLIからGithubのaccess tokenを取得する

at https://github.com/ymgyt/syndicationd/blob/a70475ec3a3dc284b1a209ace81bd29dcaaee00d/crates/synd_term/src/ui/components/subscription.rs#L314

To Reproduce

git clone https://github.com/ymgyt/ratatui-repro.git
cd ratatui-repro
cargo run
[dependencies]
ratatui = "0.26.2"
use ratatui::{
    layout::Constraint,
    text::Span,
    widgets::{Row, Table, Widget as _},
};

fn main() {
    let backend = ratatui::backend::CrosstermBackend::new(std::io::stdout());
    let mut terminal = ratatui::Terminal::new(backend).unwrap();

    terminal.draw(|frame| {

        let rows = vec![
            Row::new([Span::from("🦀 RFC8628 OAuth 2.0 Device Authorization GrantでCLIからGithubのaccess tokenを取得する")])
        ];
        let widths = [Constraint::Length(83)];
        let table = Table::new(rows, widths);
        table.render(frame.size(), frame.buffer_mut());
    }).unwrap();
}

Expected behavior

does not panic

Screenshots

Environment

  • OS: Mac
  • Terminal Emulator: Alacritty
  • Font:
  • Crate version: 0.26.2
  • Backend: crossterm

Additional context

@ymgyt ymgyt added the bug Something isn't working label Apr 15, 2024
@joshka
Copy link
Member

joshka commented Apr 15, 2024

I suspect this might break many situations with emojis and not just in the table.

@joshka
Copy link
Member

joshka commented Apr 15, 2024

Minimal repro:

    #[test]
    fn truncation_works_with_emoji() {
        let line = Line::raw( "123456789🦀");
        let mut buf = Buffer::empty(Rect::new(0, 0, 10, 1));
        line.render(buf.area, &mut buf);
        assert_buffer_eq!(buf, Buffer::with_lines(vec!["123456789 "]));
    }

@Thorongil80
Copy link

Thorongil80 commented Apr 18, 2024

Hi there, I notice this as well in table columns that are truncated because the terminal is resized so they overflow. The line that is drawn in the truncated table column is cut at byte positions.

In our app we draw a progress bar using unicode block characters in the upper right corner. We can reproduce easily by resizing our terminal to a smaller size, so the column widget cuts into the progress bar.

Please confirm if this is a bug to be solved in ratatui. We use Crossterm 0.27.0 as backend on Linux / X11.

@Thorongil80
Copy link

Thorongil80 commented Apr 18, 2024

/// Returns a line that's truncated corresponding to it's alignment and result width
    #[must_use = "method returns the modified value"]
    fn truncated(&'a self, result_width: u16) -> Self {
        let mut truncated_line = Line::default();
        let width = self.width() as u16;
        let mut offset = match self.alignment {
            Some(Alignment::Center) => (width.saturating_sub(result_width)) / 2,
            Some(Alignment::Right) => width.saturating_sub(result_width),
            _ => 0,
        };
        let mut x = 0;
        for span in &self.spans {
            let span_width = span.width() as u16;
            if offset >= span_width {
                offset -= span_width;
                continue;
            }
            let mut new_span = span.clone();
            let new_span_width = span_width - offset;
            if x + new_span_width > result_width {
                let span_end = (result_width - x + offset) as usize;
                new_span.content = Cow::from(&span.content[offset as usize..span_end]);
                truncated_line.spans.push(new_span);
                break;
            }

            **new_span.content = Cow::from(&span.content[offset as usize..]);**
            truncated_line.spans.push(new_span);
            x += new_span_width;
            offset = 0;
        }
        truncated_line
    }

is it here?

            new_span.content = Cow::from(&span.content[offset as usize..span_end]);

@Thorongil80
Copy link

Thorongil80 commented Apr 18, 2024

Should one iterate over chars() constructed from span.content?

    /// Returns a line that's truncated corresponding to it's alignment and result width
    #[must_use = "method returns the modified value"]
    fn truncated(&'a self, result_width: u16) -> Self {
        let mut truncated_line = Line::default();
        let width = self.width() as u16;
        let mut offset = match self.alignment {
            Some(Alignment::Center) => (width.saturating_sub(result_width)) / 2,
            Some(Alignment::Right) => width.saturating_sub(result_width),
            _ => 0,
        };
        let mut x = 0;
        for span in &self.spans {
            let span_width = span.width() as u16;
            if offset >= span_width {
                offset -= span_width;
                continue;
            }
            let mut new_span = span.clone();
            let new_span_width = span_width - offset;
            if x + new_span_width > result_width {
                let span_end = (result_width - x + offset) as usize;
                new_span.content = Cow::from( span.content.as_ref().chars().skip(offset.into()).take(span_end - (offset as usize)).collect::
                truncated_line.spans.push(new_span);
                break;
            }
            new_span.content = Cow::from( span.content.as_ref().chars().skip(offset.into()).collect::<String>());
            truncated_line.spans.push(new_span);
            x += new_span_width;
            offset = 0;
        }
        truncated_line
    }

This compiles, I think it is what is needed.

@joshka
Copy link
Member

joshka commented Apr 19, 2024

Please confirm if this is a bug to be solved in ratatui. We use Crossterm 0.27.0 as backend on Linux / X11.

Yes, it's a bug - feel free to submit a PR to fix and we can get a quick release out to fix it.

Should one iterate over chars() constructed from span.content?

Yes likely. Can you make sure to add some unit tests with the PR.

@Thorongil80
Copy link

Thorongil80 commented Apr 22, 2024

Hi,
I am unfamiliar with unit test structure in this project, so it might take some time until I can provide something.

is

new_span.content = Cow::from( span.content.as_ref().chars().skip(offset.into()).take(span_end - (offset as usize)).collect::<String>());

equivalent to

new_span.content = Cow::from(&span.content[offset as usize..span_end]);

?

e.g. it could all be off by one or something (is skip (n) same as indexing [n..] )?

Cheers

@EdJoPaTo
Copy link
Member

The slice type is independent from this project and a Rust type commonly used. Its basically slice[start..end] (<end) or slice[start..=end] (=end). The = tells whether the end is included or not.

@TadoTheMiner
Copy link
Contributor

I apologize for panics caused by my PR

@joshka
Copy link
Member

joshka commented Apr 24, 2024

I apologize for panics caused by my PR

It's all good. Unicode is hard. No one gets it right the first time they have to deal with it.

@EdJoPaTo
Copy link
Member

I apologize for panics caused by my PR

To add on @joshka: Multiple reviewers of #987 did not notice this either. It's definitely not an easy to spot issue. As humans are bad at such stuff #1056 can spot something like this in the future.

kxxt added a commit to kxxt/tracexec that referenced this issue Apr 30, 2024
The right padding border symbols I added might caused a crash
when resizing the terminal, which is an upstream bug:
ratatui-org/ratatui#1032
joshka added a commit to joshka/ratatui that referenced this issue May 6, 2024
This has 3 bugs still I think in the rendering, but these are not panics.
Fixes ratatui-org#1032
joshka added a commit that referenced this issue May 12, 2024
- Rewrote the line / span rendering code to take into account how
multi-byte / wide emoji characters are truncated when rendering into
areas that cannot accommodate them in the available space
- Added comprehensive coverage over the edge cases
- Adds a benchmark to ensure perf

Fixes: #1032
Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
Co-authored-by: EdJoPaTo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants