Don't warn about plugin options added to empty subgroups.

This was an annoyance with GltfImporter's customSceneFieldTypes, where
it just wasn't possible to set the options through AnySceneImporter or
magnum-sceneconverter -i argument without causing a warning to be
printed.

The behavior is now that if a plugin configuration subgroup (but not the
root group) is empty, it's assumed that new values are meant to be added
to it, and thus it doesn't warn on them.

src/Magnum/Implementation/converterUtilities.h

@@ -25,6 +25,7 @@
     DEALINGS IN THE SOFTWARE.
 */
 
+#include <unordered_set>
 #include <Corrade/Containers/Array.h>
 #include <Corrade/Containers/StaticArray.h>
 #include <Corrade/PluginManager/AbstractPlugin.h>
@@ -38,6 +39,7 @@ namespace Magnum { namespace Implementation {
 namespace {
 
 void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringView anyPluginName, const Containers::StringView options) {
+    std::unordered_set<const Utility::ConfigurationGroup*> emptySubgroups;
     for(const Containers::StringView option: options.splitWithoutEmptyParts(',')) {
         auto keyValue = option.partition('=');
         keyValue[0] = keyValue[0].trimmed();
@@ -52,8 +54,19 @@ void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringV
             if(!subgroup) {
                 groupNotRecognized = true;
                 subgroup = group->addGroup(keyParts[i]);
+            /* For existing subgroups (i.e., not the root configuration)
+               remember if the group was initially empty (no subgroups, no
+               values; comments can be there). For those we won't warn about
+               unrecognized options below as it's a common use case (for
+               example GltfImporter's customSceneFieldTypes). Has to be done
+               upfront in case more than one option is added to the same group
+               -- otherwise adding the second would warn again, as the group
+               is no longer empty at that point. */
+            } else if(!subgroup->hasGroups() && !subgroup->hasValues()) {
+                emptySubgroups.insert(subgroup);
             }
             group = subgroup;
+
         }
 
         /* Provide a warning message in case the plugin doesn't define given
@@ -62,9 +75,15 @@ void setOptions(PluginManager::AbstractPlugin& plugin, const Containers::StringV
            not an error.
 
            If it's an Any* plugin, then this check is provided by it directly,
-           and since the Any* plugin obviously don't expose the options of the concrete plugins, this warning would fire for them always, which
+           and since the Any* plugin obviously don't expose the options of the
+           concrete plugins, this warning would fire for them always, which
            wouldn't help anything. */
-        if((groupNotRecognized || !group->hasValue(keyParts.back())) && plugin.plugin() != anyPluginName) {
+        if((groupNotRecognized || (!group->hasValue(keyParts.back()) &&
+                /* The warning isn't printed in case a value is added into an
+                   empty subgroup, see above */
+                emptySubgroups.find(group) == emptySubgroups.end()
+            )) && plugin.plugin() != anyPluginName)
+        {
             Warning{} << "Option" << keyValue[0] << "not recognized by" << plugin.plugin();
         }
 

src/Magnum/Test/ConverterUtilitiesTest.cpp

@@ -150,6 +150,41 @@ option=value
 )",
         /* The trailing space is there because the plugin name is empty */
         "Option group/notFound/nested/option not recognized by \n"},
+    /* This should not warn for emptyGroup, since it's a common use case (for
+       example GltfImporter's customSceneFields). It should also remember that
+       the group was initially empty to not warn again when more options are
+       subsequently added. OTOH, for subgroups added to an empty group it
+       still warns. */
+    {"unrecognized option in empty config subgroup", R"([configuration]
+option=
+[configuration/emptyGroup]
+# No values originally here
+[configuration/nonEmptyGroup]
+option=
+)",
+        "emptyGroup/notFound=value,nonEmptyGroup/notFound=value,emptyGroup/another,emptyGroup/subgroup/notFound=value", "AnyPlugin", R"([configuration]
+option=
+[configuration/emptyGroup]
+# No values originally here
+notFound=value
+another=true
+[configuration/emptyGroup/subgroup]
+notFound=value
+[configuration/nonEmptyGroup]
+option=
+notFound=value
+)",
+        /* The trailing space is there because the plugin name is empty */
+        "Option nonEmptyGroup/notFound not recognized by \n"
+        "Option emptyGroup/subgroup/notFound not recognized by \n"},
+    /* OTOH this should warn, as it's an option in the root configuration */
+    {"unrecognized option in empty root config", R"([configuration]
+)",
+        "notFound=value", "AnyPlugin", R"([configuration]
+notFound=value
+)",
+        /* The trailing space is there because the plugin name is empty */
+        "Option notFound not recognized by \n"},
 };
 
 ConverterUtilitiesTest::ConverterUtilitiesTest() {

src/MagnumPlugins/AnySceneImporter/Test/AnySceneImporterTest.cpp

@@ -63,6 +63,7 @@ struct AnySceneImporterTest: TestSuite::Tester {
     void propagateFlags();
     void propagateConfiguration();
     void propagateConfigurationUnknown();
+    void propagateConfigurationUnknownInEmptySubgroup();
     void propagateFileCallback();
 
     void sceneFieldName();
@@ -115,6 +116,7 @@ AnySceneImporterTest::AnySceneImporterTest() {
               &AnySceneImporterTest::propagateFlags,
               &AnySceneImporterTest::propagateConfiguration,
               &AnySceneImporterTest::propagateConfigurationUnknown,
+              &AnySceneImporterTest::propagateConfigurationUnknownInEmptySubgroup,
               &AnySceneImporterTest::propagateFileCallback,
 
               &AnySceneImporterTest::sceneFieldName,
@@ -293,6 +295,32 @@ void AnySceneImporterTest::propagateConfigurationUnknown() {
         "Trade::AnySceneImporter::openFile(): option postprocess/feh/noHereNotEither not recognized by AssimpImporter\n");
 }
 
+void AnySceneImporterTest::propagateConfigurationUnknownInEmptySubgroup() {
+    PluginManager::Manager<AbstractImporter> manager{MAGNUM_PLUGINS_IMPORTER_INSTALL_DIR};
+    #ifdef ANYSCENEIMPORTER_PLUGIN_FILENAME
+    CORRADE_VERIFY(manager.load(ANYSCENEIMPORTER_PLUGIN_FILENAME) & PluginManager::LoadState::Loaded);
+    #endif
+
+    if(manager.load("GltfImporter") < PluginManager::LoadState::Loaded)
+        CORRADE_SKIP("GltfImporter plugin can't be loaded.");
+
+    Containers::Pointer<AbstractImporter> importer = manager.instantiate("AnySceneImporter");
+    importer->configuration().addGroup("customSceneFieldTypes");
+    importer->configuration().group("customSceneFieldTypes")->setValue("field", "Float");
+    importer->configuration().group("customSceneFieldTypes")->setValue("another", "Int");
+    importer->configuration().group("customSceneFieldTypes")->addGroup("notFound")->setValue("noHereNotEither", false);
+
+    std::ostringstream out;
+    Warning redirectWarning{&out};
+    CORRADE_VERIFY(importer->openFile(Utility::Path::join(ANYSCENEIMPORTER_TEST_DIR, "empty.gltf")));
+    CORRADE_COMPARE(out.str(),
+        /* Should not warn for values added to the empty customSceneFieldTypes
+           group, but should warn if a subgroup is added there. This is
+           consistent with how the magnum-*converter -i / -c options are
+           handled in Magnum/Implementation/converterUtilities.h. */
+        "Trade::AnySceneImporter::openFile(): option customSceneFieldTypes/notFound/noHereNotEither not recognized by GltfImporter\n");
+}
+
 void AnySceneImporterTest::propagateFileCallback() {
     if(!(_manager.loadState("ObjImporter") & PluginManager::LoadState::Loaded))
         CORRADE_SKIP("ObjImporter plugin not enabled, cannot test");

src/MagnumPlugins/AnySceneImporter/Test/CMakeLists.txt

@@ -52,6 +52,7 @@ corrade_add_test(AnySceneImporterTest AnySceneImporterTest.cpp
     LIBRARIES MagnumTrade
     FILES
         ${PROJECT_SOURCE_DIR}/src/MagnumPlugins/ObjImporter/Test/mesh-primitive-points.obj
+        empty.gltf
         mesh-custom-attribute.gltf
         scene-custom-field.gltf
         triangle.ply)

src/MagnumPlugins/AnySceneImporter/Test/empty.gltf

@@ -0,0 +1,5 @@
+{
+  "asset": {
+    "version": "2.0"
+  }
+}

src/MagnumPlugins/Implementation/propagateConfiguration.h

@@ -35,19 +35,21 @@
    implementation. Assumes that the Any* plugin itself doesn't have any
    configuration options and so propagates all groups and values that were
    set, emitting a warning if the target doesn't have such option in its
-   default configuration. */
+   default configuration.
+
+   Thoroughly tested in AnySceneImporterTest. */
 
 namespace Magnum { namespace Implementation {
 
 /* Used only in plugins where we don't want it to be exported */
 namespace {
 
-void propagateConfiguration(const char* warningPrefix, const Containers::String& groupPrefix, const Containers::StringView plugin, const Utility::ConfigurationGroup& src, Utility::ConfigurationGroup& dst) {
+void propagateConfiguration(const char* warningPrefix, const Containers::String& groupPrefix, const Containers::StringView plugin, const Utility::ConfigurationGroup& src, Utility::ConfigurationGroup& dst, bool warnUnrecognized = true) {
     using namespace Containers::Literals;
 
     /* Propagate values */
     for(Containers::Pair<Containers::StringView, Containers::StringView> value: src.values()) {
-        if(!dst.hasValue(value.first())) {
+        if(!dst.hasValue(value.first()) && warnUnrecognized) {
             Warning{} << warningPrefix << "option" << "/"_s.joinWithoutEmptyParts({groupPrefix, value.first()}) << "not recognized by" << plugin;
         }
 
@@ -57,8 +59,20 @@ void propagateConfiguration(const char* warningPrefix, const Containers::String&
     /* Recursively propagate groups */
     for(Containers::Pair<Containers::StringView, Containers::Reference<const Utility::ConfigurationGroup>> group: src.groups()) {
         Utility::ConfigurationGroup* dstGroup = dst.group(group.first());
-        if(!dstGroup) dstGroup = dst.addGroup(group.first());
+
-        propagateConfiguration(warningPrefix, "/"_s.joinWithoutEmptyParts({groupPrefix, group.first()}), plugin, group.second(), *dstGroup);
+        /* If the group exists but is empty (such as GltfImporter's
+           customSceneFieldTypes), don't warn about unrecognized values. This
+           logic is repeated for nested subgroups instead of being inherited,
+           meaning that adding a nonexistent subgroup to an empty group will
+           warn again. */
+        bool warnUnrecognizedSubgroup = true;
+        if(!dstGroup) {
+            dstGroup = dst.addGroup(group.first());
+        } else if(!dstGroup->hasGroups() && !dstGroup->hasValues()) {
+            warnUnrecognizedSubgroup = false;
+        }
+
+        propagateConfiguration(warningPrefix, "/"_s.joinWithoutEmptyParts({groupPrefix, group.first()}), plugin, group.second(), *dstGroup, warnUnrecognizedSubgroup);
     }
 }