dechamps / FlexASIO

A flexible universal ASIO driver that uses the PortAudio sound I/O library. Supports WASAPI (shared and exclusive), KS, DirectSound and MME.
Other
1.29k stars 70 forks source link

Do support non-latin character sets with windows error text and do not limit in size. #229

Closed aholzinger closed 1 month ago

aholzinger commented 1 month ago

FormatMessageA does not work with a lot of languages. As the function returns utf8 anyhow it's better to use FormatMessageW. The new implementation also makes use of the abbility of FormatMessage to allocate the needed buffer. Like this the messages are not limited in size anymore.

aholzinger commented 1 month ago

The failing GitHub actions belong to fetching the ASIO SDK: -- verifying file... file='D:/a/FlexASIO/FlexASIO/src/out/build/x86-Debug/dechamps_ASIOUtil-prefix/src/dechamps_ASIOUtil-build/_deps/asiosdk-subbuild/asiosdk-populate-prefix/src/asiosdk_2.3.3_2019-06-14.zip' -- SHA1 hash of D:/a/FlexASIO/FlexASIO/src/out/build/x86-Debug/dechamps_ASIOUtil-prefix/src/dechamps_ASIOUtil-build/_deps/asiosdk-subbuild/asiosdk-populate-prefix/src/asiosdk_2.3.3_2019-06-14.zip does not match expected value expected: '33c44f0c919a7218413ea81e06a66bb8b47be292' actual: '4d0097725bcf1015c91fb84f89e1a888141bd131' -- Hash mismatch, removing... CMake Error at asiosdk-subbuild/asiosdk-populate-prefix/src/asiosdk-populate-stamp/download-asiosdk-populate.cmake:162 (message): Each download failed! In my local repo I used '33c44f0c919a7218413ea81e06a66bb8b47be292' instead of '4d0097725bcf1015c91fb84f89e1a888141bd131' then it works. You might want to update this.

dechamps commented 1 month ago

The failing GitHub actions belong to fetching the ASIO SDK

Yeah, sorry about that, I know about this issue.

Thanks for the contribution by the way - in FlexASIO's 6-year history, you are literally the first person to open a PR against FlexASIO with actual code changes 😅

I don't have major objections to this change at first glance. I'll take a closer look and merge it as soon as I find the time to resume work on FlexASIO (which may not be any time soon, to be perfectly honest).

Out of curiosity, how did you come across this issue? Were you trying to read the log on a non-latin Windows install?

aholzinger commented 1 month ago

Never mind.

There's always a first time :-)

I just studied the code, beginning with the headers and was curious about the error text function delivering UTF-8 and wanted to know if it would use FormatMessageW or FormatMessageA. IMHO it's a pitty that MS didn't decide to add U functions that use UTF-8. I think it would be high time for this, because nearly all portable libraries use UTF-8. It would make life much easier on Windows.

Take your time, there's no hurry.

dechamps commented 1 month ago

I took a closer look at this and realized that actually, this function is not really needed, as the standard C++ library already knows how to produce these messages through std::system_error. Most FlexASIO code is already using that, and I cleaned up the remaining callsites in 71aab1a656c63807f91647871c7ed31bce8d95ae, culminating in the removal of GetWindowsErrorString(). This makes your PR moot I'm afraid, but thanks anyway for the contribution!

aholzinger commented 1 month ago

Even better like this.

Did you check that it's behaving correctly on non-latin systems or at least non-US ones (German umlauts or French cedille for example)?

dechamps commented 1 month ago

No - I don't have such systems at hand. That being said, if it doesn't work, at this point it would be a bug in the MSVC standard library (i.e. their implementation of std::system_error), so it seems like something that should be reported and fixed there.

Quite frankly I am reluctant to add extra complexity to the code for something that users are never supposed to see, anyway: Windows system errors are rare in FlexASIO, and even when they do occur they are usually only visible in the log, which most users wouldn't look at anyway. I'd be open to revisit if I see reports from users where system error messages are not making it through properly.

aholzinger commented 1 month ago

Fair enough, but I doubt that std::system_error::what() will produce UTF8 on Windows. I think it will produce ANSI and thus fail on non-Latin systems. And it seems (I only studied cppreference.com) that the standard is not saying that the string will be UTF8. So IMO the result will be the same as FormatMessageA. It's the same situation as with std::exception::what, isn't it.

Unfortunately Microsoft never adopted the UTF8 concept.

But I don't want to be nit picking. I'm fine with std::system_error::what. My approach with my PR was if using FormatMessage, then doing it right and use FormatMessageW.