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

Warn if settings.json is not UTF-8 (at least in key binding) #10330

Open
KalleOlaviNiemitalo opened this issue Jun 4, 2021 · 2 comments
Open
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Milestone

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jun 4, 2021

Description of the new feature/enhancement

If settings.json is encoded in e.g. Windows-1252 rather than UTF-8, then Terminal should warn about that, rather than silently substitute U+FFFD replacement characters in strings. This would help users who edit settings.json with Visual Studio, which saves in the ANSI code page by default if the file does not appear to be UTF-8 already. The silent substitution is especially hard to diagnose if the only non-ASCII character is in a key binding that then just does not work.

Proposed technical implementation details (optional)

Apply settings.json as much as possible, but then pop up a message saying that the encoding is wrong.

AFAICT, jsoncpp gives the raw bytes of JSON string values to Terminal if there is are no backslash escapes. These bytes are UTF-8 if the file was correctly encoded. In Terminal, ConversionTrait<KeyChord>::FromJson calls til::u8u16, which calls MultiByteToWideChar without the MB_ERR_INVALID_CHARS flag. There are more til::u8u16 calls in other ConversionTrait specializations.

return _fromString(til::u8u16(keyChordText));

return til::u8u16(Detail::GetStringView(json));

const int lengthOut = MultiByteToWideChar(gsl::narrow_cast<UINT>(CP_UTF8), 0ul, in.data(), lengthRequired, out.data(), lengthRequired);

@KalleOlaviNiemitalo KalleOlaviNiemitalo added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 4, 2021
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 4, 2021
@KalleOlaviNiemitalo
Copy link
Author

It seems it would be much easier to throw an exception right away than save the error and keep processing the rest of the JSON tree. Wouldn't be much worse for the user, either.

@orcmid
Copy link

orcmid commented Jun 7, 2021

It seems it would be much easier to throw an exception right away than save the error and keep processing the rest of the JSON tree. Wouldn't be much worse for the user, either.

Yes, once it is not known exactly what the damage involves, stopping is better than continuing. A compromise (not unlike preprocessor/VC fails) is to count and quit when it is clear there is no simple continuation. But absent that, stopping and identifying the point of difficulty is appropriate, just like hitting an XML cliff. The error message should not be misleading - i.e., presume too much about the root cause.

@DHowett DHowett added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 6, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 6, 2021
@DHowett DHowett added this to the Terminal Backlog milestone Jul 6, 2021
@DHowett DHowett added the Help Wanted We encourage anyone to jump in on these. label Jul 6, 2021
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

4 participants