AnyImageImporter: recognize uppercase .JPEG, .JPG

[xqms]

I'm loading textures in my application from a source I have no control over - and these strange people use .JPEG as file suffix, which is not recognized by AnyImageImporter.

I briefly looked into implementing a case-insensitive version of Corrade::Utility::String::endsWith, but I'm not sure if you would want that. It also runs into problems with UTF8, but I guess in this case that would be fine (nobody uses UTF8 inside suffixes).

[mosra]

Huh, interesting, why I didn't do this already...

What about applying Utility::String::lowercase() on the filename first and then do all these comparisons on lowercase? Since there could be also file.GIF or the good ol' CLOUDS~1.BMP files :D

Bonus point: if you'd do this also for all other Any* plugins, that would be great :)

[xqms]

What about applying Utility::String::lowercase() on the filename first and then do all these comparisons on lowercase? Since there could be also file.GIF or the good ol' CLOUDS~1.BMP files :D

Ah okay, that would also work. Does some unnecessary work since we are just interested in the suffix, but that sounds ok to me.

Bonus point: if you'd do this also for all other Any* plugins, that would be great :)

Sure, will do so :-)

[mosra]

Does some unnecessary work since we are just interested in the suffix

Yep, you're right. Ideally having some "split extension" function and then lowercase just the suffix. Such API addition is buried somewhere on my TODO list, once it gets implemented I'll update this :) So far I think using lowercase() is good enough, image loading isn't usually done in a hot code path anyway.

Sure, will do so :-)

Thanks! 👍

[xqms]

I haven't actually tested it yet, will do so tomorrow.

[mosra]

Thanks a lot! For the testing -- I think it could be enough to just copy one small file in each test, rename it with uppercase extension (of course also a different basename, to avoid issues on case-insensitive filesystems) and duplicate the test case with different filename.

As far as I understand Git internals, such a copy won't result in any repository bloat since files with the same contents are stored just once.

[codecov-io]

Codecov Report

Merging #312 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   53.36%   53.37%   +0.01%     
==========================================
  Files         325      325              
  Lines       16742    16746       +4     
==========================================
+ Hits         8934     8938       +4     
  Misses       7808     7808

Impacted Files	Coverage Δ
...numPlugins/AnyImageConverter/AnyImageConverter.cpp	67.56% <100%> (+0.9%)	⬆️
src/MagnumPlugins/AnyAudioImporter/AnyImporter.cpp	71.42% <100%> (+1.05%)	⬆️
...agnumPlugins/AnyImageImporter/AnyImageImporter.cpp	91.58% <100%> (+0.07%)	⬆️
...agnumPlugins/AnySceneImporter/AnySceneImporter.cpp	36.92% <100%> (+0.48%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cef34e3...8aa1d24. Read the comment docs.

[xqms]

Added unit tests.

[mosra]

Wonderful, I'm 99.9% happy :) Let's see what the CIs say, but apart from the one thing below (which I think would cause the Android builds to fail) I think this is good to merge :)

Great work! 👍

[mosra]

uppercase.WAV? :)

[xqms]

Thanks for the catch - found another of these typos. The AnyImageImporterTest::detect() test does not actually open the files, right? Then we wouldn't actually need the test files in src/MagnumPlugins/AnyImageImporterTest/Test...

[mosra]

The AnyImageImporterTest::detect() test does not actually open the files, right?

Yeah, you're right :) Except the cases suffixed "data", as they peek into the contents to check file signatures (but that's not your case). So I guess you can remove that one added file, yes.

Thinking more about this, I should rewrite tests for the others in a similar way, to have better coverage. And now I'm also realizing the extension matching should fall back to detection by contents if no extension gets matched. So it's possible to also open random internet files like file.jpg-large. Will do that later, I'm not saying you should do it ;)

[xqms]

And now I'm also realizing the extension matching should fall back to detection by contents if no extension gets matched.

Okay, should we leave the test file there, then? I don't really care either way ;-)

Will do that later, I'm not saying you should do it ;)

👍

[mosra]

Okay, should we leave the test file there, then?

I will remove the uppercase.JPG file when I get to squashing & merging this tomorrow. The other files need to stay for the by-contents checks.

[mosra]

Merged as a480c70. I did the test rewrite and applied this after, which made the diff significantly smaller (and without any extra files needed). Thanks a lot for the contribution! 👍