Spy UI improvements (#11570)

* Minor linting

* More linting

* Consistent naming city/location, cache city on Spy (tile visibility perf)

* Empower SpyAction enum (minor perf)

* NotificationAction for Espionage, shorthand on the Spy instance

* Fix National Intelligence Agency giving one extra Spy level

* Fix "Move Spies" NextTurnButton prompt appearing every turn when spies are in Surveillance

* Fix failed tech theft rewarding tech anyway (and open spy kill chance when theft ordered but nothing to steal)

* Finally refactor SmallButtonStyle as standalone component

* Fix unable to assign spies for counter-intelligence

* Shorten establish-network phase for domestic spy placement

* Refactor and prettify MoveToCityButton, reuse as pointer who is to move (and some tiny changes)

* Decorate Spy icons by rank and show those in the hideout too

* Make city name labels in Espionage screen clickable

* Umm... duplicate targets behave same as single targets

* Spy mechanics - no establish network before counter-intelligence, commenting

* Oops, those lines do not belong in this branch anyway

android/assets/jsons/Civ V - Gods & Kings/Buildings.json

@@ -986,8 +986,11 @@
         "cost": 120,
         "culture": 1,
         "isNationalWonder": true,
-        "uniques": ["Hidden when espionage is disabled", "Gain an extra spy", "Promotes all spies",
+        "uniques": ["Hidden when espionage is disabled",
-			"[-15]% enemy spy effectiveness [in this city]", "New spies start with [1] level(s)",
+            "New spies start with [1] level(s)",
+            "Promotes all spies",
+            "Gain an extra spy", // Order is significant here
+			"[-15]% enemy spy effectiveness [in this city]",
 			"Only available <if [Police Station] is constructed in all [non-[Puppeted]] cities>",
 			"Cost increases by [30] per owned city"],
         "requiredTech": "Radio"

core/src/com/unciv/logic/automation/unit/EspionageAutomation.kt

@@ -1,6 +1,5 @@
 package com.unciv.logic.automation.unit
 
-import com.unciv.logic.city.City
 import com.unciv.logic.civilization.Civilization
 import com.unciv.models.Spy
 import com.unciv.models.SpyAction
@@ -15,7 +14,7 @@ class EspionageAutomation(val civInfo: Civilization) {
 
     private val getCivsToStealFromSorted: List<Civilization> =
         civsToStealFrom.sortedBy { otherCiv -> civInfo.espionageManager.spyList
-            .count { it.isDoingWork() && it.getLocation()?.civ == otherCiv }
+            .count { it.isDoingWork() && it.getCityOrNull()?.civ == otherCiv }
         }.toList()
 
     private val cityStatesToRig: List<Civilization> by lazy {

core/src/com/unciv/logic/city/managers/CityEspionageManager.kt

@@ -26,7 +26,7 @@ class CityEspionageManager : IsPartOfGameInfoSerialization {
     }
 
     fun hasSpyOf(civInfo: Civilization): Boolean {
-        return civInfo.espionageManager.spyList.any { it.getLocation() == city }
+        return civInfo.espionageManager.spyList.any { it.getCityOrNull() == city }
     }
 
     private fun getAllStationedSpies(): List<Spy> {
@@ -35,13 +35,12 @@ class CityEspionageManager : IsPartOfGameInfoSerialization {
 
     fun removeAllPresentSpies(reason: SpyFleeReason) {
         for (spy in getAllStationedSpies()) {
-            val owningCiv = spy.civInfo
             val notificationString = when (reason) {
                 SpyFleeReason.CityDestroyed -> "After the city of [${city.name}] was destroyed, your spy [${spy.name}] has fled back to our hideout."
                 SpyFleeReason.CityCaptured -> "After the city of [${city.name}] was conquered, your spy [${spy.name}] has fled back to our hideout."
                 else -> "Due to the chaos ensuing in [${city.name}], your spy [${spy.name}] has fled back to our hideout."
             }
-            owningCiv.addNotification(notificationString, city.location, NotificationCategory.Espionage, NotificationIcon.Spy)
+            spy.addNotification(notificationString)
             spy.moveTo(null)
         }
     }

core/src/com/unciv/logic/civilization/NotificationActions.kt

@@ -12,6 +12,7 @@ import com.unciv.ui.screens.civilopediascreen.CivilopediaScreen
 import com.unciv.ui.screens.diplomacyscreen.DiplomacyScreen
 import com.unciv.ui.screens.overviewscreen.EmpireOverviewCategories
 import com.unciv.ui.screens.overviewscreen.EmpireOverviewScreen
+import com.unciv.ui.screens.overviewscreen.EspionageOverviewScreen
 import com.unciv.ui.screens.pickerscreens.PolicyPickerScreen
 import com.unciv.ui.screens.pickerscreens.PromotionPickerScreen
 import com.unciv.ui.screens.pickerscreens.TechPickerScreen
@@ -165,6 +166,18 @@ class PolicyAction(
     }
 }
 
+/** Open [EspionageOverviewScreen] */
+class EspionageAction : NotificationAction {
+    override fun execute(worldScreen: WorldScreen) {
+        worldScreen.game.pushScreen(EspionageOverviewScreen(worldScreen.selectedCiv, worldScreen))
+    }
+    companion object {
+        fun withLocation(location: Vector2?): Sequence<NotificationAction> =
+            LocationAction(location) + EspionageAction()
+    }
+}
+
+
 @Suppress("PrivatePropertyName")  // These names *must* match their class name, see below
 internal class NotificationActionsDeserializer {
     /* This exists as trick to leverage readFields for Json deserialization.
@@ -187,12 +200,14 @@ internal class NotificationActionsDeserializer {
     private val PromoteUnitAction: PromoteUnitAction? = null
     private val OverviewAction: OverviewAction? = null
     private val PolicyAction: PolicyAction? = null
+    private val EspionageAction: EspionageAction? = null
 
     fun read(json: Json, jsonData: JsonValue): List<NotificationAction> {
         json.readFields(this, jsonData)
         return listOfNotNull(
             LocationAction, TechAction, CityAction, DiplomacyAction, MayaLongCountAction,
-            MapUnitAction, CivilopediaAction, PromoteUnitAction, OverviewAction, PolicyAction
+            MapUnitAction, CivilopediaAction, PromoteUnitAction, OverviewAction, PolicyAction,
+            EspionageAction
         )
     }
 }

core/src/com/unciv/logic/civilization/managers/EspionageManager.kt

@@ -45,8 +45,7 @@ class EspionageManager : IsPartOfGameInfoSerialization {
     fun getSpyName(): String {
         val usedSpyNames = spyList.map { it.name }.toHashSet()
         val validSpyNames = civInfo.nation.spyNames.filter { it !in usedSpyNames }
-        if (validSpyNames.isEmpty()) { return "Spy ${spyList.size+1}" } // +1 as non-programmers count from 1
+        return validSpyNames.randomOrNull() ?: "Spy ${spyList.size+1}" // +1 as non-programmers count from 1
-        return validSpyNames.random()
     }
 
     fun addSpy(): Spy {
@@ -60,7 +59,7 @@ class EspionageManager : IsPartOfGameInfoSerialization {
     fun getTilesVisibleViaSpies(): Sequence<Tile> {
         return spyList.asSequence()
             .filter { it.isSetUp() }
-            .mapNotNull { it.getLocation() }
+            .mapNotNull { it.getCityOrNull() }
             .flatMap { it.getCenterTile().getTilesInDistance(1) }
     }
 
@@ -74,23 +73,31 @@ class EspionageManager : IsPartOfGameInfoSerialization {
         return techsToSteal
     }
 
-    fun getSpiesInCity(city: City): MutableList<Spy> {
+    fun getSpiesInCity(city: City): List<Spy> {
-        return spyList.filter { it.getLocation() == city }.toMutableList()
+        return spyList.filterTo(mutableListOf()) { it.getCityOrNull() == city }
     }
 
     fun getStartingSpyRank(): Int = 1 + civInfo.getMatchingUniques(UniqueType.SpyStartingLevel).sumOf { it.params[0].toInt() }
 
     /**
      * Returns a list of all cities with our spies in them.
-     * The list needs to be stable accross calls on the same turn.
+     * The list needs to be stable across calls on the same turn.
      */
-    fun getCitiesWithOurSpies(): List<City> = spyList.filter { it.isSetUp() }.mapNotNull { it.getLocation() }
+    fun getCitiesWithOurSpies(): List<City> = spyList.filter { it.isSetUp() }.mapNotNull { it.getCityOrNull() }
 
-    fun getSpyAssignedToCity(city: City): Spy? = spyList.firstOrNull {it.getLocation() == city}
+    fun getSpyAssignedToCity(city: City): Spy? = spyList.firstOrNull { it.getCityOrNull() == city }
 
     /**
      * Determines whether the NextTurnAction MoveSpies should be shown or not
      * @return true if there are spies waiting to be moved
      */
-    fun shouldShowMoveSpies(): Boolean = !dismissedShouldMoveSpies && spyList.any { it.isIdle() }
+    fun shouldShowMoveSpies(): Boolean = !dismissedShouldMoveSpies && hasIdleSpies()
+
+    /** Are any spies in the hideout?
+     *  @see shouldShowMoveSpies */
+    fun hasIdleSpies() = spyList.any { it.isIdle() }
+
+    fun getIdleSpies(): List<Spy> {
+        return spyList.filterTo(mutableListOf()) { it.isIdle() }
+    }
 }

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

@@ -5,6 +5,7 @@ import com.unciv.Constants
 import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.city.City
 import com.unciv.logic.civilization.Civilization
+import com.unciv.logic.civilization.EspionageAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.diplomacy.DiplomaticModifiers
@@ -15,35 +16,50 @@ import com.unciv.models.ruleset.unique.UniqueType
 import kotlin.random.Random
 
 
-enum class SpyAction(val displayString: String) {
+enum class SpyAction(val displayString: String, val hasTurns: Boolean, internal val isSetUp: Boolean, private val isDoingWork: Boolean = false) {
-    None("None"),
+    None("None", false, false),
-    Moving("Moving"),
+    Moving("Moving", true, false, true),
-    EstablishNetwork("Establishing Network"),
+    EstablishNetwork("Establishing Network", true, false, true),
-    Surveillance("Observing City"),
+    Surveillance("Observing City", false, true),
-    StealingTech("Stealing Tech"),
+    StealingTech("Stealing Tech", false, true, true),
-    RiggingElections("Rigging Elections"),
+    RiggingElections("Rigging Elections", true, true) {
-    CounterIntelligence("Conducting Counter-intelligence"),
+        override fun isDoingWork(spy: Spy) = !spy.civInfo.isAtWarWith(spy.getCity().civ)
-    Dead("Dead")
+    },
+    CounterIntelligence("Conducting Counter-intelligence", false, true) {
+        override fun isDoingWork(spy: Spy) = spy.turnsRemainingForAction > 0
+    },
+    Dead("Dead", true, false),
+    ;
+    internal open fun isDoingWork(spy: Spy) = isDoingWork
 }
 
 
-class Spy() : IsPartOfGameInfoSerialization {
+class Spy private constructor() : IsPartOfGameInfoSerialization {
-    // `location == null` means that the spy is in its hideout
-    private var location: Vector2? = null
     lateinit var name: String
-    var action = SpyAction.None
         private set
     var rank: Int = 1
+        private set
+
+    // `location == null` means that the spy is in its hideout
+    private var location: Vector2? = null
+
+    var action = SpyAction.None
+        private set
+
     var turnsRemainingForAction = 0
         private set
     private var progressTowardsStealingTech = 0
 
     @Transient
     lateinit var civInfo: Civilization
+        private set
 
     @Transient
     private lateinit var espionageManager: EspionageManager
 
+    @Transient
+    private var city: City? = null
+
     constructor(name: String, rank:Int) : this() {
         this.name = name
         this.rank = rank
@@ -63,49 +79,49 @@ class Spy() : IsPartOfGameInfoSerialization {
         this.espionageManager = civInfo.espionageManager
     }
 
+    private fun setAction(newAction: SpyAction, turns: Int = 0) {
+        assert(!newAction.hasTurns || turns > 0) // hasTurns==false but turns > 0 is allowed (CounterIntelligence), hasTurns==true and turns==0 is not.
+        action = newAction
+        turnsRemainingForAction = turns
+    }
+
     fun endTurn() {
+        if (action.hasTurns && --turnsRemainingForAction > 0) return
         when (action) {
             SpyAction.None -> return
             SpyAction.Moving -> {
-                --turnsRemainingForAction
+                if (getCity().civ == civInfo)
-                if (turnsRemainingForAction > 0) return
+                    // Your own cities are certainly familiar surroundings, so skip establishing a network
-
+                    setAction(SpyAction.CounterIntelligence, 10)
-                action = SpyAction.EstablishNetwork
+                else
-                turnsRemainingForAction = 3 // Depending on cultural familiarity level if that is ever implemented
+                    // Should depend on cultural familiarity level if that is ever implemented inter-civ
+                    setAction(SpyAction.EstablishNetwork, 3)
             }
             SpyAction.EstablishNetwork -> {
-                --turnsRemainingForAction
+                val city = getCity() // This should never throw an exception, as going to the hideout sets your action to None.
-                if (turnsRemainingForAction > 0) return
+                if (city.civ.isCityState())
-
+                    setAction(SpyAction.RiggingElections, 10)
-                val location = getLocation()!! // This should never throw an exception, as going to the hideout sets your action to None.
+                else if (city.civ == civInfo)
-                if (location.civ.isCityState()) {
+                    setAction(SpyAction.CounterIntelligence, 10)
-                    action = SpyAction.RiggingElections
+                else
-                    turnsRemainingForAction = 10
-                } else if (location.civ == civInfo) {
-                    action = SpyAction.CounterIntelligence
-                    turnsRemainingForAction = 10
-                } else {
                     startStealingTech()
             }
-            }
             SpyAction.Surveillance -> {
-                if (!getLocation()!!.civ.isMajorCiv()) return
+                if (!getCity().civ.isMajorCiv()) return
 
-                val stealableTechs = espionageManager.getTechsToSteal(getLocation()!!.civ)
+                val stealableTechs = espionageManager.getTechsToSteal(getCity().civ)
                 if (stealableTechs.isEmpty()) return
-                action = SpyAction.StealingTech // There are new techs to steal!
+                setAction(SpyAction.StealingTech) // There are new techs to steal!
             }
             SpyAction.StealingTech -> {
-                val stealableTechs = espionageManager.getTechsToSteal(getLocation()!!.civ)
+                val stealableTechs = espionageManager.getTechsToSteal(getCity().civ)
                 if (stealableTechs.isEmpty()) {
-                    action = SpyAction.Surveillance
+                    setAction(SpyAction.Surveillance)
-                    turnsRemainingForAction = 0
+                    addNotification("Your spy [$name] cannot steal any more techs from [${getCity().civ}] as we've already researched all the technology they know!")
-                    val notificationString = "Your spy [$name] cannot steal any more techs from [${getLocation()!!.civ}] as we've already researched all the technology they know!"
-                    civInfo.addNotification(notificationString, getLocation()!!.location, NotificationCategory.Espionage, NotificationIcon.Spy)
                     return
                 }
                 val techStealCost = stealableTechs.maxOfOrNull { civInfo.gameInfo.ruleset.technologies[it]!!.cost }!!
-                var progressThisTurn = getLocation()!!.cityStats.currentCityStats.science
+                var progressThisTurn = getCity().cityStats.currentCityStats.science
                 // 33% spy bonus for each level
                 progressThisTurn *= (rank + 2f) / 3f
                 progressThisTurn *= getEfficiencyModifier().toFloat()
@@ -115,80 +131,71 @@ class Spy() : IsPartOfGameInfoSerialization {
                 }
             }
             SpyAction.RiggingElections -> {
-                --turnsRemainingForAction
-                if (turnsRemainingForAction > 0) return
-
                 rigElection()
             }
             SpyAction.Dead -> {
-                --turnsRemainingForAction
-                if (turnsRemainingForAction > 0) return
-
                 val oldSpyName = name
                 name = espionageManager.getSpyName()
-                action = SpyAction.None
+                setAction(SpyAction.None)
                 rank = espionageManager.getStartingSpyRank()
-                civInfo.addNotification("We have recruited a new spy name [$name] after [$oldSpyName] was killed.",
+                addNotification("We have recruited a new spy name [$name] after [$oldSpyName] was killed.")
-                    NotificationCategory.Espionage, NotificationIcon.Spy)
             }
             SpyAction.CounterIntelligence -> {
-                // Counter inteligence spies don't do anything here
+                // Counter intelligence spies don't do anything here
                 // However the AI will want to keep track of how long a spy has been doing counter intelligence for
-                // Once turnRemainingForAction is <= 0 the spy won't be considered to be doing work any more
+                // Once turnsRemainingForAction is <= 0 the spy won't be considered to be doing work any more
                 --turnsRemainingForAction
                 return
             }
-            else -> return // Not implemented yet, so don't do anything
         }
     }
 
-    fun startStealingTech() {
+    private fun startStealingTech() {
-        action = SpyAction.StealingTech
+        setAction(SpyAction.StealingTech)
-        turnsRemainingForAction = 0
         progressTowardsStealingTech = 0
     }
 
     private fun stealTech() {
-        val city = getLocation()!!
+        val city = getCity()
         val otherCiv = city.civ
-        val randomSeed = city.location.x * city.location.y + 123f * civInfo.gameInfo.turns
+        val randomSeed = randomSeed()
+
+        val stolenTech = espionageManager.getTechsToSteal(getCity().civ)
+            .randomOrNull(Random(randomSeed)) // Could be improved to for example steal the most expensive tech or the tech that has the least progress as of yet
 
-        val stolenTech = espionageManager.getTechsToSteal(getLocation()!!.civ)
-            .randomOrNull(Random(randomSeed.toInt())) // Could be improved to for example steal the most expensive tech or the tech that has the least progress as of yet
-        if (stolenTech != null) {
-            civInfo.tech.addTechnology(stolenTech)
-        }
         // Lower is better
-        var spyResult = Random(randomSeed.toInt()).nextInt(300)
+        var spyResult = Random(randomSeed).nextInt(300)
         // Add our spies experience
         spyResult -= getSkillModifier()
-        // Subtract the experience of the counter inteligence spies
+        // Subtract the experience of the counter intelligence spies
         val defendingSpy = city.civ.espionageManager.getSpyAssignedToCity(city)
         spyResult += defendingSpy?.getSkillModifier() ?: 0
 
         val detectionString = when {
-            spyResult < 0 -> null // Not detected
+            spyResult >= 200 -> { // The spy was killed in the attempt (should be able to happen even if there's nothing to steal?)
-            spyResult < 100 -> "An unidentified spy stole the Technology [$stolenTech] from [$city]!"
-            spyResult < 200 -> "A spy from [${civInfo.civName}] stole the Technology [$stolenTech] from [$city]!"
-            else -> { // The spy was killed in the attempt
                 if (defendingSpy == null) "A spy from [${civInfo.civName}] was found and killed trying to steal Technology in [$city]!"
                 else "A spy from [${civInfo.civName}] was found and killed by [${defendingSpy.name}] trying to steal Technology in [$city]!"
             }
+            stolenTech == null -> null // Nothing to steal
+            spyResult < 0 -> null // Not detected
+            spyResult < 100 -> "An unidentified spy stole the Technology [$stolenTech] from [$city]!"
+            else -> "A spy from [${civInfo.civName}] stole the Technology [$stolenTech] from [$city]!"
         }
         if (detectionString != null)
+            // Not using Spy.addNotification, shouldn't open the espionage screen
             otherCiv.addNotification(detectionString, city.location, NotificationCategory.Espionage, NotificationIcon.Spy)
 
-        if (spyResult < 200) {
+        if (spyResult < 200 && stolenTech != null) {
-            civInfo.addNotification("Your spy [$name] stole the Technology [$stolenTech] from [$city]!", city.location,
+            civInfo.tech.addTechnology(stolenTech)
-                NotificationCategory.Espionage, NotificationIcon.Spy)
+            addNotification("Your spy [$name] stole the Technology [$stolenTech] from [$city]!")
-            startStealingTech()
             levelUpSpy()
-        } else {
+        }
-            civInfo.addNotification("Your spy [$name] was killed trying to steal Technology in [$city]!", city.location,
+
-                NotificationCategory.Espionage, NotificationIcon.Spy)
+        if (spyResult >= 200) {
+            addNotification("Your spy [$name] was killed trying to steal Technology in [$city]!")
             defendingSpy?.levelUpSpy()
             killSpy()
-        }
+        } else startStealingTech()  // reset progress
 
         if (spyResult >= 100) {
             otherCiv.getDiplomacyManager(civInfo).addModifier(DiplomaticModifiers.SpiedOnUs, -15f)
@@ -196,25 +203,23 @@ class Spy() : IsPartOfGameInfoSerialization {
     }
 
     private fun rigElection() {
-        val city = getLocation()!!
+        val city = getCity()
         val cityStateCiv = city.civ
         // TODO: Simple implementation, please implement this in the future. This is a guess.
         turnsRemainingForAction = 10
 
         if (cityStateCiv.getAllyCiv() != null && cityStateCiv.getAllyCiv() != civInfo.civName) {
             val allyCiv = civInfo.gameInfo.getCivilization(cityStateCiv.getAllyCiv()!!)
-            val defendingSpy = allyCiv.espionageManager.getSpyAssignedToCity(getLocation()!!)
+            val defendingSpy = allyCiv.espionageManager.getSpyAssignedToCity(city)
             if (defendingSpy != null) {
-                val randomSeed = city.location.x * city.location.y + 123f * civInfo.gameInfo.turns
+                var spyResult = Random(randomSeed()).nextInt(120)
-                var spyResult = Random(randomSeed.toInt()).nextInt(120)
                 spyResult -= getSkillModifier()
                 spyResult += defendingSpy.getSkillModifier()
                 if (spyResult > 100) {
-                    // The Spy was killed
+                    // The Spy was killed (use the notification without EspionageAction)
                     allyCiv.addNotification("A spy from [${civInfo.civName}] tried to rig elections and was found and killed in [${city}] by [${defendingSpy.name}]!",
-                        getLocation()!!.location, NotificationCategory.Espionage, NotificationIcon.Spy)
+                        city.location, NotificationCategory.Espionage, NotificationIcon.Spy)
-                    civInfo.addNotification("Your spy [$name] was killed trying to rig the election in [$city]!", city.location,
+                    addNotification("Your spy [$name] was killed trying to rig the election in [$city]!")
-                        NotificationCategory.Espionage, NotificationIcon.Spy)
                     killSpy()
                     defendingSpy.levelUpSpy()
                     return
@@ -222,7 +227,7 @@ class Spy() : IsPartOfGameInfoSerialization {
             }
         }
         // Starts at 10 influence and increases by 3 for each extra rank.
-        cityStateCiv.getDiplomacyManager(civInfo).addInfluence(7f + getSpyRank() * 3)
+        cityStateCiv.getDiplomacyManager(civInfo).addInfluence(7f + rank * 3)
         civInfo.addNotification("Your spy successfully rigged the election in [$city]!", city.location,
             NotificationCategory.Espionage, NotificationIcon.Spy)
     }
@@ -230,81 +235,82 @@ class Spy() : IsPartOfGameInfoSerialization {
     fun moveTo(city: City?) {
         if (city == null) { // Moving to spy hideout
             location = null
-            action = SpyAction.None
+            this.city = null
-            turnsRemainingForAction = 0
+            setAction(SpyAction.None)
             return
         }
         location = city.location
-        action = SpyAction.Moving
+        this.city = city
-        turnsRemainingForAction = 1
+        setAction(SpyAction.Moving, 1)
     }
 
     fun canMoveTo(city: City): Boolean {
-        if (getLocation() == city) return true
+        if (getCityOrNull() == city) return true
         if (!city.getCenterTile().isExplored(civInfo)) return false
         return espionageManager.getSpyAssignedToCity(city) == null
     }
 
-    fun isSetUp() = action !in listOf(SpyAction.Moving, SpyAction.None, SpyAction.EstablishNetwork)
+    fun isSetUp() = action.isSetUp
 
-    fun isIdle(): Boolean =action == SpyAction.None || action == SpyAction.Surveillance
+    fun isIdle() = action == SpyAction.None
 
-    fun isDoingWork(): Boolean {
+    fun isDoingWork() = action.isDoingWork(this)
-        if (action == SpyAction.StealingTech || action == SpyAction.EstablishNetwork || action == SpyAction.Moving) return true
-        if (action == SpyAction.RiggingElections && !civInfo.isAtWarWith(getLocation()!!.civ)) return true
-        if (action == SpyAction.CounterIntelligence && turnsRemainingForAction > 0) return true
-        else return false
-    }
 
-    fun getLocation(): City? {
+    /** Returns the City this Spy is in, or `null` if it is in the hideout. */
+    fun getCityOrNull(): City? {
         if (location == null) return null
-        return civInfo.gameInfo.tileMap[location!!].getCity()
+        if (city == null) city = civInfo.gameInfo.tileMap[location!!].getCity()
+        return city
     }
 
-    fun getLocationName(): String {
+    /** Non-null version of [getCityOrNull] for the frequent case it is known the spy cannot be in the hideout.
-        return getLocation()?.name ?: Constants.spyHideout
+     *  @throws NullPointerException if the spy is in the hideout */
-    }
+    fun getCity(): City = getCityOrNull()!!
 
-    fun getSpyRank(): Int {
+    fun getLocationName() = getCityOrNull()?.name ?: Constants.spyHideout
-        return rank
-    }
 
     fun levelUpSpy() {
         //TODO: Make the spy level cap dependent on some unique
         if (rank >= 3) return
-        if (getLocation() != null) {
+        addNotification("Your spy [$name] has leveled up!")
-            civInfo.addNotification("Your spy [$name] has leveled up!", getLocation()!!.location,
-                NotificationCategory.Espionage, NotificationIcon.Spy)
-        } else {
-            civInfo.addNotification("Your spy [$name] has leveled up!",
-                NotificationCategory.Espionage, NotificationIcon.Spy)
-        }
         rank++
     }
 
-    fun getSkillModifier(): Int {
+    /** Zero-based modifier expressing shift of probabilities from Spy Rank
-        return getSpyRank() * 30
+     *
-    }
+     *  100 units change one step in results, there are 4 such steps, and the default random spans 300 units and excludes the best result (undetected success).
+     *  Thus the return value translates into (return / 3) percent chance to get the very best result, reducing the chance to get the worst result (kill) by the same amount.
+     *  The same modifier from defending counter-intelligence spies goes linearly in the opposite direction.
+     *  With the range of this function being hardcoded to 30..90 (and 0 for no defensive spy present), ranks cannot guarantee either best or worst outcome.
+     *  Or - chance range of best result is 0% (rank 1 vs rank 3 defender) to 30% (rank 3 vs no defender), range of worst is 53% to 3%, respectively.
+     */
+    // Todo Moddable as some global and/or in-game-gainable Uniques?
+    private fun getSkillModifier() = rank * 30
 
     /**
      * Gets a friendly and enemy efficiency uniques for the spy at the location
-     * @return a value centered around 100 for the work efficiency of the spy, won't be negative
+     * @return a value centered around 1.0 for the work efficiency of the spy, won't be negative
      */
     fun getEfficiencyModifier(): Double {
-        lateinit var friendlyUniques: Sequence<Unique>
+        val friendlyUniques: Sequence<Unique>
-        lateinit var enemyUniques: Sequence<Unique>
+        val enemyUniques: Sequence<Unique>
-        if (getLocation() != null) {
+        val city = getCityOrNull()
-            val city = getLocation()!!
+        when {
-            if (city.civ == civInfo) {
+            city == null -> {
+                // Spy is in hideout - effectiveness won't matter
+                friendlyUniques = civInfo.getMatchingUniques(UniqueType.SpyEffectiveness)
+                enemyUniques = sequenceOf()
+            }
+            city.civ == civInfo -> {
+                // Spy is in our own city
                 friendlyUniques = city.getMatchingUniques(UniqueType.SpyEffectiveness, StateForConditionals(city), includeCivUniques = true)
                 enemyUniques = sequenceOf()
-            } else {
+            }
+            else -> {
+                // Spy is active in a foreign city
                 friendlyUniques = civInfo.getMatchingUniques(UniqueType.SpyEffectiveness)
                 enemyUniques = city.getMatchingUniques(UniqueType.EnemySpyEffectiveness, StateForConditionals(city), includeCivUniques = true)
             }
-        } else {
-            friendlyUniques = civInfo.getMatchingUniques(UniqueType.SpyEffectiveness)
-            enemyUniques = sequenceOf()
         }
         var totalEfficiency = 1.0
         totalEfficiency *= (100.0 + friendlyUniques.sumOf { it.params[0].toInt() }) / 100
@@ -312,13 +318,20 @@ class Spy() : IsPartOfGameInfoSerialization {
         return totalEfficiency.coerceAtLeast(0.0)
     }
 
-    fun killSpy() {
+    private fun killSpy() {
         // We don't actually remove this spy object, we set them as dead and let them revive
         moveTo(null)
-        action = SpyAction.Dead
+        setAction(SpyAction.Dead, 5)
-        turnsRemainingForAction = 5
         rank = 1
     }
 
     fun isAlive(): Boolean = action != SpyAction.Dead
+
+    /** Shorthand for [Civilization.addNotification] specialized for espionage - action, category and icon are always the same */
+    fun addNotification(text: String) =
+        civInfo.addNotification(text, EspionageAction.withLocation(location), NotificationCategory.Espionage, NotificationIcon.Spy)
+
+    /** Anti-save-scum: Deterministic random from city and turn
+     *  @throws NullPointerException for spies in the hideout */
+    private fun randomSeed() = (getCity().run { location.x * location.y } + 123f * civInfo.gameInfo.turns).toInt()
 }

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

@@ -808,17 +808,13 @@ object UniqueTriggerActivation {
                     val currentEra = civInfo.getEra().name
                     for (otherCiv in civInfo.gameInfo.getAliveMajorCivs()) {
                         if (currentEra !in otherCiv.espionageManager.erasSpyEarnedFor) {
-                            val spyName = otherCiv.espionageManager.addSpy().name
+                            val spy = otherCiv.espionageManager.addSpy()
                             otherCiv.espionageManager.erasSpyEarnedFor.add(currentEra)
                             if (otherCiv == civInfo || otherCiv.knows(civInfo))
                             // We don't tell which civilization entered the new era, as that is done in the notification directly above this one
-                                otherCiv.addNotification("We have recruited [${spyName}] as a spy!", NotificationCategory.Espionage, NotificationIcon.Spy)
+                                spy.addNotification("We have recruited [${spy.name}] as a spy!")
                             else
-                                otherCiv.addNotification(
+                                spy.addNotification("After an unknown civilization entered the [$currentEra], we have recruited [${spy.name}] as a spy!")
-                                    "After an unknown civilization entered the [${currentEra}], we have recruited [${spyName}] as a spy!",
-                                    NotificationCategory.Espionage,
-                                    NotificationIcon.Spy
-                                )
                         }
                     }
                     true
@@ -840,8 +836,8 @@ object UniqueTriggerActivation {
                 if (!civInfo.gameInfo.isEspionageEnabled()) return null
 
                 return {
-                    val spyName = civInfo.espionageManager.addSpy().name
+                    val spy = civInfo.espionageManager.addSpy()
-                    civInfo.addNotification("We have recruited [${spyName}] as a spy!", NotificationCategory.Espionage, NotificationIcon.Spy)
+                    spy.addNotification("We have recruited [${spy.name}] as a spy!")
                     true
                 }
             }

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

@@ -228,8 +228,8 @@ enum class UniqueType(
     FaithCostOfGreatProphetChange("[relativeAmount]% Faith cost of generating Great Prophet equivalents", UniqueTarget.Global),
 
     /// Espionage
-    SpyEffectiveness("[relativeAmount]% spy effectiveness [cityFilter]", UniqueTarget.Global, UniqueTarget.Global),
+    SpyEffectiveness("[relativeAmount]% spy effectiveness [cityFilter]", UniqueTarget.Global),
-    EnemySpyEffectiveness("[relativeAmount]% enemy spy effectiveness [cityFilter]", UniqueTarget.Global, UniqueTarget.Global),
+    EnemySpyEffectiveness("[relativeAmount]% enemy spy effectiveness [cityFilter]", UniqueTarget.Global),
     SpyStartingLevel("New spies start with [amount] level(s)", UniqueTarget.Global),
 
     /// Things you get at the start of the game

core/src/com/unciv/ui/components/SmallButtonStyle.kt

@@ -0,0 +1,43 @@
+package com.unciv.ui.components
+
+import com.badlogic.gdx.graphics.Color
+import com.badlogic.gdx.graphics.g2d.NinePatch
+import com.badlogic.gdx.scenes.scene2d.ui.TextButton
+import com.badlogic.gdx.scenes.scene2d.utils.NinePatchDrawable
+import com.unciv.ui.images.ImageGetter
+import com.unciv.ui.screens.basescreen.BaseScreen
+
+class SmallButtonStyle : TextButton.TextButtonStyle(BaseScreen.skin[TextButton.TextButtonStyle::class.java]) {
+    /** Modify NinePatch geometry so the roundedEdgeRectangleMidShape button is 38f high instead of 48f,
+     *  Otherwise this excercise would be futile - normal roundedEdgeRectangleShape based buttons are 50f high.
+     */
+    private fun NinePatchDrawable.reduce(): NinePatchDrawable {
+        val patch = NinePatch(this.patch)
+        patch.padTop = 10f
+        patch.padBottom = 10f
+        patch.topHeight = 10f
+        patch.bottomHeight = 10f
+        return NinePatchDrawable(this).also { it.patch = patch }
+    }
+
+    init {
+        val upColor = BaseScreen.skin.getColor("color")
+        val downColor = BaseScreen.skin.getColor("pressed")
+        val overColor = BaseScreen.skin.getColor("highlight")
+        val disabledColor = BaseScreen.skin.getColor("disabled")
+        // UiElementDocsWriter inspects source, which is why this isn't prettified better
+        val shape = BaseScreen.run {
+            // Let's use _one_ skinnable background lookup but with different tints
+            val skinned = skinStrings.getUiBackground("AnimatedMenu/Button", skinStrings.roundedEdgeRectangleMidShape)
+            // Reduce height only if not skinned
+            val default = ImageGetter.getNinePatch(skinStrings.roundedEdgeRectangleMidShape)
+            if (skinned === default) default.reduce() else skinned
+        }
+        // Now get the tinted variants
+        up = shape.tint(upColor)
+        down = shape.tint(downColor)
+        over = shape.tint(overColor)
+        disabled = shape.tint(disabledColor)
+        disabledFontColor = Color.GRAY
+    }
+}

core/src/com/unciv/ui/popups/AnimatedMenuPopup.kt

@@ -1,7 +1,6 @@
 package com.unciv.ui.popups
 
 import com.badlogic.gdx.graphics.Color
-import com.badlogic.gdx.graphics.g2d.NinePatch
 import com.badlogic.gdx.math.Interpolation
 import com.badlogic.gdx.math.Vector2
 import com.badlogic.gdx.scenes.scene2d.Actor
@@ -10,14 +9,13 @@ import com.badlogic.gdx.scenes.scene2d.Touchable
 import com.badlogic.gdx.scenes.scene2d.actions.Actions
 import com.badlogic.gdx.scenes.scene2d.ui.Container
 import com.badlogic.gdx.scenes.scene2d.ui.Table
-import com.badlogic.gdx.scenes.scene2d.ui.TextButton
 import com.badlogic.gdx.scenes.scene2d.utils.NinePatchDrawable
+import com.unciv.ui.components.SmallButtonStyle
 import com.unciv.ui.components.extensions.toTextButton
 import com.unciv.ui.components.input.KeyCharAndCode
 import com.unciv.ui.components.input.KeyboardBinding
 import com.unciv.ui.components.input.keyShortcuts
 import com.unciv.ui.components.input.onActivation
-import com.unciv.ui.images.ImageGetter
 import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.utils.Concurrency
 
@@ -163,40 +161,4 @@ open class AnimatedMenuPopup(
                 close()
             }
         }
-
-    //todo Reused in SpecialistAllocationTable - refactor to another package
-    class SmallButtonStyle : TextButton.TextButtonStyle(BaseScreen.skin[TextButton.TextButtonStyle::class.java]) {
-        /** Modify NinePatch geometry so the roundedEdgeRectangleMidShape button is 38f high instead of 48f,
-         *  Otherwise this excercise would be futile - normal roundedEdgeRectangleShape based buttons are 50f high.
-         */
-        private fun NinePatchDrawable.reduce(): NinePatchDrawable {
-            val patch = NinePatch(this.patch)
-            patch.padTop = 10f
-            patch.padBottom = 10f
-            patch.topHeight = 10f
-            patch.bottomHeight = 10f
-            return NinePatchDrawable(this).also { it.patch = patch }
-        }
-
-        init {
-            val upColor = BaseScreen.skin.getColor("color")
-            val downColor = BaseScreen.skin.getColor("pressed")
-            val overColor = BaseScreen.skin.getColor("highlight")
-            val disabledColor = BaseScreen.skin.getColor("disabled")
-            // UiElementDocsWriter inspects source, which is why this isn't prettified better
-            val shape = BaseScreen.run {
-                // Let's use _one_ skinnable background lookup but with different tints
-                val skinned = skinStrings.getUiBackground("AnimatedMenu/Button", skinStrings.roundedEdgeRectangleMidShape)
-                // Reduce height only if not skinned
-                val default = ImageGetter.getNinePatch(skinStrings.roundedEdgeRectangleMidShape)
-                if (skinned === default) default.reduce() else skinned
-            }
-            // Now get the tinted variants
-            up = shape.tint(upColor)
-            down = shape.tint(downColor)
-            over = shape.tint(overColor)
-            disabled = shape.tint(disabledColor)
-            disabledFontColor = Color.GRAY
-        }
-    }
 }

core/src/com/unciv/ui/screens/cityscreen/SpecialistAllocationTable.kt

@@ -5,6 +5,7 @@ import com.badlogic.gdx.scenes.scene2d.Actor
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.badlogic.gdx.utils.Align
 import com.unciv.Constants
+import com.unciv.ui.components.SmallButtonStyle
 import com.unciv.ui.components.UncivTooltip.Companion.addTooltip
 import com.unciv.ui.components.extensions.addSeparatorVertical
 import com.unciv.ui.components.extensions.darken
@@ -16,15 +17,14 @@ import com.unciv.ui.components.input.onActivation
 import com.unciv.ui.components.input.onClick
 import com.unciv.ui.components.widgets.ExpanderTab
 import com.unciv.ui.images.ImageGetter
-import com.unciv.ui.popups.AnimatedMenuPopup
 import com.unciv.ui.screens.basescreen.BaseScreen
 
 class SpecialistAllocationTable(private val cityScreen: CityScreen) : Table(BaseScreen.skin) {
     val city = cityScreen.city
-    private val smallButtonStyle = AnimatedMenuPopup.SmallButtonStyle()
+    private val smallButtonStyle = SmallButtonStyle()
 
     fun update() {
-        // 5 columns: "-" unassignButton, AllocationTable, "+" assignButton, SeparatorVertical, SpecialistsStatsTabe
+        // 5 columns: "-" unassignButton, AllocationTable, "+" assignButton, SeparatorVertical, SpecialistsStatsTable
         clear()
 
         // Auto/Manual Specialists Toggle

core/src/com/unciv/ui/screens/newgamescreen/MapFileSelectTable.kt

@@ -19,6 +19,7 @@ import com.unciv.models.metadata.Player
 import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.ruleset.nation.Nation
 import com.unciv.models.translations.tr
+import com.unciv.ui.components.SmallButtonStyle
 import com.unciv.ui.components.extensions.disable
 import com.unciv.ui.components.extensions.enable
 import com.unciv.ui.components.extensions.pad
@@ -28,7 +29,6 @@ import com.unciv.ui.components.extensions.toTextButton
 import com.unciv.ui.components.input.onActivation
 import com.unciv.ui.components.input.onChange
 import com.unciv.ui.components.widgets.LoadingImage
-import com.unciv.ui.popups.AnimatedMenuPopup
 import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.screens.victoryscreen.LoadMapPreview
 import com.unciv.utils.Concurrency
@@ -53,7 +53,7 @@ class MapFileSelectTable(
     private val mapCategorySelectBox = SelectBox<String>(BaseScreen.skin)
     private val mapFileSelectBox = SelectBox<MapWrapper>(BaseScreen.skin)
     private val loadingIcon = LoadingImage(30f, LoadingImage.Style(loadingColor = Color.SCARLET))
-    private val useNationsFromMapButton = "Select players from starting locations".toTextButton(AnimatedMenuPopup.SmallButtonStyle())
+    private val useNationsFromMapButton = "Select players from starting locations".toTextButton(SmallButtonStyle())
     private val useNationsButtonCell: Cell<Actor?>
     private var mapNations = emptyList<Nation>()
     private var mapHumanPick: String? = null

core/src/com/unciv/ui/screens/overviewscreen/EspionageOverviewScreen.kt

@@ -10,8 +10,8 @@ import com.unciv.UncivGame
 import com.unciv.logic.city.City
 import com.unciv.logic.civilization.Civilization
 import com.unciv.models.Spy
-import com.unciv.models.SpyAction
 import com.unciv.models.translations.tr
+import com.unciv.ui.components.SmallButtonStyle
 import com.unciv.ui.components.extensions.addSeparatorVertical
 import com.unciv.ui.components.extensions.disable
 import com.unciv.ui.components.extensions.setSize
@@ -41,7 +41,10 @@ class EspionageOverviewScreen(val civInfo: Civilization, val worldScreen: WorldS
     private var selectedSpy: Spy? = null
 
     // if the value == null, this means the Spy Hideout.
-    private var moveSpyHereButtons = hashMapOf<Button, City?>()
+    private var moveSpyHereButtons = hashMapOf<MoveToCityButton, City?>()
+
+    /** Readability shortcut */
+    private val manager get() = civInfo.espionageManager
 
     init {
         spySelectionTable.defaults().pad(10f)
@@ -73,15 +76,12 @@ class EspionageOverviewScreen(val civInfo: Civilization, val worldScreen: WorldS
         spySelectionTable.add("Rank".toLabel())
         spySelectionTable.add("Location".toLabel())
         spySelectionTable.add("Action".toLabel()).row()
-        for (spy in civInfo.espionageManager.spyList) {
+        for (spy in manager.spyList) {
             spySelectionTable.add(spy.name.toLabel())
             spySelectionTable.add(spy.rank.toLabel())
             spySelectionTable.add(spy.getLocationName().toLabel())
-            val actionString =
+            val actionString = if (spy.action.hasTurns) "[${spy.action.displayString}] ${spy.turnsRemainingForAction}${Fonts.turn}"
-                when (spy.action) {
+                else spy.action.displayString
-                    SpyAction.None, SpyAction.StealingTech, SpyAction.Surveillance, SpyAction.CounterIntelligence -> spy.action.displayString
-                    SpyAction.Moving, SpyAction.EstablishNetwork, SpyAction.Dead, SpyAction.RiggingElections -> "[${spy.action.displayString}] ${spy.turnsRemainingForAction}${Fonts.turn}"
-                }
             spySelectionTable.add(actionString.toLabel())
 
             val moveSpyButton = "Move".toTextButton()
@@ -95,10 +95,14 @@ class EspionageOverviewScreen(val civInfo: Civilization, val worldScreen: WorldS
                 selectedSpy = spy
                 selectedSpyButton!!.label.setText(Constants.cancel.tr())
                 for ((button, city) in moveSpyHereButtons) {
-                    // Not own cities as counterintelligence isn't implemented
+                    if (city == spy.getCityOrNull()) {
-                    // Not city-state civs as rigging elections isn't implemented
+                        button.isVisible = true
+                        button.setDirection(Align.right)
+                    } else {
                         button.isVisible = city == null // hideout
-                        || (city.civ != civInfo && !city.espionage.hasSpyOf(civInfo))
+                            || !city.espionage.hasSpyOf(civInfo)
+                        button.setDirection(Align.left)
+                    }
                 }
             }
             if (!worldScreen.canChangeState || !spy.isAlive()) {
@@ -113,15 +117,15 @@ class EspionageOverviewScreen(val civInfo: Civilization, val worldScreen: WorldS
         citySelectionTable.clear()
         moveSpyHereButtons.clear()
         citySelectionTable.add()
-        citySelectionTable.add("City".toLabel())
+        citySelectionTable.add("City".toLabel()).padTop(10f)
-        citySelectionTable.add("Spy present".toLabel()).row()
+        citySelectionTable.add("Spy present".toLabel()).padTop(10f).row()
 
         // First add the hideout to the table
 
         citySelectionTable.add()
         citySelectionTable.add("Spy Hideout".toLabel())
-        citySelectionTable.add()
+        citySelectionTable.add(getSpyIcons(manager.getIdleSpies()))
-        val moveSpyHereButton = getMoveToCityButton(null)
+        val moveSpyHereButton = MoveToCityButton(null)
         citySelectionTable.add(moveSpyHereButton).row()
 
         // Then add all cities
@@ -147,41 +151,70 @@ class EspionageOverviewScreen(val civInfo: Civilization, val worldScreen: WorldS
     private fun addCityToSelectionTable(city: City) {
         citySelectionTable.add(ImageGetter.getNationPortrait(city.civ.nation, 30f))
             .padLeft(20f)
-        citySelectionTable.add(city.name.toLabel(hideIcons = true))
+        val label = city.name.toLabel(hideIcons = true)
-        if (city.espionage.hasSpyOf(civInfo)) {
+        label.onClick {
-            citySelectionTable.add(
+            worldScreen.game.popScreen() // If a detour to this screen (i.e. not directly from worldScreen) is made possible, use resetToWorldScreen instead
-                ImageGetter.getImage("OtherIcons/Spy_White").apply {
+            worldScreen.mapHolder.setCenterPosition(city.location)
-                    setSize(30f)
-                    color = Color.WHITE
-                }
-            )
-        } else {
-            citySelectionTable.add()
         }
+        citySelectionTable.add(label).fill()
+        citySelectionTable.add(getSpyIcons(manager.getSpiesInCity(city)))
 
-        val moveSpyHereButton = getMoveToCityButton(city)
+        val moveSpyHereButton = MoveToCityButton(city)
         citySelectionTable.add(moveSpyHereButton)
         citySelectionTable.row()
     }
 
+    private fun getSpyIcon(spy: Spy) = Table().apply {
+        add (ImageGetter.getImage("OtherIcons/Spy_White").apply {
+            color = Color.WHITE
+        }).size(30f)
+        val color = when(spy.rank) {
+            1 -> Color.BROWN
+            2 -> Color.LIGHT_GRAY
+            3 -> Color.GOLD
+            else -> return@apply
+        }
+        val starTable = Table()
+        repeat(spy.rank) {
+            val star = ImageGetter.getImage("OtherIcons/Star")
+            star.color = color
+            starTable.add(star).size(8f).pad(1f).row()
+        }
+        add(starTable).center().padLeft(-4f)
+    }
+
+    private fun getSpyIcons(spies: Iterable<Spy>) = Table().apply {
+        defaults().space(0f, 2f, 0f, 2f)
+        for (spy in spies)
+            add(getSpyIcon(spy))
+    }
+
     // city == null is interpreted as 'spy hideout'
-    private fun getMoveToCityButton(city: City?): Button {
+    private inner class MoveToCityButton(city: City?) : Button(SmallButtonStyle()) {
-        val moveSpyHereButton = Button(skin)
+        val arrow = ImageGetter.getArrowImage(Align.left)
-        moveSpyHereButton.add(ImageGetter.getArrowImage(Align.left).apply { color = Color.WHITE })
+        init {
-        moveSpyHereButton.onClick {
+            arrow.setSize(24f)
+            add(arrow).size(24f)
+            arrow.setOrigin(Align.center)
+            arrow.color = Color.WHITE
+            onClick {
                 selectedSpy!!.moveTo(city)
                 resetSelection()
                 update()
             }
-        moveSpyHereButtons[moveSpyHereButton] = city
+            moveSpyHereButtons[this] = city
-        moveSpyHereButton.isVisible = false
+            isVisible = false
-        return moveSpyHereButton
+        }
+
+        fun setDirection(align: Int) {
+            arrow.rotation = if (align == Align.right) 0f else 180f
+            isDisabled = align == Align.right
+        }
     }
 
     private fun resetSelection() {
         selectedSpy = null
-        if (selectedSpyButton != null)
+        selectedSpyButton?.label?.setText("Move".tr())
-            selectedSpyButton!!.label.setText("Move".tr())
         selectedSpyButton = null
         for ((button, _) in moveSpyHereButtons)
             button.isVisible = false