A few more useful notification actions (#9811)

* Minor UI tweaks - mainly duplicate icons on ResourcesOverviewTab and EspionageOverviewScreen

* Bugfix and expand NotificationActions

* Switch NotificationAction migration to Phase IV

* Tweak a few Notifications to have more useful actions

* Remove one `run {}`

* Better predictability of clicks on Notifications pulled out of history

* Unit creation notifications can now select the unit

* Linting

* ClearBarbarianCamp quest Notification shows map location first

* More Linting

* Hide City-state call for help from aggressor

core/src/com/unciv/logic/battle/Battle.kt

@@ -9,10 +9,12 @@ import com.unciv.logic.city.City
 import com.unciv.logic.civilization.AlertType
 import com.unciv.logic.civilization.Civilization
 import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.MapUnitAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PlayerType
 import com.unciv.logic.civilization.PopupAlert
+import com.unciv.logic.civilization.PromoteUnitAction
 import com.unciv.logic.civilization.diplomacy.DiplomaticModifiers
 import com.unciv.logic.civilization.diplomacy.DiplomaticStatus
 import com.unciv.logic.map.mapunit.MapUnit
@@ -582,8 +584,12 @@ object Battle {
             civ.greatPeople.greatGeneralPoints += greatGeneralPointsGained
         }
 
-        if (!thisCombatant.isDefeated() && !unitCouldAlreadyPromote && promotions.canBePromoted())
+        if (!thisCombatant.isDefeated() && !unitCouldAlreadyPromote && promotions.canBePromoted()) {
-            civ.addNotification("[${thisCombatant.unit.displayName()}] can be promoted!",thisCombatant.getTile().position, NotificationCategory.Units, thisCombatant.unit.name)
+            val pos = thisCombatant.getTile().position
+            civ.addNotification("[${thisCombatant.unit.displayName()}] can be promoted!",
+                listOf(MapUnitAction(pos), PromoteUnitAction(thisCombatant.getName(), pos)),
+                NotificationCategory.Units, thisCombatant.unit.name)
+        }
     }
 
     private fun conquerCity(city: City, attacker: MapUnitCombatant) {

core/src/com/unciv/logic/city/CityConstructions.kt

@@ -6,6 +6,9 @@ import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.automation.Automation
 import com.unciv.logic.automation.city.ConstructionAutomation
 import com.unciv.logic.civilization.AlertType
+import com.unciv.logic.civilization.CivilopediaAction
+import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.MapUnitAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PopupAlert
@@ -14,6 +17,7 @@ import com.unciv.logic.multiplayer.isUsersTurn
 import com.unciv.models.ruleset.Building
 import com.unciv.models.ruleset.IConstruction
 import com.unciv.models.ruleset.INonPerpetualConstruction
+import com.unciv.models.ruleset.IRulesetObject
 import com.unciv.models.ruleset.PerpetualConstruction
 import com.unciv.models.ruleset.RejectionReasonType
 import com.unciv.models.ruleset.Ruleset
@@ -447,33 +451,40 @@ class CityConstructions : IsPartOfGameInfoSerialization {
 
         validateConstructionQueue() // if we've build e.g. the Great Lighthouse, then Lighthouse is no longer relevant in the queue
 
+        construction as IRulesetObject // Always OK for INonPerpetualConstruction, but compiler doesn't know
+
         val buildingIcon = "BuildingIcons/${construction.name}"
+        val pediaAction = CivilopediaAction(construction.makeLink())
+        val locationAction = if (construction is BaseUnit) MapUnitAction(city.location)
+            else LocationAction(city.location)
+        val locationAndPediaActions = listOf(locationAction, pediaAction)
+
         if (construction is Building && construction.isWonder) {
             city.civ.popupAlerts.add(PopupAlert(AlertType.WonderBuilt, construction.name))
             for (civ in city.civ.gameInfo.civilizations) {
                 if (civ.hasExplored(city.getCenterTile()))
-                    civ.addNotification("[${construction.name}] has been built in [${city.name}]", city.location,
+                    civ.addNotification("[${construction.name}] has been built in [${city.name}]",
+                        locationAndPediaActions,
                         if (civ == city.civ) NotificationCategory.Production else NotificationCategory.General, buildingIcon)
                 else
-                    civ.addNotification("[${construction.name}] has been built in a faraway land", NotificationCategory.General, buildingIcon)
+                    civ.addNotification("[${construction.name}] has been built in a faraway land",
+                        pediaAction, NotificationCategory.General, buildingIcon)
             }
         } else {
             val icon = if (construction is Building) buildingIcon else construction.name // could be a unit, in which case take the unit name.
             city.civ.addNotification(
                 "[${construction.name}] has been built in [${city.name}]",
-                    city.location, NotificationCategory.Production, NotificationIcon.Construction, icon)
+                locationAndPediaActions, NotificationCategory.Production, NotificationIcon.Construction, icon)
         }
 
-        if (construction is Building && construction.hasUnique(UniqueType.TriggersAlertOnCompletion,
+        if (construction.hasUnique(UniqueType.TriggersAlertOnCompletion, StateForConditionals(city.civ, city))) {
-                StateForConditionals(city.civ, city)
-            )) {
             for (otherCiv in city.civ.gameInfo.civilizations) {
                 // No need to notify ourself, since we already got the building notification anyway
                 if (otherCiv == city.civ) continue
                 val completingCivDescription =
                     if (otherCiv.knows(city.civ)) "[${city.civ.civName}]" else "An unknown civilization"
                 otherCiv.addNotification("$completingCivDescription has completed [${construction.name}]!",
-                    NotificationCategory.General, NotificationIcon.Construction, buildingIcon)
+                    pediaAction, NotificationCategory.General, NotificationIcon.Construction, buildingIcon)
             }
         }
         return true

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

@@ -3,10 +3,14 @@ package com.unciv.logic.city.managers
 import com.unciv.logic.city.City
 import com.unciv.logic.city.CityFlags
 import com.unciv.logic.city.CityFocus
+import com.unciv.logic.civilization.CityAction
+import com.unciv.logic.civilization.LocationAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
+import com.unciv.logic.civilization.OverviewAction
 import com.unciv.models.ruleset.tile.ResourceType
 import com.unciv.models.ruleset.unique.UniqueType
+import com.unciv.ui.screens.overviewscreen.EmpireOverviewCategories
 import kotlin.math.min
 import kotlin.random.Random
 
@@ -50,7 +54,7 @@ class CityTurnManager(val city: City) {
             city.setFlag(CityFlags.WeLoveTheKing, 20 + 1) // +1 because it will be decremented by 1 in the same startTurn()
             city.civ.addNotification(
                 "Because they have [${city.demandedResource}], the citizens of [${city.name}] are celebrating We Love The King Day!",
-                city.location, NotificationCategory.General, NotificationIcon.City, NotificationIcon.Happiness)
+                CityAction.withLocation(city), NotificationCategory.General, NotificationIcon.City, NotificationIcon.Happiness)
         }
     }
 
