From 363803fc458c049cc3488e2b4a40cc67583b7eb2 Mon Sep 17 00:00:00 2001
From: pietervdvn <pietervdvn@posteo.net>
Date: Sun, 20 Feb 2022 00:51:11 +0100
Subject: [PATCH 1/3] Improvement in logging

---
 Models/ThemeConfig/Conversion/Validation.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Models/ThemeConfig/Conversion/Validation.ts b/Models/ThemeConfig/Conversion/Validation.ts
index e7624b0dce..dc17d0d506 100644
--- a/Models/ThemeConfig/Conversion/Validation.ts
+++ b/Models/ThemeConfig/Conversion/Validation.ts
@@ -305,9 +305,9 @@ export class DetectMappingsWithImages extends DesugaringStep<TagRenderingConfigJ
             const ctx = `${context}.mappings[${i}]`
             if (images.length > 0) {
                 if(!ignore){
-                    errors.push(`${ctx}: A mapping has an image in the 'then'-clause. Remove the image there and use \`"icon": <your-image>\` instead. The images found are ${images.join(", ")}. (Ignore this warning by adding "#": "${ignoreToken}" to the mapping`)
+                    errors.push(`${ctx}: A mapping has an image in the 'then'-clause. Remove the image there and use \`"icon": <your-image>\` instead. The images found are ${images.join(", ")}. (This check can be turned of by adding "#": "${ignoreToken}" in the mapping, but this is discouraged`)
                 }else{
-                    information.push(`${ctx}: Ignored images in then`)
+                    information.push(`${ctx}: Ignored image ${images.join(", ")} in 'then'-clause of a mapping as this check has been disabled`)
                 }
             }else if (ignore){
                 warnings.push(`${ctx}: unused '${ignoreToken}' - please remove this`)

From 125b63927dd0dbe40ae2c4745ced7520e548968c Mon Sep 17 00:00:00 2001
From: pietervdvn <pietervdvn@posteo.net>
Date: Sun, 20 Feb 2022 01:39:12 +0100
Subject: [PATCH 2/3] Improve shadowing detection of mappings

---
 Models/ThemeConfig/Conversion/Validation.ts | 32 ++++++++++++++-------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/Models/ThemeConfig/Conversion/Validation.ts b/Models/ThemeConfig/Conversion/Validation.ts
index dc17d0d506..5380360d36 100644
--- a/Models/ThemeConfig/Conversion/Validation.ts
+++ b/Models/ThemeConfig/Conversion/Validation.ts
@@ -248,9 +248,19 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
         if (json.mappings === undefined || json.mappings.length === 0) {
             return {result: json}
         }
-        const parsedConditions = json.mappings.map(m => TagUtils.Tag(m.if))
+        const parsedConditions = json.mappings.map(m => {
+            const ifTags = TagUtils.Tag(m.if);
+            if(m.hideInAnswer !== undefined && m.hideInAnswer !== false && m.hideInAnswer !== true){
+                let conditionTags = TagUtils.Tag( m.hideInAnswer)
+                // Merge the condition too!
+                return new And([conditionTags, ifTags])
+            }
+            return ifTags
+        })
         for (let i = 0; i < json.mappings.length; i++) {
-            if(json.mappings[i].hideInAnswer === true){
+            if(!parsedConditions[i].isUsableAsAnswer()){
+                // There is no straightforward way to convert this mapping.if into a properties-object, so we simply skip this one
+                // Yes, it might be shadowed, but running this check is to difficult right now
                 continue
             }
             const keyValues = parsedConditions[i].asChange({});
@@ -259,14 +269,11 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
                 properties[k] = v
             })
             for (let j = 0; j < i; j++) {
-                if(json.mappings[j].hideInAnswer === true){
-                    continue
-                }
                 const doesMatch = parsedConditions[j].matchesProperties(properties)
                 if (doesMatch) {
                     // The current mapping is shadowed!
                     errors.push(`At ${context}: Mapping ${i} is shadowed by mapping ${j} and will thus never be shown:
-    The mapping ${parsedConditions[i].asHumanString(false, false, {})} is fully matched by a previous mapping, which matches:
+    The mapping ${parsedConditions[i].asHumanString(false, false, {})} is fully matched by a previous mapping (namely ${j}), which matches:
     ${parsedConditions[j].asHumanString(false, false, {})}.
     
     Move the mapping up to fix this problem
@@ -276,6 +283,10 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
 
         }
 
+        // TODO make this errors again
+        warnings.push(...errors)
+        errors.splice(0, errors.length)
+
         return {
             errors,
             warnings,
@@ -314,8 +325,10 @@ export class DetectMappingsWithImages extends DesugaringStep<TagRenderingConfigJ
             }
         }
 
-        return {
-            errors,warnings,information,
+         return {
+            errors,
+            warnings,
+            information,
             result: json
         };
     }
@@ -324,8 +337,7 @@ export class DetectMappingsWithImages extends DesugaringStep<TagRenderingConfigJ
 export class ValidateTagRenderings extends Fuse<TagRenderingConfigJson> {
     constructor() {
         super("Various validation on tagRenderingConfigs",
-        // TODO enable these checks again
-        //    new DetectShadowedMappings(),
+            new DetectShadowedMappings(),
             new DetectMappingsWithImages()    
         );
     }

From 30f4be183ee232d4f941c0987b24dbf0d2dd3b71 Mon Sep 17 00:00:00 2001
From: pietervdvn <pietervdvn@posteo.net>
Date: Sun, 20 Feb 2022 02:02:09 +0100
Subject: [PATCH 3/3] Fix tests

---
 Models/ThemeConfig/Conversion/Validation.ts | 2 +-
 test/LegacyThemeLoader.spec.ts              | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Models/ThemeConfig/Conversion/Validation.ts b/Models/ThemeConfig/Conversion/Validation.ts
index 5380360d36..f8174c5b0e 100644
--- a/Models/ThemeConfig/Conversion/Validation.ts
+++ b/Models/ThemeConfig/Conversion/Validation.ts
@@ -264,7 +264,7 @@ export class DetectShadowedMappings extends DesugaringStep<TagRenderingConfigJso
                 continue
             }
             const keyValues = parsedConditions[i].asChange({});
-            const properties = []
+            const properties = {}
             keyValues.forEach(({k, v}) => {
                 properties[k] = v
             })
diff --git a/test/LegacyThemeLoader.spec.ts b/test/LegacyThemeLoader.spec.ts
index e3b66ff655..8b1972120d 100644
--- a/test/LegacyThemeLoader.spec.ts
+++ b/test/LegacyThemeLoader.spec.ts
@@ -422,8 +422,8 @@ export default class LegacyThemeLoaderSpec extends T {
                                 }
                             ]
                         }, "test");
-                        T.isTrue(r.errors.length > 0, "Failing case 0 is not detected")
-
+                        T.isTrue(r.warnings.length > 0, "Failing case 0 is not detected")
+                        T.isTrue(r.warnings[0].indexOf("The mapping key=value is fully matched by a previous mapping (namely 0)") >= 0, "Error message does not contain tag and indices")
                         const r0 = new DetectShadowedMappings().convert({
                             mappings: [
                                 {
@@ -436,7 +436,7 @@ export default class LegacyThemeLoaderSpec extends T {
                                 }
                             ]
                         }, "test");
-                        T.isTrue(r0.errors.length > 0, "Failing case 1 is not detected")
+                        T.isTrue(r0.warnings.length > 0, "Failing case 1 is not detected")
                     }
                 ],
                 ["Images are rewritten", () => {