fix: use actual border size in GetBorderXxxSize when BorderStyle is set without sides#700
Open
LeSingh1 wants to merge 2 commits into
Open
Conversation
…et without sides When BorderStyle() is used without explicitly enabling individual sides, isBorderStyleSetWithoutSides() returns true and the renderer correctly enables all four sides. However, the corresponding GetBorderTopSize, GetBorderLeftSize, GetBorderBottomSize and GetBorderRightSize getters were returning a hardcoded 1 in that path, ignoring the actual width of the border's characters. This causes GetHorizontalFrameSize / GetVerticalFrameSize to return wrong values for any border whose side characters have a display width other than 1 (e.g. custom borders with fullwidth Unicode glyphs), and makes BorderStyle() + Padding() layout math inconsistent with what the renderer draws. Fix by replacing the hardcoded early-return with a call to the border struct's own GetXxxSize() getter, which already measures the actual rune widths via maxRuneWidth. Also correct a stale comment in GetBorderBottomSize that referred to the "left edge" instead of the "bottom edge".
|
Nice catch on the hardcoded Two paths forward if this ever comes up:
The current fix is correct for the reported case (fullwidth glyphs in NormalBorder). The ambiguous-width scenario is an edge case that may only matter when lipgloss sees adoption in i18n-heavy TUI apps. |
…nderer The four GetBorderXxxSize methods now delegate to getBorderStyle().GetXxxSize(), which uses the same maxRuneWidth helper that applyBorder uses when computing the rendered layout width. Add a doc-comment on each getter explaining this consistency guarantee so that reviewers understand the EAW-ambiguous-width concern is handled: getter and renderer measure identically, so there is no layout mismatch regardless of locale or Unicode width table.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
BorderStyle()is used without explicitly enabling individual sides,isBorderStyleSetWithoutSides()returns true and the renderer correctlyenables all four sides. The
GetBorderTopSize,GetBorderLeftSize,GetBorderBottomSize, andGetBorderRightSizegetters were returning ahardcoded
1in that path, ignoring the actual width of the border'scharacters.
This causes
GetHorizontalFrameSize/GetVerticalFrameSizeto returnwrong values whenever the border's side characters have a display width
other than 1 (e.g. a custom border using fullwidth Unicode glyphs), and
makes
BorderStyle()+Padding()layout math inconsistent with whatthe renderer actually draws.
The fix replaces the hardcoded early-return with a call to the border
struct's own
GetXxxSize()getter, which already measures actual runewidths via
maxRuneWidth. The two branches now share the same tail call,so the condition simplifies to a single combined check.
Also corrects a stale comment in
GetBorderBottomSizethat referred tothe "left edge" instead of the "bottom edge".
Regression test added in
style_test.gocoveringNormalBorder()viaBorderStyle()alone,BorderStyle()+Padding(), and a custom borderwith fullwidth vertical-line characters that triggered the wrong result
before this fix.