From 1260d51dc11100a34bf00de46f3e23f0b29f6765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 16 Aug 2018 17:28:30 +0200 Subject: [PATCH] Trade: redesign importer file callbacks. The original implementation had a few problems: - If a file callback was set, openFile() was unconditionally calling right into doOpenData(), making it impossible for the importer to know the original path for correctly supplying paths to additional files. Now, if the importer supports Feature::FileCallback, doOpenFile() is always called. It's also possible for the importer to save the path and then just delegate to the base doOpenFile() implementation -- it will handle the file callbacks correctly too. - If the importer supported neither FileCallback nor OpenData and callbacks were set, the original doOpenFile() implementation was called without any warning or anything, doing silently a bad thing. Now in this case setFileCallbacks() asserts -- programmer has to check for feature support first. - It was not possible for the file callback to indicate file opening failure -- in general, empty files are valid, so a nullptr ArrayView is also a valid file. Now the callback return an Optional instead. --- doc/snippets/MagnumTrade.cpp | 21 +- src/Magnum/Trade/AbstractImporter.cpp | 68 +++++-- src/Magnum/Trade/AbstractImporter.h | 98 ++++++---- .../Trade/Test/AbstractImporterTest.cpp | 183 +++++++++++++++--- .../Test/AnyImageImporterTest.cpp | 6 +- 5 files changed, 280 insertions(+), 96 deletions(-) diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 14c392c62..a5318d486 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -71,20 +71,22 @@ struct Data { } data; importer->setFileCallback([](const std::string& filename, - Trade::ImporterFileCallbackPolicy policy, Data& data) { + Trade::ImporterFileCallbackPolicy policy, Data& data) + -> Containers::Optional> + { auto found = data.files.find(filename); /* Discard the memory mapping, if not needed anymore */ if(policy == Trade::ImporterFileCallbackPolicy::Close) { if(found != data.files.end()) data.files.erase(found); - return Containers::ArrayView{}; + return {}; } /* Load if not there yet */ if(found == data.files.end()) found = data.files.emplace( filename, Utility::Directory::mapRead(filename)).first; - return Containers::ArrayView{found->second}; + return Containers::arrayView(found->second); }, data); importer->openFile("scene.gltf"); // memory-maps all files @@ -113,7 +115,7 @@ std::unique_ptr importer; importer->setFileCallback([](const std::string& filename, Trade::ImporterFileCallbackPolicy, void*) { Utility::Resource rs("data"); - return rs.getRaw(filename); + return Containers::optional(rs.getRaw(filename)); }); /* [AbstractImporter-setFileCallback] */ } @@ -126,10 +128,15 @@ struct Data { } data; importer->setFileCallback([](const std::string& filename, - Trade::ImporterFileCallbackPolicy, Data& data) { + Trade::ImporterFileCallbackPolicy, Data& data) + -> Containers::Optional> + { auto found = data.files.find(filename); - if(found == data.files.end()) found = data.files.emplace( - filename, Utility::Directory::read(filename)).first; + if(found == data.files.end()) { + if(!Utility::Directory::fileExists(filename)) + return Containers::NullOpt; + found = data.files.emplace(filename, Utility::Directory::read(filename)).first; + } return Containers::ArrayView{found->second}; }, data); /* [AbstractImporter-setFileCallback-template] */ diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index c5c406211..c27b704b7 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -73,19 +73,16 @@ AbstractImporter::AbstractImporter(PluginManager::Manager& man AbstractImporter::AbstractImporter(PluginManager::AbstractManager& manager, const std::string& plugin): PluginManager::AbstractManagingPlugin{manager, plugin} {} -void AbstractImporter::setFileCallback(Containers::ArrayView(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* const userData) { +void AbstractImporter::setFileCallback(Containers::Optional>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* const userData) { CORRADE_ASSERT(!isOpened(), "Trade::AbstractImporter::setFileCallback(): can't be set while a file is opened", ); - if(!(features() & (Feature::FileCallback|Feature::OpenData))) { - Warning{} << "Trade::AbstractImporter::setFileCallback(): importer supports neither loading from data nor via callbacks, ignoring"; - return; - } + CORRADE_ASSERT(features() & (Feature::FileCallback|Feature::OpenData), "Trade::AbstractImporter::setFileCallback(): importer supports neither loading from data nor via callbacks, callbacks can't be used", ); _fileCallback = callback; _fileCallbackUserData = userData; doSetFileCallback(callback, userData); } -void AbstractImporter::doSetFileCallback(Containers::ArrayView(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) {} +void AbstractImporter::doSetFileCallback(Containers::Optional>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) {} bool AbstractImporter::openData(Containers::ArrayView data) { CORRADE_ASSERT(features() & Feature::OpenData, @@ -116,15 +113,35 @@ void AbstractImporter::doOpenState(const void*, const std::string&) { bool AbstractImporter::openFile(const std::string& filename) { close(); - /* If file loading callbacks are set and the importer can open data, do - that. Mark the file as ready to be closed once opening is finished. */ - if((doFeatures() & Feature::OpenData) && _fileCallback) { - doOpenData(_fileCallback(filename, ImporterFileCallbackPolicy::LoadTemporary, _fileCallbackUserData)); + /* If file loading callbacks are not set or the importer supports handling + them directly, call into the implementation */ + if(!_fileCallback || (doFeatures() & Feature::FileCallback)) { + doOpenFile(filename); + + /* Otherwise, if loading from data is supported, use the callback and pass + the data through to openData(). Mark the file as ready to be closed once + opening is finished. */ + } else if(doFeatures() & Feature::OpenData) { + /* This needs to be duplicated here and in the doOpenFile() + implementation in order to support both following cases: + - plugins that don't support FileCallback but have their own + doOpenFile() implementation (callback needs to be used here, + because the base doOpenFile() implementation might never get + called) + - plugins that support FileCallback but want to delegate the actual + file loading to the default implementation (callback used in the + base doOpenFile() implementation, because this branch is never + taken in that case) */ + const Containers::Optional> data = _fileCallback(filename, ImporterFileCallbackPolicy::LoadTemporary, _fileCallbackUserData); + if(!data) { + Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; + return isOpened(); + } + doOpenData(*data); _fileCallback(filename, ImporterFileCallbackPolicy::Close, _fileCallbackUserData); - /* Otherwise (either no callbacks set or opening data is not supported) - just call the default implementation */ - } else doOpenFile(filename); + /* Shouldn't get here, the assert is fired already in setFileCallback() */ + } else CORRADE_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ return isOpened(); } @@ -132,13 +149,26 @@ bool AbstractImporter::openFile(const std::string& filename) { void AbstractImporter::doOpenFile(const std::string& filename) { CORRADE_ASSERT(features() & Feature::OpenData, "Trade::AbstractImporter::openFile(): not implemented", ); - /* Open file */ - if(!Utility::Directory::fileExists(filename)) { - Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; - return; - } + /* If callbacks are set, use them. This is the same implementation as in + openFile(), see the comment there for details. */ + if(_fileCallback) { + const Containers::Optional> data = _fileCallback(filename, ImporterFileCallbackPolicy::LoadTemporary, _fileCallbackUserData); + if(!data) { + Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; + return; + } + doOpenData(*data); + _fileCallback(filename, ImporterFileCallbackPolicy::Close, _fileCallbackUserData); - doOpenData(Utility::Directory::read(filename)); + /* Otherwise open the file directly */ + } else { + if(!Utility::Directory::fileExists(filename)) { + Error() << "Trade::AbstractImporter::openFile(): cannot open file" << filename; + return; + } + + doOpenData(Utility::Directory::read(filename)); + } } void AbstractImporter::close() { diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index 856032063..862afbb63 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -123,13 +123,23 @@ you may want to intercept those references and load them in a custom way as well. For importers that advertise support for this with @ref Feature::FileCallback this is done by specifying a file loading callback using @ref setFileCallback(). The callback gets a filename, @ref ImporterFileCallbackPolicy and an user -pointer as parameters. For example, loading a scene from memory-mapped files -could look like this. Note that the file loading callback affects @ref openFile() -as well --- you don't have to load the top-level file manually and pass it to -@ref openData(), it's done implicitly by the importer: +pointer as parameters; returns a non-owning view on the loaded data or a +@ref Corrade::Containers::NullOpt "Containers::NullOpt" to indicate the file +loading failed. For example, loading a scene from memory-mapped files could +look like below. Note that the file loading callback affects @ref openFile() as +well --- you don't have to load the top-level file manually and pass it to +@ref openData(), any importer supporting the callback feature handles that +correctly. @snippet MagnumTrade.cpp AbstractImporter-usage-callbacks +For importers that don't support @ref Feature::FileCallback directly, the base +@ref openFile() implementation will use the file callback to pass the loaded +data through to @ref openData(), in case the importer supports at least +@ref Feature::OpenData. If the importer supports neither @ref Feature::FileCallback +nor @ref Feature::OpenData, @ref setFileCallback() doesn't allow the callbacks +to be set. + @subsection Trade-AbstractImporter-usage-state Internal importer state Some importers, especially ones that make use of well-known external libraries, @@ -178,10 +188,16 @@ into* a @ref std::shared_ptr instance and that might not be desirable. The plugin needs to implement the @ref doFeatures(), @ref doIsOpened() functions, at least one of @ref doOpenData() / @ref doOpenFile() / -@ref doOpenState() functions, function @ref doClose(), function -@ref doSetFileCallback() in case it's desired to respond on file loading -callback setup and one or more tuples of data access functions, based on what -features are supported in given format. +@ref doOpenState() functions, function @ref doClose() and one or more tuples of +data access functions, based on what features are supported in given format. + +In order to support @ref Feature::FileCallback, the importer needs to properly +use the callbacks to both load the top-level file in @ref doOpenFile() and also +load any external files when needed. The @ref doOpenFile() can delegate back +into the base implementation, but it should remember at least the base file +path to pass correct paths to subsequent file callbacks. The +@ref doSetFileCallback() can be overriden in case it's desired to respond to +file loading callback setup, but doesn't have to be. For multi-data formats the file opening shouldn't take long and all parsing should be done in the data parsing functions instead, because the user might @@ -296,7 +312,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * * @see @ref Trade-AbstractImporter-usage-callbacks */ - auto fileCallback() const -> Containers::ArrayView(*)(const std::string&, ImporterFileCallbackPolicy, void*) { return _fileCallback; } + auto fileCallback() const -> Containers::Optional>(*)(const std::string&, ImporterFileCallbackPolicy, void*) { return _fileCallback; } /** * @brief File opening callback user data @@ -308,35 +324,41 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi /** * @brief Set file opening callback * - * In case a scene file opened using @ref openData() references other - * files such as images and @ref Feature::FileCallback is supported by - * the importer, @p callback will be used to load file data. The - * callback function gets a filename, @ref ImporterFileCallbackPolicy - * and the @p userData pointer as input and returns a non-owning view - * on the loaded data as output. - * - * In case @ref openFile() is used and at least @ref Feature::OpenData - * is supported, the callback is used for loading the top-level file. - * First the file is loaded with @ref ImporterFileCallbackPolicy::LoadTemporary - * passed to the callback, then the returned memory view is passed to + * In case the importer supports @ref Feature::FileCallback, files + * opened through @ref openFile() will be loaded through the provided + * callback. Besides that, all external files referenced by the + * top-level file will be loaded through the callback function as well, + * usually on demand. The callback function gets a filename, + * @ref ImporterFileCallbackPolicy and the @p userData pointer as input + * and returns a non-owning view on the loaded data as output or a + * @ref Corrade::Containers::NullOpt if loading failed --- because + * empty files might also be valid in some circumstances, @cpp nullptr @ce + * can't be used to indicate a failure. + * + * In case the importer doesn't support @ref Feature::FileCallback but + * supports at least @ref Feature::OpenData, a file opened through + * @ref openFile() will be internally loaded through the provided + * callback and then passed to @ref openData(). First the file is + * loaded with @ref ImporterFileCallbackPolicy::LoadTemporary passed to + * the callback, then the returned memory view is passed to * @ref openData() (sidestepping the potential @ref openFile() - * implementation of that particular plugin) and after that the + * implementation of that particular importer) and after that the * callback is called again with @ref ImporterFileCallbackPolicy::Close * because the semantics of @ref openData() don't require the data to * be alive after. In case you need a different behavior, use * @ref openData() directly. * - * In case the importer supports neither @ref Feature::FileCallback nor - * @ref Feature::OpenData, the callback won't have a chance to be used - * in any of the above scenarios and thus the function prints a warning - * and returns without setting anything. In case @p callback is - * @cpp nullptr @ce, the current callback (if any) is reset. + * In case @p callback is @cpp nullptr @ce, the current callback (if + * any) is reset. This function expects that the importer supports + * either @ref Feature::FileCallback or @ref Feature::OpenData. If an + * importer supports neither, callbacks can't be used. * * It's expected that this function is called *before* a file is * opened. It's also expected that the loaded data are kept in scope * for as long as the importer needs them, based on the value of - * @ref ImporterFileCallbackPolicy. Particular importer documentation - * provides * more information about the behavior. + * @ref ImporterFileCallbackPolicy. Documentation of particular + * importers provides more information about the expected callback + * behavior. * * Following is an example of setting up a file loading callback for * fetching compiled-in resources from @ref Corrade::Utility::Resource. @@ -347,7 +369,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * * @see @ref Trade-AbstractImporter-usage-callbacks */ - void setFileCallback(Containers::ArrayView(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData = nullptr); + void setFileCallback(Containers::Optional>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData = nullptr); /** * @brief Set file opening callback @@ -361,7 +383,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @see @ref Trade-AbstractImporter-usage-callbacks */ #ifdef DOXYGEN_GENERATING_OUTPUT - template void setFileCallback(Containers::ArrayView(*callback)(const std::string&, ImporterFileCallbackPolicy, T&), T& userData); + template void setFileCallback(Containers::Optional>(*callback)(const std::string&, ImporterFileCallbackPolicy, T&), T& userData); #else /* Otherwise the user would be forced to use the + operator to convert a lambda to a function pointer and (besides being weird and @@ -808,7 +830,13 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * the file and calls @ref Magnum::Trade::AbstractImporter::doOpenData() "doOpenData()" * with its contents. It is allowed to call this function from your * @ref Magnum::Trade::AbstractImporter::doOpenFile() "doOpenFile()" - * implementation. + * implementation --- in particular, this implementation will also + * correctly handle callbacks set through @ref setFileCallback(). + * + * This function is not called when file callbacks are set through + * @ref setFileCallback() and @ref Feature::FileCallback is not + * supported --- instead, file is loaded though the callback and data + * passed through to @ref Magnum::Trade::AbstractImporter::doOpenData() "doOpenData()". */ virtual void doOpenFile(const std::string& filename); @@ -829,7 +857,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * and user data pointer are available through @ref fileCallback() and * @ref fileCallbackUserData(). */ - virtual void doSetFileCallback(Containers::ArrayView(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData); + virtual void doSetFileCallback(Containers::Optional>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData); /** @brief Implementation for @ref isOpened() */ virtual bool doIsOpened() const = 0; @@ -1166,7 +1194,7 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi virtual const void* doImporterState() const; private: - Containers::ArrayView(*_fileCallback)(const std::string&, ImporterFileCallbackPolicy, void*){}; + Containers::Optional>(*_fileCallback)(const std::string&, ImporterFileCallbackPolicy, void*){}; void* _fileCallbackUserData{}; /* Used by the templated version only */ @@ -1180,13 +1208,13 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi template void AbstractImporter::setFileCallback(Callback callback, T& userData) { /* Don't try to wrap a null function pointer. Need to cast first because MSVC (even 2017) can't apply ! to a lambda. Ugh. */ - const auto callbackPtr = static_cast(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(callback); + const auto callbackPtr = static_cast>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(callback); if(!callbackPtr) return setFileCallback(nullptr); _fileCallbackTemplate = { reinterpret_cast(callbackPtr), &userData }; setFileCallback([](const std::string& filename, const ImporterFileCallbackPolicy flags, void* const userData) { auto& s = *reinterpret_cast(userData); - return reinterpret_cast(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(s.callback)(filename, flags, *static_cast(s.userData)); + return reinterpret_cast>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(s.callback)(filename, flags, *static_cast(s.userData)); }, &_fileCallbackTemplate); } #endif diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index 5c44ae951..ec7eabe68 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -67,8 +67,11 @@ class AbstractImporterTest: public TestSuite::Tester { void setFileCallbackFileOpened(); void setFileCallbackNotImplemented(); void setFileCallbackNotSupported(); + void setFileCallbackOpenFileDirectly(); + void setFileCallbackOpenFileThroughBaseImplementation(); + void setFileCallbackOpenFileThroughBaseImplementationFailed(); void setFileCallbackOpenFileAsData(); - void setFileCallbackOpenFileAsDataNotSupported(); + void setFileCallbackOpenFileAsDataFailed(); void defaultScene(); void defaultSceneNotImplemented(); @@ -258,8 +261,11 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::setFileCallbackFileOpened, &AbstractImporterTest::setFileCallbackNotImplemented, &AbstractImporterTest::setFileCallbackNotSupported, + &AbstractImporterTest::setFileCallbackOpenFileDirectly, + &AbstractImporterTest::setFileCallbackOpenFileThroughBaseImplementation, + &AbstractImporterTest::setFileCallbackOpenFileThroughBaseImplementationFailed, &AbstractImporterTest::setFileCallbackOpenFileAsData, - &AbstractImporterTest::setFileCallbackOpenFileAsDataNotSupported, + &AbstractImporterTest::setFileCallbackOpenFileAsDataFailed, &AbstractImporterTest::defaultScene, &AbstractImporterTest::defaultSceneNotImplemented, @@ -586,7 +592,7 @@ void AbstractImporterTest::setFileCallback() { Features doFeatures() const override { return Feature::OpenData|Feature::FileCallback; } bool doIsOpened() const override { return false; } void doClose() override {} - void doSetFileCallback(Containers::ArrayView(*)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override { + void doSetFileCallback(Containers::Optional>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override { *static_cast(userData) = 1337; } }; @@ -594,7 +600,7 @@ void AbstractImporterTest::setFileCallback() { int a = 0; Importer importer; auto lambda = [](const std::string&, ImporterFileCallbackPolicy, void*) { - return Containers::ArrayView{}; + return Containers::Optional>{}; }; importer.setFileCallback(lambda, &a); CORRADE_COMPARE(importer.fileCallback(), lambda); @@ -607,7 +613,7 @@ void AbstractImporterTest::setFileCallbackTemplate() { Features doFeatures() const override { return Feature::OpenData|Feature::FileCallback; } bool doIsOpened() const override { return false; } void doClose() override {} - void doSetFileCallback(Containers::ArrayView(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) override { + void doSetFileCallback(Containers::Optional>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) override { called = true; } @@ -616,7 +622,7 @@ void AbstractImporterTest::setFileCallbackTemplate() { int a = 0; auto lambda = [](const std::string&, ImporterFileCallbackPolicy, int&) { - return Containers::ArrayView{}; + return Containers::Optional>{}; }; Importer importer; importer.setFileCallback(lambda, a); @@ -625,7 +631,7 @@ void AbstractImporterTest::setFileCallbackTemplate() { CORRADE_VERIFY(importer.called); /* The data pointers should be wrapped, thus not the same */ - CORRADE_VERIFY(reinterpret_cast(importer.fileCallback()) != reinterpret_cast(static_cast(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(lambda))); + CORRADE_VERIFY(reinterpret_cast(importer.fileCallback()) != reinterpret_cast(static_cast>(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(lambda))); CORRADE_VERIFY(importer.fileCallbackUserData() != &a); } @@ -634,7 +640,7 @@ void AbstractImporterTest::setFileCallbackTemplateNull() { Features doFeatures() const override { return Feature::OpenData|Feature::FileCallback; } bool doIsOpened() const override { return false; } void doClose() override {} - void doSetFileCallback(Containers::ArrayView(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override { + void doSetFileCallback(Containers::Optional>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override { called = !callback && !userData; } @@ -643,7 +649,7 @@ void AbstractImporterTest::setFileCallbackTemplateNull() { int a = 0; Importer importer; - importer.setFileCallback(static_cast(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(nullptr), a); + importer.setFileCallback(static_cast>(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(nullptr), a); CORRADE_VERIFY(!importer.fileCallback()); CORRADE_VERIFY(!importer.fileCallbackUserData()); CORRADE_VERIFY(importer.called); @@ -661,7 +667,7 @@ void AbstractImporterTest::setFileCallbackFileOpened() { Importer importer; importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, void*) { - return Containers::ArrayView{}; + return Containers::Optional>{}; }); CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::setFileCallback(): can't be set while a file is opened\n"); } @@ -675,7 +681,7 @@ void AbstractImporterTest::setFileCallbackNotImplemented() { int a; auto lambda = [](const std::string&, ImporterFileCallbackPolicy, void*) { - return Containers::ArrayView{}; + return Containers::Optional>{}; }; Importer importer; importer.setFileCallback(lambda, &a); @@ -692,29 +698,65 @@ void AbstractImporterTest::setFileCallbackNotSupported() { }; std::ostringstream out; - Warning redirectWarning{&out}; + Error redirectError{&out}; int a; Importer importer; importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, void*) { - return Containers::ArrayView{}; + return Containers::Optional>{}; }, &a); - CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::setFileCallback(): importer supports neither loading from data nor via callbacks, ignoring\n"); - CORRADE_VERIFY(!importer.fileCallback()); - CORRADE_VERIFY(!importer.fileCallbackUserData()); + CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::setFileCallback(): importer supports neither loading from data nor via callbacks, callbacks can't be used\n"); } -void AbstractImporterTest::setFileCallbackOpenFileAsData() { +void AbstractImporterTest::setFileCallbackOpenFileDirectly() { class Importer: public Trade::AbstractImporter { - Features doFeatures() const override { return Feature::OpenData; } + Features doFeatures() const override { return Feature::FileCallback|Feature::OpenData; } + bool doIsOpened() const override { return _opened; } + void doClose() override { _opened = false; } + + void doOpenFile(const std::string& filename) override { + /* Called because FileCallback is supported */ + _opened = filename == "file.dat" && fileCallback() && fileCallbackUserData(); + } + + void doOpenData(Containers::ArrayView) override { + /* Shouldn't be called because FileCallback is supported */ + openDataCalledNotSureWhy = true; + } + + bool _opened = false; + public: bool openDataCalledNotSureWhy = false; + }; + + bool calledNotSureWhy = false; + Importer importer; + importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, bool& calledNotSureWhy) -> Containers::Optional> { + calledNotSureWhy = true; + return {}; + }, calledNotSureWhy); + + CORRADE_VERIFY(importer.openFile("file.dat")); + CORRADE_VERIFY(!calledNotSureWhy); + CORRADE_VERIFY(!importer.openDataCalledNotSureWhy); +} + +void AbstractImporterTest::setFileCallbackOpenFileThroughBaseImplementation() { + class Importer: public Trade::AbstractImporter { + Features doFeatures() const override { return Feature::FileCallback|Feature::OpenData; } bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } + void doOpenFile(const std::string& filename) override { + openFileCalled = filename == "file.dat" && fileCallback() && fileCallbackUserData(); + AbstractImporter::doOpenFile(filename); + } + void doOpenData(Containers::ArrayView data) override { _opened = (data.size() == 1 && data[0] == '\xb0'); } bool _opened = false; + public: bool openFileCalled = false; }; struct State { @@ -724,49 +766,126 @@ void AbstractImporterTest::setFileCallbackOpenFileAsData() { bool calledNotSureWhy = false; } state; Importer importer; - importer.setFileCallback([](const std::string& filename, ImporterFileCallbackPolicy policy, State& state) { + importer.setFileCallback([](const std::string& filename, ImporterFileCallbackPolicy policy, State& state) -> Containers::Optional> { if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::LoadTemporary) { state.loaded = true; - return Containers::ArrayView{&state.data, 1}; + return Containers::arrayView(&state.data, 1); } if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::Close) { state.closed = true; - return Containers::ArrayView{}; + return {}; } state.calledNotSureWhy = true; - return Containers::ArrayView{}; + return {}; }, state); CORRADE_VERIFY(importer.openFile("file.dat")); + CORRADE_VERIFY(importer.openFileCalled); CORRADE_VERIFY(state.loaded); CORRADE_VERIFY(state.closed); CORRADE_VERIFY(!state.calledNotSureWhy); } -void AbstractImporterTest::setFileCallbackOpenFileAsDataNotSupported() { +void AbstractImporterTest::setFileCallbackOpenFileThroughBaseImplementationFailed() { class Importer: public Trade::AbstractImporter { - Features doFeatures() const override { return Feature::FileCallback; } + Features doFeatures() const override { return Feature::FileCallback|Feature::OpenData; } + bool doIsOpened() const override { return false; } + void doClose() override {} + + void doOpenFile(const std::string& filename) override { + openFileCalled = true; + AbstractImporter::doOpenFile(filename); + } + + public: bool openFileCalled = false; + }; + + Importer importer; + importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, void*) -> Containers::Optional> { + return {}; + }); + + std::ostringstream out; + Error redirectError{&out}; + + CORRADE_VERIFY(!importer.openFile("file.dat")); + CORRADE_VERIFY(importer.openFileCalled); + CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::openFile(): cannot open file file.dat\n"); +} + +void AbstractImporterTest::setFileCallbackOpenFileAsData() { + class Importer: public Trade::AbstractImporter { + Features doFeatures() const override { return Feature::OpenData; } bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - void doOpenFile(const std::string& filename) override { - _opened = filename == "file.dat"; + void doOpenFile(const std::string&) override { + openFileCalled = true; + } + + void doOpenData(Containers::ArrayView data) override { + _opened = (data.size() == 1 && data[0] == '\xb0'); } bool _opened = false; + public: bool openFileCalled = false; }; - bool calledNotSureWhy = false; + struct State { + const char data = '\xb0'; + bool loaded = false; + bool closed = false; + bool calledNotSureWhy = false; + } state; Importer importer; - importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, bool& calledNotSureWhy) { - calledNotSureWhy = true; - return Containers::ArrayView{}; - }, calledNotSureWhy); + importer.setFileCallback([](const std::string& filename, ImporterFileCallbackPolicy policy, State& state) -> Containers::Optional> { + if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::LoadTemporary) { + state.loaded = true; + return Containers::arrayView(&state.data, 1); + } + + if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::Close) { + state.closed = true; + return {}; + } + + state.calledNotSureWhy = true; + return {}; + }, state); CORRADE_VERIFY(importer.openFile("file.dat")); - CORRADE_VERIFY(!calledNotSureWhy); + CORRADE_VERIFY(!importer.openFileCalled); + CORRADE_VERIFY(state.loaded); + CORRADE_VERIFY(state.closed); + CORRADE_VERIFY(!state.calledNotSureWhy); +} + +void AbstractImporterTest::setFileCallbackOpenFileAsDataFailed() { + class Importer: public Trade::AbstractImporter { + Features doFeatures() const override { return Feature::OpenData; } + bool doIsOpened() const override { return false; } + void doClose() override {} + + void doOpenFile(const std::string&) override { + openFileCalled = true; + } + + public: bool openFileCalled = false; + }; + + Importer importer; + importer.setFileCallback([](const std::string&, ImporterFileCallbackPolicy, void*) { + return Containers::Optional>{}; + }); + + std::ostringstream out; + Error redirectError{&out}; + + CORRADE_VERIFY(!importer.openFile("file.dat")); + CORRADE_VERIFY(!importer.openFileCalled); + CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::openFile(): cannot open file file.dat\n"); } void AbstractImporterTest::defaultScene() { diff --git a/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp b/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp index 4b774aa6b..128ac30da 100644 --- a/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp +++ b/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp @@ -53,7 +53,7 @@ struct AnyImageImporterTest: TestSuite::Tester { namespace { -Containers::ArrayView fileCallback(const std::string& filename, Trade::ImporterFileCallbackPolicy, Containers::Array& storage) { +Containers::Optional> fileCallback(const std::string& filename, Trade::ImporterFileCallbackPolicy, Containers::Array& storage) { storage = Utility::Directory::read(filename); return Containers::ArrayView{storage}; } @@ -61,7 +61,7 @@ Containers::ArrayView fileCallback(const std::string& filename, Trad constexpr struct { const char* name; const char* filename; - Containers::ArrayView(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array&); + Containers::Optional>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array&); } LoadData[]{ {"TGA", TGA_FILE, nullptr}, {"TGA data", TGA_FILE, fileCallback} @@ -70,7 +70,7 @@ constexpr struct { constexpr struct { const char* name; const char* filename; - Containers::ArrayView(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array&); + Containers::Optional>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array&); const char* plugin; } DetectData[]{ {"PNG", "rgb.png", nullptr, "PngImporter"},