Minor Mod manager fix, lints and dox (#11013)

* ModOptions reorder into regions, and bring back "internal" fields into the wiki

* Fix two cases of Mod Manager displaying out-of-sync states

* ModConstants and ModConstants.UnitUpgradeCost get equality contracts and a reflection-based shorter merge

* GithubAPI.kt more documentation

core/src/com/unciv/logic/github/GithubAPI.kt

@@ -6,6 +6,13 @@ import com.unciv.json.json
  *  "Namespace" collects all Github API structural knowledge
  *  - Response schema
  *  - Query URL builders
+ *
+ *  ### Collected doc links:
+ *  - https://docs.github.com/en/repositories/working-with-files/using-files/downloading-source-code-archives#source-code-archive-urls
+ *  - https://docs.github.com/en/rest/reference/search#search-repositories--code-samples
+ *  - https://docs.github.com/en/rest/repos/repos
+ *  - https://docs.github.com/en/rest/releases/releases
+ *  - https://docs.github.com/en/rest/git/trees#get-a-tree
  */
 @Suppress("PropertyName")  // We're declaring an external API schema
 object GithubAPI {
@@ -29,6 +36,7 @@ object GithubAPI {
     private fun Repo.getUrlForReleaseZip() = "$html_url/archive/refs/tags/$release_tag.zip"
 
     /** Format a URL to query a repo tree - to calculate actual size */
+    // It's hard to see in the doc this not only accepts a commit SHA, but either branch (used here) or tag names too
     internal fun Repo.getUrlForTreeQuery() =
         "https://api.github.com/repos/$full_name/git/trees/$default_branch?recursive=true"
 

core/src/com/unciv/models/ModConstants.kt

@@ -1,5 +1,16 @@
 package com.unciv.models
 
+/** Used as a member of [ModOptions][com.unciv.models.ruleset.ModOptions] for moddable "constants" - factors in formulae and such.
+ *
+ *  When combining mods, this is [merge]d _per constant/field_, not as entire object like other RulesetObjects.
+ *  Merging happens on a very simple basis: If a Mod comes with a non-default value, it is copied, otherwise the parent value is left intact.
+ *  If several mods change the same field, the last one wins.
+ *
+ *  Supports equality contract to enable the Json serializer to recognize unchanged defaults.
+ *
+ *  Methods [merge], [equals] and [hashCode] are done through reflection so adding a field will not need to update these methods
+ *  (overhead is not a factor, these routines run very rarely).
+ */
 class ModConstants {
     // Max amount of experience that can be gained from combat with barbarians
     @Suppress("SpellCheckingInspection") // Pfrom is not a word ;)
@@ -35,13 +46,14 @@ class ModConstants {
     var minimalCityDistanceOnDifferentContinents = 2
 
     // Constants used to calculate Unit Upgrade gold Cost (can only be modded all-or-nothing)
-    class UnitUpgradeCost {
+    // This is a data class for one reason only: The equality implementation enables Gdx Json to omit it when default (otherwise only the individual fields are omitted)
-        val base = 10f
+    data class UnitUpgradeCost(
-        val perProduction = 2f
+        val base: Float = 10f,
-        val eraMultiplier = 0f  // 0.3 in Civ5 cpp sources but 0 in xml
+        val perProduction: Float = 2f,
-        val exponent = 1f
+        val eraMultiplier: Float = 0f,  // 0.3 in Civ5 cpp sources but 0 in xml
-        val roundTo = 5
+        val exponent: Float = 1f,
-    }
+        val roundTo: Int = 5
+    )
     var unitUpgradeCost = UnitUpgradeCost()
 
     // NaturalWonderGenerator uses these to determine the number of Natural Wonders to spawn for a given map size.
@@ -71,32 +83,37 @@ class ModConstants {
     var pantheonGrowth = 5
 
     fun merge(other: ModConstants) {
-        if (other.maxXPfromBarbarians != defaults.maxXPfromBarbarians) maxXPfromBarbarians = other.maxXPfromBarbarians
+        for (field in this::class.java.declaredFields) {
-        if (other.cityStrengthBase != defaults.cityStrengthBase) cityStrengthBase = other.cityStrengthBase
+            val value = field.get(other)
-        if (other.cityStrengthPerPop != defaults.cityStrengthPerPop) cityStrengthPerPop = other.cityStrengthPerPop
+            if (field.get(defaults).equals(value)) continue
-        if (other.cityStrengthFromTechsMultiplier != defaults.cityStrengthFromTechsMultiplier) cityStrengthFromTechsMultiplier = other.cityStrengthFromTechsMultiplier
+            field.set(this, value)
-        if (other.cityStrengthFromTechsExponent != defaults.cityStrengthFromTechsExponent) cityStrengthFromTechsExponent = other.cityStrengthFromTechsExponent
+        }
-        if (other.cityStrengthFromTechsFullMultiplier != defaults.cityStrengthFromTechsFullMultiplier) cityStrengthFromTechsFullMultiplier = other.cityStrengthFromTechsFullMultiplier
+    }
-        if (other.cityStrengthFromGarrison != defaults.cityStrengthFromGarrison) cityStrengthFromGarrison = other.cityStrengthFromGarrison
+
-        if (other.unitSupplyPerPopulation != defaults.unitSupplyPerPopulation) unitSupplyPerPopulation = other.unitSupplyPerPopulation
+    override fun equals(other: Any?): Boolean {
-        if (other.minimalCityDistance != defaults.minimalCityDistance) minimalCityDistance = other.minimalCityDistance
+        if (this === other) return true
-        if (other.minimalCityDistanceOnDifferentContinents != defaults.minimalCityDistanceOnDifferentContinents) minimalCityDistanceOnDifferentContinents = other.minimalCityDistanceOnDifferentContinents
+        if (other !is ModConstants) return false
-        if (other.unitUpgradeCost != defaults.unitUpgradeCost) unitUpgradeCost = other.unitUpgradeCost
+        return equalsReflected(other)
-        if (other.naturalWonderCountMultiplier != defaults.naturalWonderCountMultiplier) naturalWonderCountMultiplier = other.naturalWonderCountMultiplier
+    }
-        if (other.naturalWonderCountAddedConstant != defaults.naturalWonderCountAddedConstant) naturalWonderCountAddedConstant = other.naturalWonderCountAddedConstant
+
-        if (other.ancientRuinCountMultiplier != defaults.ancientRuinCountMultiplier) ancientRuinCountMultiplier = other.ancientRuinCountMultiplier
+    override fun hashCode(): Int {
-        if (other.spawnIceBelowTemperature != defaults.spawnIceBelowTemperature) spawnIceBelowTemperature = other.spawnIceBelowTemperature
+        var result = 0
-        if (other.maxLakeSize != defaults.maxLakeSize) maxLakeSize = other.maxLakeSize
+        for (field in this::class.java.declaredFields) {
-        if (other.riverCountMultiplier != defaults.riverCountMultiplier) riverCountMultiplier = other.riverCountMultiplier
+            result = result * 31 + field.get(this).hashCode()
-        if (other.minRiverLength != defaults.minRiverLength) minRiverLength = other.minRiverLength
+        }
-        if (other.maxRiverLength != defaults.maxRiverLength) maxRiverLength = other.maxRiverLength
+        return result
-        if (other.religionLimitBase != defaults.religionLimitBase) religionLimitBase = other.religionLimitBase
+    }
-        if (other.religionLimitMultiplier != defaults.religionLimitMultiplier) religionLimitMultiplier = other.religionLimitMultiplier
+
-        if (other.pantheonBase != defaults.pantheonBase) pantheonBase = other.pantheonBase
+    private fun equalsReflected(other: ModConstants): Boolean {
-        if (other.pantheonGrowth != defaults.pantheonGrowth) pantheonGrowth = other.pantheonGrowth
+        for (field in this::class.java.declaredFields) {
+            if (!field.get(this).equals(field.get(other))) return false
+        }
+        return true
     }
 
     companion object {
+        /** As merge will need a default instance repeatedly, store it as static */
+        // Note Json will not use this but get a fresh instance every time. A fix, if possible, could get messy.
         val defaults = ModConstants()
     }
 }

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

@@ -17,22 +17,26 @@ object ModOptionsConstants {
 }
 
 class ModOptions : IHasUniques {
-    override var name = "ModOptions"
+    //region Modder choices
-
     var isBaseRuleset = false
     var techsToRemove = HashSet<String>()
     var buildingsToRemove = HashSet<String>()
     var unitsToRemove = HashSet<String>()
     var nationsToRemove = HashSet<String>()
+    val constants = ModConstants()
+    //endregion
 
-
+    //region Metadata, automatic
-    var lastUpdated = ""
     var modUrl = ""
     var defaultBranch = "master"
     var author = ""
+    var lastUpdated = ""
     var modSize = 0
     var topics = mutableListOf<String>()
+    //endregion
 
+    //region IHasUniques
+    override var name = "ModOptions"
     override var uniques = ArrayList<String>()
 
     @delegate:Transient
@@ -41,6 +45,5 @@ class ModOptions : IHasUniques {
     override val uniqueMap: UniqueMap by lazy(::uniqueMapProvider)
 
     override fun getUniqueTarget() = UniqueTarget.ModOptions
-
+    //endregion
-    val constants = ModConstants()
 }

core/src/com/unciv/ui/screens/modmanager/ModManagementScreen.kt

@@ -536,7 +536,13 @@ class ModManagementScreen private constructor(
         val newModUIData = ModUIData(ruleset, isVisual)
         installedModInfo[modName] = newModUIData
         // The ModUIData in the actual button is now out of sync, but can be indexed using the new instance
-        modButtons[newModUIData]?.updateUIData(newModUIData)
+        modButtons[newModUIData]?.run {
+            updateUIData(newModUIData)
+            // The listeners have also captured a now outdated ModUIData
+            setModButtonOnClick(this, newModUIData)
+            // Simulate click to update the ModInfoAndActionPane
+            installedButtonAction(newModUIData, this)
+        }
     }
 
     /** Remove the visual indicators for an 'updated' mod after re-downloading it.
@@ -605,10 +611,13 @@ class ModManagementScreen private constructor(
 
     private fun getCachedModButton(mod: ModUIData) = modButtons.getOrPut(mod) {
         val newButton = ModDecoratedButton(mod)
-        if (mod.isInstalled) newButton.onClick { installedButtonAction(mod, newButton) }
+        setModButtonOnClick(newButton, mod)
-        else newButton.onClick { onlineButtonAction(mod.repo!!, newButton) }
         newButton
     }
+    private fun setModButtonOnClick(button: ModDecoratedButton, mod: ModUIData) {
+        if (mod.isInstalled) button.onClick { installedButtonAction(mod, button) }
+        else button.onClick { onlineButtonAction(mod.repo!!, button) }
+    }
 
     /** Rebuild the left-hand column containing all installed mods */
     internal fun refreshInstalledModTable() {
@@ -655,6 +664,7 @@ class ModManagementScreen private constructor(
         mod.folderLocation!!.deleteDirectory()
         reloadCachesAfterModChange()
         installedModInfo.remove(mod.name)
+        unMarkUpdatedMod(mod.name)
         refreshInstalledModTable()
     }
 
@@ -690,6 +700,7 @@ class ModManagementScreen private constructor(
         scrollOnlineMods.actor = onlineModsTable
     }
 
+    /** Updates the description label at the bottom of the screen */
     private fun showModDescription(modName: String) {
         val onlineModDescription = onlineModInfo[modName]?.description ?: "" // shows github info
         val installedModDescription = installedModInfo[modName]?.description ?: "" // shows ruleset info

docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md

@@ -127,7 +127,7 @@ This file is a little different:
 Note that this file controls _declarative mod compatibility_ (Work in progress) - e.g. there's [uniques](../uniques.md#modoptions-uniques) to say your Mod should only or never be used as 'Permanent audiovisual mod'.
 Incompatibility filtering works so far between extension and base mods, but feel free to document known extension-to-extension incompatibilities using the same Unique now. Stay tuned!
 
-The file can have the following attributes, not including the values Unciv sets (no point in a mod author setting those):
+The file can have the following attributes, not including the values Unciv sets automatically:
 
 | Attribute | Type | | Notes |
 | --------- | ---- | ------- | ----- |
@@ -139,6 +139,20 @@ The file can have the following attributes, not including the values Unciv sets
 | nationsToRemove | List | empty | List of [Nations](2-Civilization-related-JSON-files.md#nationsjson) or [nationFilter](../Unique-parameters.md#nationfilter) to remove (isBaseRuleset=false only) |
 | constants | Object | empty | See [ModConstants](#modconstants) |
 
+The values normally set automatically from github metadata are:
+
+| Attribute | Type | | Notes |
+| --------- | ---- | ------- | ----- |
+| modUrl | String | | The github page the mod was downloaded from, or empty if a freely hosted zip was used |
+| defaultBranch | String | master | The repo's default branch |
+| author | String | | Repo owner |
+| lastUpdated | String | | ISO date |
+| modSize | Integer | 0 | Size in kB |
+| topics | List | empty | A list of "unciv-mod-*" github topics |
+
+To clarify: When your Mod is distributed via github, including these in the Mod repo has no effect.
+However, when a Mod is distributed _without_ a github repository, these values can and _should_ be set by the author in the distributed `ModOptions.json`.
+
 ### ModConstants
 
 Stored in ModOptions.constants, this is a collection of constants used internally in Unciv.