[victorwon2001]

Fixes #500

Implementation summary

Enabled locale-aware cJSON parsing and added regression coverage for Genius, Musixmatch, and LRCLIB-shaped responses under a comma-decimal locale.

Changes

build/foo_openlyrics.vcxproj |   1 +
 src/sources/lrclib.cpp       | 124 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

Testing

• gcc -std=c99 -Wall -Wextra -Werror -DENABLE_LOCALES -I3rdparty/cJSON -c 3rdparty/cJSON/cJSON.c -o /tmp/foo_openlyrics-cjson-independent.o — passed: cJSON compiled cleanly with ENABLE_LOCALES.
• gcc ... && /tmp/foo_openlyrics-cjson-locale-test — passed: Compiled and passed the independent simulated comma-locale parser test for all three response shapes.

[jacquesh]

You mention it fixes lrclib, genius and muximatch but have not changed the code for musixmatch or genius and have not explained how your change fixes those. You've also haven't provided tests to cover the case you're attempting to fix.

[victorwon2001]

Genius, Musixmatch, and LRCLIB do not need separate locale fixes because build/foo_openlyrics.vcxproj compiles the shared 3rdparty/cJSON/cJSON.c implementation with ENABLE_LOCALES, and all three sources use that same implementation through cJSON_Parse or cJSON_ParseWithLength. The regression test cjson_parses_source_responses_with_comma_decimal_locale in src/sources/lrclib.cpp directly covers representative responses for all three sources under a comma-decimal locale and restores LC_NUMERIC afterward. The x86/x64 Build & test checks on GitHub head 1fb54f2 were successful; this later follow-up has not yet received those GitHub runs. Separately, the independent local comma-locale cJSON harness passed.

[jacquesh]

I see, although I find your commit messages pretty non-descriptive. So I gather your problem is that the parsing functions used by cJSON are locale-aware by default and you want it to not be locale-aware so that it will always correctly parse 1.5 even in locales where the convention would have it be 1,5 and therefore fail to parse by default?

Also, a quick search is not finding me any documentation on ENABLE_LOCALES. Could you please provide a link either to some docs or some reference info somewhere (even if its just in the headers) that defines what that macro does?

[victorwon2001]

Almost, but the direction is the opposite: strtod() is locale-sensitive, and cJSON 1.7.14 only compensates for that when ENABLE_LOCALES is enabled. With the macro enabled, get_decimal_point() uses localeconv(), and parse_number() copies the JSON number to a temporary buffer, replacing the JSON . with the current locale's decimal point before passing it to strtod(). Without the macro, cJSON hard-codes ., so in a comma-decimal locale strtod() can stop at the dot and the JSON parse subsequently fails.

Upstream references:

• README: https://github.com/DaveGamble/cJSON/blob/v1.7.14/README.md — documents -DENABLE_LOCALES=On as enabling localeconv() and says it is on by default.
• CMake option: https://github.com/DaveGamble/cJSON/blob/v1.7.14/CMakeLists.txt
• Implementation: https://github.com/DaveGamble/cJSON/blob/v1.7.14/cJSON.c

That is also why Genius and Musixmatch do not need separate changes: this project compiles one shared 3rdparty/cJSON/cJSON.c with ENABLE_LOCALES, and Genius, Musixmatch, and LRCLIB all use that implementation through cJSON_Parse or cJSON_ParseWithLength. The added regression test switches to a comma-decimal locale and verifies dotted decimal values in representative responses for all three sources.

[jacquesh]

Aha, I see ok awesome. Sure that's fine. The only thing I'd like to see added is a comment explaining what you just explained here (but in the code so its easy to find in future). The problem is just that I'm not sure where the best place to put it is, because really the comment applies to the macro in the build config.

I forget off the top of my head, does the build for cjson also include the precompiled headers? If it does then maybe its better to put the #define in there so we can add a comment next to it...otherwise...probably at the top of the ScopedLocale is fine?

[monk-time]

I didn't even stop to think that issue I had could be connected to the locale! I do have a locale that uses a comma for decimal. Hopefully this gets merged, the plugin is basically unusable without it (haven't seen it fetch a single lyric from the web in a long while).