Mod error detection improvements!

Separated Warning vs Error, show "options only" warning in options only, color warnings by severity

core/src/com/unciv/logic/GameInfo.kt

@@ -135,9 +135,7 @@ class GameInfo {
     fun isReligionEnabled(): Boolean {
         if (ruleSet.eras[gameParameters.startingEra]!!.hasUnique("Starting in this era disables religion")
             || ruleSet.modOptions.uniques.contains(ModOptionsConstants.disableReligion)
-        ) {
+        ) return false
-            return false
-        }
         return gameParameters.religionEnabled
     }
 

core/src/com/unciv/models/ruleset/Ruleset.kt

@@ -278,34 +278,8 @@ class Ruleset {
         return stringList.joinToString { it.tr() }
     }
 
-    /** Severity level of Mod RuleSet check */
-    enum class CheckModLinksStatus {OK, Warning, Error}
-    /** Result of a Mod RuleSet check */
-    // essentially a named Pair with a few shortcuts
-    class CheckModLinksResult(val status: CheckModLinksStatus, val message: String) {
-        // Empty constructor just makes the Complex Mod Check on the new game screen shorter
-        constructor(): this(CheckModLinksStatus.OK, "")
-        // Constructor that joins lines
-        constructor(status: CheckModLinksStatus, lines: ArrayList<String>):
-                this (status,
-                    lines.joinToString("\n"))
-        // Constructor that auto-determines severity
-        constructor(warningCount: Int, lines: ArrayList<String>):
-                this (
-                    when {
-                        lines.isEmpty() -> CheckModLinksStatus.OK
-                        lines.size == warningCount -> CheckModLinksStatus.Warning
-                        else -> CheckModLinksStatus.Error
-                    },
-                    lines)
-        // Allows $this in format strings
-        override fun toString() = message
-        // Readability shortcuts
-        fun isError() = status == CheckModLinksStatus.Error
-        fun isNotOK() = status != CheckModLinksStatus.OK
-    }
 
