Detect valid UTF-8 as UTF-8

[filip-hejsek]

This PR modifies Aegisub's charset detection function so that if the input is valid UTF-8, it will always be detected as UTF-8 instead of using uchardet's heuristics (which sometimes fail to correctly detect UTF-8).

ICU charset conversion API is used for UTF-8 validation. Because this API is designed for conversion, not validation, a dummy output buffer has to be provided.

A charset detection test suite has been added. For testing purposes, an additional "detect reason" output parameter has been added to the charset detection function, which allows distinguishing between e.g. detecting utf-8 from byte order mark, UTF-8 validation or uchardet heuristics.

Remaining issues:

• The added tests need to know if libaegisub was compiled with or without uchardet, but the WITH_UCHARDET macro is not available when compiling tests.

Fixes #370

[filip-hejsek]

I've added some basic tests. However, the tests need to know if Aegisub was compiled with uchardet, but the WITH_UCHARDET macro is not available when compiling the tests.

[arch1t3cht]

Thanks! And sorry for the delay; I ended up postponing absolutely everything else until I finished #594 since I probably would have never gotten it done otherwise.

The first commit looks good to me and fixes the wrongly detected files I've been sent in the past. For the second commit (thanks for adding tests!):

• I like the added TEST_DATA_DIR logic in principle (though maybe it'd be nice to prefix the env variable with AEGISUB_) but as it is it's inconsistent with the way test files are handled for the other test variants. The other tests run tests/setup.{sh,bat} to copy the test data to the build directory so that they can be found via a relative path from the test binary's CWD.

  Now, these dedicated setup scripts are probably not actually needed any more. They used to set up some extra files with different permissions, but that was removed in 5da48ec . So in principle they could just be removed entirely and be replaced with the AEGISUB_TEST_DATA_DIR logic everywhere.

  If you agree I can do this myself sometime soon unless you want to do it yourself.

• The configuration macros not being available to the tests is unfortunate, yeah. I think the simplest solution might be to move the logic in meson.build that's generating aegisub_defines outside of the if/else branch and pass those flags to the tests, as in the attached patch: testflags.patch

[arch1t3cht]

Thanks for the update!

I pushed to your branch to fix the compilation on Windows and remove the no longer needed test_data_des argument for setup-test-data. From my side, this is good to merge now, unless you disagree.

[filip-hejsek]

I pushed to your branch to fix the compilation on Windows

Thanks for fixing this.

and remove the no longer needed test_data_des argument for setup-test-data.

Actually it was already unused even before these changes.

From my side, this is good to merge now, unless you disagree.

I guess it's good. Some thoughts:

• You suggested removing the setup scripts, but that couldn't be done because they also setup some output directories (and the one file that the tests touch and then check the mod time of).
• mru_ok.json, mru_invalid.json and ten_bytes are still created by the setup script, it might be better to change these too?
• If a bug were to be introduced in the agi::Options::FLUSH_SKIP implementation, the tests would now end up overwriting the original files, but I don't think that's a problem.

[arch1t3cht]

Thanks!

You suggested removing the setup scripts, but that couldn't be done because they also setup some output directories (and the one file that the tests touch and then check the mod time of).

Yeah, I only realized that afterwards. But, either way, I think it's better to simplify the script as much as possible, especially since it runs on every ninja invocation.

mru_ok.json, mru_invalid.json and ten_bytes are still created by the setup script, it might be better to change these too?

Probably, but that can be done later on.