Fix T93960: Asset Catalogs I/O fails with unicode file paths on Windows
ClosedPublic
Actions

Authored by Sybren A. Stüvel (sybren) on Dec 20 2021, 4:29 PM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Commits

rB8cf199445574: Fix T93960: Asset Catalogs I/O fails with unicode file paths on Windows

Summary

On Windows, encode file paths as UTF-16 before trying to open the file for reading/writing.

This introduces a new class blender::fstream, which wraps std::fstream and provides this UTF-16 encoding. This class should also be used in other areas, like the Alembic importer/exporter.

Manifest Task: T93960

Diff Detail

Repository: rB Blender

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Dec 20 2021, 4:29 PM

Sybren A. Stüvel (sybren) created this revision.

Harbormaster completed remote builds in B19541: Diff 46231.Dec 20 2021, 4:29 PM

Note that this patch consists of two commits; I intend to keep them separated.

Jacques Lucke (JacquesLucke) requested changes to this revision.Dec 20 2021, 5:20 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenlib/BLI_fileops.hh
17	Outdated license header.
47	Is this `filename` or `filepath`, or are these synonym in this context?
51	Why do you delete this explicitly? Looks like `std::fstream` deletes the copy constructor already.
53	Both `open` functions could be merged by using `StringRef`.
source/blender/blenlib/CMakeLists.txt
427	For consistency the file name should have a `_test` suffix.
source/blender/blenlib/intern/fileops.cc
29	Use `= default;`. Clang tidy might warn here, haven't tried.
33	Clang tidy might warn here as well. It's not necessary to call the constructor of the base class explicitly.
48	I'm surprised that `std::fstream::open` seems to just do nothing in case of an error (would have expected it to throw an exception). So this is not important, but wanted to mention it anyway: If it would throw an exception, then this function would not be exception safe, because it would leak memory in this case.

This revision now requires changes to proceed.Dec 20 2021, 5:20 PM

Sybren A. Stüvel (sybren) added inline comments.Dec 20 2021, 7:22 PM

source/blender/blenlib/BLI_fileops.hh
47	Synonym, these declarations were copied verbatim from https://en.cppreference.com/w/cpp/io/basic_fstream
53	That could be done, but would remove the symmetry with what's declared on https://en.cppreference.com/w/cpp/io/basic_fstream Is there a guarantee that the compiler will choose our `StringRef` override over the (then not overridden) `const char *` and `const std::string &` functions of `std::fstream`?
source/blender/blenlib/intern/fileops.cc
48	It doesn't raise an exception, which is why T93960 silently fails.

Sybren A. Stüvel (sybren) added inline comments.Dec 21 2021, 10:36 AM

source/blender/blenlib/BLI_fileops.hh
47	I like `filepath` more though, so I'll update the code for that.
53	Never mind, C++ is being C++. I added an `override` keyword and the compiler puked at me. At least the VS2019 implementation of STL uses different arguments than documented at https://en.cppreference.com/w/cpp/io/basic_fstream/open, so I'll just embrace the "fluid standard" and have one `StringRef` implementation.

Review comments from Jacques

Harbormaster completed remote builds in B19554: Diff 46260.Dec 21 2021, 12:41 PM

Sybren A. Stüvel (sybren) marked 3 inline comments as done.Dec 21 2021, 12:41 PM

Sybren A. Stüvel (sybren) marked an inline comment as done.

The tests seem to be failing for me, not sure if I have to set anything up to run them correctly? If do you just have to commit something to svn separately?

39: [----------] 2 tests from fileops
39: [ RUN      ] fileops.fstream_open_string_filename
39: source/blender/blenlib/tests/BLI_fileops_test.cc:18: Failure
39: Value of: in.is_open()
39:   Actual: false
39: Expected: true
39: could not open /home/jacques/blender-git/blender/../lib/tests/asset_library/новый/blender_assets.cats.txt
39: [  FAILED  ] fileops.fstream_open_string_filename (0 ms)
39: [ RUN      ] fileops.fstream_open_charptr_filename
39: source/blender/blenlib/tests/BLI_fileops_test.cc:34: Failure
39: Value of: in.is_open()
39:   Actual: false
39: Expected: true
39: could not open /home/jacques/blender-git/blender/../lib/tests/asset_library/новый/blender_assets.cats.txt
39: [  FAILED  ] fileops.fstream_open_charptr_filename (0 ms)
39: [----------] 2 tests from fileops (0 ms total)

Besides that, this is looking good to me.

Jacques Lucke (JacquesLucke) accepted this revision.Dec 21 2021, 2:39 PM

This revision is now accepted and ready to land.Dec 21 2021, 2:39 PM

Yeah, I have to commit a file to SVN. Didn't want to do that until the patch lands.

Closed by commit rB8cf199445574: Fix T93960: Asset Catalogs I/O fails with unicode file paths on Windows (authored by Sybren A. Stüvel (sybren)). · Explain WhyDec 21 2021, 3:54 PM

This revision was automatically updated to reflect the committed changes.

Sybren A. Stüvel (sybren) mentioned this in rBd66a6525c361: Assets: log message when catalog definitions cannot be loaded.

Sybren A. Stüvel (sybren) added a commit: rB8cf199445574: Fix T93960: Asset Catalogs I/O fails with unicode file paths on Windows.