@@ -70,14 +74,14 @@ class CityTurnManager(val city: City) {
                     CityFlags.WeLoveTheKing.name -> {
                         city.civ.addNotification(
                             "We Love The King Day in [${city.name}] has ended.",
-                            city.location, NotificationCategory.General, NotificationIcon.City)
+                            CityAction.withLocation(city), NotificationCategory.General, NotificationIcon.City)
                         demandNewResource()
                     }
                     CityFlags.Resistance.name -> {
                         city.updateCitizens = true
                         city.civ.addNotification(
                             "The resistance in [${city.name}] has ended!",
-                            city.location, NotificationCategory.General, "StatIcons/Resistance")
+                            CityAction.withLocation(city), NotificationCategory.General, "StatIcons/Resistance")
                     }
                 }
             }
@@ -105,7 +109,8 @@ class CityTurnManager(val city: City) {
             city.setFlag(CityFlags.ResourceDemand, 15 + Random.Default.nextInt(10))
         else
             city.civ.addNotification("[${city.name}] demands [${city.demandedResource}]!",
-                city.location, NotificationCategory.General, NotificationIcon.City, "ResourceIcons/${city.demandedResource}")
+                listOf(LocationAction(city.location), OverviewAction(EmpireOverviewCategories.Resources)),
+                NotificationCategory.General, NotificationIcon.City, "ResourceIcons/${city.demandedResource}")
     }
 
 
@@ -144,5 +149,4 @@ class CityTurnManager(val city: City) {
         }
     }
 
-
 }

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

@@ -91,6 +91,10 @@ open class Notification() : IsPartOfGameInfoSerialization {
         index = ++index % actions.size // cycle through tiles
     }
 
+    fun resetExecuteRoundRobin() {
+        index = 0
+    }
+
     /**
      *  Custom [Gdx.Json][Json] serializer/deserializer for one [Notification].
      *
@@ -111,7 +115,7 @@ open class Notification() : IsPartOfGameInfoSerialization {
         companion object {
             /** The switch that starts Phase III and dies with Phase V
             *   @see Serializer */
-            private const val compatibilityMode = true
+            private const val compatibilityMode = false
         }
 
         override fun write(json: Json, notification: Notification, knownType: Class<*>?) {

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

@@ -4,11 +4,13 @@ import com.badlogic.gdx.math.Vector2
 import com.badlogic.gdx.utils.Json
 import com.badlogic.gdx.utils.JsonValue
 import com.unciv.logic.IsPartOfGameInfoSerialization
+import com.unciv.logic.city.City
 import com.unciv.ui.components.MayaCalendar
 import com.unciv.ui.screens.cityscreen.CityScreen
-import com.unciv.ui.screens.civilopediascreen.CivilopediaCategories
 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.pickerscreens.PromotionPickerScreen
 import com.unciv.ui.screens.pickerscreens.TechPickerScreen
 import com.unciv.ui.screens.worldscreen.WorldScreen
@@ -68,6 +70,9 @@ class CityAction(private val city: Vector2 = Vector2.Zero): NotificationAction {
         if (cityObject.civ == worldScreen.viewingCiv)
             worldScreen.game.pushScreen(CityScreen(cityObject))
     }
+    companion object {
+        fun withLocation(city: City) = listOf(LocationAction(city.location), CityAction(city.location))
+    }
 }
 
 /** enter diplomacy screen */
@@ -90,12 +95,17 @@ class MapUnitAction(private val location: Vector2 = Vector2.Zero) : Notification
     override fun execute(worldScreen: WorldScreen) {
         worldScreen.mapHolder.setCenterPosition(location, selectUnit = true)
     }
+    companion object {
+        // Convenience shortcut as it makes replacing LocationAction calls easier (see above)
+        operator fun invoke(locations: Iterable<Vector2>): Sequence<MapUnitAction> =
+            locations.asSequence().map { MapUnitAction(it) }
+    }
 }
 
-/** A notification action that shows the Civilopedia entry for a Wonder. */
+/** A notification action that shows a Civilopedia entry, e.g. for a Wonder. */
-class WonderAction(private val wonderName: String = "") : NotificationAction {
+class CivilopediaAction(private val link: String = "") : NotificationAction {
     override fun execute(worldScreen: WorldScreen) {
-        worldScreen.game.pushScreen(CivilopediaScreen(worldScreen.gameInfo.ruleset, CivilopediaCategories.Wonder, wonderName))
+        worldScreen.game.pushScreen(CivilopediaScreen(worldScreen.gameInfo.ruleset, link = link))
     }
 }
 
@@ -109,6 +119,16 @@ class PromoteUnitAction(private val name: String = "", private val location: Vec
     }
 }
 
+/** Open the Empire Overview to a specific page, potentially "selecting" some entry */
+class OverviewAction(
+    private val page: EmpireOverviewCategories = EmpireOverviewCategories.Resources,
+    private val select: String = ""
+) : NotificationAction {
+    override fun execute(worldScreen: WorldScreen) {
+        worldScreen.game.pushScreen(EmpireOverviewScreen(worldScreen.selectedCiv, page, select))
+    }
+}
+
 @Suppress("PrivatePropertyName")  // These names *must* match their class name, see below
 internal class NotificationActionsDeserializer {
     /* This exists as trick to leverage readFields for Json deserialization.
@@ -127,14 +147,15 @@ internal class NotificationActionsDeserializer {
     private val DiplomacyAction: DiplomacyAction? = null
     private val MayaLongCountAction: MayaLongCountAction? = null
     private val MapUnitAction: MapUnitAction? = null
-    private val WonderAction: WonderAction? = null
+    private val CivilopediaAction: CivilopediaAction? = null
     private val PromoteUnitAction: PromoteUnitAction? = null
+    private val OverviewAction: OverviewAction? = null
 
     fun read(json: Json, jsonData: JsonValue): List<NotificationAction> {
         json.readFields(this, jsonData)
         return listOfNotNull(
-            LocationAction, TechAction, CityAction, DiplomacyAction,
+            LocationAction, TechAction, CityAction, DiplomacyAction, MayaLongCountAction,
-            MayaLongCountAction, MapUnitAction, WonderAction, PromoteUnitAction
+            MapUnitAction, CivilopediaAction, PromoteUnitAction, OverviewAction
         )
     }
 }

core/src/com/unciv/logic/civilization/diplomacy/CityStateFunctions.kt

@@ -8,6 +8,7 @@ import com.unciv.logic.civilization.CivFlags
 import com.unciv.logic.civilization.Civilization
 import com.unciv.logic.civilization.DiplomacyAction
 import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.MapUnitAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PlayerType
@@ -115,12 +116,12 @@ class CityStateFunctions(val civInfo: Civilization) {
             placedUnit.promotions.XP += unique.params[0].toInt()
         }
 
-        // Point to the places mentioned in the message _in that order_ (debatable)
+        // Point to the gifted unit, then to the other places mentioned in the message
-        val placedLocation = placedUnit.getTile().position
+        val unitAction = sequenceOf(MapUnitAction(placedUnit.getTile().position))
-        val locations = LocationAction(placedLocation, cities.city2.location, city.location)
+        val notificationActions = unitAction + LocationAction(cities.city2.location, city.location)
         receivingCiv.addNotification(
             "[${civInfo.civName}] gave us a [${militaryUnit.name}] as gift near [${city.name}]!",
-            locations,
+            notificationActions,
             NotificationCategory.Units,
             civInfo.civName,
             militaryUnit.name
@@ -228,18 +229,11 @@ class CityStateFunctions(val civInfo: Civilization) {
             val oldAllyName = civInfo.getAllyCiv()
             civInfo.setAllyCiv(newAllyName)
 
-            // If the city-state is captured by a civ, it stops being the ally of the civ it was previously an ally of.
-            //  This means that it will NOT HAVE a capital at that time, so if we run getCapital we'll get a crash!
-            val capitalLocation = if (civInfo.cities.isNotEmpty() && civInfo.getCapital() != null) civInfo.getCapital()!!.location else null
-
             if (newAllyName != null) {
                 val newAllyCiv = civInfo.gameInfo.getCivilization(newAllyName)
                 val text = "We have allied with [${civInfo.civName}]."
-                if (capitalLocation != null) newAllyCiv.addNotification(text, capitalLocation,
+                newAllyCiv.addNotification(text,
-                    NotificationCategory.Diplomacy, civInfo.civName,
+                    getNotificationActions(),
-                    NotificationIcon.Diplomacy
-                )
-                else newAllyCiv.addNotification(text,
                     NotificationCategory.Diplomacy, civInfo.civName,
                     NotificationIcon.Diplomacy
                 )
@@ -262,11 +256,8 @@ class CityStateFunctions(val civInfo: Civilization) {
             if (oldAllyName != null && civInfo.isAlive()) {
                 val oldAllyCiv = civInfo.gameInfo.getCivilization(oldAllyName)
                 val text = "We have lost alliance with [${civInfo.civName}]."
-                if (capitalLocation != null) oldAllyCiv.addNotification(text, capitalLocation,
+                oldAllyCiv.addNotification(text,
-                    NotificationCategory.Diplomacy, civInfo.civName,
+                    getNotificationActions(),
-                    NotificationIcon.Diplomacy
-                )
-                else oldAllyCiv.addNotification(text,
                     NotificationCategory.Diplomacy, civInfo.civName,
                     NotificationIcon.Diplomacy
                 )
@@ -276,6 +267,21 @@ class CityStateFunctions(val civInfo: Civilization) {
         }
     }
 
+    /** @return a Sequence of NotificationActions for use in addNotification, showing Capital on map if any, then opening diplomacy */
+    fun getNotificationActions() = sequence {
+        // Notification click will first point to CS location, if any, then open diplomacy.
+        // That's fine for the influence notifications and for afraid too.
+        //
+        // If the city-state is captured by a civ, it stops being the ally of the civ it was previously an ally of.
+        //  This means that it will NOT HAVE a capital at that time, so if we run getCapital()!! we'll get a crash!
+        // Or, City States can get stuck with only their Settler and no cities until late into a game if city placements are rare
+        // We also had `cities.asSequence() // in practice 0 or 1 entries, that's OK` before (a CS *can* have >1 cities but it will always raze conquests).
+        val capital = civInfo.getCapital()
+        if (capital != null)
+            yield(LocationAction(capital.location))
+        yield(DiplomacyAction(civInfo.civName))
+    }
+
     fun getDiplomaticMarriageCost(): Int {
         // https://github.com/Gedemon/Civ5-DLL/blob/master/CvGameCoreDLL_Expansion1/CvMinorCivAI.cpp, line 7812
         var cost = (500 * civInfo.gameInfo.speed.goldCostModifier).toInt()

core/src/com/unciv/logic/civilization/diplomacy/DiplomacyManager.kt

@@ -5,6 +5,8 @@ import com.unciv.Constants
 import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.civilization.AlertType
 import com.unciv.logic.civilization.Civilization
+import com.unciv.logic.civilization.DiplomacyAction
+import com.unciv.logic.civilization.LocationAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PopupAlert
@@ -527,18 +529,15 @@ class DiplomacyManager() : IsPartOfGameInfoSerialization {
         }
 
         if (!civInfo.isDefeated()) { // don't display city state relationship notifications when the city state is currently defeated
-            val civCapitalLocation = if (civInfo.cities.any() && civInfo.getCapital() != null) civInfo.getCapital()!!.location else null
+            val notificationActions = civInfo.cityStateFunctions.getNotificationActions()
             if (getTurnsToRelationshipChange() == 1) {
                 val text = "Your relationship with [${civInfo.civName}] is about to degrade"
-                if (civCapitalLocation != null) otherCiv().addNotification(text,
+                otherCiv().addNotification(text, notificationActions, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
-                    civCapitalLocation, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
-                else otherCiv().addNotification(text, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
             }
 
             if (initialRelationshipLevel >= RelationshipLevel.Friend && initialRelationshipLevel != relationshipIgnoreAfraid()) {
                 val text = "Your relationship with [${civInfo.civName}] degraded"
-                if (civCapitalLocation != null) otherCiv().addNotification(text, civCapitalLocation, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
+                otherCiv().addNotification(text, notificationActions, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
-                else otherCiv().addNotification(text, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
             }
 
             // Potentially notify about afraid status
@@ -549,8 +548,7 @@ class DiplomacyManager() : IsPartOfGameInfoSerialization {
             ) {
                 setFlag(DiplomacyFlags.NotifiedAfraid, 20)  // Wait 20 turns until next reminder
                 val text = "[${civInfo.civName}] is afraid of your military power!"
-                if (civCapitalLocation != null) otherCiv().addNotification(text, civCapitalLocation, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
+                otherCiv().addNotification(text, notificationActions, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
-                else otherCiv().addNotification(text, NotificationCategory.Diplomacy, civInfo.civName, NotificationIcon.Diplomacy)
             }
         }
     }

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

@@ -3,6 +3,7 @@ package com.unciv.logic.civilization.managers
 import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.civilization.AlertType
 import com.unciv.logic.civilization.Civilization
+import com.unciv.logic.civilization.CivilopediaAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.PopupAlert
 import com.unciv.models.ruleset.unique.UniqueTriggerActivation
@@ -44,7 +45,9 @@ class GoldenAgeManager : IsPartOfGameInfoSerialization {
 
     fun enterGoldenAge(unmodifiedNumberOfTurns: Int = 10) {
         turnsLeftForCurrentGoldenAge += calculateGoldenAgeLength(unmodifiedNumberOfTurns)
-        civInfo.addNotification("You have entered a Golden Age!", NotificationCategory.General, "StatIcons/Happiness")
+        civInfo.addNotification("You have entered a Golden Age!",
+            CivilopediaAction("Tutorial/Golden Age"),
+            NotificationCategory.General, "StatIcons/Happiness")
         civInfo.popupAlerts.add(PopupAlert(AlertType.GoldenAge, ""))
 
         for (unique in civInfo.getTriggeredUniques(UniqueType.TriggerUponEnteringGoldenAge))

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

@@ -1,3 +1,5 @@
+@file:Suppress("ConvertArgumentToSet")  // Flags all assignedQuests.removeAll(List) - not worth it
+
 package com.unciv.logic.civilization.managers
 
 import com.badlogic.gdx.math.Vector2
@@ -8,6 +10,9 @@ import com.unciv.logic.IsPartOfGameInfoSerialization
 import com.unciv.logic.civilization.CivFlags
 import com.unciv.logic.civilization.Civilization
 import com.unciv.logic.civilization.DiplomacyAction
+import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.Notification  // for Kdoc
+import com.unciv.logic.civilization.NotificationAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PlayerType
@@ -30,7 +35,6 @@ import com.unciv.ui.components.extensions.toPercent
 import kotlin.math.max
 import kotlin.random.Random
 
-@Suppress("NON_EXHAUSTIVE_WHEN")  // Many when uses in here are much clearer this way
 class QuestManager : IsPartOfGameInfoSerialization {
 
     companion object {
@@ -81,10 +85,8 @@ class QuestManager : IsPartOfGameInfoSerialization {
     /** Returns the influence multiplier for [donor] from a Investment quest that [civInfo] might have (assumes only one) */
     fun getInvestmentMultiplier(donor: String): Float {
         val investmentQuest = assignedQuests.firstOrNull { it.questName == QuestName.Invest.value && it.assignee == donor }
-        return if (investmentQuest == null)
+            ?: return 1f
-            1f
+        return investmentQuest.data1.toPercent()
-        else
-            investmentQuest.data1.toPercent()
     }
 
     fun clone(): QuestManager {
@@ -311,12 +313,14 @@ class QuestManager : IsPartOfGameInfoSerialization {
 
             var data1 = ""
             var data2 = ""
+            var notificationActions: List<NotificationAction> = listOf(DiplomacyAction(civInfo.civName))
 
             when (quest.name) {
                 QuestName.ClearBarbarianCamp.value -> {
                     val camp = getBarbarianEncampmentForQuest()!!
                     data1 = camp.position.x.toInt().toString()
                     data2 = camp.position.y.toInt().toString()
+                    notificationActions = listOf(LocationAction(camp.position), notificationActions.first())
                 }
                 QuestName.ConnectResource.value -> data1 = getResourceForQuest(assignee)!!.name
                 QuestName.ConstructWonder.value -> data1 = getWonderToBuildForQuest(assignee)!!.name
@@ -328,8 +332,10 @@ class QuestManager : IsPartOfGameInfoSerialization {
                 QuestName.PledgeToProtect.value -> data1 = getMostRecentBully()!!
                 QuestName.GiveGold.value -> data1 = getMostRecentBully()!!
                 QuestName.DenounceCiv.value -> data1 = getMostRecentBully()!!
-                QuestName.SpreadReligion.value -> { data1 = playerReligion!!.getReligionDisplayName() // For display
+                QuestName.SpreadReligion.value -> {
-                                                    data2 = playerReligion.name } // To check completion
+                    data1 = playerReligion!!.getReligionDisplayName() // For display
+                    data2 = playerReligion.name // To check completion
+                }
                 QuestName.ContestCulture.value -> data1 = assignee.totalCultureForContests.toString()
                 QuestName.ContestFaith.value -> data1 = assignee.totalFaithForContests.toString()
                 QuestName.ContestTech.value -> data1 = assignee.tech.getNumberOfTechsResearched().toString()
@@ -348,7 +354,7 @@ class QuestManager : IsPartOfGameInfoSerialization {
 
             assignedQuests.add(newQuest)
             assignee.addNotification("[${civInfo.civName}] assigned you a new quest: [${quest.name}].",
-                    DiplomacyAction(civInfo.civName),
+                notificationActions,
                 NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
 
             if (quest.isIndividual())
@@ -553,20 +559,23 @@ class QuestManager : IsPartOfGameInfoSerialization {
         val unitsToKill = max(3, totalMilitaryUnits / 4)
         unitsToKillForCiv[attacker.civName] = unitsToKill
 
-
-        val location = if (civInfo.cities.isEmpty() || civInfo.getCapital() == null) null
-            else civInfo.getCapital()!!.location
-
         // Ask for assistance
-        for (thirdCiv in civInfo.getKnownCivs().filter { it.isAlive() && !it.isAtWarWith(civInfo) && it.isMajorCiv() }) {
+        val location = civInfo.getCapital(firstCityIfNoCapital = true)?.location
-            if (location != null)
+        for (thirdCiv in civInfo.getKnownCivs()) {
-                thirdCiv.addNotification("[${civInfo.civName}] is being attacked by [${attacker.civName}]! Kill [$unitsToKill] of the attacker's military units and they will be immensely grateful.",
+            if (!thirdCiv.isMajorCiv() || thirdCiv.isDefeated() || thirdCiv.isAtWarWith(civInfo))
-                    location, NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
+                continue
-            else thirdCiv.addNotification("[${civInfo.civName}] is being attacked by [${attacker.civName}]! Kill [$unitsToKill] of the attacker's military units and they will be immensely grateful.",
+            notifyAskForAssistance(thirdCiv, attacker.civName, unitsToKill, location)
-                NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
         }
     }
 
+    private fun notifyAskForAssistance(assignee: Civilization, attackerName: String, unitsToKill: Int, location: Vector2?) {
+        if (attackerName == assignee.civName) return  // No "Hey Bob help us against Bob"
+        val message = "[${civInfo.civName}] is being attacked by [$attackerName]!" +
+            "Kill [$unitsToKill] of the attacker's military units and they will be immensely grateful."
+        // Note: that LocationAction pseudo-constructor is able to filter out null location(s), no need for `if`
+        assignee.addNotification(message, LocationAction(location), NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
+    }
+
     /** Gets notified when [killed]'s military unit was killed by [killer], for war with major pseudo-quest */
     fun militaryUnitKilledBy(killer: Civilization, killed: Civilization) {
         if (!warWithMajorActive(killed)) return
@@ -593,16 +602,10 @@ class QuestManager : IsPartOfGameInfoSerialization {
 
     /** Called when a major civ meets the city-state for the first time. Mainly for war with major pseudo-quest. */
     fun justMet(otherCiv: Civilization) {
-        val location = if (civInfo.cities.isEmpty() || civInfo.getCapital() == null) null
+        if (unitsToKillForCiv.isEmpty()) return
-            else civInfo.getCapital()!!.location
+        val location = civInfo.getCapital(firstCityIfNoCapital = true)?.location
-
+        for ((attackerName, unitsToKill) in unitsToKillForCiv)
-        for ((attackerName, unitsToKill) in unitsToKillForCiv) {
+            notifyAskForAssistance(otherCiv, attackerName, unitsToKill, location)
-            if (location != null)
-                otherCiv.addNotification("[${civInfo.civName}] is being attacked by [$attackerName]! Kill [$unitsToKill] of the attacker's military units and they will be immensely grateful.",
-                    location, NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
-            else otherCiv.addNotification("[${civInfo.civName}] is being attacked by [$attackerName]! Kill [$unitsToKill] of the attacker's military units and they will be immensely grateful.",
-                NotificationCategory.Diplomacy, civInfo.civName, "OtherIcons/Quest")
-        }
     }
 
     /** Ends War with Major pseudo-quests that aren't relevant any longer */

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

@@ -1,6 +1,7 @@
 package com.unciv.models.ruleset
 
 import com.unciv.models.stats.INamed
+import com.unciv.logic.civilization.Civilization // for Kdoc
 
 enum class QuestName(val value: String) {
     Route("Route"),
@@ -29,6 +30,10 @@ enum class QuestType {
 }
 
 /** [Quest] class holds all functionality relative to a quest */
+// Notes: This is **not** `IsPartOfGameInfoSerialization`, only Ruleset.
+// Saves contain [QuestManager]s instead, which contain lists of [AssignedQuest] instances.
+// These are matched to this Quest **by name**.
+// Note [name] must match one of the [QuestName] _values_ above for the Quest to have any functionality.
 class Quest : INamed {
 
     /** Unique identifier name of the quest, it is also shown */
@@ -46,7 +51,7 @@ class Quest : INamed {
     /** Maximum number of turns to complete the quest, 0 if there's no turn limit */
     var duration: Int = 0
 
-    /** Minimum number of [CivInfo] needed to start the quest. It is meaningful only for [QuestType.Global]
+    /** Minimum number of [Civilization]s needed to start the quest. It is meaningful only for [QuestType.Global]
      *  quests [type]. */
     var minimumCivs: Int = 1
 
@@ -55,7 +60,7 @@ class Quest : INamed {
      * Both are mapped here as 'how much to multiply the weight of this quest for this kind of city-state' */
     var weightForCityStateType = HashMap<String, Float>()
 
-    /** Checks if [this] is a Global quest */
+    /** Checks if `this` is a Global quest */
     fun isGlobal(): Boolean = type == QuestType.Global
     fun isIndividual(): Boolean = !isGlobal()
 }

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

@@ -7,6 +7,7 @@ import com.unciv.logic.city.City
 import com.unciv.logic.civilization.CivFlags
 import com.unciv.logic.civilization.Civilization
 import com.unciv.logic.civilization.LocationAction
+import com.unciv.logic.civilization.MapUnitAction
 import com.unciv.logic.civilization.MayaLongCountAction
 import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.NotificationIcon
@@ -65,7 +66,7 @@ object UniqueTriggerActivation {
 
                 val placedUnit = if (city != null || tile == null)
                     civInfo.units.addUnit(unitName, chosenCity) ?: return false
-                    else civInfo.units.placeUnitNearTile(tile!!.position, unitName) ?: return false
+                    else civInfo.units.placeUnitNearTile(tile.position, unitName) ?: return false
 
                 val notificationText = getNotificationText(notification, triggerNotificationText,
                     "Gained [1] [$unitName] unit(s)")
@@ -73,7 +74,7 @@ object UniqueTriggerActivation {
 
                 civInfo.addNotification(
                     notificationText,
-                    placedUnit.getTile().position,
+                    MapUnitAction(placedUnit.getTile().position),
                     NotificationCategory.Units,
                     placedUnit.name
                 )
@@ -97,7 +98,7 @@ object UniqueTriggerActivation {
                 val tilesUnitsWerePlacedOn: MutableList<Vector2> = mutableListOf()
                 repeat(actualAmount) {
                     val placedUnit = if (city != null || tile == null) civInfo.units.addUnit(unitName, chosenCity)
-                        else civInfo.units.placeUnitNearTile(tile!!.position, unitName)
+                        else civInfo.units.placeUnitNearTile(tile.position, unitName)
                     if (placedUnit != null)
                         tilesUnitsWerePlacedOn.add(placedUnit.getTile().position)
                 }
@@ -109,7 +110,7 @@ object UniqueTriggerActivation {
 
                 civInfo.addNotification(
                     notificationText,
-                    LocationAction(tilesUnitsWerePlacedOn),
+                    MapUnitAction(tilesUnitsWerePlacedOn),
                     NotificationCategory.Units,
                     civInfo.getEquivalentUnit(unit).name
                 )
@@ -118,8 +119,11 @@ object UniqueTriggerActivation {
             UniqueType.OneTimeFreeUnitRuins -> {
                 var unit = civInfo.getEquivalentUnit(unique.params[0])
                 if ( unit.hasUnique(UniqueType.FoundCity) && civInfo.isOneCityChallenger()) {
-                     val replacementUnit = ruleSet.units.values.firstOrNull{it.getMatchingUniques(UniqueType.BuildImprovements)
+                     val replacementUnit = ruleSet.units.values
-                            .any { it.params[0] == "Land" }} ?: return false
+                         .firstOrNull {
+                             it.getMatchingUniques(UniqueType.BuildImprovements)
+                                .any { unique -> unique.params[0] == "Land" }
+                         } ?: return false
                     unit = civInfo.getEquivalentUnit(replacementUnit.name)
                 }
 
@@ -134,7 +138,10 @@ object UniqueTriggerActivation {
                         else notification
                     civInfo.addNotification(
                         notificationText,
-                        LocationAction(placedUnit.getTile().position, tile?.position),
+                        sequence {
+                            yield(MapUnitAction(placedUnit.getTile().position))
+                            yieldAll(LocationAction(tile?.position))
+                        },
                         NotificationCategory.Units,
                         placedUnit.name
                     )
@@ -393,7 +400,7 @@ object UniqueTriggerActivation {
                 if (notification != null) {
                     civInfo.addNotification(
                         notification,
-                        LocationAction(promotedUnitLocations),
+                        MapUnitAction(promotedUnitLocations),
                         NotificationCategory.Units,
                         "unitPromotionIcons/${unique.params[1]}"
                     )
@@ -411,10 +418,10 @@ object UniqueTriggerActivation {
              * The very first time after acquiring this policy, the timer is set to half of its normal value
              * This is the basics, and apart from this, there is some randomness in the exact turn count, but I don't know how much
              * There is surprisingly little information findable online about this policy, and the civ 5 source files are
-             *  also quite though to search through, so this might all be incorrect.
+             *  also quite tough to search through, so this might all be incorrect.
              * For now this mechanic seems decent enough that this is fine.
              * Note that the way this is implemented now, this unique does NOT stack
-             * I could parametrize the [Allied], but eh.
+             * I could parametrize the 'Allied' of the Unique text, but eh.
              */
             UniqueType.CityStateCanGiftGreatPeople -> {
                 civInfo.addFlag(
@@ -643,7 +650,7 @@ object UniqueTriggerActivation {
         return false
     }
 
-    fun getNotificationText(notification: String?, triggerNotificationText: String?, effectNotificationText:String):String?{
+    private fun getNotificationText(notification: String?, triggerNotificationText: String?, effectNotificationText: String): String? {
         return if (!notification.isNullOrEmpty()) notification
         else if (triggerNotificationText != null)
         {
@@ -659,7 +666,7 @@ object UniqueTriggerActivation {
         unique: Unique,
         unit: MapUnit,
         notification: String? = null,
-        triggerNotificationText:String? = null
+        triggerNotificationText: String? = null
     ): Boolean {
         when (unique.type) {
             UniqueType.OneTimeUnitHeal -> {

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

@@ -36,6 +36,7 @@ abstract class EmpireOverviewTab (
         val worldScreen = GUI.getWorldScreen()
         worldScreen.notificationsScroll.oneTimeNotification = notification
         UncivGame.Current.resetToWorldScreen()
+        notification.resetExecuteRoundRobin()
         notification.execute(worldScreen)
     }
 }

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

@@ -41,6 +41,8 @@ class EspionageOverviewScreen(val civInfo: Civilization) : PickerScreen(true) {
     private var moveSpyHereButtons = hashMapOf<Button, City?>()
 
     init {
+        spySelectionTable.defaults().pad(10f)
+        citySelectionTable.defaults().pad(5f)
         middlePanes.add(spyScrollPane)
         middlePanes.addSeparatorVertical()
         middlePanes.add(cityScrollPane)
@@ -64,12 +66,12 @@ class EspionageOverviewScreen(val civInfo: Civilization) : PickerScreen(true) {
 
     private fun updateSpyList() {
         spySelectionTable.clear()
-        spySelectionTable.add("Spy".toLabel()).pad(10f)
+        spySelectionTable.add("Spy".toLabel())
-        spySelectionTable.add("Location".toLabel()).pad(10f)
+        spySelectionTable.add("Location".toLabel())
-        spySelectionTable.add("Action".toLabel()).pad(10f).row()
+        spySelectionTable.add("Action".toLabel()).row()
         for (spy in civInfo.espionageManager.spyList) {
-            spySelectionTable.add(spy.name.toLabel()).pad(10f)
+            spySelectionTable.add(spy.name.toLabel())
-            spySelectionTable.add(spy.getLocationName().toLabel()).pad(10f)
+            spySelectionTable.add(spy.getLocationName().toLabel())
             val actionString =
                 when (spy.action) {
                     SpyAction.None, SpyAction.StealingTech, SpyAction.Surveillance -> spy.action.displayString
@@ -77,7 +79,7 @@ class EspionageOverviewScreen(val civInfo: Civilization) : PickerScreen(true) {
                     SpyAction.RiggingElections -> TODO()
                     SpyAction.CounterIntelligence -> TODO()
                 }
-            spySelectionTable.add(actionString.toLabel()).pad(10f)
+            spySelectionTable.add(actionString.toLabel())
 
             val moveSpyButton = "Move".toTextButton()
             moveSpyButton.onClick {
@@ -89,36 +91,33 @@ class EspionageOverviewScreen(val civInfo: Civilization) : PickerScreen(true) {
                 selectedSpyButton = moveSpyButton
                 selectedSpy = spy
                 selectedSpyButton!!.label.setText("Cancel".tr())
-                for ((button, city) in moveSpyHereButtons)
+                for ((button, city) in moveSpyHereButtons) {
-                // For now, only allow spies to be send to cities of other major civs and their hideout
+                    // For now, only allow spies to be sent to cities of other major civs and their hideout
-                // Not own cities as counterintelligence isn't implemented
+                    // Not own cities as counterintelligence isn't implemented
-                // Not city-state civs as rigging elections isn't implemented
+                    // Not city-state civs as rigging elections isn't implemented
-                // Technically, stealing techs from other civs also isn't implemented, but its the first thing I'll add so this makes the most sense to allow.
+                    button.isVisible = city == null // hideout
-                    if (city == null // hideout
                         || (city.civ.isMajorCiv()
                             && city.civ != civInfo
                             && !city.espionage.hasSpyOf(civInfo)
                         )
-                    ) {
+                }
-                        button.isVisible = true
-                    }
             }
-            spySelectionTable.add(moveSpyButton).pad(5f).row()
+            spySelectionTable.add(moveSpyButton).pad(5f, 10f, 5f, 20f).row()
         }
     }
 
     private fun updateCityList() {
         citySelectionTable.clear()
         moveSpyHereButtons.clear()
-        citySelectionTable.add().pad(5f)
+        citySelectionTable.add()
-        citySelectionTable.add("City".toLabel()).pad(5f)
+        citySelectionTable.add("City".toLabel())
-        citySelectionTable.add("Spy present".toLabel()).pad(5f).row()
+        citySelectionTable.add("Spy present".toLabel()).row()
 
         // First add the hideout to the table
 
-        citySelectionTable.add().pad(5f)
+        citySelectionTable.add()
-        citySelectionTable.add("Spy Hideout".toLabel()).pad(5f)
+        citySelectionTable.add("Spy Hideout".toLabel())
-        citySelectionTable.add().pad(5f)
+        citySelectionTable.add()
         val moveSpyHereButton = getMoveToCityButton(null)
         citySelectionTable.add(moveSpyHereButton).row()
 
@@ -143,21 +142,22 @@ class EspionageOverviewScreen(val civInfo: Civilization) : PickerScreen(true) {
     }
 
     private fun addCityToSelectionTable(city: City) {
-        citySelectionTable.add(ImageGetter.getNationPortrait(city.civ.nation, 30f)).pad(5f)
+        citySelectionTable.add(ImageGetter.getNationPortrait(city.civ.nation, 30f))
-        citySelectionTable.add(city.name.toLabel()).pad(5f)
+            .padLeft(20f)
+        citySelectionTable.add(city.name.toLabel(hideIcons = true))
         if (city.espionage.hasSpyOf(civInfo)) {
             citySelectionTable.add(
                 ImageGetter.getImage("OtherIcons/Spy_White").apply {
                     setSize(30f)
                     color = Color.WHITE
                 }
-            ).pad(5f)
+            )
         } else {
-            citySelectionTable.add().pad(5f)
+            citySelectionTable.add()
         }
 
         val moveSpyHereButton = getMoveToCityButton(city)
-        citySelectionTable.add(moveSpyHereButton).pad(5f)
+        citySelectionTable.add(moveSpyHereButton)
         citySelectionTable.row()
     }
 

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

@@ -99,7 +99,7 @@ class ResourcesOverviewTab(
                 gameInfo.getExploredResourcesNotification(viewingPlayer, name)
             ) }
         }
-    private fun TileResource.getLabel() = name.toLabel().apply {
+    private fun TileResource.getLabel() = name.toLabel(hideIcons = true).apply {
         onClick {
             overviewScreen.game.pushScreen(CivilopediaScreen(gameInfo.ruleset, CivilopediaCategories.Resource, this@getLabel.name))
         }

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

@@ -126,8 +126,8 @@ class AlertPopup(
             player.getDiplomacyManager(bullyOrAttacker).sideWithCityState()
         }.row()
         addCloseButton("Very well.", KeyboardBinding.Cancel) {
-            val capitalLocation = LocationAction(cityState.cities.asSequence().map { it.location }) // in practice 0 or 1 entries, that's OK
+            player.addNotification("You have broken your Pledge to Protect [${cityState.civName}]!",
-            player.addNotification("You have broken your Pledge to Protect [${cityState.civName}]!", capitalLocation, NotificationCategory.Diplomacy, cityState.civName)
+                cityState.cityStateFunctions.getNotificationActions(), NotificationCategory.Diplomacy, cityState.civName)
             cityState.cityStateFunctions.removeProtectorCiv(player, forced = true)
         }.row()
     }