Skip to content

Commit b47d01e

Browse files
committed
fix(tui): measure popup wrapping in display columns, not chars
wrap_value packed rows by chars().count(), and a CJK glyph is one char but two terminal columns. A double-width rejection reason therefore produced rows about twice the popup's width: the right half of every row was clipped, and unlike the vertical case there is no horizontal scroll to recover it. Measure width with Span::width(), the same measurement ratatui applies when it lays cells out, so the wrap agrees with the renderer by construction and no new dependency is needed. This covers word packing, the hard-break path for a word wider than the line, the label indent, the footer hint packing, and the denial row's fits-on-one-line check. The list row's shortened copy had the same defect through truncate_str, which counts chars. Leave truncate_str alone for its two existing callers and add truncate_display for the guidance row. Cover it with an 80x24 render regression that interleaves markers through the CJK text: a trailing marker lands on its own short row in both the broken and fixed layouts, so it would not detect this. The two unit tests measure with an independent column oracle rather than the helper under test, which would make them tautological. Part of NVIDIA#1098. Signed-off-by: Vyncint Ng <[email protected]>
1 parent 03c67b1 commit b47d01e

1 file changed

Lines changed: 161 additions & 28 deletions

File tree

‎crates/openshell-tui/src/ui/sandbox_draft.rs‎

