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

Incorrect table alignment under some terminal emulators due to double-width characters #111

Closed
ronjouch opened this issue Mar 4, 2017 · 15 comments
Milestone

Comments

@ronjouch
Copy link

ronjouch commented Mar 4, 2017

This is a followup to discussion on Alacritty commit 3ad68699 (Alacritty is a "cross-platform, GPU-accelerated terminal emulator").

The problem is that, in some terminals, wego 2.0 table alignment suffers from the width of the unicode characters used (cloud, sun, etc). See for yourself:

  • GNOME Terminal:
    selection_002
  • Alacritty:
    selection_001

To me that looked like a bug in Alacritty, but its main developer, @jwilm, comments that:

I believe this is a bug with wego. They don't account that terminals may display a cloud as 1 or 2 columns. GNOME Terminal appears to think the cloud icons are only single width. Alacritty (correctly) handles them as double width. We are using the Unicode 9 definitions for wide chars, so that may account for the difference.

In the wego emoji frontend,

ret = append(ret, fmt.Sprintf("%v %v %v", cur[1], icon[0], c.formatTemp(cond)))

you can see that there's a space after the icon in the Sprintf format string. It's not visible in GNOME Terminal because it's under the emoji. You can see it in Alacritty because we account for the double width characters.

wego should be using the goto functionality of terminals to handle both cases.

@schachmat
Copy link
Owner

Heyho @ronjouch and @jwilm,

Both Icons you are using in your screenshot (U+26c5, U+26c8) are so called "east asian ambiguous" codepoints. These must be rendered differently depending on your locale. I just added a check to the emoji frontend and tested it works for two differently used locales and in two terminal emulators.

@ronjouch
Copy link
Author

ronjouch commented Mar 6, 2017

@schachmat thanks for the fast response. That doesn't look fixed to me, though; see screenshot below. Am I missing something?

wego-alacritty-not-fixed_2

@schachmat
Copy link
Owner

schachmat commented Mar 6, 2017

Heyho @ronjouch,

The issue seems to be that alacritty does not support east asian ambiguous characters yet. In your screenshot the icons seem to occupy 2 cells, so I assume alacritty currently always renders as in an east-asian locale. The following dirty workaround might provide a correct rendering until the alacritty issue is resolved:

  • Install an east-asian locale like ja_JP.UTF-8.
  • Always run wego like this: LC_CTYPE="ja_JP.UTF-8" wego [PARAMETERS].

If you installed the locale as above you can test with xterm -cjk_width and the LC_CTYPE setting against xterm without the LC_CTYPE. It should provide a two cell wide "partly cloudy" icon in the first and a one cell wide icon in the second case.

@jwilm
Copy link

jwilm commented Mar 6, 2017

@schachmat only one of the glyphs you linked to is ambiguous width. These classifications changed in Unicode 9.0, and it might be that wego and Alacritty are using different definitions.

I assume alacritty currently always renders as in an east-asian locale.

Quite the opposite, actually. We don't correctly handle ambiguous width characters for those locales, but they should be fine for everyone else.

@ronjouch
Copy link
Author

ronjouch commented Mar 6, 2017

I'm a bit lost among the terminalese, but in case that helps/confirms what you are discussing, @schachmat @jwilm, the LC_CTYPE trick improves things, but only partially works around the issue:

wego-lc_ctype

@schachmat
Copy link
Owner

Heyho @jwilm, @ronjouch,

Thanks for the update. wego is using go-runewidth from @mattn to compute rune widths. This package seems to use unicode 9.0 for a while now, so if alacritty is also using unicode 9.0 the definitions should be the same, right?

The issue in the workaround screenshot is probably related to the dash between the min and max temperatures. Maybe the width property of that rune changed as well or the original author of the frontend did not notice the issue in the first place. I'll investigate.

@schachmat schachmat reopened this Mar 7, 2017
@schachmat schachmat added this to the 3.0 milestone Mar 7, 2017
@mattn
Copy link
Contributor

