Skip to content

text: apply the box drawing width override in StringWidth - #429

Open
yu2971512385-ui wants to merge 1 commit into
jedib0t:mainfrom
yu2971512385-ui:fix/stringwidth-box-drawing
Open

yu2971512385-ui wants to merge 1 commit into
jedib0t:mainfrom
yu2971512385-ui:fix/stringwidth-box-drawing

Conversation

@yu2971512385-ui

Copy link
Copy Markdown

Problem

RuneWidth reports box drawing (U+2500-U+257F) and block element (U+2580-U+259F) characters as one cell wide regardless of locale, and its doc comment says so explicitly ("always treated as width 1, regardless of locale"). StringWidthWithoutEscSequences inherits that, because it sums RuneWidth over the string.

StringWidth does not: it forwards straight to rwCondition.StringWidth, which has no such override. go-runewidth initialises EastAsianWidth from LANG/LC_ALL/RUNEWIDTH_EASTASIAN, so on a machine with e.g. LANG=zh_CN.UTF-8 the two functions disagree:

text.OverrideRuneWidthEastAsianWidth(true) // what LANG=zh_CN.UTF-8 does by itself

text.StringWidth("┌──┐")                      // 8
text.StringWidthWithoutEscSequences("┌──┐")   // 4
text.RuneWidth('┌')                           // 1

This looks like an oversight when #220 / #204 were fixed: the override was added to RuneWidth (and so to StringWidthWithoutEscSequences) but not to the sibling StringWidth, which is public API and documented as "similar to RuneWidth, except it works on a string". The renderers all use StringWidthWithoutEscSequences, so table/progress output is unaffected — callers of StringWidth are.

Fix

Apply the same correction in StringWidth, and only when EastAsianWidth is set, so the common path is unchanged. The count still comes from rwCondition.StringWidth, which is grapheme cluster aware, so emoji and combining marks are unaffected. The range check is factored into isSingleCellRune and shared with RuneWidth.

before after
StringWidth("┌──┐"), EastAsianWidth=true 8 4
StringWidth("┌──┐"), EastAsianWidth=false 4 4
StringWidth("│ツ│"), EastAsianWidth=true 6 4
StringWidth("Ghost 生命") 10 10

Tests

TestOverrideRuneWidthEastAsianWidth now asserts StringWidth agrees with StringWidthWithoutEscSequences for both settings, plus a case in TestStringWidth. Verified the new assertions fail on main (expected: 1, actual: 2) and pass with the change; go test ./... is green.

AI assistance was used to help find and fix this; I have reviewed and tested every line, and the failing/passing runs above were done locally.

🤖 Generated with Claude Code

@coveralls

coveralls commented Sep 30, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 40327139338

Coverage remained the same at 100.0%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 15 of 15 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 5175
Covered Lines: 5175
Line Coverage: 100.0%
Coverage Strength: 1.21 hits per line

💛 - Coveralls

@jedib0t

jedib0t commented Oct 19, 2026

Copy link
Copy Markdown
Owner

Hey @yu2971512385-ui, thanks for the PR, the fix and tests look good. I found one small edge case, if you'd like to tighten it up.

runewidth's StringWidth counts width per grapheme cluster, using only the first visible character in each cluster. The correction loop here subtracts 1 for every box-drawing character in the string, including ones runewidth never counted. That happens when a "prepend" character such as U+0600 (Arabic number sign) comes right before a box-drawing character, because they form one cluster:

text.OverrideRuneWidthEastAsianWidth(true)
text.StringWidth("aツ\u0600─") // 3, expected 4

It's rare, but a cleaner fix is to count per grapheme cluster with RuneWidth directly. That matches runewidth's own loop, plus the box-drawing rule. uniseg is already imported in string.go:

func StringWidth(str string) (width int) {
	g := uniseg.NewGraphemes(str)
	for g.Next() {
		for _, r := range g.Runes() {
			if w := RuneWidth(r); w > 0 {
				width += w
				break
			}
		}
	}
	return
}

This also removes the need for the EastAsianWidth check and the second loop. With East Asian width off, it gives the same results as before. Could you also add a test case like "aツ\u0600─" -> 4 to TestOverrideRuneWidthEastAsianWidth?

Also, you'd have to rebase your branch to the latest main.

StringWidth went through runewidth's own StringWidth, which does not know
about the box drawing and block element override that RuneWidth applies, so
the two disagreed in an East Asian locale: StringWidth("┌──┐") came back 8
where RuneWidth says each of those is one cell.

Counted per grapheme cluster through RuneWidth instead, which is runewidth's
own loop plus the override. That also fixes a case the first version of this
patch got wrong: it corrected by subtracting for every box drawing character
in the string, including ones runewidth had never counted, so a Prepend
character joining the box drawing character after it into one cluster lost a
cell -- StringWidth("aツ؀─") came back 3 instead of 4.

With East Asian width off the results are unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@yu2971512385-ui
yu2971512385-ui force-pushed the fix/stringwidth-box-drawing branch from ebc4796 to 9e7ca00 Compare October 26, 2026 13:01
@yu2971512385-ui

Copy link
Copy Markdown
Author

Thanks — you were right, and I reproduced it before changing anything:

StringWidth("aツ؀─") = 3, want 4
StringWidth("┌──┐")        = 4  ok
StringWidth("aツ─")        = 4  ok

Taken your suggestion as written. StringWidth now counts per grapheme cluster through RuneWidth, which is runewidth's own loop plus the override, so the EastAsianWidth check and the correction loop are both gone:

func StringWidth(str string) (width int) {
	g := uniseg.NewGraphemes(str)
	for g.Next() {
		for _, r := range g.Runes() {
			if w := RuneWidth(r); w > 0 {
				width += w
				break
			}
		}
	}
	return width
}

isSingleCellRune stays — RuneWidth still uses it.

Added "aツ؀─" -> 4 to TestOverrideRuneWidthEastAsianWidth, asserted under both EastAsianWidth settings alongside the existing cases. It fails on the previous implementation at string_test.go:165.

Also checked that StringWidth still agrees with StringWidthWithoutEscSequences on ╋ █ ▒ ▓ ┌──┐ │ツ│ Ghost 生命 aツ─ under both settings, and that results with East Asian width off are unchanged.

Rebased onto main at 9a59d89. go test ./... passes across list, progress, table and text; go vet and gofmt clean.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants