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

@@ -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<Containers::ArrayView<const char>>
+    {
         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<const char>{};
+            return {};
         }
 
         /* Load if not there yet */
         if(found == data.files.end()) found = data.files.emplace(
             filename, Utility::Directory::mapRead(filename)).first;
 
-        return Containers::ArrayView<const char>{found->second};
+        return Containers::arrayView(found->second);
     }, data);
 
 importer->openFile("scene.gltf"); // memory-maps all files
@@ -113,7 +115,7 @@ std::unique_ptr<Trade::AbstractImporter> 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<Containers::ArrayView<const char>>
+    {
         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<const char>{found->second};
     }, data);
 /* [AbstractImporter-setFileCallback-template] */

src/Magnum/Trade/AbstractImporter.cpp

@@ -73,19 +73,16 @@ AbstractImporter::AbstractImporter(PluginManager::Manager<AbstractImporter>& man
 
 AbstractImporter::AbstractImporter(PluginManager::AbstractManager& manager, const std::string& plugin): PluginManager::AbstractManagingPlugin<AbstractImporter>{manager, plugin} {}
 
-void AbstractImporter::setFileCallback(Containers::ArrayView<const char>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* const userData) {
+void AbstractImporter::setFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*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 char>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) {}
+void AbstractImporter::doSetFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) {}
 
 bool AbstractImporter::openData(Containers::ArrayView<const char> 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<Containers::ArrayView<const char>> 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<Containers::ArrayView<const char>> 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() {

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 char>(*)(const std::string&, ImporterFileCallbackPolicy, void*) { return _fileCallback; }
+        auto fileCallback() const -> Containers::Optional<Containers::ArrayView<const char>>(*)(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<const char>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData = nullptr);
+        void setFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*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<class T> void setFileCallback(Containers::ArrayView<const char>(*callback)(const std::string&, ImporterFileCallbackPolicy, T&), T& userData);
+        template<class T> void setFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*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<const char>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData);
+        virtual void doSetFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*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<const char>(*_fileCallback)(const std::string&, ImporterFileCallbackPolicy, void*){};
+        Containers::Optional<Containers::ArrayView<const char>>(*_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<class Callback, class T> 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<Containers::ArrayView<const char>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(callback);
+    const auto callbackPtr = static_cast<Containers::Optional<Containers::ArrayView<const char>>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(callback);
     if(!callbackPtr) return setFileCallback(nullptr);
 
     _fileCallbackTemplate = { reinterpret_cast<void(*)()>(callbackPtr), &userData };
     setFileCallback([](const std::string& filename, const ImporterFileCallbackPolicy flags, void* const userData) {
         auto& s = *reinterpret_cast<FileCallbackTemplate*>(userData);
-        return reinterpret_cast<Containers::ArrayView<const char>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(s.callback)(filename, flags, *static_cast<T*>(s.userData));
+        return reinterpret_cast<Containers::Optional<Containers::ArrayView<const char>>(*)(const std::string&, ImporterFileCallbackPolicy, T&)>(s.callback)(filename, flags, *static_cast<T*>(s.userData));
     }, &_fileCallbackTemplate);
 }
 #endif

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 char>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override {
+        void doSetFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override {
             *static_cast<int*>(userData) = 1337;
         }
     };
@@ -594,7 +600,7 @@ void AbstractImporterTest::setFileCallback() {
     int a = 0;
     Importer importer;
     auto lambda = [](const std::string&, ImporterFileCallbackPolicy, void*) {
-        return Containers::ArrayView<const char>{};
+        return Containers::Optional<Containers::ArrayView<const char>>{};
     };
     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 char>(*)(const std::string&, ImporterFileCallbackPolicy, void*), void*) override {
+        void doSetFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*)(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<const char>{};
+        return Containers::Optional<Containers::ArrayView<const char>>{};
     };
     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<void(*)()>(importer.fileCallback()) != reinterpret_cast<void(*)()>(static_cast<Containers::ArrayView<const char>(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(lambda)));
+    CORRADE_VERIFY(reinterpret_cast<void(*)()>(importer.fileCallback()) != reinterpret_cast<void(*)()>(static_cast<Containers::Optional<Containers::ArrayView<const char>>(*)(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<const char>(*callback)(const std::string&, ImporterFileCallbackPolicy, void*), void* userData) override {
+        void doSetFileCallback(Containers::Optional<Containers::ArrayView<const char>>(*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<Containers::ArrayView<const char>(*)(const std::string&, ImporterFileCallbackPolicy, int&)>(nullptr), a);
+    importer.setFileCallback(static_cast<Containers::Optional<Containers::ArrayView<const char>>(*)(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<const char>{};
+        return Containers::Optional<Containers::ArrayView<const char>>{};
     });
     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<const char>{};
+        return Containers::Optional<Containers::ArrayView<const char>>{};
     };
     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<const char>{};
+        return Containers::Optional<Containers::ArrayView<const char>>{};
     }, &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<const char>) 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<Containers::ArrayView<const char>> {
+        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<const char> 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<Containers::ArrayView<const char>> {
         if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::LoadTemporary) {
             state.loaded = true;
-            return Containers::ArrayView<const char>{&state.data, 1};
+            return Containers::arrayView(&state.data, 1);
         }
 
         if(filename == "file.dat" && policy == ImporterFileCallbackPolicy::Close) {
             state.closed = true;
-            return Containers::ArrayView<const char>{};
+            return {};
         }
 
         state.calledNotSureWhy = true;
-        return Containers::ArrayView<const char>{};
+        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<Containers::ArrayView<const char>> {
+        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<const char> 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<const char>{};
-    }, calledNotSureWhy);
+    importer.setFileCallback([](const std::string& filename, ImporterFileCallbackPolicy policy, State& state) -> Containers::Optional<Containers::ArrayView<const char>> {
+        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<Containers::ArrayView<const char>>{};
+    });
+
+    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() {

src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp

@@ -53,7 +53,7 @@ struct AnyImageImporterTest: TestSuite::Tester {
 
 namespace {
 
-Containers::ArrayView<const char> fileCallback(const std::string& filename, Trade::ImporterFileCallbackPolicy, Containers::Array<char>& storage) {
+Containers::Optional<Containers::ArrayView<const char>> fileCallback(const std::string& filename, Trade::ImporterFileCallbackPolicy, Containers::Array<char>& storage) {
     storage = Utility::Directory::read(filename);
     return Containers::ArrayView<const char>{storage};
 }
@@ -61,7 +61,7 @@ Containers::ArrayView<const char> fileCallback(const std::string& filename, Trad
 constexpr struct {
     const char* name;
     const char* filename;
-    Containers::ArrayView<const char>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array<char>&);
+    Containers::Optional<Containers::ArrayView<const char>>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array<char>&);
 } 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<const char>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array<char>&);
+    Containers::Optional<Containers::ArrayView<const char>>(*callback)(const std::string&, Trade::ImporterFileCallbackPolicy, Containers::Array<char>&);
     const char* plugin;
 } DetectData[]{
     {"PNG", "rgb.png", nullptr, "PngImporter"},