Global review of Constants use, linting, maxXPfromBarbarians (#6327)

Co-authored-by: Yair Morgenstern <yairm210@hotmail.com>

core/src/com/unciv/Constants.kt

@@ -9,7 +9,12 @@ object Constants {
 
     const val impassable = "Impassable"
     const val ocean = "Ocean"
+
+    /** The "Coast" _terrain_ */
     const val coast = "Coast"
+    /** The "Coastal" terrain _filter_ */
+    const val coastal = "Coastal"
+
     const val mountain = "Mountain"
     const val hill = "Hill"
     const val plains = "Plains"
@@ -25,7 +30,11 @@ object Constants {
     val vegetation = arrayOf(forest, jungle)
     val sea = arrayOf(ocean, coast)
 
+    // Note the difference in case. **Not** interchangeable!
+    /** The "Fresh water" terrain _unique_ */
     const val freshWater = "Fresh water"
+    /** The "Fresh Water" terrain _filter_ */
+    const val freshWaterFilter = "Fresh Water"
 
     const val barbarianEncampment = "Barbarian encampment"
 
@@ -53,7 +62,6 @@ object Constants {
 
     const val barbarians = "Barbarians"
     const val spectator = "Spectator"
-    const val custom = "Custom"
 
     const val rising = "Rising"
     const val lowering = "Lowering"

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

@@ -5,6 +5,7 @@ import com.unciv.logic.city.PerpetualConstruction
 import com.unciv.logic.civilization.TechManager
 import com.unciv.logic.civilization.diplomacy.DiplomacyFlags
 import com.unciv.logic.civilization.diplomacy.DiplomacyManager
+import com.unciv.models.ruleset.ModOptions
 import com.unciv.models.ruleset.Ruleset
 
 /**
@@ -141,4 +142,11 @@ object BackwardCompatibility {
         }
     }
 
+    /** Move max XP from barbarians to new home */
+    fun ModOptions.updateDeprecations() {
+        if (maxXPfromBarbarians != 30) {
+            constants.maxXPfromBarbarians = maxXPfromBarbarians
+            maxXPfromBarbarians = 30
+        }
+    }
 }

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

@@ -226,10 +226,12 @@ object GameStarter {
         val presetMajors = Stack<String>()
         presetMajors.addAll(availableCivNames.filter { it in civNamesWithStartingLocations })
 
-        for (player in newGameParameters.players.sortedBy { it.chosenCiv == "Random" }) {
+        for (player in newGameParameters.players.sortedBy { it.chosenCiv == Constants.random }) {
-            val nationName = if (player.chosenCiv != "Random") player.chosenCiv
+            val nationName = when {
-            else if (presetMajors.isNotEmpty()) presetMajors.pop()
+                player.chosenCiv != Constants.random -> player.chosenCiv
-            else availableCivNames.pop()
+                presetMajors.isNotEmpty() -> presetMajors.pop()
+                else -> availableCivNames.pop()
+            }
             availableCivNames.remove(nationName) // In case we got it from a map preset
 
             val playerCiv = CivilizationInfo(nationName)
@@ -409,7 +411,7 @@ object GameStarter {
             when {
                 civ.civName in tileMap.startingLocationsByNation -> 1 // harshest requirements
                 civ.nation.startBias.any { it in tileMap.naturalWonders } -> 2
-                civ.nation.startBias.contains("Tundra") -> 3    // Tundra starts are hard to find, so let's do them first
+                civ.nation.startBias.contains(Constants.tundra) -> 3    // Tundra starts are hard to find, so let's do them first
                 civ.nation.startBias.isNotEmpty() -> 4 // less harsh
                 else -> 5  // no requirements
             }

core/src/com/unciv/logic/map/MapParameters.kt

@@ -1,11 +1,9 @@
 package com.unciv.logic.map
 
-import com.unciv.Constants
 import com.unciv.logic.HexMath.getEquivalentHexagonalRadius
 import com.unciv.logic.HexMath.getEquivalentRectangularSize
 import com.unciv.logic.HexMath.getNumberOfTilesInHexagon
 import com.unciv.models.metadata.BaseRuleset
-import com.unciv.models.ruleset.RulesetCache
 
 
 /* Predefined Map Sizes - ours are a little lighter than the original values. For reference those are:
@@ -21,7 +19,14 @@ enum class MapSize(val radius: Int, val width: Int, val height: Int) {
     Small(15, 33, 21),
     Medium(20, 44, 29),
     Large(30, 66, 43),
-    Huge(40, 87, 57)
+    Huge(40, 87, 57);
+
+    companion object {
+        /** Not a predefined [MapSize] enum value, but a String
+         * used in [MapParameters.mapSize] to indicate user-defined dimensions.
+         * Do not mistake for [MapType.custom]. */
+        const val custom = "Custom"
+    }
 }
 
 class MapSizeNew {
@@ -54,12 +59,12 @@ class MapSizeNew {
     }
 
     constructor(radius: Int) {
-        name = Constants.custom
+        name = MapSize.custom
         setNewRadius(radius)
     }
 
     constructor(width: Int, height: Int) {
-        name = Constants.custom
+        name = MapSize.custom
         this.width = width
         this.height = height
         this.radius = getEquivalentHexagonalRadius(width, height)
@@ -77,7 +82,7 @@ class MapSizeNew {
      * @return null if size was acceptable, otherwise untranslated reason message
      */
     fun fixUndesiredSizes(worldWrap: Boolean): String? {
-        if (name != Constants.custom) return null  // predefined sizes are OK
+        if (name != MapSize.custom) return null  // predefined sizes are OK
         // world-wrap mas must always have an even width, so round down silently
         if (worldWrap && width % 2 != 0 ) width--
         // check for any bad condition and bail if none of them
@@ -113,7 +118,7 @@ class MapSizeNew {
     }
 
     // For debugging and MapGenerator console output
-    override fun toString() = if (name == Constants.custom) "${width}x${height}" else name
+    override fun toString() = if (name == MapSize.custom) "${width}x${height}" else name
 }
 
 object MapShape {
@@ -227,7 +232,7 @@ class MapParameters {
     override fun toString() = sequence {
         if (name.isNotEmpty()) yield("\"$name\" ")
         yield("(")
-        if (mapSize.name != Constants.custom) yield(mapSize.name + " ")
+        if (mapSize.name != MapSize.custom) yield(mapSize.name + " ")
         if (worldWrap) yield("wrapped ")
         yield(shape)
         yield(" " + displayMapDimensions())

core/src/com/unciv/logic/map/MapUnit.kt

@@ -1085,7 +1085,7 @@ class MapUnit {
             // todo: unit filters should be adjectives, fitting "[filterType] units"
             // This means converting "wounded units" to "Wounded", "Barbarians" to "Barbarian"
             "Wounded", "wounded units" -> health < 100
-            "Barbarians", "Barbarian" -> civInfo.isBarbarian()
+            Constants.barbarians, "Barbarian" -> civInfo.isBarbarian()
             "City-State" -> civInfo.isCityState()
             "Embarked" -> isEmbarked()
             "Non-City" -> true

core/src/com/unciv/logic/map/TileInfo.kt

@@ -556,7 +556,7 @@ open class TileInfo {
             baseTerrain -> true
             "Water" -> isWater
             "Land" -> isLand
-            "Coastal" -> isCoastalTile()
+            Constants.coastal -> isCoastalTile()
             "River" -> isAdjacentToRiver()
             naturalWonder -> true
             "Open terrain" -> !isRoughTerrain()
@@ -568,7 +568,7 @@ open class TileInfo {
             "Water resource" -> isWater && observingCiv != null && hasViewableResource(observingCiv)
             "Natural Wonder" -> naturalWonder != null
             "Featureless" -> terrainFeatures.isEmpty()
-            "Fresh Water" -> isAdjacentTo(Constants.freshWater)
+            Constants.freshWaterFilter -> isAdjacentTo(Constants.freshWater)
             else -> {
                 if (terrainFeatures.contains(filter)) return true
                 if (getAllTerrains().any { it.hasUnique(filter) }) return true

core/src/com/unciv/logic/map/mapgenerator/NaturalWonderGenerator.kt

@@ -180,7 +180,7 @@ class NaturalWonderGenerator(val ruleset: Ruleset, val randomness: MapGeneration
         "Elevated" -> baseTerrain == Constants.mountain || isHill()
         "Water" -> isWater
         "Land" -> isLand
-        "Hill" -> isHill()
+        Constants.hill -> isHill()
         naturalWonder -> true
         in allTerrainFeatures -> getLastTerrain().name == filter
         else -> baseTerrain == filter

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

@@ -5,6 +5,7 @@ import com.badlogic.gdx.files.FileHandle
 import com.badlogic.gdx.graphics.Color
 import com.unciv.Constants
 import com.unciv.JsonParser
+import com.unciv.logic.BackwardCompatibility.updateDeprecations
 import com.unciv.logic.UncivShowableException
 import com.unciv.models.Counter
 import com.unciv.models.ModConstants
@@ -52,7 +53,7 @@ class ModOptions : IHasUniques {
     var modSize = 0
  
     @Deprecated("As of 3.18.15")
-    val maxXPfromBarbarians = 30
+    var maxXPfromBarbarians = 30
 
     override var uniques = ArrayList<String>()
 
@@ -166,8 +167,7 @@ class Ruleset {
         if (modOptionsFile.exists()) {
             try {
                 modOptions = jsonParser.getFromJson(ModOptions::class.java, modOptionsFile)
-                if (modOptions.maxXPfromBarbarians != 30)
+                modOptions.updateDeprecations()
-                    modOptions.constants.maxXPfromBarbarians = modOptions.constants.maxXPfromBarbarians
             } catch (ex: Exception) {}
             modOptions.uniqueObjects = modOptions.uniques.map { Unique(it, UniqueTarget.ModOptions) }
             modOptions.uniqueMap = modOptions.uniqueObjects.groupBy { it.placeholderText }

core/src/com/unciv/models/ruleset/unique/UniqueParameterType.kt

@@ -33,7 +33,7 @@ enum class UniqueParameterType(var parameterName:String) {
 
     },
     MapUnitFilter("mapUnitFilter") {
-        private val knownValues = setOf("Wounded", "Barbarians", "City-State", "Embarked", "Non-City")
+        private val knownValues = setOf("Wounded", Constants.barbarians, "City-State", "Embarked", "Non-City")
         override fun getErrorSeverity(parameterText: String, ruleset: Ruleset):
                 UniqueType.UniqueComplianceErrorSeverity? {
             if ('{' in parameterText) // "{filter} {filter}" for and logic
@@ -138,9 +138,9 @@ enum class UniqueParameterType(var parameterName:String) {
     //
     TerrainFilter("terrainFilter") {
         private val knownValues = setOf("All",
-            "Coastal", "River", "Open terrain", "Rough terrain", "Water resource",
+            Constants.coastal, "River", "Open terrain", "Rough terrain", "Water resource",
             "Foreign Land", "Foreign", "Friendly Land", "Friendly", "Enemy Land", "Enemy",
-            "Featureless", "Fresh Water", "Natural Wonder")
+            "Featureless", Constants.freshWaterFilter, "Natural Wonder")
         override fun getErrorSeverity(parameterText: String, ruleset: Ruleset):
                 UniqueType.UniqueComplianceErrorSeverity? {
             if (parameterText in knownValues) return null

core/src/com/unciv/models/ruleset/unique/UniqueType.kt

@@ -1,5 +1,6 @@
 package com.unciv.models.ruleset.unique
 
+import com.unciv.Constants
 import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.translations.getPlaceholderParameters
 import com.unciv.models.translations.getPlaceholderText
@@ -253,7 +254,6 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags:
     TriggersCulturalVictory("Triggers a Cultural Victory upon completion", UniqueTarget.Global),
 
     BetterDefensiveBuildings("[amount]% City Strength from defensive buildings", UniqueTarget.Global),
-
     TileImprovementTime("[amount]% tile improvement construction time", UniqueTarget.Global, UniqueTarget.Unit),
     PercentGoldFromTradeMissions("[amount]% Gold from Great Merchant trade missions", UniqueTarget.Global),
     // Todo: Lowercase the 'U' of 'Units' in this unique
@@ -275,6 +275,7 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags:
     EmbarkAndEnterOcean("Can embark and move over Coasts and Oceans immediately", UniqueTarget.Global),
 
     PopulationLossFromNukes("Population loss from nuclear attacks [amount]% [cityFilter]", UniqueTarget.Global),
+  
     NaturalReligionSpreadStrength("[amount]% Natural religion spread [cityFilter]", UniqueTarget.FollowerBelief, UniqueTarget.Global),
     ReligionSpreadDistance("Religion naturally spreads to cities [amount] tiles away", UniqueTarget.Global, UniqueTarget.FollowerBelief),
 
@@ -361,7 +362,6 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags:
     // This doesn't actually directly affect anything, the "Only available <if [Manhattan Project] is constructed>" of the nuclear weapons does that.
     EnablesNuclearWeapons("Enables nuclear weapon", UniqueTarget.Building),
 
-
     MustBeOn("Must be on [terrainFilter]", UniqueTarget.Building),
     MustNotBeOn("Must not be on [terrainFilter]", UniqueTarget.Building),
     MustBeNextTo("Must be next to [terrainFilter]", UniqueTarget.Building, UniqueTarget.Improvement),
@@ -542,7 +542,7 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags:
     ResistsNukes("Resistant to nukes", UniqueTarget.Terrain),
     DestroyableByNukes("Can be destroyed by nukes", UniqueTarget.Terrain),
 
-    FreshWater("Fresh water", UniqueTarget.Terrain),
+    FreshWater(Constants.freshWater, UniqueTarget.Terrain),
     RoughTerrain("Rough terrain", UniqueTarget.Terrain),
 
     /////// Resource uniques

core/src/com/unciv/ui/mapeditor/MapEditorOptionsTable.kt

@@ -325,7 +325,7 @@ class MapEditorOptionsTable(val mapEditorScreen: MapEditorScreen): Table(BaseScr
             if (terrain.type == TerrainType.TerrainFeature) {
                 tileInfo.baseTerrain = when {
                     terrain.occursOn.isNotEmpty() -> terrain.occursOn.first()
-                    else -> "Grassland"
+                    else -> Constants.grassland
                 }
                 tileInfo.addTerrainFeature(terrain.name)
             } else tileInfo.baseTerrain = terrain.name

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

@@ -5,7 +5,6 @@ import com.badlogic.gdx.scenes.scene2d.ui.CheckBox
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.badlogic.gdx.scenes.scene2d.ui.TextField
 import com.badlogic.gdx.scenes.scene2d.ui.TextField.TextFieldFilter.DigitsOnlyFilter
-import com.unciv.Constants
 import com.unciv.UncivGame
 import com.unciv.logic.map.*
 import com.unciv.ui.utils.*
@@ -97,7 +96,7 @@ class MapParametersTable(
     }
 
     private fun addWorldSizeTable() {
-        val mapSizes = MapSize.values().map { it.name } + listOf(Constants.custom)
+        val mapSizes = MapSize.values().map { it.name } + listOf(MapSize.custom)
         worldSizeSelectBox = TranslatedSelectBox(mapSizes, mapParameters.mapSize.name, skin)
         worldSizeSelectBox.onChange { updateWorldSizeTable() }
 
@@ -155,9 +154,9 @@ class MapParametersTable(
     private fun updateWorldSizeTable() {
         customWorldSizeTable.clear()
 
-        if (mapParameters.shape == MapShape.hexagonal && worldSizeSelectBox.selected.value == Constants.custom)
+        if (mapParameters.shape == MapShape.hexagonal && worldSizeSelectBox.selected.value == MapSize.custom)
             customWorldSizeTable.add(hexagonalSizeTable).grow().row()
-        else if (mapParameters.shape == MapShape.rectangular && worldSizeSelectBox.selected.value == Constants.custom)
+        else if (mapParameters.shape == MapShape.rectangular && worldSizeSelectBox.selected.value == MapSize.custom)
             customWorldSizeTable.add(rectangularSizeTable).grow().row()
         else
             mapParameters.mapSize = MapSizeNew(worldSizeSelectBox.selected.value)

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

@@ -128,7 +128,7 @@ class PlayerPickerTable(
      * @param player for which [Table] is generated
      * @return [Table] containing the all the elements
      */
-    fun getPlayerTable(player: Player): Table {
+    private fun getPlayerTable(player: Player): Table {
         val playerTable = Table()
         playerTable.pad(5f)
         playerTable.background = ImageGetter.getBackground(ImageGetter.getBlue().darken(0.8f))
@@ -274,7 +274,7 @@ private class NationPickerPopup(
         add(nationDetailsScroll).size(civBlocksWidth + 10f, partHeight) // Same here, see above
 
         val randomNation = Nation().apply {
-            name = "Random"
+            name = Constants.random
             innerColor = listOf(255, 255, 255)
             outerColor = listOf(0, 0, 0)
             setTransients()
@@ -285,7 +285,7 @@ private class NationPickerPopup(
         if (spectator != null) nations += spectator
 
         nations += playerPicker.getAvailablePlayerCivs(player.chosenCiv)
-            .sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale(), { it.name.tr() }))
+            .sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.name.tr() })
 
         var nationListScrollY = 0f
         var currentY = 0f

core/src/com/unciv/ui/pickerscreens/ModManagementScreen.kt

@@ -10,6 +10,7 @@ import com.badlogic.gdx.utils.Align
 import com.badlogic.gdx.utils.Json
 import com.unciv.JsonParser
 import com.unciv.MainMenuScreen
+import com.unciv.logic.BackwardCompatibility.updateDeprecations
 import com.unciv.models.ruleset.ModOptions
 import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.ruleset.RulesetCache
@@ -422,6 +423,7 @@ class ModManagementScreen(
         modOptions.lastUpdated = repo.pushed_at
         modOptions.author = repo.owner.login
         modOptions.modSize = repo.size
+        modOptions.updateDeprecations()
         Json().toJson(modOptions, modOptionsFile)
     }
 

core/src/com/unciv/ui/worldscreen/AlertPopup.kt

@@ -306,7 +306,7 @@ class AlertPopup(val worldScreen: WorldScreen, val popupAlert: PopupAlert): Popu
         val responseTable = Table()
         responseTable.defaults()
             .pad(0f, 30f) // Small buttons, plenty of pad so we don't fat-finger it
-        responseTable.add(getCloseButton("Yes", 'y') {
+        responseTable.add(getCloseButton(Constants.yes, 'y') {
             // Return it to original owner
             val unitName = capturedUnit.baseUnit.name
             capturedUnit.destroy()
@@ -325,7 +325,7 @@ class AlertPopup(val worldScreen: WorldScreen, val popupAlert: PopupAlert): Popu
                     .setModifier(DiplomaticModifiers.ReturnedCapturedUnits, 20f)
             }
         })
-        responseTable.add(getCloseButton("No", 'n') {
+        responseTable.add(getCloseButton(Constants.no, 'n') {
             // Take it for ourselves
             // Settlers become workers at this point
             if (capturedUnit.hasUnique(UniqueType.FoundCity)) {