-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
wcwidth calculation #144
wcwidth calculation #144
Conversation
Hi @jerch, thanks for your contribution! Before moving on with testing this out, could you please mention the particular GitHub issues that this PR is supposed to fix? |
*/ | ||
var wcswidth = (function() { | ||
var _WIDTH_COMBINING = [ | ||
[0x0300, 0x036F], [0x0483, 0x0486], [0x0488, 0x0489], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where was this list sourced from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted from here https://www.cl.cam.ac.uk/%7Emgk25/ucs/wcwidth.c
I have cleaned up the code and fixed the indentation. |
Thanks @jerch! I'll have to take a chunk of time to look at this in detail, can you think of some test cases that could be added for |
yes I have some test cases, gonna try to apply them to xterm.js. There are some edge cases that need proper testing:
Offtopic: |
One test case fails - I commented it out since the it refers to a missing feature in default state (wrap true/false is not handled there at all). --> see #147 |
Okay, just found the chance to take a deeper look into this. #72 does not seem to be closed by this (still weird characters appear, but different than the ones mentioned in this issue). This PR closes #70 and closes #62 successfully though. I will move on with merging this PR, since it great steps forward have been done here. I also believe that #72 can be tackled autonomously, since it appears only when moving the cursor. Thanks a lot! |
Quick replacement of
isWide()
withwcwidth
. Since xterm.js has no frontend packaging atm I just copied my version into the source file.wcwidth fixes several issues:
Misaligned glyphs are not fixed by this pull request. This would need a way to introspect the current font and glyph widths. This could be done by prerendering faulty glyphs onto a canvas, getting their width with
measureText
and aligning them with a CSS rule. I played with this approach here https://github.com/netzkolchose/jquery.browserterminal/blob/master/dist/js/jquery.browserterminal.js#L289, the glyph width calculation is pretty slow but is only needed once.