Use lipgloss.Width to calculate DisplayWidth

[maaslalani]

Fixes cli/cli#9018

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

[maaslalani]

[maaslalani]

@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!

[williammartin]

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]

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]

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]

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]

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]

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]

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]

Before

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

After

[williammartin]

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

LGTM thanks a lot.