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

Display density changes lead to corrupted characters #2137

Closed
Eugeny opened this issue May 28, 2019 · 13 comments · Fixed by #2892
Closed

Display density changes lead to corrupted characters #2137

Eugeny opened this issue May 28, 2019 · 13 comments · Fixed by #2892
Labels
Milestone

Comments

@Eugeny
Copy link
Member

Eugeny commented May 28, 2019

When switching between a built-in Retina display and an external normal-density display, some characters will be drawn with incorrect size by the canvas renderer.

Now, I can't confirm whether it's possibly Electron's fault, but I suspect that Chrome would behave the same.

xterm 3.13.2

image

@Tyriar
Copy link
Member

Tyriar commented May 28, 2019

Might be rel;ated to ligature problems in #2135?

@Eugeny
Copy link
Member Author

Eugeny commented May 28, 2019

Might be, but I don't think so. Looks like some chars might be cached in the renderer atlas with a smaller resolution?

@Tyriar
Copy link
Member

Tyriar commented May 28, 2019

@Eugeny seems to me like the cached parts are -r, -x and --.

@Eugeny
Copy link
Member Author

Eugeny commented May 28, 2019

Good point - these are the ones that are incorrectly rendered in #2135

@Eugeny
Copy link
Member Author

Eugeny commented May 28, 2019

Weirdly enough, switching to a higher density makes these characters larger, and switching to a smaller density display makes them smaller.

@Tyriar
Copy link
Member

Tyriar commented May 28, 2019

The APIs that https://github.com/xtermjs/xterm-addon-ligatures use probably just need some updating for the recent updates to renderers.

@jerch
Copy link
Member

jerch commented May 28, 2019

Cannot repro this and the other issue #2135 (on Ubuntu though, with Fira Mono and ligatures enabled).
Kinda sounds as the dpr change gets not applied? @Eugeny Could you test, if the demo shows the same wrongly scaled glyphs when you switch the screens?

@Eugeny
Copy link
Member Author

Eugeny commented Jun 25, 2019

Still reliably reproducible in master with the webgl renderer:
image

The font doesn't get corrupted immediately after the DPI switch, but rather after font-family and font-size are updated (re-set to the same values) in xterm.

@yuezk
Copy link

yuezk commented Apr 20, 2020

@jerch @Tyriar
I came from the VS Code issue microsoft/vscode#94656
It can be reliably reproduced reliably on my macOS 10.15.3 with VS Code 1.44.2 and enabled the experimentalWebgl renderer type.

image

I have two displays show below
image

  1. Keep the external display connected.
  2. Open the Terminal if it hasn't been.
  3. Move the VS Code to the external display if it hasn't been.
    image
  4. Disconnect the external display.
  5. The VS Code will be moved to the builtin display automatically.
  6. The Terminal font corrupted.
    image

@Tyriar
Copy link
Member

Tyriar commented Apr 20, 2020

I've seen this happen a bunch, we're probably just missing a case in https://github.com/xtermjs/xterm.js/blob/master/src/browser/ScreenDprMonitor.ts, or something is not listening to the monitor.

@Eugeny
Copy link
Member Author

Eugeny commented May 2, 2020

@Tyriar in case this helps, it can be reliably reproduced with the demo page, but only for unicode characters. ASCII will get occasionally corrupted, but not always.

Changing font size to something else and back immediately fixes it.

@jerch
Copy link
Member

jerch commented May 2, 2020

@Eugeny Could you test if this can be repro'ed for ASCII as well, if the fg color is set to RGB?

I suspect that _drawUncachedChars does not get the DPR changes for some reason.

@Eugeny
Copy link
Member Author

Eugeny commented May 2, 2020

@jerch I've just pushed a PR with the fix. The listener wasn't being called at all.

@Tyriar Tyriar added this to the 4.6.0 milestone May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants