-
Notifications
You must be signed in to change notification settings - Fork 50
Windows: Always set codepage explicitely #143
base: master
Are you sure you want to change the base?
Conversation
if (json.xwalk_windows_codepage) { | ||
this._windowsCodepage = json.xwalk_windows_codepage; | ||
} else { | ||
this._windowsCodepage = "1252"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment, describing what '1252' actually means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even have a const variable with descriptive name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we not just demand the use of UTF-16? isn't that not already standardized in JSON as it is JS after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenchris because Windows (WiX) does not support it for strings that are displayed in the installer UI.
lgtm with comment |
By default, use codepage 1252 explicitely as default in manifest.json. Always put the explicit codepage info into the installer. When using non-ANSI strings in manifest.json, it is needed to adjust the codepage accordingly. BUG=APPTOOLS-338
* When using ISO 8859-1 unsupported characters in the manifest, | ||
* the codepage needs to be adjusted accordingly. | ||
*/ | ||
Manifest.WINDOWS_DEFAULT_CODEPAGE = "1252"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenchris because Windows (WiX) does not support it for strings that are displayed in the installer UI.
You have "lang" so if "lang": "cn"
you can convert UTF-16 to the codepage required for CN. That would be the proper fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lang is part of the manifest spec @anssik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might have to merge those two tables and use it then.
https://msdn.microsoft.com/en-us/library/ee825488(v=cs.20).aspx
https://msdn.microsoft.com/en-us/library/ms930130.aspx
By default, use codepage 1252 explicitely as default in manifest.json.
Always put the explicit codepage info into the installer. When using
non-ANSI strings in manifest.json, it is needed to adjust the codepage
accordingly.
FIXES=APPTOOLS-338