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

Support options to treat ambiguous width characters as double width #1453

Closed
Tyriar opened this issue May 14, 2018 · 24 comments
Closed

Support options to treat ambiguous width characters as double width #1453

Tyriar opened this issue May 14, 2018 · 24 comments
Labels
help wanted type/enhancement Features or improvements to existing features

Comments

@Tyriar
Copy link
Member

Tyriar commented May 14, 2018

iTerm has a workaround for some powerline issues here https://github.com/bhilburn/powerlevel9k/wiki/Troubleshooting#strange-character--segment-spacing

VS Code got a report for this at microsoft/vscode#48775 (comment)

image

/cc @jerch

@jerch
Copy link
Member

jerch commented May 14, 2018

Would need the codepoints of that line to track down the unicode segment they belong to. There are several ambiguous width codepoint segments, not sure which need to be customizable. In general it is a good idea to offer this since it might solve some asian glyph alignments (see http://unicode.org/reports/tr11/#Ambiguous).

At the moment wcwidth treats most ambiguous codepoints as 1, some of the asians always as 2. Might be the problem here.
Because of this I already started some conceptual work to make wcwidth customizable to better reflect the system env. What still bugs me is the grapheme clustering, not sure yet if it is worth to get those working.

@j0hnm4r5
Copy link

j0hnm4r5 commented May 14, 2018

Codepoints for these icons Nerd Fonts:

Left Half Circle: e0b6
Node Logo: e718

The issue occurs regardless of the exact icon, though. Looking at the hundreds codepoints for Nerd Fonts (which include Powerline codepoints), all of them seem to be eXXX or fXXX. It looks like Powerline starts at e0a0, and Pomicons starts at e000, and Material Design ends at f999

@jerch
Copy link
Member

jerch commented May 19, 2018

@j0hnm4r5
Tried to reproduce the problem for e0b6 and e718 but cant. Both chars map to 1 here and are shown as single cell char (ubuntu, tested with vscode and hyper)

The current wcwidth sets a width of 2 for the following segments:

        ucs >= 0x1100 && (
        ucs <= 0x115f ||                // Hangul Jamo init. consonants
        ucs === 0x2329 ||
        ucs === 0x232a ||
        (ucs >= 0x2e80 && ucs <= 0xa4cf && ucs !== 0x303f) ||  // CJK..Yi
        (ucs >= 0xac00 && ucs <= 0xd7a3) ||    // Hangul Syllables
        (ucs >= 0xf900 && ucs <= 0xfaff) ||    // CJK Compat Ideographs
        (ucs >= 0xfe10 && ucs <= 0xfe19) ||    // Vertical forms
        (ucs >= 0xfe30 && ucs <= 0xfe6f) ||    // CJK Compat Forms
        (ucs >= 0xff00 && ucs <= 0xff60) ||    // Fullwidth Forms
        (ucs >= 0xffe0 && ucs <= 0xffe6)));

eXXX is not mapped to 2 anywhere, there are only those above in the fXXX range mapped to 2.
What is your OS and your emulator (electron based?, browser based?) Have you made changes to wcwidth? What is the exact font file you are using?

@Tyriar
Is there any render logic that tries to adjust the wcwidth for some chars/glyphs?

@Tyriar
Copy link
Member Author

Tyriar commented May 19, 2018

Well there's this section which checks if the character will physically overlap and the next char is a space (32) it will be 2 wide.

// If the character is an overlapping char and the character to the right is a
// space, take ownership of the cell to the right.
if (this._isOverlapping(charData)) {
// If the character is overlapping, we want to force a re-render on every
// frame. This is specifically to work around the case where two
// overlaping chars `a` and `b` are adjacent, the cursor is moved to b and a
// space is added. Without this, the first half of `b` would never
// get removed, and `a` would not re-render because it thinks it's
// already in the correct state.
// this._state.cache[x][y] = OVERLAP_OWNED_CHAR_DATA;
if (x < line.length - 1 && line[x + 1][CHAR_DATA_CODE_INDEX] === 32 /*' '*/) {
width = 2;
// this._clearChar(x + 1, y);
// The overlapping char's char data will force a clear and render when the
// overlapping char is no longer to the left of the character and also when
// the space changes to another character.
// this._state.cache[x + 1][y] = OVERLAP_OWNED_CHAR_DATA;
}
}

@j0hnm4r5
Copy link

@jerch, it happens in both VSCode and Hyper, but not in iTerm. I’m on macOS 10.13.

I have not done anything with wcwidth; should I?

I’m using Hurmit Nerd Font

@jerch
Copy link
Member

jerch commented May 20, 2018

@Tyriar
Thx for pointing me to this. It is also the place where grapheme support would need to be hooked into once we decide to support them (see these older issues about it #72, #942). Grapheme support might also interact with font ligatures if the font supports them for a given grapheme cluster, therefore I gonna wait until we have settled that.
I am not yet conviced it causes the problem of this issue though.

@j0hnm4r5
I need to track down first where the additional space is coming from. I have installed the powerline thing now and it still gives me the 1 width chars only (the prompt renders correctly here). Maybe it is caused by zsh on the slave side thinking it needs to give 2 cells width. Can you try to get the prompt bytes values? (Preferably as a hex sequence, you can grab it by starting up a shell with node-pty and reading the response)

@j0hnm4r5
Copy link

I don't think it's an extra space; I think it's rendering the space without a background color. You can see it in these two images:

Hyper
Hyper

iTerm2
iTerm2

The character length of the prompts remains the same between the two, but the Hyper version is not giving some of the spaces a background color.


I think I figured out the byte thing; I'm getting three Buffers on starting a new ZSH session:

1b5b316d1b5b376d251b5b32376d1b5b316d1b5b306d202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020200d200d

0d1b5b306d1b5b32376d1b5b32346d1b5b4a0d0a1b5b33396d1b5b306d1b5b34396d1b5b34306d201b5b39336def84891b5b33396d201b5b39336d4d4152532d446565706c6f63616c201b5b34366d1b5b33306dee82b4201b5b33306def829e1b5b33396d201b5b33306d31302e302e312e313035201b5b34396d1b5b33366dee82b41b5b33396d0d0a1b5b34346d201b5b33306def80951b5b33396d201b5b33306d7e201b5b34396d1b5b33346dee82b41b5b33396d201b5b4b1b5b3638431b5b33396d1b5b306d1b5b34396d1b5b33306dee82b61b5b33396d1b5b34306d1b5b33326d201b5b33326def808c1b5b33396d201b5b33396d1b5b30306d1b5b34396d1b5b373244

1b5b3f31681b3d1b5b3f3230303468

And just to double check to make sure I'm doing it right (I've never done anything with node-pty before, let alone any terminal development), here's what my code looks like, and the simultaneous output:
VSCode

@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2018

We can get more readable output by running the xterm.js demo, opening devtools console and running term.setOption('debug', true). That will output to the console the text form of what's interpreted by xterm.js.

@j0hnm4r5
Copy link

