-
Notifications
You must be signed in to change notification settings - Fork 1
fix(tui): truncate the spinner label by display width, not rune count #1433
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,3 +116,40 @@ func TestSpinnerHintSurvivesEvenWhenLabelCannotFit(t *testing.T) { | |
| t.Errorf("cancel hint should outrank the label when space is scarce, got %q", view) | ||
| } | ||
| } | ||
|
|
||
| // TestSpinnerKeepsCancelHintWithWideRunes pins issue #1432: the hint survives | ||
| // regardless of the label's script. | ||
| // | ||
| // shortenLabel budgeted in display columns but truncated by rune count. For | ||
| // CJK and emoji, which occupy two columns per rune, the label overran its | ||
| // budget and the final MaxWidth clamp ate the hint — the exact outcome | ||
| // shortenLabel exists to prevent. Every existing test passed throughout, | ||
| // because they were all ASCII, where columns and runes coincide. | ||
| // | ||
| // The assertion is on the hint's survival, not on the line's width: the width | ||
| // was always correct, which is why this went unnoticed. | ||
| func TestSpinnerKeepsCancelHintWithWideRunes(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| label string | ||
| }{ | ||
| {name: "cjk", label: "Waiting for 模型模型模型模型模型模型模型模型"}, | ||
| {name: "emoji", label: "Running 🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀"}, | ||
| // ASCII control: a fix that over-truncates everything to satisfy the | ||
| // hint assertion would show up here as a needlessly stunted label. | ||
| {name: "ascii control", label: "Waiting for some-long-model-name-v2"}, | ||
|
Comment on lines
+138
to
+140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This control does not detect the over-truncation it claims to guard against: an implementation that reduces every overlong label to only AGENTS.md reference: AGENTS.md:L31-L31 Useful? React with 👍 / 👎. |
||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| m := New(0).Start().SetAction(tc.label) | ||
| for _, width := range []int{40, 50} { | ||
| view := m.View(width) | ||
| if !strings.Contains(view, CancelHint) { | ||
| t.Errorf("width %d: cancel hint truncated, got %q", width, view) | ||
| } | ||
| if got := lipgloss.Width(view); got > width { | ||
| t.Errorf("width %d: line is %d columns wide: %q", width, got, view) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For multi-rune emoji such as
❤️or1️⃣, display width is not additive per rune: the variation-selector/keycap runes measure as zero alone, while the completed grapheme occupies two columns. This loop therefore undercharges these labels, can return a value wider thanbudget, and causesView's finalMaxWidthclamp to truncate the cancel hint again at narrow widths. Iterate grapheme clusters (or use an ANSI/grapheme-aware width truncator) so the accumulated width matcheslipgloss.Widthof the returned prefix.Useful? React with 👍 / 👎.