From c3299ad3c5a3746bf1545132b33709701b9940cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 28 Mar 2020 13:30:40 +0100 Subject: [PATCH] Trade: print a message when named thing is not found. Otherwise it's hard to distinguish whether it's a particular importer fault (failing without a message) or if the name is not found. --- src/Magnum/Trade/AbstractImporter.cpp | 60 ++++++++-- src/Magnum/Trade/AbstractImporter.h | 83 ++++++++------ .../Trade/Test/AbstractImporterTest.cpp | 105 +++++++++++------- 3 files changed, 164 insertions(+), 84 deletions(-) diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 3486743e6..39227ace0 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -251,7 +251,10 @@ Containers::Optional AbstractImporter::doScene(UnsignedInt) { Containers::Optional AbstractImporter::scene(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::scene(): no file opened", {}); const Int id = doSceneForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::scene(): scene" << name << "not found"; + return {}; + } return scene(id); /* not doScene(), so we get the range checks also */ } @@ -295,7 +298,10 @@ Containers::Optional AbstractImporter::doAnimation(UnsignedInt) { Containers::Optional AbstractImporter::animation(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::animation(): no file opened", {}); const Int id = doAnimationForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::animation(): animation" << name << "not found"; + return {}; + } return animation(id); /* not doAnimation(), so we get the checks also */ } @@ -334,7 +340,10 @@ Containers::Optional AbstractImporter::doLight(UnsignedInt) { Containers::Optional AbstractImporter::light(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::light(): no file opened", {}); const Int id = doLightForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::light(): light" << name << "not found"; + return {}; + } return light(id); /* not doLight(), so we get the range checks also */ } @@ -373,7 +382,10 @@ Containers::Optional AbstractImporter::doCamera(UnsignedInt) { Containers::Optional AbstractImporter::camera(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::camera(): no file opened", {}); const Int id = doCameraForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::camera(): camera" << name << "not found"; + return {}; + } return camera(id); /* not doCamera(), so we get the range checks also */ } @@ -412,7 +424,10 @@ Containers::Pointer AbstractImporter::doObject2D(UnsignedInt) { Containers::Pointer AbstractImporter::object2D(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::object2D(): no file opened", {}); const Int id = doObject2DForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::object2D(): object" << name << "not found"; + return {}; + } return object2D(id); /* not doObject2D(), so we get the range checks also */ } @@ -451,7 +466,10 @@ Containers::Pointer AbstractImporter::doObject3D(UnsignedInt) { Containers::Pointer AbstractImporter::object3D(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::object3D(): no file opened", {}); const Int id = doObject3DForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::object3D(): object" << name << "not found"; + return {}; + } return object3D(id); /* not doObject3D(), so we get the range checks also */ } @@ -517,7 +535,10 @@ Containers::Optional AbstractImporter::doMesh(UnsignedInt, UnsignedInt Containers::Optional AbstractImporter::mesh(const std::string& name, const UnsignedInt level) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::mesh(): no file opened", {}); const Int id = doMeshForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::mesh(): mesh" << name << "not found"; + return {}; + } return mesh(id, level); /* not doMesh(), so we get the checks also */ } @@ -665,7 +686,10 @@ Containers::Pointer AbstractImporter::doMaterial(UnsignedI Containers::Pointer AbstractImporter::material(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::material(): no file opened", {}); const Int id = doMaterialForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::material(): material" << name << "not found"; + return {}; + } return material(id); /* not doMaterial(), so we get the range checks also */ } @@ -704,7 +728,10 @@ Containers::Optional AbstractImporter::doTexture(UnsignedInt) { Containers::Optional AbstractImporter::texture(const std::string& name) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::texture(): no file opened", {}); const Int id = doTextureForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::texture(): texture" << name << "not found"; + return {}; + } return texture(id); /* not doTexture(), so we get the range checks also */ } @@ -767,7 +794,10 @@ Containers::Optional AbstractImporter::doImage1D(UnsignedInt, Unsig Containers::Optional AbstractImporter::image1D(const std::string& name, const UnsignedInt level) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image1D(): no file opened", {}); const Int id = doImage1DForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::image1D(): image" << name << "not found"; + return {}; + } /* not doImage1D(), so we get the range checks also */ return image1D(id, level); } @@ -831,7 +861,10 @@ Containers::Optional AbstractImporter::doImage2D(UnsignedInt, Unsig Containers::Optional AbstractImporter::image2D(const std::string& name, const UnsignedInt level) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image2D(): no file opened", {}); const Int id = doImage2DForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::image2D(): image" << name << "not found"; + return {}; + } /* not doImage2D(), so we get the range checks also */ return image2D(id, level); } @@ -895,7 +928,10 @@ Containers::Optional AbstractImporter::doImage3D(UnsignedInt, Unsig Containers::Optional AbstractImporter::image3D(const std::string& name, const UnsignedInt level) { CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image3D(): no file opened", {}); const Int id = doImage3DForName(name); - if(id == -1) return {}; + if(id == -1) { + Error{} << "Trade::AbstractImporter::image3D(): image" << name << "not found"; + return {}; + } /* not doImage3D(), so we get the range checks also */ return image3D(id, level); } diff --git a/src/Magnum/Trade/AbstractImporter.h b/src/Magnum/Trade/AbstractImporter.h index c509e043d..b47f7b5ae 100644 --- a/src/Magnum/Trade/AbstractImporter.h +++ b/src/Magnum/Trade/AbstractImporter.h @@ -519,8 +519,9 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref sceneForName() and - * @ref scene(UnsignedInt). Returns @ref Containers::NullOpt either - * if @ref sceneForName() returns @cpp -1 @ce or if importing fails. + * @ref scene(UnsignedInt). If @ref sceneForName() returns @cpp -1 @ce, + * prints an error message and returns @ref Containers::NullOpt, + * otherwise propagates the result from @ref scene(UnsignedInt). * Expects that a file is opened. */ Containers::Optional scene(const std::string& name); @@ -565,9 +566,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref animationForName() and - * @ref animation(UnsignedInt). Returns @ref Containers::NullOpt either - * if @ref animationForName() returns @cpp -1 @ce or if importing - * fails. Expects that a file is opened. + * @ref animation(UnsignedInt). If @ref animationForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref animation(UnsignedInt). Expects that a file is opened. */ Containers::Optional animation(const std::string& name); @@ -611,8 +613,9 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref lightForName() and - * @ref light(UnsignedInt). Returns @ref Containers::NullOpt either if - * @ref lightForName() returns @cpp -1 @ce or if importing fails. + * @ref light(UnsignedInt). If @ref lightForName() returns @cpp -1 @ce, + * prints an error message and returns @ref Containers::NullOpt, + * otherwise propagates the result from @ref light(UnsignedInt). * Expects that a file is opened. */ Containers::Optional light(const std::string& name); @@ -657,9 +660,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref cameraForName() and - * @ref camera(UnsignedInt). Returns @ref Containers::NullOpt either if - * @ref cameraForName() returns @cpp -1 @ce or if importing fails. - * Expects that a file is opened. + * @ref camera(UnsignedInt). If @ref cameraForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref camera(UnsignedInt). Expects that a file is opened. */ Containers::Optional camera(const std::string& name); @@ -703,9 +707,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref object2DForName() and - * @ref object2D(UnsignedInt). Returns @cpp nullptr @ce either if - * @ref object2DForName() returns @cpp -1 @ce or if importing fails. - * Expects that a file is opened. + * @ref object2D(UnsignedInt). If @ref object2DForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref object2D(UnsignedInt). Expects that a file is opened. */ Containers::Pointer object2D(const std::string& name); @@ -749,9 +754,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref object3DForName() and - * @ref object3D(UnsignedInt). Returns @cpp nullptr @ce either if - * @ref object3DForName() returns @cpp -1 @ce or if importing fails. - * Expects that a file is opened. + * @ref object3D(UnsignedInt). If @ref object3DForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref object3D(UnsignedInt). Expects that a file is opened. */ Containers::Pointer object3D(const std::string& name); @@ -816,9 +822,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref meshForName() and - * @ref mesh(UnsignedInt, UnsignedInt). Returns - * @ref Containers::NullOpt either if @ref meshForName() returns - * @cpp -1 @ce or if importing fails. Expects that a file is opened. + * @ref mesh(UnsignedInt, UnsignedInt). If @ref meshForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref mesh(UnsignedInt, UnsignedInt). Expects that a file is opened. */ Containers::Optional mesh(const std::string& name, UnsignedInt level = 0); @@ -972,9 +979,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref materialForName() and - * @ref material(UnsignedInt). Returns @ref Containers::NullOpt either - * if @ref materialForName() returns @cpp -1 @ce or if importing fails. - * Expects that a file is opened. + * @ref material(UnsignedInt). If @ref materialForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref material(UnsignedInt). Expects that a file is opened. */ Containers::Pointer material(const std::string& name); @@ -1018,9 +1026,10 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref textureForName() and - * @ref texture(UnsignedInt). Returns @ref Containers::NullOpt either - * if @ref textureForName() returns @cpp -1 @ce or if importing fails. - * Expects that a file is opened. + * @ref texture(UnsignedInt). If @ref textureForName() returns + * @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref texture(UnsignedInt). Expects that a file is opened. */ Containers::Optional texture(const std::string& name); @@ -1076,9 +1085,11 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref image1DForName() and - * @ref image1D(UnsignedInt, UnsignedInt). Returns - * @ref Containers::NullOpt either if @ref image1DForName() returns - * @cpp -1 @ce or if importing fails. Expects that a file is opened. + * @ref image1D(UnsignedInt, UnsignedInt). If @ref image1DForName() + * returns @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref image1D(UnsignedInt, UnsignedInt). Expects that a file is + * opened. */ Containers::Optional image1D(const std::string& name, UnsignedInt level = 0); @@ -1134,9 +1145,11 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref image2DForName() and - * @ref image2D(UnsignedInt, UnsignedInt). Returns - * @ref Containers::NullOpt either if @ref image2DForName() returns - * @cpp -1 @ce or if importing fails. Expects that a file is opened. + * @ref image2D(UnsignedInt, UnsignedInt). If @ref image2DForName() + * returns @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref image2D(UnsignedInt, UnsignedInt). Expects that a file is + * opened. */ Containers::Optional image2D(const std::string& name, UnsignedInt level = 0); @@ -1192,9 +1205,11 @@ class MAGNUM_TRADE_EXPORT AbstractImporter: public PluginManager::AbstractManagi * @m_since_latest * * A convenience API combining @ref image3DForName() and - * @ref image3D(UnsignedInt, UnsignedInt). Returns - * @ref Containers::NullOpt either if @ref image3DForName() returns - * @cpp -1 @ce or if importing fails. Expects that a file is opened. + * @ref image3D(UnsignedInt, UnsignedInt). If @ref image3DForName() + * returns @cpp -1 @ce, prints an error message and returns + * @ref Containers::NullOpt, otherwise propagates the result from + * @ref image3D(UnsignedInt, UnsignedInt). Expects that a file is + * opened. */ Containers::Optional image3D(const std::string& name, UnsignedInt level = 0); diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index c8abd1097..35a43a155 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -89,6 +89,7 @@ struct AbstractImporterTest: TestSuite::Tester { void thingCountNoFile(); void thingForNameNotImplemented(); void thingForNameNoFile(); + void thingByNameNotFound(); void thingNameNoFile(); void thingNoFile(); @@ -242,6 +243,14 @@ struct AbstractImporterTest: TestSuite::Tester { void debugFeatures(); }; +constexpr struct { + const char* name; + bool checkMessage; +} ThingByNameData[] { + {"check it's not an assert", false}, + {"verify the message", true} +}; + AbstractImporterTest::AbstractImporterTest() { addTests({&AbstractImporterTest::construct, &AbstractImporterTest::constructWithPluginManagerReference, @@ -272,8 +281,12 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::thingCountNotImplemented, &AbstractImporterTest::thingCountNoFile, &AbstractImporterTest::thingForNameNotImplemented, - &AbstractImporterTest::thingForNameNoFile, - &AbstractImporterTest::thingNameNoFile, + &AbstractImporterTest::thingForNameNoFile}); + + addInstancedTests({&AbstractImporterTest::thingByNameNotFound}, + Containers::arraySize(ThingByNameData)); + + addTests({&AbstractImporterTest::thingNameNoFile, &AbstractImporterTest::thingNoFile, &AbstractImporterTest::defaultScene, @@ -1047,6 +1060,58 @@ void AbstractImporterTest::thingForNameNoFile() { "Trade::AbstractImporter::image3DForName(): no file opened\n"); } +void AbstractImporterTest::thingByNameNotFound() { + auto&& data = ThingByNameData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + } importer; + + std::ostringstream out; + { + Containers::Optional redirectError; + if(data.checkMessage) redirectError.emplace(&out); + + CORRADE_VERIFY(!importer.scene("foobar")); + CORRADE_VERIFY(!importer.animation("foobar")); + CORRADE_VERIFY(!importer.light("foobar")); + CORRADE_VERIFY(!importer.camera("foobar")); + + CORRADE_VERIFY(!importer.object2D("foobar")); + CORRADE_VERIFY(!importer.object3D("foobar")); + + CORRADE_VERIFY(!importer.mesh("foobar")); + CORRADE_VERIFY(!importer.material("foobar")); + CORRADE_VERIFY(!importer.texture("foobar")); + + CORRADE_VERIFY(!importer.image1D("foobar")); + CORRADE_VERIFY(!importer.image2D("foobar")); + CORRADE_VERIFY(!importer.image3D("foobar")); + } + + if(data.checkMessage) { + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::scene(): scene foobar not found\n" + "Trade::AbstractImporter::animation(): animation foobar not found\n" + "Trade::AbstractImporter::light(): light foobar not found\n" + "Trade::AbstractImporter::camera(): camera foobar not found\n" + + "Trade::AbstractImporter::object2D(): object foobar not found\n" + "Trade::AbstractImporter::object3D(): object foobar not found\n" + + "Trade::AbstractImporter::mesh(): mesh foobar not found\n" + "Trade::AbstractImporter::material(): material foobar not found\n" + "Trade::AbstractImporter::texture(): texture foobar not found\n" + + "Trade::AbstractImporter::image1D(): image foobar not found\n" + "Trade::AbstractImporter::image2D(): image foobar not found\n" + "Trade::AbstractImporter::image3D(): image foobar not found\n"); + } +} + void AbstractImporterTest::thingNameNoFile() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; } @@ -1216,9 +1281,6 @@ void AbstractImporterTest::scene() { auto data = importer.scene("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.scene("foo")); } } @@ -1320,9 +1382,6 @@ void AbstractImporterTest::animation() { auto data = importer.animation("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.animation("foo")); } } @@ -1506,9 +1565,6 @@ void AbstractImporterTest::light() { auto data = importer.light("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.light("foo")); } } @@ -1605,9 +1661,6 @@ void AbstractImporterTest::camera() { auto data = importer.camera("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.camera("foo")); } } @@ -1704,9 +1757,6 @@ void AbstractImporterTest::object2D() { auto data = importer.object2D("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.object2D("foo")); } } @@ -1803,9 +1853,6 @@ void AbstractImporterTest::object3D() { auto data = importer.object3D("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.object3D("foo")); } } @@ -1908,9 +1955,6 @@ void AbstractImporterTest::mesh() { auto data = importer.mesh("eighth", 2); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.mesh("foo")); } } @@ -2683,9 +2727,6 @@ void AbstractImporterTest::material() { auto data = importer.material("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.material("foo")); } } void AbstractImporterTest::materialNameNotImplemented() { @@ -2781,9 +2822,6 @@ void AbstractImporterTest::texture() { auto data = importer.texture("eighth"); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.texture("foo")); } } @@ -2885,9 +2923,6 @@ void AbstractImporterTest::image1D() { auto data = importer.image1D("eighth", 2); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.image1D("foo")); } } @@ -3121,9 +3156,6 @@ void AbstractImporterTest::image2D() { auto data = importer.image2D("eighth", 2); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.image2D("foo")); } } @@ -3357,9 +3389,6 @@ void AbstractImporterTest::image3D() { auto data = importer.image3D("eighth", 2); CORRADE_VERIFY(data); CORRADE_COMPARE(data->importerState(), &state); - } { - /* This should fail gracefully, not assert */ - CORRADE_VERIFY(!importer.image3D("foo")); } }