sshd/terminal: Add fullwidth check for CJK in visualLength#339
sshd/terminal: Add fullwidth check for CJK in visualLength#339shazow merged 1 commit intoshazow:masterfrom
Conversation
shazow
left a comment
shazow
left a comment
There was a problem hiding this comment.
Interesting. I'll have to try it out. Any interest in trying to submit this upstream to https://pkg.go.dev/golang.org/x/crypto/ssh that this is forked from?
| default: | ||
| length++ | ||
| kind := width.LookupRune(r).Kind() | ||
| if kind == width.EastAsianFullwidth || kind == width.EastAsianWide { |
There was a problem hiding this comment.
What happens if we don't check the kind?
Does it make sense for this calculation to only happen for EastAsian runes?
There was a problem hiding this comment.
If we don't check the kind, the cursor will move half steps than needed in fullwidth words. then leads to #336.
According to Halfwidth_and_fullwidth_forms in wiki,
Unicode assigns every code point an "East Asian width" property.
Terminal emulators can use this property to decide if a character should consume one or two "columns" when figuring out tabs and cursor position.
It's a property for every runes, there are ASCII characters in fullwidth (e.g. "ＡＢ" not "AB").
Kind indicates the type of width property.
|
Looks great, I just want to take some time to test it with a few different terminals then I'll merge. If you're at all interested in trying to upstream these changes, that would be quite helpful too. :) |
|
Sorry haven't gotten around to testing and merging this yet. Also just stumbled on this project, do you think it would be helpful for generalizing this logic? https://github.com/rivo/uniseg |
|
Just had a chance to test this, it even fixes our indent bug we had with emoji rendering! Fantastic. Thank you. :) |
|
@yumaokao Again, I highly encourage you to upstream this change to https://github.com/golang/crypto, that's a great fix. |
|
@yumaokao Hi there, hope you're well. Any chance I can work with you to upstream this fix to https://github.com/golang/term (new package)? I'd like to get it done but technically you're the copyright holder of the change and I think we'd need you to sign the golang CLA to do that. |
|
@shazow Sure, but don't know how to achieve it cause I barely program in golang. And my thought is that the change was contributed to ssh-chat, so you could have it upstream or something else. |
2 participants
Fix for #336,