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.

Sign up for GitHub

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

Jump to bottom

Possible regression of emoji rendering since 0.6 #3553

Closed
zadjii-msft opened this issue Nov 13, 2019 · 15 comments · Fixed by #3582
Closed

Possible regression of emoji rendering since 0.6 #3553

zadjii-msft opened this issue Nov 13, 2019 · 15 comments · Fixed by #3582
Assignees
@DHowett-MSFT
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal-1911

Comments

@zadjii-msft
Copy link
Member

@DHowett-MSFT my man. I'm seeing regression on current master 306e751

图片

Originally posted by @skyline75489 in #3546 (comment)
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Nov 13, 2019
@zadjii-msft zadjii-msft added this to the Terminal-1911 milestone Nov 13, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 13, 2019
@zadjii-msft
Copy link
Member Author

I'm going to pre-emptively mark this as blocking. We can discuss in triage if this isn't that bad of a regression.

UNFORTUNATELY, I'm not seeing this at all myself :( Black is 0.6, purple is master:
image

@skyline75489
Copy link
Collaborator

Could it be my locale setting? I'm in zh-CN locale.

@zadjii-msft zadjii-msft mentioned this issue Nov 13, 2019
Closed
@zadjii-msft
Copy link
Member Author

@skyline75489 That's the first thing I thought of. I'd really hate if a CRT change like that would cause this, but hey, who knows.

Just to try and rule out other factors, could you try the following python script?

print('''\
Combining Diacritics    | [a\u0300\u0304]
Polytonic Greek         | [ᾊ] [Α\u0313\u0300\u0345]
Ideographic Variation   | [東京都葛\U000E0101飾区] [奈良県葛\U000E0100城市]
Composite Hangul        | [\u1100\u116A\u11B3]
Combining Dakuten       | [か\u309A]
Emoji vs Text           | [\u2602] [\u2602\uFE0E] [\u2602\uFE0F]
Emoji                   | [\U0001F469]
Emoji with Skin Tone    | [\U0001F469\U0001F3FB]
Emoji Variation and ZWJ | [\U0001F469\U0001F3FB\u200D\U0001F4BB]
''')

That should be the same as the Node.JS one - I just want to make sure to rule Node out in this case.

@skyline75489
Copy link
Collaborator

I'm seeing the same thing with python.

图片

@skyline75489
Copy link
Collaborator

skyline75489 commented Nov 13, 2019 •
edited
Loading

As long as I revert ddcc06e (even on master), everything is back to normal. Well not really “normal” but relatively “normal”. You got the idea.

@DHowett-MSFT
Copy link
Contributor

@skyline75489 what if you only revert the _VC_Target_Library_Platform part?

@miniksa
Copy link
Member

miniksa commented Nov 13, 2019

You've got to be kidding me. The App CRT is different by locale than the Desktop CRT? :|

@skyline75489
Copy link
Collaborator

Would it be even more weird if I say that DynamicLibrary is the key? I change winconpty back to DynamicLibrary and it works. Both _VC_Target_Library_Platform and onecoreuap_apiset.lib are seemingly irrelevant.

@DHowett-MSFT
Copy link
Contributor

Of course! This is the 19H1 emoji bug! @miniksa we aren’t linking the right CreatePseudoConsole. It’s probably the linkage declspec...

@DHowett-MSFT
Copy link
Contributor

We can’t repro it locally because the Vb conhost has the fixes.

@skyline75489
Copy link
Collaborator

Glad you know where the problem is. Guess now I don't need to change my PC's locale to en-US 😄

@DHowett-MSFT
Copy link
Contributor

I’m assigning myself because I have the most context.

@DHowett-MSFT DHowett-MSFT self-assigned this Nov 14, 2019
@DHowett-MSFT DHowett-MSFT added Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface and removed Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 14, 2019
DHowett-MSFT pushed a commit that referenced this issue Nov 14, 2019
@DHowett
Force the use of the static pseudoconsole functions in TConn
0d106c8 
Fixes #3553.
@DHowett-MSFT DHowett-MSFT mentioned this issue Nov 14, 2019
Merged
@ghost ghost added the In-PR This issue has a related PR label Nov 14, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Nov 15, 2019
Closed
@JustinGrote
Copy link

JustinGrote commented Nov 15, 2019 •
edited
Loading

Potentially related, some emojis are super tiny on my system in the console but fine in the titlebar? (They are fine in VSCode et. al.)
image

Just checked and I don't think it's a DPI issue, I dropped mine down to 100% from 175% and same issue.

DHowett-MSFT pushed a commit that referenced this issue Nov 16, 2019
@DHowett
Force the use of the static pseudoconsole functions in TConn (#3582)
cc8faaf 
This commit renames the functions in conpty.lib to Conpty* so that they
can be explicitly linked and introduces a header so they can be located.

It also updates the DEF for conpty.dll to reexport them with their
original names.

The crux of the issue here is that TerminalConnection is consuming the
_import_ symbols for the *PseudoConsole family of APIs, which simply
cannot be supplanted by a static library.

Avenues explored: * Exporting __imp_x from the static library to get all
up in kernel32's business.  * Using /ALTERNATENAME:__imp_X=StaticX. It
turns out ALTERNATENAME is only consulted when the symbol isn't found
through traditional means.

This, renaming them, is the straightest path forward.

Fixes #3553.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Nov 16, 2019
@DHowett-MSFT
Copy link
Contributor

@JustinGrote that one is #900 😄

@DHowett-MSFT
Copy link
Contributor

(also, we don't support VS16/U+FE0F for a bunch of reasons that are being covered in active threads about unicode (#1472))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DHowett-MSFT DHowett-MSFT

Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal-1911
Development

Successfully merging a pull request may close this issue.

Force the use of the static pseudoconsole functions in TConn microsoft/terminal
5 participants
@skyline75489 @DHowett-MSFT @JustinGrote @miniksa @zadjii-msft