Lines changed: 161 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ pub fn draw(frame: &mut Frame<'_>, app: &mut App, area: Rect) {
152152
}
153153
if let Some(reason) = rejection_guidance(chunk) {
154154
spans.push(Span::styled(
155-
format!(" \"{}\"", truncate_str(reason, 32)),
155+
format!(" \"{}\"", truncate_display(reason, 32)),
156156
t.muted,
157157
));
158158
}
@@ -285,10 +285,10 @@ pub fn draw_detail_popup(
285285
format_short_time(chunk.first_seen_ms),
286286
format_short_time(chunk.last_seen_ms),
287287
);
288-
let denied_width = denied_label.chars().count()
289-
+ denied_count.chars().count()
288+
let denied_width = display_width(denied_label)
289+
+ display_width(&denied_count)
290290
+ 2
291-
+ denied_seen.chars().count();
291+
+ display_width(&denied_seen);
292292
if denied_width <= text_width {
293293
lines.push(Line::from(vec![
294294
Span::styled(denied_label, t.muted),
@@ -306,7 +306,7 @@ pub fn draw_detail_popup(
306306
);
307307
push_wrapped(
308308
&mut lines,
309-
&" ".repeat(denied_label.chars().count()),
309+
&" ".repeat(display_width(denied_label)),
310310
t.muted,
311311
&denied_seen,
312312
t.muted,
@@ -548,56 +548,95 @@ fn truncate_str(s: &str, max_len: usize) -> String {
548548
}
549549
}
550550

551-
/// Word-wrap `text` into rows of at most `width` columns.
551+
/// Terminal display width of `text` in columns.
552552
///
553-
/// A word longer than `width` is hard-broken rather than allowed to overflow.
553+
/// Uses the same measurement ratatui applies when it lays cells out, so a
554+
/// double-width glyph such as CJK counts as the two columns it will occupy.
555+
/// Counting `chars` here instead would let CJK text render past the popup edge
556+
/// with no way to scroll to the missing tail.
557+
fn display_width(text: &str) -> usize {
558+
Span::raw(text).width()
559+
}
560+
561+
/// Word-wrap `text` into rows of at most `width` display columns.
562+
///
563+
/// A word wider than `width` is hard-broken rather than allowed to overflow.
554564
/// Always returns at least one row so callers can index the first row safely.
555565
fn wrap_value(text: &str, width: usize) -> Vec<String> {
556566
if width == 0 {
557567
return vec![text.to_string()];
558568
}
559569
let mut rows: Vec<String> = Vec::new();
560570
let mut cur = String::new();
561-
let mut cur_len = 0usize;
571+
let mut cur_width = 0usize;
572+
let mut buf = [0u8; 4];
562573
for word in text.split_whitespace() {
563-
let word_len = word.chars().count();
564-
if word_len > width {
565-
if cur_len > 0 {
574+
let word_width = display_width(word);
575+
if word_width > width {
576+
if cur_width > 0 {
566577
rows.push(std::mem::take(&mut cur));
567-
cur_len = 0;
578+
cur_width = 0;
568579
}
569580
for ch in word.chars() {
570-
if cur_len == width {
581+
let ch_width = display_width(ch.encode_utf8(&mut buf));
582+
if cur_width + ch_width > width && cur_width > 0 {
571583
rows.push(std::mem::take(&mut cur));
572-
cur_len = 0;
584+
cur_width = 0;
573585
}
574586
cur.push(ch);
575-
cur_len += 1;
587+
cur_width += ch_width;
576588
}
577589
continue;
578590
}
579-
let needed = if cur_len == 0 {
580-
word_len
591+
let needed = if cur_width == 0 {
592+
word_width
581593
} else {
582-
cur_len + 1 + word_len
594+
cur_width + 1 + word_width
583595
};
584596
if needed > width {
585597
rows.push(std::mem::take(&mut cur));
586-
cur_len = 0;
598+
cur_width = 0;
587599
}
588-
if cur_len > 0 {
600+
if cur_width > 0 {
589601
cur.push(' ');
590-
cur_len += 1;
602+
cur_width += 1;
591603
}
592604
cur.push_str(word);
593-
cur_len += word_len;
605+
cur_width += word_width;
594606
}
595-
if cur_len > 0 || rows.is_empty() {
607+
if cur_width > 0 || rows.is_empty() {
596608
rows.push(cur);
597609
}
598610
rows
599611
}
600612

613+
/// Truncate `text` to `max_columns` display columns, appending `...` when cut.
614+
///
615+
/// `truncate_str` counts chars, which is right for its existing callers but
616+
/// would let a CJK value take twice its budget on the list row.
617+
fn truncate_display(text: &str, max_columns: usize) -> String {
618+
if display_width(text) <= max_columns {
619+
return text.to_string();
620+
}
621+
if max_columns <= 3 {
622+
return ".".repeat(max_columns);
623+
}
624+
let budget = max_columns - 3;
625+
let mut out = String::new();
626+
let mut width = 0usize;
627+
let mut buf = [0u8; 4];
628+
for ch in text.chars() {
629+
let ch_width = display_width(ch.encode_utf8(&mut buf));
630+
if width + ch_width > budget {
631+
break;
632+
}
633+
out.push(ch);
634+
width += ch_width;
635+
}
636+
out.push_str("...");
637+
out
638+
}
639+
601640
/// Push `text` wrapped to `width`, with `prefix` on the first row and a matching
602641
/// indent on continuation rows so the label column stays aligned.
603642
fn push_wrapped(
@@ -608,7 +647,7 @@ fn push_wrapped(
608647
text_style: Style,
609648
width: usize,
610649
) {
611-
let indent = prefix.chars().count();
650+
let indent = display_width(prefix);
612651
let available = width.saturating_sub(indent).max(1);
613652
for (i, row) in wrap_value(text, available).into_iter().enumerate() {
614653
let head = if i == 0 {
@@ -633,7 +672,7 @@ fn hint_units(chunk: &PolicyChunk, t: &crate::theme::Theme, scrollable: bool) ->
633672
Span::styled(key.to_string(), key_style),
634673
Span::styled(label.to_string(), label_style),
635674
],
636-
key.chars().count() + label.chars().count(),
675+
display_width(key) + display_width(label),
637676
));
638677
};
639678
match chunk.status.as_str() {
@@ -940,6 +979,25 @@ mod tests {
940979
use ratatui::Terminal;
941980
use ratatui::backend::TestBackend;
942981

982+
/// Column width computed independently of the production `display_width`.
983+
///
984+
/// Asserting with the helper under test would be tautological: if it
985+
/// regressed to counting chars, the assertion would regress with it.
986+
fn expected_columns(text: &str) -> usize {
987+
text.chars()
988+
.map(|c| {
989+
let wide = ('\u{1100}'..='\u{115f}').contains(&c)
990+
|| ('\u{2e80}'..='\u{a4cf}').contains(&c)
991+
|| ('\u{ac00}'..='\u{d7a3}').contains(&c)
992+
|| ('\u{f900}'..='\u{faff}').contains(&c)
993+
|| ('\u{fe30}'..='\u{fe6f}').contains(&c)
994+
|| ('\u{ff00}'..='\u{ff60}').contains(&c)
995+
|| ('\u{ffe0}'..='\u{ffe6}').contains(&c);
996+
usize::from(wide) + 1
997+
})
998+
.sum()
999+
}
1000+
9431001
fn make_chunk(status: &str, rejection_reason: &str) -> PolicyChunk {
9441002
PolicyChunk {
9451003
status: status.to_string(),
@@ -985,8 +1043,8 @@ mod tests {
9851043
#[test]
9861044
fn long_reason_truncates_for_the_list_row() {
9871045
let reason = "rejected because the endpoint list is far too permissive";
988-
let shown = truncate_str(reason, 32);
989-
assert_eq!(shown.chars().count(), 32);
1046+
let shown = truncate_display(reason, 32);
1047+
assert_eq!(display_width(&shown), 32);
9901048
assert!(shown.ends_with("..."));
9911049
assert!(shown.starts_with("rejected because"));
9921050
}
@@ -1020,10 +1078,47 @@ mod tests {
10201078
fn wrap_value_never_exceeds_the_width() {
10211079
let text = "reject this rule because the endpoint list is far too permissive";
10221080
for row in wrap_value(text, 17) {
1023-
assert!(row.chars().count() <= 17, "row too wide: {row:?}");
1081+
assert!(display_width(&row) <= 17, "row too wide: {row:?}");
10241082
}
10251083
}
10261084

1085+
#[test]
1086+
fn wrap_value_measures_display_columns_not_chars() {
1087+
// CJK glyphs occupy two columns each, and with no spaces the whole
1088+
// string takes the hard-break path.
1089+
let cjk = "这条规则的范围太广".repeat(20);
1090+
for row in wrap_value(&cjk, 40) {
1091+
assert!(
1092+
expected_columns(&row) <= 40,
1093+
"row is {} columns: {row:?}",
1094+
expected_columns(&row)
1095+
);
1096+
}
1097+
1098+
// Mixed script exercises the word-packing path instead.
1099+
let mixed = "scope 这条规则 to docs 路径 only ".repeat(20);
1100+
for row in wrap_value(&mixed, 33) {
1101+
assert!(
1102+
expected_columns(&row) <= 33,
1103+
"row is {} columns: {row:?}",
1104+
expected_columns(&row)
1105+
);
1106+
}
1107+
}
1108+
1109+
#[test]
1110+
fn truncate_display_counts_columns_not_chars() {
1111+
let cjk = "这条规则的范围太广".repeat(10);
1112+
let shown = truncate_display(&cjk, 32);
1113+
assert!(
1114+
expected_columns(&shown) <= 32,
1115+
"truncated value is {} columns",
1116+
expected_columns(&shown)
1117+
);
1118+
assert!(shown.ends_with("..."));
1119+
assert_eq!(truncate_display("too broad", 32), "too broad");
1120+
}
1121+
10271122
// --- rendering --------------------------------------------------------
10281123

10291124
fn render(chunk: &PolicyChunk, width: u16, height: u16, scroll: usize) -> (String, usize) {
@@ -1144,6 +1239,44 @@ mod tests {
11441239
assert!(bottom.contains("Close"));
11451240
}
11461241

1242+
/// Double-width guidance must stay reachable too. Counting chars rather than
1243+
/// columns made every wrapped row about twice as wide as the popup, so the
1244+
/// right half of each row was clipped with nowhere to scroll. Markers are
1245+
/// interleaved through the text rather than appended, because a trailing
1246+
/// marker lands on its own short row either way and would not detect this.
1247+
#[test]
1248+
fn cjk_guidance_is_fully_reachable_at_80x24() {
1249+
use std::fmt::Write as _;
1250+
1251+
const MARKERS: usize = 16;
1252+
let mut reason = String::new();
1253+
for i in 0..MARKERS {
1254+
let _ = write!(reason, "M{i:02}");
1255+
reason.push_str(&"这条规则范围太广".repeat(4));
1256+
}
1257+
let chunk = PolicyChunk {
1258+
status: "rejected".to_string(),
1259+
rejection_reason: reason,
1260+
rule_name: "allow-github".to_string(),
1261+
..Default::default()
1262+
};
1263+
1264+
let (_, max_scroll) = render(&chunk, 80, 24, 0);
1265+
assert!(max_scroll > 0, "content should overflow an 80x24 popup");
1266+
1267+
let mut seen = String::new();
1268+
for scroll in 0..=max_scroll {
1269+
seen.push_str(&render(&chunk, 80, 24, scroll).0);
1270+
}
1271+
for i in 0..MARKERS {
1272+
let marker = format!("M{i:02}");
1273+
assert!(
1274+
seen.contains(&marker),
1275+
"guidance marker {marker} was clipped and is unreachable at every scroll offset"
1276+
);
1277+
}
1278+
}
1279+
11471280
#[test]
11481281
fn short_content_does_not_scroll() {
11491282
let chunk = PolicyChunk {

0 commit comments

Comments
 (0)