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

wcwidth calculation #144

Merged
merged 4 commits into from
Jun 27, 2016
Merged

wcwidth calculation #144

merged 4 commits into from
Jun 27, 2016

Conversation

jerch
Copy link
Member

@jerch jerch commented Jun 24, 2016

Quick replacement of isWide() with wcwidth. Since xterm.js has no frontend packaging atm I just copied my version into the source file.

wcwidth fixes several issues:

  • full unicode (all planes supported at data stage, output depends on font capability)
  • combining characters
  • half/fullwidth distinction for all unicode codepoints (though lookup table needs fixes to get xterm or font compatibility)

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.

@parisk
Copy link
Contributor

parisk commented Jun 24, 2016

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?

@jerch
Copy link
Member Author

jerch commented Jun 24, 2016

Issues affected by this: #72 , maybe #70 , #62

and yeah, imho xterm.js needs some more test cases...

*/
var wcswidth = (function() {
var _WIDTH_COMBINING = [
[0x0300, 0x036F], [0x0483, 0x0486], [0x0488, 0x0489],
Copy link
Member

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?

Copy link
Member Author

@jerch jerch Jun 25, 2016

Choose a reason for hiding this comment

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

@jerch
Copy link
Member Author

jerch commented Jun 25, 2016

I have cleaned up the code and fixed the indentation.

@Tyriar
Copy link
Member

Tyriar commented Jun 25, 2016

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 wcwidth?

@jerch
Copy link
Member Author

jerch commented Jun 26, 2016

yes I have some test cases, gonna try to apply them to xterm.js.

There are some edge cases that need proper testing:

  • fullwidth chars just go to next line if entered at cols-2, what to do with the last cell itself? It will need at least the attributes set.
  • combining chars should go to last active cell - needs tests for all cells, no clue yet how solve this after cursor jumps (xterm just insert them at the cursor after jumps)

Offtopic:
Also the rendering needs some fixes, the fullwidth chars are not cell aligned for most of my fonts.

@jerch
Copy link
Member Author

jerch commented Jun 26, 2016

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

@parisk
Copy link
Contributor

parisk commented Jun 27, 2016

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!

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.

3 participants