Skip to content

Use lipgloss.Width to calculate DisplayWidth - #159

Merged
williammartin merged 2 commits into
cli:trunkfrom
maaslalani:trunk
Apr 29, 2024
Merged

williammartin merged 2 commits into
cli:trunkfrom
maaslalani:trunk

Conversation

@maaslalani

@maaslalani maaslalani commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor

Fixes cli/cli#9018

Uses lipgloss.Width to calculate DisplayWidth which correctly aligns emojis.

@maaslalani

Copy link
Copy Markdown
Contributor Author
image

Comment thread pkg/text/text.go Outdated
@maaslalani

maaslalani commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

@williammartin Had to switch to lipgloss.Width to account for ANSI sequences since uniseg doesn't strip ANSI before calculating width (one of the test cases included ANSI sequences), but lipgloss does then uses uniseg.StringWidth under the hood.

Added the multi-byte emoji test case as well!

@maaslalani maaslalani changed the title Use uniseg to calculate DisplayWidth Use lipgloss.Width to calculate DisplayWidth Apr 29, 2024
Comment thread go.mod
@williammartin

Copy link
Copy Markdown
Member

Thanks. If we're expecting DisplayWidth to handle ANSI escape sequences then let's add a test for that as well.

I suppose for someone out there this could be a breaking change but I think it's edge case enough that I'd rather just accept it and then people could fix any hacky workarounds they might have had.

@maaslalani

Copy link
Copy Markdown
Contributor Author

Yep, there is already a test case for that! The tests were failing when I simply had uniseg.StringWidth because of the ANSI test case. Which is why I needed to swap out for lipgloss to pass all tests cases.

@maaslalani

maaslalani commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

Test case for ignoring ANSI sequences when calculating width is here: https://github.com/cli/go-gh/blob/trunk/pkg/text/text_test.go#L282-L287 (fails when just using uniseg.StringWidth, passes when using lipgloss.Width)

@williammartin

Copy link
Copy Markdown
Member

Well, that's testing the public API of PadRight but it's just an implementation detail that PadRight uses DisplayWidth. Refactoring PadRight would then lose all our confidence that DisplayWidth handles ANSI codes. I want to both ensure that DisplayWidth enforces this behaviour, and I want it to be communicated to future maintainers in the tests because I had no idea that it was expected when you first opened the PR.

@maaslalani

maaslalani commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

Ah you're totally right, there is also a test case directly for color codes in DisplayWidth as well:

go-gh/pkg/text/text_test.go

Lines 360 to 363 in 5840c44

name: "color codes",
text: "\x1b[0;31mred\x1b[0m",
want: 3,
},

Happy to add more test cases though if you want / prefer!

@williammartin

williammartin commented Apr 29, 2024 •

Copy link
Copy Markdown
Member

Haha sorry I should have also looked closer. I think I got red herring-ed by your link because I assumed if it existed already you would definitely have linked it 😅

I can confirm that test fails just using uniseg.StringWidth:

--- FAIL: TestDisplayWidth (0.00s)
    --- FAIL: TestDisplayWidth/color_codes (0.00s)
        /Users/williammartin/workspace/go-gh/pkg/text/text_test.go:388: 
            	Error Trace:	text_test.go:388
            	Error:      	Not equal: 
            	            	expected: 3
            	            	actual  : 12
            	Test:       	TestDisplayWidth/color_codes

No need for extra tests, this looks great to me, once I pull it into cli/cli and play with it, cheers.

@maaslalani

maaslalani commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

I think I got red herring-ed by your link because I assumed if it existed already you would definitely have linked it 😅

Haha all good! I kind of missed that test case the first time as well and definitely should have linked to that!

@williammartin

Copy link
Copy Markdown
Member

Before

image

(wow such codepoints: https://ardislu.dev/biggest-unicode-grapheme-cluster)

After

image

@williammartin williammartin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this into cli/cli and it passed all the tests.

LGTM thanks a lot.

@williammartin
williammartin merged commit dbd982e into cli:trunk Apr 29, 2024
Abeabenut

This comment was marked as spam.

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.

Repository list table is misaligned when rendering emojis

3 participants