Skip to content

Commit

Permalink
Fix clipping logic
Browse files Browse the repository at this point in the history
As it turns out our clipping logic is subtly broken in the presence of
multi-byte characters. Because we unconditionally cut off the input
string once we exceeded a number of bytes, we may inadvertently violate
character boundaries. That in turn can result in a panic being reported.
This change fixes the problem by introducing a proper clipping routine
that honors graphemes and will only cut off entire graphemes instead of
individual bytes. It will also take into account the actual display
width of the string as opposed to merely counting bytes.
  • Loading branch information
d-e-s-o committed Jan 10, 2024
1 parent 3ba1e66 commit fc48f60
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 5 deletions.
44 changes: 44 additions & 0 deletions src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,29 @@ fn cursor_index(string: &str, byte_position: usize) -> usize {
}


/// Clip a string at `max_width` characters, at a proper character
/// boundary.
pub(crate) fn clip(string: &str, max_width: usize) -> &str {
let extended = true;
let result =
string
.grapheme_indices(extended)
.try_fold(0, |mut total_width, (byte_idx, grapheme)| {
total_width += grapheme.width();
if total_width > max_width {
ControlFlow::Break(byte_idx)
} else {
ControlFlow::Continue(total_width)
}
});

match result {
ControlFlow::Break(byte_idx) => &string[..byte_idx],
ControlFlow::Continue(..) => string,
}
}


/// Some Unicode aware text.
#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct Text {
Expand Down Expand Up @@ -350,6 +373,27 @@ mod tests {
assert_eq!(cursor_index(s, 6), 4);
}

/// Check that we can correctly "clip" a string to a maximum width.
#[test]
fn string_clipping() {
assert_eq!(clip("", 0), "");
assert_eq!(clip("a", 0), "");
assert_eq!(clip("ab", 0), "");

assert_eq!(clip("", 1), "");
assert_eq!(clip("a", 1), "a");
assert_eq!(clip("ab", 1), "a");

assert_eq!(clip("⚠️attn⚠️", 0), "");
assert_eq!(clip("⚠️attn⚠️", 1), "⚠️");
assert_eq!(clip("⚠️attn⚠️", 2), "⚠️a");

assert_eq!(clip("|a|b|", 0), "");
assert_eq!(clip("|a|b|", 1), "");
assert_eq!(clip("|a|b|", 2), "|");
assert_eq!(clip("|a|b|", 3), "|a");
}

/// Check that `EditableText::substr` behaves as it should.
#[test]
fn text_substr() {
Expand Down
7 changes: 2 additions & 5 deletions src/ui/term_renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use gui::Renderer;

use crate::colors::Color;
use crate::colors::Colors;
use crate::text;

use super::dialog::Dialog;
use super::dialog::SetUnsetTag;
Expand Down Expand Up @@ -97,11 +98,7 @@ fn clip(x: u16, y: u16, string: &str, bbox: BBox) -> &str {
let h = bbox.h;

if y < h {
if x + string.len() as u16 >= w {
&string[..(w - x) as usize]
} else {
string
}
text::clip(string, (w - x).into())
} else {
""
}
Expand Down

0 comments on commit fc48f60

Please sign in to comment.