search

serialport / bindings-cpp

The C++ bindings for the node serialport project.
MIT License
21 stars 40 forks source link

fix: support wchar_t and Unicode encoding in .list() parameters #63

Closed artus9033 closed 1 year ago

artus9033 commented 2 years ago

Fixes #62

reconbot commented 2 years ago

We'd have this problem on OS X too right?

artus9033 commented 2 years ago

I don't have a way to test that now, but according to this it seems like chars outside of Windows do encode Unicode properly by default, so there should be no need to modify this implementation.

gcampbell-msft commented 2 years ago

@artus9033 @reconbot @HipsterBrown.

Bringing this up again, is this a valid fix for the issue? If chars outside of Windows do encode Unicode properly by default, that's great, but this fix is for the serialport_win.cpp, right?

I've made a comment here regarding why I'm inquiring about a fix for this. We have a feature that has some issues posted for this and I noticed the bug there that hasn't gotten much traction.

Let me know if there's any way I can help!

artus9033 commented 2 years ago

@artus9033 @reconbot @HipsterBrown.

Bringing this up again, is this a valid fix for the issue? If chars outside of Windows do encode Unicode properly by default, that's great, but this fix is for the serialport_win.cpp, right?

I've made a comment here regarding why I'm inquiring about a fix for this. We have a feature that has some issues posted for this and I noticed the bug there that hasn't gotten much traction.

Let me know if there's any way I can help!

Indeed, it is the fix you are thinking of. I believe your comment there is a bit off, as this is not about converting to UTF, but rather converting the code to a data structure that supports UTF. On Windows, char doesn't do that, but wchar_t does.

@reconbot are you going to review and merge this PR?

gcampbell-msft commented 2 years ago

@artus9033 Thanks for the comment and inquiring about it this is going to be merged. I'll keep an eye out for when this goes in!

gcampbell-msft commented 2 years ago

@reconbot @artus9033 Re-pinging this in the hope of getting it merged. Thanks!

artus9033 commented 1 year ago

@reconbot is there any chance that this will be verified and merged? The problem still persists and there has been no progress with this PR in months.

reconbot commented 1 year ago

I have no way of testing this personally but the code reads well. None of the maintainers have had time to work on serialport right now.

reconbot commented 1 year ago

:tada: This PR is included in version 10.7.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: