*.rc: escape non-ASCII/non-UTF-8 character for clarity #1217

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@vszakats
Contributor

No description provided.

@vszakats vszakats changed the title from *.rc: escape non-ASCII/non-UTF8 character for clarity to *.rc: escape non-ASCII/non-UTF-8 character for clarity Jan 17, 2017
@bagder
bagder approved these changes Jan 17, 2017 View changes

I'm fine with this. Maybe add a comment above, explaining what \xA9 will render as?

@vszakats
Contributor

Agreed. Added a comment about the meaning of \xA9.

@jay
Member
jay commented Jan 17, 2017

I'm just curious in your codepage what does © look like

@vszakats
Contributor
vszakats commented Jan 17, 2017 edited

It comes down to how the codepage of the file is (mis)detected/-interpreted by the editor/viewer at hand.

F.e. in my regular editor (mcedit), I've set the default encoding to UTF-8, so it looks like this:
screen shot 2017-01-17 at 19 39 13

In the browser (Safari) this particular github.com PR diff looks like this for the first hunk (and appears fine for the second hunk):
screen shot 2017-01-17 at 19 45 11

It's generally difficult to predict how 8-bit codepages behave using default configurations in different environments, so one solution I found to work is to stick to 7-bit ASCII or UTF-8 (where 7-bit ASCII is not enough) for text files. In case of Windows Resources this may not always be practical due to the spotty support for UTF-8 encoded .rc files, though it can work with the more popular C toolchains.

@vszakats
Contributor

FWIW, here's a commit that implements UTF-8 Windows Resources for another project:
vszakats/harbour-core@10d2537

@jay
Member
jay commented Jan 18, 2017

Looking at that makes me wonder why our resource file translation field is currently set to 1200 (Unicode) if the block is not in any Unicode encoding. Is that a mistake and should it be 1252 (ANSI/Latin1) instead?

Also, though there's no hard-and-fast rule for this the traditional practice for PR branches is to go on your origin instead of upstream. An exception would be multiple project collaborators working on a branch like recall the https proxy branch that was around for a while or now to a much lesser extent generate-curl-1. A small change like this will be transient though and not that.

@vszakats
Contributor
vszakats commented Jan 18, 2017 edited

@jay As for 1200 vs 1252 I think it's fine as it as, and although I've yet to find definitive information, my experience is that this value doesn't influence how resource compilers interpret the .rc file in any particular codepage. For that, most RC tools offer a command-line option (usually -c). Nor does it seem to control how values are stored inside the PE image, which is always UTF-16. So what does it control? Hard to tell. Maybe \xA9 works because it means copyright symbol in Unicode as well.

I've updated the patch to leave out guesses about the codepage being used in relation to \xA9.

The branching is noted, again something that crossed my mind, but decided this is such a simple change that the branch will be short lived.

@vszakats vszakats *.rc: escape non-ASCII/non-UTF-8 character for clarity
023cf1a
@vszakats
Contributor

Converted hex to lowercase to match rest of file and cleaned the comment further.

@jay
Member
jay commented Jan 19, 2017

Ok LGTM, please make sure to reference this discussion in the body of the upstream commit message with a Ref: or Closes: line, for example one of these
Ref: <url>
Ref: https://github.com/curl/curl/pull/1217
Closes https://github.com/curl/curl/pull/1217
Closes #1217 etc

@vszakats vszakats closed this in df86db7 Jan 19, 2017
@vszakats
Contributor

Thank you @jay. I hope I got it right!

@vszakats vszakats deleted the winresesc branch Jan 19, 2017
@jkralik jkralik added a commit to jkralik/curl that referenced this pull request Jan 23, 2017
@vszakats @jkralik vszakats + jkralik *.rc: escape non-ASCII/non-UTF-8 character for clarity
Closes curl#1217
165832f
@peterpih peterpih pushed a commit to railsnewbie257/curl that referenced this pull request Jan 24, 2017
@vszakats @railsnewbie257 vszakats + railsnewbie257 *.rc: escape non-ASCII/non-UTF-8 character for clarity
Closes curl#1217
b8a1015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment