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

Fix incorrect code page in console title #25

Closed
wants to merge 1 commit into from

Conversation

gdh1995
Copy link

@gdh1995 gdh1995 commented Jul 31, 2020

WriteConsoleA always follows the current code page of a console window, so it's not suitable to pass a multi-byte string in get_ttyp ()->term_code_page to it directly.

This PR turns to WriteConsoleW so that most characters will be translated "as is", and I've tested it on Win 10 v2004 (CMD: ver 10.0.19041.388).

This PR will fix git-for-windows/git#2738 .

`WriteConsoleA` always follows the current code page of a console window, so it's not suitable to pass a multi-byte string in `get_ttyp ()->term_code_page` to it directly.

This PR turns to `WriteConsoleW` so that most characters will be translated "as is", and I've tested it on Win 10 v2004 (CMD: ver 10.0.19041.388).

Signed-off-by: gdh1995 <gdh1995@qq.com>
@gdh1995 gdh1995 force-pushed the fix-title-for-conpty branch from 21d48bb to 334f52a Compare July 31, 2020 14:19
@dscho
Copy link
Member

dscho commented Aug 11, 2020

Interesting find!

However, wouldn't it make sense to change that call to WriteConsoleA() in wpbuf's send() method to convert to Unicode and call WriteConsoleW() instead?

Also, since this is safely in the non-MSYS2 part of Cygwin, have you considered contributing this to cygwin-patches?

@gdh1995
Copy link
Author

gdh1995 commented Aug 11, 2020

Um, I'm not sure what it will influence if update wpbuf.send directly. Maybe it will make most code work better, but I have no enough knowledge about how to test it and what is expected. I don't know how to use cygwin-patches, either.

So, please feel free to replace this PR with some other commits of yours.

@dscho
Copy link
Member

dscho commented Aug 12, 2020

I'm not sure what it will influence if update wpbuf.send directly.

If your patch is correct, it will only make things better, not worse.

However, the current patch does make things worse because it fixes the encoding issue only in one location, by completely re-implementing the send() method, leaving the send() method itself broken.

So, please feel free to replace this PR with some other commits of yours.

I do not have the time.

@dscho
Copy link
Member

dscho commented Aug 25, 2020

Hmm. Having had a look at the callers of send(), I agree that this would be too dangerous.

Maybe you want to take this discussion to Cygwin, maybe by sending your patch as an "RFC" to https://cygwin.com/mailman/listinfo/cygwin-patches/?

@gdh1995
Copy link
Author

gdh1995 commented Aug 26, 2020

OK, I'll take a try on cygwin-patches

@dscho
Copy link
Member

dscho commented Aug 27, 2020

I cherry-picked Takashi's fix, and am building a new msys2-runtime package: https://dev.azure.com/Git-for-Windows/git/_build/results?buildId=61752&view=results

@dscho dscho closed this Aug 27, 2020
@gdh1995 gdh1995 deleted the fix-title-for-conpty branch August 29, 2020 11:27
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.

v2.27.0 cannot show Chinese characters on the console window's title
2 participants