-
Notifications
You must be signed in to change notification settings - Fork 150
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
fix decode error #491
fix decode error #491
Conversation
Thanks for this PR! Why are you removing the And I thought this only affects ASIO? Why are you applying it to all host APIs? |
sounddevice.py
Outdated
try: | ||
name = name_bytes.decode(_locale.getpreferredencoding()) | ||
except UnicodeDecodeError: | ||
name = repr(name_bytes)[2:-1] |
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.
What about using this instead?
name = repr(name_bytes)[2:-1] | |
name = name_bytes.decode('ascii', errors='backslashreplace') |
The result should be the same, but this would be more descriptive, I think.
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.
But maybe we don't need this fallback anyway.
I think I would prefer to get another error report (just like yours) if it turns out that some system uses yet another unexpected encoding.
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 we can keep this fallback, but use print(e)
to output the error so the user can continue to use it or come and submit the error.
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.
I'm strongly against using print()
.
As far as I know, the default encoding for some regions of Windows is UTF-16, and mbcs can't decode it, so I tried to get the default encoding for the user's system and use it.
I don't think judging the host API is necessary, if an error occurs decoding in UTF-8, then just try to decode it using another encoding. |
This may well be, but do you know for sure whether each single host API actually uses the default encoding?
It may not be, but I don't know that for sure. I only know that this was tested on multiple systems in #72. And AFAIU, you only saw the problem with ASIO and not with any other host API, right? The problem is that I cannot test any of this myself and I have to rely on reports by other people. |
I certainly can't test all versions of Windows, so I added the fallback just so I can continue to use it if the name decoding fails. PS. I think it's very frustrating to have an entire program unavailable because the name of some mysterious device was decoded incorrectly.
Yes, but I don't think it's going to get any worse, for the reasons I stated above, utf-8 is still the first-order decoding scheme. |
Yeah, but maybe this leads to worse behavior (unreadable device names) in some cases that did work before. We don't know that, but I would like to not risk that.
I agree. That's why it would be great to fix the error with ASIO that you actually witnessed. However, it would also be frustrating if the program does not crash but instead some device names contain gibberish. In either case (crash or gibberish), I have to rely on user feedback (like yours) to be able to fix the problem. And I'm open to fix every single problem that comes up. I just want to be cautious not to re-introduce errors on systems that have been working correctly before.
That's the problem, I don't know that. Maybe it gets worse, maybe it doesn't. That's why I would like to keep the behavior that has actually be tested and confirmed to work correctly and only fix the behavior that has actually been observed to be broken. |
Hello @shoucandanghehe! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
I have created an alternative to this PR: #512. @shoucandanghehe Can you please check if that also fixes your problem? |
Actually, I don't know what happened to my driver, anyway that device disappeared😇 |
I have merged #512 instead. |
I tried to introduce the locale package to get the default encoding of the user's system.
And I think there shouldn't be any encoding types other than those two.
But I can't be sure, so I just output bytes. that way there won't be any more errors caused by decoding failures.
close #490