14:18:23.332 term.setOption('debug', true)
14:18:23.336 undefined
14:18:25.421 xterm.js:4795 data: p undefined
14:18:25.522 xterm.js:4795 data: �pw undefined
14:18:25.780 xterm.js:4795 data: d undefined
14:18:26.714 xterm.js:4795 data: �[?1l�> undefined
14:18:26.715 xterm.js:4795 Switching back to normal keypad. undefined
14:18:26.715 xterm.js:4795 data: �[?2004l

 undefined
14:18:26.715 xterm.js:4795 data: /Users/jmars/Downloads/xterm.js-master
 undefined
14:18:26.716 xterm.js:4795 data: �[1m�[7m%�[m�[1m�[m                                                                                      
 
 undefined
14:18:27.097 xterm.js:4795 data: 
�[m�[m�[m�[J
�[39m�[m�[49m�[40m �[39m�[39m �[39mMARS-Deeplocal �[46m�[30m �[30m�[39m �[30m10.0.1.105 �[49m�[36m�[39m
�[44m �[30m�[39m �[30m~�[37m  �[30mDow…�[37m  �[30mxterm.js-master �[49m�[34m�[39m �[K�[50C�[39m�[m�[49m�[30m�[39m�[40m�[32m �[32m�[39m �[39m�[00m�[49m�[54D undefined
14:18:27.097 xterm.js:4795 data: �[?1h�= undefined
14:18:27.098 xterm.js:4795 Serial port requested application keypad. undefined
14:18:27.098 xterm.js:4795 data: �[?2004h undefined

@jerch
Copy link
Member

jerch commented May 21, 2018

@j0hnm4r5 Oh weird, your left side looks normal. (in hyper, the web thingy seems broken. Missing font maybe?)

@j0hnm4r5
Copy link

j0hnm4r5 commented May 21, 2018

I just changed the browser font; it does look normal here. Very weird.

@jerch
Copy link
Member

jerch commented May 21, 2018

Just to make sure - can you temporarily comment out the snippet Tyriar posted and rerun it? (If you cannot edit it you can step debug it in developer tab and alter the stack values)

Edit: Or even simpler - place a breakpoint at width = 2 - this should not trigger at all if the chars are ok.

@j0hnm4r5
Copy link

Lines 82-100 in TextRenderLayer.ts? Still looks okay:

@jerch
Copy link
Member

jerch commented May 21, 2018

Can you do the breakpoint thing in hyper instead?

Btw your posted escape sequences above look good, there is no additional space, so it does not come from the shell.

@j0hnm4r5
Copy link

What do you mean by the breakpoint thing in Hyper?

@jerch
Copy link
Member

jerch commented May 21, 2018

Sorry that I cant help you better, but as long as I cant repro the issue I cant do it myself.

With breakpoint I mean:

  • open the developer tab
  • go to sources
  • select 'bundle.js'
  • hit the '{}' button below the right window
  • search for overlapping in the code
  • place a breakpoint there by clicking on the line number column (note the code looks abit different since it got drilled down to the JS version, the line should still be there in a slightly different version)
  • press enter in the terminal window

Now if the prompt shows up normally, the window is not halted by the breakpoint (you can check by simply pressing some keys, if they show up the terminal still runs). If it got halted, then the overlapping code got executed.

@j0hnm4r5
Copy link

Hey, I'm learning a ton! And I really appreciate you helping me out.

It is definitely getting caught at that breakpoint, and running each character displayed in the entire terminal through that function every update.

It's different code than what @Tyriar posted:

TextRenderLayer.prototype._isOverlapping = function(e) {
            if (1 !== e[o.CHAR_DATA_WIDTH_INDEX])
                return !1;
            var t = e[o.CHAR_DATA_CODE_INDEX];
            if (256 > t)
                return !1;
            var n = e[o.CHAR_DATA_CHAR_INDEX];
            if (this._characterOverlapCache.hasOwnProperty(n))
                return this._characterOverlapCache[n];
            this._ctx.save(),
            this._ctx.font = this._characterFont;
            var r = Math.floor(this._ctx.measureText(n).width) > this._characterWidth;
            return this._ctx.restore(),
            this._characterOverlapCache[n] = r,
            r
        }

I put breakpoints at all three of those return statements, and (I'm not sure if this is helpful at all), but here's where it's stopping at each character, as it goes through "MARS-Deeplocal" and the IP address.

@jerch
Copy link
Member

jerch commented May 21, 2018

Thx for getting this running. Yes it looks like the overlapping is triggering it.

@Tyriar Any suggestions on how to fix this? (Edit: Prolly fixed already)

Edit:
@j0hnm4r5 Seems this code was changed recently and hyper uses an older xterm.js version. This might explain why you see this in hyper but not in the web demo. I fear you have to wait for an updated version. Anyways - thx a lot for your debugging, it was very helpful. 👍

@Tyriar
Copy link
Member Author

Tyriar commented May 21, 2018

Hmm. Well one important difference with Hyper and the demo is that Hyper is running on the dynamic char atlas. If you add , experimentalCharAtlas: 'dynamic' to this line to can turn it on in the demo:

term = new Terminal({

If it still works after that change I would wait on Hyper to pick up v3.4 that we released yesterday and maybe jump on the Canary build if you're not already so you get it faster. vercel/hyper#3032

@j0hnm4r5
Copy link

j0hnm4r5 commented May 21, 2018

@jerch Same with VSCode?

@Tyriar That does still work! I'll just wait over here patiently for it to drop.

Thank you both for all the help, and for putting up with my lack of knowledge!

@Tyriar
Copy link
Member Author

Tyriar commented May 22, 2018

@j0hnm4r5 tomorrow's VS Code Insiders build will have almost what's on xterm.js' master branch. It just lacks the new parser PR which I don't think would impact your issue.

@jerch
Copy link
Member

jerch commented May 23, 2018

@Tyriar
Since the problem behind this issue is directly renderer related, i gonna open a new issue that will address the problem of a configurable wcwidth and therefore track the ambiguous width handling as well. #1467

@j0hnm4r5
Copy link

Great news! It has been fixed in the VSCode Insiders build!

Thanks for all of your help.

@Tyriar
Copy link
Member Author

Tyriar commented May 23, 2018

@j0hnm4r5 nice 👍, @bgw probably fixed it. Closing this off since we have #1467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted type/enhancement Features or improvements to existing features
Projects
None yet
Development

No branches or pull requests

3 participants