Skip to content
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

changed line display to be calculated with runewidth #4463

Merged
merged 5 commits into from
May 16, 2024
Merged

Conversation

joshyan1
Copy link
Contributor

Calculates line length using runewidth instead of len(string) as len(string) produces the number of bytes in that string (often resulting in an incorrect calculation of display length for multi-width runes). Now cuts lines accordingly for multi-byte characters (Russian, latin, etc) as well as word-wraps for multi-width characters (Chinese, Japanese, Korean)

Old Russian:
Screenshot 2024-05-15 at 5 05 42 PM

New: Russian
Screenshot 2024-05-15 at 5 01 21 PM

Old: Chinese
Screenshot 2024-05-15 at 5 10 09 PM

New: Chinese
Screenshot 2024-05-15 at 5 15 12 PM

Copy link
Contributor

@pdevine pdevine left a comment

Choose a reason for hiding this comment

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

This works so much better with multibyte char/single width alphabets (Greek/Russian/Bulgarian) and even when those are mixed (German/Finnish/Hungarian/etc.)

cmd/cmd.go Outdated
@@ -739,25 +739,33 @@ type displayResponseState struct {
wordBuffer string
}

// using runewidth instead of len (cus length is number of bytes, we want display length)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this comment.

@joshyan1 joshyan1 merged commit 5bece94 into main May 16, 2024
12 checks passed
@joshyan1 joshyan1 deleted the jyan/line-display branch May 16, 2024 21:15
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.

None yet

2 participants