mattn commented Mar 7, 2017

Unfortunately, go-runewidth had switch to Unicode 9.0 . I'm sorry if you get wrong.
As you mentioned at here, some character's widths are changed at the commit. If you want to specify CJK width forcibly, please set runewidth.DefaultCondition.EastAsian = true.

@schachmat
Copy link
Owner

Heyho @mattn, @jwilm,

I am totally for using the latest unicode standard! My problem is that still I don't quite understand where the issue comes from exactly.

To provide a correct table layout, wego needs to know how wide each rune is going to be rendered. Therefore I use go-runewidth which should give the correct width according to the unicode 9.0 standard. Now for the whole system to work there are two requirements. First, the terminal emulator obviously has to use the same standard, which is why I prefer the latest one available ("support latest standard" is a better feature-request than "please don't support the latest standard" 😉). Second, go-runewidth and the terminal emulator must agree on the ambiguous wide runes (do we use double width or single width?). To my knowledge of terminal emulators this can only be achieved portably with locales - there does not seem to be a way for applications to force the terminal emulator to use a specific setting. Therefore setting runewidth.DefaultCondition.EastAsian = true will not work as it only fixes the setting for one of both components.

Now the issue could be located in either of our three projects. I'll check my code again and also try to verify that go-runewidth returns the width I am expecting for each rune. Since I don't have a rust environment installed I can't check alacritty currently.

@jwilm
Copy link

jwilm commented Mar 7, 2017

Thanks for the detailed follow up @schachmat.

My problem is that still I don't quite understand where the issue comes from exactly.

That makes (at least) two of us!

I've spent some more time investigating on my side this morning, and there's a chance this is due to Alacritty not handling the variation selector on some of those glyphs (perhaps rendering them as an extra space). I don't know conclusively yet, but I'll follow up as time permits.

@jwilm
Copy link

jwilm commented Mar 7, 2017

I just noticed that rendering appears to be broken in gnome-terminal in the latest version.

image

The same version in Alacritty has the opposite spacing problem:

image

Thinking about it a bit more, command-line applications are always going to have the problem that some terminals use older Unicode definitions, and others newer. The only way to handle both properly is to avoid reliance on the terminal spacing for wide characters. This could be done via goto column commands. The escape sequence is \x1b[%dG to go to a specific column. The emoji renderer should probably use that for maximum portability. For a well-known example, vim does something very similar when encountering wide characters.

Naturally there's still an issue with Alacritty and the variation selector handling.

@jwilm
Copy link

jwilm commented Mar 7, 2017

After fixing the variation selector bug in Alacritty, the current version of wego renders correctly:

image

@schachmat
Copy link
Owner

Heyho @jwilm,

Thanks for your thoughts on the issue and the quick fix of the variation selector code!

To be honest I don't even know, why the author of this frontend used a variation selector for the PartlyCloudy icon in the first place.

I'll think about using the goto column escape code, but intuitively I don't think it's the best solution:

  • It provides maintainers with an argument for laziness (well, we don't have to adapt the new unicode standard yet…).
  • There might be overdraw after the border at the end of line. Assume a japanese description text rendered longer than the cells right-border.

I'll leave this issue open until I have a final decision. Thank you all for participating!

@ronjouch
Copy link
Author

After fixing the variation selector bug in Alacritty, the current version of wego renders correctly

@jwilm just to be sure: I don't see it fixed here (fresh Alacritty + wego HEAD builds), I guess your fix is still in an unmerged branch?

@jwilm
Copy link

jwilm commented Mar 15, 2017

@ronjouch Yeah, still on my machine. I'll push up a branch, but it's not ready to merge into master. Let's continue this discussion on the Alacritty tracker or IRC.

@schachmat
Copy link
Owner

So I assume there is nothing left to do here and will close the issue now. If you find further issues feel free to reopen or create a new issue.

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

No branches or pull requests

4 participants