Linting and a little hardening of the UI-Skins code (#9007)

core/src/com/unciv/models/skins/SkinCache.kt

@@ -22,38 +22,40 @@ object SkinCache : HashMap<String, SkinConfig>() {
     fun assembleSkinConfigs(ruleSetMods: Set<String>) {
         // Needs to be a list and not a set, so subsequent mods override the previous ones
         // Otherwise you rely on hash randomness to determine override order... not good
-        val mods = mutableListOf("")
+        val mods = mutableListOf("")  // Not an emptyList - placeholder for built-in skin
         if (UncivGame.isCurrentInitialized()) {
             mods.addAll(UncivGame.Current.settings.visualMods)
         }
         mods.addAll(ruleSetMods)
         clear()
         for (mod in mods.distinct()) {
-            for (entry in allConfigs.entries.filter { it.key.mod == mod } ) { // Built-in skins all have empty strings as their `.mod`, so loop through all of them.
+            for ((key, config) in allConfigs.filter { it.key.mod == mod } ) { // Built-in skins all have empty strings as their `.mod`, so loop through all of them.
-                val skin = entry.key.skin
+                val skin = key.skin
-                if (skin in this) this[skin]!!.updateConfig(entry.value)
+                if (skin in this) this[skin]!!.updateConfig(config)
-                else this[skin] = entry.value.clone()
+                else this[skin] = config.clone()
             }
         }
     }
 
-    fun loadSkinConfigs(consoleMode: Boolean = false){
+    fun loadSkinConfigs(consoleMode: Boolean = false) {
         allConfigs.clear()
         var skinName = ""
 
         //load internal Skins
         val fileHandles: Sequence<FileHandle> =
                 if (consoleMode) FileHandle("jsons/Skins").list().asSequence()
-                else ImageGetter.getAvailableSkins().map { Gdx.files.internal("jsons/Skins/$it.json")}.filter { it.exists() }
+                else ImageGetter.getAvailableSkins()
+                    .map { Gdx.files.internal("jsons/Skins/$it.json") }
+                    .filter { it.exists() }
 
-        for (configFile in fileHandles){
+        for (configFile in fileHandles) {
             skinName = configFile.nameWithoutExtension().removeSuffix("Config")
             try {
                 val key = SkinAndMod(skinName, "")
                 assert(key !in allConfigs)
                 allConfigs[key] = json().fromJsonFile(SkinConfig::class.java, configFile)
                 debug("SkinConfig loaded successfully: %s", configFile.name())
-            } catch (ex: Exception){
+            } catch (ex: Exception) {
                 debug("Exception loading SkinConfig '%s':", configFile.path())
                 debug("  %s", ex.localizedMessage)
                 debug("  (Source file %s line %s)", ex.stackTrace[0].fileName, ex.stackTrace[0].lineNumber)
@@ -70,21 +72,21 @@ object SkinCache : HashMap<String, SkinConfig>() {
             if (modName.startsWith('.')) continue
             if (!modFolder.isDirectory) continue
 
-            try {
+            for (configFile in modFolder.child("jsons/Skins").list()) {
-                for (configFile in modFolder.child("jsons/Skins").list()){
+                try {
                     skinName = configFile.nameWithoutExtension().removeSuffix("Config")
                     val key = SkinAndMod(skinName, modName)
                     assert(key !in allConfigs)
                     allConfigs[key] = json().fromJsonFile(SkinConfig::class.java, configFile)
                     debug("Skin loaded successfully: %s", configFile.path())
+                } catch (ex: Exception) {
+                    debug("Exception loading Skin '%s/jsons/Skins/%s':", modFolder.name(), skinName)
+                    debug("  %s", ex.localizedMessage)
+                    debug("  (Source file %s line %s)", ex.stackTrace[0].fileName, ex.stackTrace[0].lineNumber)
                 }
-            } catch (ex: Exception){
-                debug("Exception loading Skin '%s/jsons/Skins/%s':", modFolder.name(), skinName)
-                debug("  %s", ex.localizedMessage)
-                debug("  (Source file %s line %s)", ex.stackTrace[0].fileName, ex.stackTrace[0].lineNumber)
             }
         }
 
-        assembleSkinConfigs(hashSetOf()) // no game is loaded, this is just the initial game setup
+        assembleSkinConfigs(emptySet()) // no game is loaded, this is just the initial game setup
     }
 }

core/src/com/unciv/models/skins/SkinConfig.kt

@@ -2,38 +2,31 @@ package com.unciv.models.skins
 
 import com.badlogic.gdx.graphics.Color
 
-class SkinElement {
+class SkinConfig(initialCapacity: Int) {
-    var image: String? = null
-    var tint: Color? = null
-    var alpha: Float? = null
-
-    fun clone(): SkinElement {
-        val toReturn = SkinElement()
-        toReturn.image = image
-        toReturn.tint = tint?.cpy()
-        toReturn.alpha = alpha
-        return toReturn
-    }
-}
-
-class SkinConfig {
     var baseColor: Color = Color(0x004085bf)
     var clearColor: Color = Color(0x000033ff)
-    var skinVariants: HashMap<String, SkinElement> = HashMap()
+    var skinVariants: HashMap<String, SkinElement> = HashMap(initialCapacity)
 
-    fun clone(): SkinConfig {
+    constructor() : this(16)  // = HashMap.DEFAULT_INITIAL_CAPACITY which is private
-        val toReturn = SkinConfig()
+
-        toReturn.baseColor = baseColor.cpy()
+    /** Skin element, read from UI SKin json
-        toReturn.clearColor = clearColor.cpy()
+     *
-        toReturn.skinVariants.putAll(skinVariants.map { Pair(it.key, it.value.clone()) })
+     *  **Immutable** */
-        return toReturn
+    class SkinElement {
+        val image: String? = null
+        val tint: Color? = null
+        val alpha: Float? = null
     }
 
+    fun clone() = SkinConfig(skinVariants.size).also { it.updateConfig(this) }
+
+    /** 'Merges' [other] into **`this`**
+     *
+     *  [baseColor] and [clearColor] are overwritten with clones from [other].
+     *  [skinVariants] with the same key are copied and overwritten, new [skinVariants] are added. */
     fun updateConfig(other: SkinConfig) {
         baseColor = other.baseColor.cpy()
         clearColor = other.clearColor.cpy()
-        for ((variantName, element) in other.skinVariants){
+        skinVariants.putAll(other.skinVariants)
-            skinVariants[variantName] = element.clone()
-        }
     }
 }

core/src/com/unciv/models/skins/SkinStrings.kt

@@ -40,18 +40,27 @@ class SkinStrings(skin: String = UncivGame.Current.settings.skin) {
      * @param default   The path to the background which should be used if path is not available.
      *                  Should be one of the predefined ones inside SkinStrings or null to get a
      *                  solid background.
+     *
+     * @param tintColor Default tint color if the UI Skin doesn't specify one. If both not specified,
+     *                  the returned background will not be tinted. If the UI Skin specifies a
+     *                  separate alpha value, it will be applied to a clone of either color.
      */
     fun getUiBackground(path: String, default: String? = null, tintColor: Color? = null): NinePatchDrawable {
         val locationByName = skinLocation + path
-        val locationByConfigVariant = skinLocation + skinConfig.skinVariants[path]?.image
+        val skinVariant = skinConfig.skinVariants[path]
-        val tint = (skinConfig.skinVariants[path]?.tint ?: tintColor)?.apply {
+        val locationByConfigVariant = if (skinVariant?.image != null) skinLocation + skinVariant.image else null
-            a = skinConfig.skinVariants[path]?.alpha ?: a
+        val tint = (skinVariant?.tint ?: tintColor)?.run {
+            if (skinVariant?.alpha == null) this
+            else cpy().apply { a = skinVariant.alpha }
         }
-
+        val location = when {
-        return when {
+            locationByConfigVariant != null && ImageGetter.ninePatchImageExists(locationByConfigVariant) ->
-            ImageGetter.ninePatchImageExists(locationByConfigVariant) -> ImageGetter.getNinePatch(locationByConfigVariant, tint)
+                locationByConfigVariant
-            ImageGetter.ninePatchImageExists(locationByName) -> ImageGetter.getNinePatch(locationByName, tint)
+            ImageGetter.ninePatchImageExists(locationByName) ->
-            else -> ImageGetter.getNinePatch(default, tint)
+                locationByName
+            else ->
+                default
         }
+        return ImageGetter.getNinePatch(location, tint)
     }
 }