-    fun checkUniques(uniqueContainer:IHasUniques, lines:ArrayList<String>,
+    fun checkUniques(uniqueContainer:IHasUniques, lines:RulesetErrorList,
                      severityToReport: UniqueType.UniqueComplianceErrorSeverity) {
         val name = if (uniqueContainer is INamed) uniqueContainer.name else ""
 
@@ -323,15 +297,47 @@ class Ruleset {
                 .getAnnotation(Deprecated::class.java)
             if (deprecationAnnotation != null) {
                 // Not user-visible
-                println("$name's unique \"${unique.text}\" is deprecated ${deprecationAnnotation.message}," +
+                lines.add("$name's unique \"${unique.text}\" is deprecated ${deprecationAnnotation.message}," +
-                        " replace with \"${deprecationAnnotation.replaceWith.expression}\"")
+                        " replace with \"${deprecationAnnotation.replaceWith.expression}\"",
+                    RulesetErrorSeverity.WarningOptionsOnly)
             }
         }
     }
 
-    fun checkModLinks(): CheckModLinksResult {
+
-        val lines = ArrayList<String>()
+    class RulesetError(val text:String, val errorSeverityToReport: RulesetErrorSeverity)
-        var warningCount = 0
+    enum class RulesetErrorSeverity{
+        OK,
+        Warning,
+        WarningOptionsOnly,
+        Error,
+    }
+
+    class RulesetErrorList:ArrayList<RulesetError>() {
+        operator fun plusAssign(text: String) {
+            add(text, RulesetErrorSeverity.Error)
+        }
+
+        fun add(text: String, errorSeverityToReport: RulesetErrorSeverity) {
+            add(RulesetError(text, errorSeverityToReport))
+        }
+
+        fun getFinalSeverity(): RulesetErrorSeverity {
+            if (isEmpty()) return RulesetErrorSeverity.OK
+            return this.maxOf { it.errorSeverityToReport }
+        }
+
+        fun isError() = getFinalSeverity() == RulesetErrorSeverity.Error
+        fun isNotOK() = getFinalSeverity() != RulesetErrorSeverity.OK
+
+        fun getErrorText() =
+            filter { it.errorSeverityToReport != RulesetErrorSeverity.WarningOptionsOnly }
+                .sortedByDescending { it.errorSeverityToReport }
+                .joinToString { it.errorSeverityToReport.name + ": " + it.text }
+    }
+
+    fun checkModLinks(): RulesetErrorList {
+        val lines = RulesetErrorList()
 
         // Checks for all mods - only those that can succeed without loading a base ruleset
         // When not checking the entire ruleset, we can only really detect ruleset-invariant errors in uniques
@@ -373,7 +379,7 @@ class Ruleset {
         }
 
         // Quit here when no base ruleset is loaded - references cannot be checked
-        if (!modOptions.isBaseRuleset) return CheckModLinksResult(warningCount, lines)
+        if (!modOptions.isBaseRuleset) return lines
 
         val baseRuleset = RulesetCache.getBaseRuleset()  // for UnitTypes fallback
 
@@ -401,8 +407,8 @@ class Ruleset {
                 else if ((tileImprovements[improvementName] as Stats).none() &&
                         unit.isCivilian() &&
                         !unit.hasUnique("Bonus for units in 2 tile radius 15%")) {
-                    lines += "${unit.name} can place improvement $improvementName which has no stats, preventing unit automation!"
+                    lines.add("${unit.name} can place improvement $improvementName which has no stats, preventing unit automation!",
-                    warningCount++
+                        RulesetErrorSeverity.Warning)
                 }
             }
 
@@ -474,8 +480,8 @@ class Ruleset {
 
                 if (tech.prerequisites.asSequence().filterNot { it == prereq }
                         .any { getPrereqTree(it).contains(prereq) }){
-                    lines += "No need to add $prereq as a prerequisite of ${tech.name} - it is already implicit from the other prerequisites!"
+                    lines.add("No need to add $prereq as a prerequisite of ${tech.name} - it is already implicit from the other prerequisites!",
-                    warningCount++
+                        RulesetErrorSeverity.Warning)
                 }
             }
             if (tech.era() !in eras)
@@ -485,7 +491,6 @@ class Ruleset {
 
         if (eras.isEmpty()) {
             lines += "Eras file is empty! This will likely lead to crashes. Ask the mod maker to update this mod!"
-            warningCount++
         }
         
         for (era in eras.values) {
@@ -504,7 +509,7 @@ class Ruleset {
             checkUniques(era, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific)
         }
 
-        return CheckModLinksResult(warningCount, lines)
+        return lines
     }
 }
 
@@ -536,7 +541,7 @@ object RulesetCache : HashMap<String,Ruleset>() {
                 this[modRuleset.name] = modRuleset
                 if (printOutput) {
                     println("Mod loaded successfully: " + modRuleset.name)
-                    println(modRuleset.checkModLinks())
+                    println(modRuleset.checkModLinks().getErrorText())
                 }
             } catch (ex: Exception) {
                 if (printOutput) {
@@ -588,7 +593,7 @@ object RulesetCache : HashMap<String,Ruleset>() {
     /**
      * Runs [Ruleset.checkModLinks] on a temporary [combined Ruleset][getComplexRuleset] for a list of [mods]
      */
-    fun checkCombinedModLinks(mods: LinkedHashSet<String>): Ruleset.CheckModLinksResult {
+    fun checkCombinedModLinks(mods: LinkedHashSet<String>): Ruleset.RulesetErrorList {
         return try {
             val newRuleset = getComplexRuleset(mods)
             newRuleset.modOptions.isBaseRuleset = true // This is so the checkModLinks finds all connections
@@ -596,7 +601,8 @@ object RulesetCache : HashMap<String,Ruleset>() {
         } catch (ex: Exception) {
             // This happens if a building is dependent on a tech not in the base ruleset
             //  because newRuleset.updateBuildingCosts() in getComplexRuleset() throws an error
-            Ruleset.CheckModLinksResult(Ruleset.CheckModLinksStatus.Error, ex.localizedMessage)
+            Ruleset.RulesetErrorList()
+                .apply { add(ex.localizedMessage, Ruleset.RulesetErrorSeverity.Error) }
         }
     }
 

core/src/com/unciv/ui/newgamescreen/ModCheckboxTable.kt

@@ -4,7 +4,6 @@ import com.badlogic.gdx.graphics.Color
 import com.badlogic.gdx.scenes.scene2d.ui.CheckBox
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.unciv.models.ruleset.Ruleset
-import com.unciv.models.ruleset.Ruleset.CheckModLinksResult
 import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.translations.tr
 import com.unciv.ui.utils.*
@@ -61,12 +60,10 @@ class ModCheckboxTable(
             if (modLinkErrors.isError()) {
                 lastToast?.close()
                 val toastMessage =
-                    "The mod you selected is incorrectly defined!".tr() + "\n\n$modLinkErrors"
+                    "The mod you selected is incorrectly defined!".tr() + "\n\n${modLinkErrors.getErrorText()}"
                 lastToast = ToastPopup(toastMessage, screen, 5000L)
-                if (modLinkErrors.isError()) {
+                checkBox.isChecked = false
-                    checkBox.isChecked = false
+                return false
-                    return false
-                }
             }
 
             // Save selection for a rollback
@@ -88,7 +85,7 @@ class ModCheckboxTable(
                 val toastMessage = (
                         if (complexModLinkCheck.isError()) "The mod combination you selected is incorrectly defined!"
                         else "{The mod combination you selected has problems.}\n{You can play it, but don't expect everything to work!}"
-                        ).tr() + "\n\n$complexModLinkCheck"
+                        ).tr() + "\n\n${complexModLinkCheck.getErrorText()}"
                 lastToast = ToastPopup(toastMessage, screen, 5000L)
 
                 if (complexModLinkCheck.isError()) {

core/src/com/unciv/ui/worldscreen/mainmenu/OptionsPopup.kt

@@ -14,7 +14,7 @@ import com.unciv.logic.MapSaver
 import com.unciv.logic.civilization.PlayerType
 import com.unciv.models.UncivSound
 import com.unciv.models.metadata.BaseRuleset
-import com.unciv.models.ruleset.Ruleset.CheckModLinksStatus
+import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.tilesets.TileSetCache
 import com.unciv.models.translations.TranslationFileWriter
@@ -275,19 +275,22 @@ class OptionsPopup(val previousScreen: CameraStageBaseScreen) : Popup(previousSc
             val lines = ArrayList<FormattedLine>()
             var noProblem = true
             for (mod in RulesetCache.values.sortedBy { it.name }) {
-                val modLinks = if (complex) RulesetCache.checkCombinedModLinks(linkedSetOf(mod.name))
-                    else mod.checkModLinks()
-                val color = when (modLinks.status) {
-                    CheckModLinksStatus.OK -> "#0F0"
-                    CheckModLinksStatus.Warning -> "#FF0"
-                    CheckModLinksStatus.Error -> "#F00"
-                }
                 val label = if (mod.name.isEmpty()) BaseRuleset.Civ_V_Vanilla.fullName else mod.name
-                lines += FormattedLine("$label{}", starred = true, color = color, header = 3)
+                lines += FormattedLine("$label{}", starred = true, header = 3)
-                if (modLinks.isNotOK()) {
+
-                    lines += FormattedLine(modLinks.message)
+                val modLinks =
-                    noProblem = false
+                    if (complex) RulesetCache.checkCombinedModLinks(linkedSetOf(mod.name))
+                    else mod.checkModLinks()
+                for (error in modLinks) {
+                    val color = when (error.errorSeverityToReport) {
+                        Ruleset.RulesetErrorSeverity.OK -> "#0F0"
+                        Ruleset.RulesetErrorSeverity.Warning,
+                        Ruleset.RulesetErrorSeverity.WarningOptionsOnly -> "#FF0"
+                        Ruleset.RulesetErrorSeverity.Error -> "#F00"
+                    }
+                    lines += FormattedLine(error.text, color = color)
                 }
+                if (modLinks.isNotOK()) noProblem = false
                 lines += FormattedLine()
             }
             if (noProblem) lines += FormattedLine("{No problems found}.")