From f1dd08ccc2f1cad9a00d5eeb78c2161ef2b61702 Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Sun, 28 Jan 2024 10:06:25 +0100 Subject: [PATCH] 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 | 8 ++ core/src/com/unciv/models/ModConstants.kt | 77 +++++++++++-------- .../com/unciv/models/ruleset/ModOptions.kt | 15 ++-- .../screens/modmanager/ModManagementScreen.kt | 17 +++- .../5-Miscellaneous-JSON-files.md | 16 +++- 5 files changed, 93 insertions(+), 40 deletions(-) diff --git a/core/src/com/unciv/logic/github/GithubAPI.kt b/core/src/com/unciv/logic/github/GithubAPI.kt index 32e416a23e..0ed10f7693 100644 --- a/core/src/com/unciv/logic/github/GithubAPI.kt +++ b/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" diff --git a/core/src/com/unciv/models/ModConstants.kt b/core/src/com/unciv/models/ModConstants.kt index d7914556c8..66035a891a 100644 --- a/core/src/com/unciv/models/ModConstants.kt +++ b/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 { - val base = 10f - val perProduction = 2f - val eraMultiplier = 0f // 0.3 in Civ5 cpp sources but 0 in xml - val exponent = 1f - val roundTo = 5 - } + // 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) + data class UnitUpgradeCost( + val base: Float = 10f, + val perProduction: Float = 2f, + val eraMultiplier: Float = 0f, // 0.3 in Civ5 cpp sources but 0 in xml + 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 - if (other.cityStrengthBase != defaults.cityStrengthBase) cityStrengthBase = other.cityStrengthBase - if (other.cityStrengthPerPop != defaults.cityStrengthPerPop) cityStrengthPerPop = other.cityStrengthPerPop - if (other.cityStrengthFromTechsMultiplier != defaults.cityStrengthFromTechsMultiplier) cityStrengthFromTechsMultiplier = other.cityStrengthFromTechsMultiplier - 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 - if (other.minimalCityDistance != defaults.minimalCityDistance) minimalCityDistance = other.minimalCityDistance - if (other.minimalCityDistanceOnDifferentContinents != defaults.minimalCityDistanceOnDifferentContinents) minimalCityDistanceOnDifferentContinents = other.minimalCityDistanceOnDifferentContinents - if (other.unitUpgradeCost != defaults.unitUpgradeCost) unitUpgradeCost = other.unitUpgradeCost - if (other.naturalWonderCountMultiplier != defaults.naturalWonderCountMultiplier) naturalWonderCountMultiplier = other.naturalWonderCountMultiplier - if (other.naturalWonderCountAddedConstant != defaults.naturalWonderCountAddedConstant) naturalWonderCountAddedConstant = other.naturalWonderCountAddedConstant - if (other.ancientRuinCountMultiplier != defaults.ancientRuinCountMultiplier) ancientRuinCountMultiplier = other.ancientRuinCountMultiplier - if (other.spawnIceBelowTemperature != defaults.spawnIceBelowTemperature) spawnIceBelowTemperature = other.spawnIceBelowTemperature - if (other.maxLakeSize != defaults.maxLakeSize) maxLakeSize = other.maxLakeSize - if (other.riverCountMultiplier != defaults.riverCountMultiplier) riverCountMultiplier = other.riverCountMultiplier - if (other.minRiverLength != defaults.minRiverLength) minRiverLength = other.minRiverLength - if (other.maxRiverLength != defaults.maxRiverLength) maxRiverLength = other.maxRiverLength - if (other.religionLimitBase != defaults.religionLimitBase) religionLimitBase = other.religionLimitBase - if (other.religionLimitMultiplier != defaults.religionLimitMultiplier) religionLimitMultiplier = other.religionLimitMultiplier - if (other.pantheonBase != defaults.pantheonBase) pantheonBase = other.pantheonBase - if (other.pantheonGrowth != defaults.pantheonGrowth) pantheonGrowth = other.pantheonGrowth + for (field in this::class.java.declaredFields) { + val value = field.get(other) + if (field.get(defaults).equals(value)) continue + field.set(this, value) + } + } + + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is ModConstants) return false + return equalsReflected(other) + } + + override fun hashCode(): Int { + var result = 0 + for (field in this::class.java.declaredFields) { + result = result * 31 + field.get(this).hashCode() + } + return result + } + + private fun equalsReflected(other: ModConstants): Boolean { + 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() } } diff --git a/core/src/com/unciv/models/ruleset/ModOptions.kt b/core/src/com/unciv/models/ruleset/ModOptions.kt index 7ed994f182..0b3c1a1b9e 100644 --- a/core/src/com/unciv/models/ruleset/ModOptions.kt +++ b/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() var buildingsToRemove = HashSet() var unitsToRemove = HashSet() var nationsToRemove = HashSet() + val constants = ModConstants() + //endregion - - var lastUpdated = "" + //region Metadata, automatic var modUrl = "" var defaultBranch = "master" var author = "" + var lastUpdated = "" var modSize = 0 var topics = mutableListOf() + //endregion + //region IHasUniques + override var name = "ModOptions" override var uniques = ArrayList() @delegate:Transient @@ -41,6 +45,5 @@ class ModOptions : IHasUniques { override val uniqueMap: UniqueMap by lazy(::uniqueMapProvider) override fun getUniqueTarget() = UniqueTarget.ModOptions - - val constants = ModConstants() + //endregion } diff --git a/core/src/com/unciv/ui/screens/modmanager/ModManagementScreen.kt b/core/src/com/unciv/ui/screens/modmanager/ModManagementScreen.kt index 9e5ee05f65..e917574648 100644 --- a/core/src/com/unciv/ui/screens/modmanager/ModManagementScreen.kt +++ b/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) } - else newButton.onClick { onlineButtonAction(mod.repo!!, newButton) } + setModButtonOnClick(newButton, mod) 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 diff --git a/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md b/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md index bbb7b68604..a839cd60ae 100644 --- a/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md +++ b/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.