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

Solves #44 wrong encoding #58

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Jakovb
Copy link

@Jakovb Jakovb commented Jun 7, 2018

Solves #44 !

Added .toString('binary') to make ä/ü/ö/etc characters readable. //avoiding null escape
Added .toString('binary') to allow special characters such as ä/ü/ö/ //etc
@RupW
Copy link

RupW commented Jun 15, 2018

I don't think this is the correct fix. The problem is that we're using RegQueryValueExA which returns strings in the current Windows code page and then we're treating that as if it's UTF-8 text. Which means that as it stands we can only correctly read ASCII (code points 0-127); this change means we now support 128-255 where the Windows code page matches code points 128-255 of Unicode, which is more or less Latin-1, but it's not a complete fix.

I think instead the fix is to use the -W functions throughout, i.e. RegQueryValueExW here, which use UTF-16 strings which can represent all Unicode code points, and use Node's Buffers to encode and decode strings in utf16le. I have spent a little time looking at this but have run into some issues getting the tests working (registry permissions on Windows 10, reg delete doesn't work?). I'll push my work-in-progress to a branch when I get a chance.

In any case please squash these five commits down to a single commit so that it's obvious you're only actually changing the one line!

@Jakovb
Copy link
Author

Jakovb commented Jun 15, 2018

I've been playing with ExW myself and that was my original idea. And alike you was getting a plenty of side effect issues.
Which made me just rewrite this and adding binary there does the trick.
Please do try with my solution and report on results.

Will squash-apologizes for that.

@RupW
Copy link

RupW commented Jun 15, 2018

I'm not sure my issues were side effects of my change though: the unit tests don't work for me either on master or on your branch. I can't test right now but I'd guess your version won't work with e.g. curly quotes, i.e. “”, since they're characters >255.

FWIW here's my branch but I'm not sure when I'll get time to work on this more: https://github.com/RupW/windows-registry-node/commits/issue-44
The last two commits are relevant.

@RupW
Copy link

RupW commented Jun 15, 2018

I have fixed up my RegQueryValueExW commit: RupW@3fdbae5

and it will successfully read value 'test_value £ “” ä/ü/ö' from the registry. This version won't return the curly quotes for me (but does return the £ and accented characters which the current master doesn't).

@Jakovb Jakovb changed the title Solvoes #44 wrong encoding Slvoes #44 wrong encoding Jun 22, 2018
@Jakovb Jakovb changed the title Slvoes #44 wrong encoding Solves #44 wrong encoding Jun 22, 2018
@zbynek
Copy link

zbynek commented May 16, 2019

For the record the changes mentioned above are part of #60

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.

3 participants