From d3d8933bbfc24d360aac66104a958bccf2496745 Mon Sep 17 00:00:00 2001
From: Timo T <azzurite@gmail.com>
Date: Mon, 27 Jan 2020 07:32:16 +0100
Subject: [PATCH] Fix popups (#1784)

* Fix community popup not opening

Fixes a regression in b95844d2f4601f726eedac93df3c12d7394f5793. This commit refactored popups and it was thought that the "screen has popup -> don't show popup" was correct for all popups. That assumption was incorrect, the community popup was not opening anymore as well as the game menu popups (editor and normal game) could not be opened over other popups anymore.

This commit fixes that by introducing a queue for popups. When you try to open a popup and one is already open, the popup you tried to open only gets shown when the popup that was already open is closed. This can be manually overridden with a calling the `open` method with a `(force = true)` argument.

Also, all popups are now and should be opened and closed only with their `open()` and `close()` methods to ensure this behavior works.

* Refactor: Remove all open() methods from popup constructors

While it may be a little less to type, it should be up to the caller to decide to open a popup over other popups (via the `force = true` parameter) or not. This is not possible if a popup is opened automatically within its constructor, which is why that is the wrong place to open the popup-
---
 .../com/unciv/ui/cityscreen/CityInfoTable.kt  |  2 +-
 .../unciv/ui/cityscreen/ConstructionsTable.kt |  3 +-
 .../com/unciv/ui/mapeditor/LoadMapScreen.kt   |  5 +-
 .../unciv/ui/mapeditor/MapDownloadPopup.kt    |  4 +-
 .../unciv/ui/mapeditor/MapEditorMenuPopup.kt  |  4 +-
 .../com/unciv/ui/mapeditor/MapEditorScreen.kt |  5 +-
 .../src/com/unciv/ui/trade/DiplomacyScreen.kt |  4 +-
 .../unciv/ui/utils/CameraStageBaseScreen.kt   | 20 +------
 core/src/com/unciv/ui/utils/Popup.kt          | 23 ++++++--
 core/src/com/unciv/ui/utils/YesNoPopup.kt     |  1 -
 .../com/unciv/ui/worldscreen/AlertPopup.kt    |  1 -
 .../com/unciv/ui/worldscreen/TradePopup.kt    | 18 +++----
 .../com/unciv/ui/worldscreen/WorldScreen.kt   |  4 +-
 .../unciv/ui/worldscreen/WorldScreenTopBar.kt |  4 +-
 .../mainmenu/WorldScreenMenuPopup.kt          | 24 ++++-----
 .../mainmenu/WorldScreenOptionsPopup.kt       | 52 ++++++++++---------
 .../unciv/ui/worldscreen/unit/UnitActions.kt  |  4 +-
 17 files changed, 86 insertions(+), 92 deletions(-)

diff --git a/core/src/com/unciv/ui/cityscreen/CityInfoTable.kt b/core/src/com/unciv/ui/cityscreen/CityInfoTable.kt
index 5cfd63c7..42cd8422 100644
--- a/core/src/com/unciv/ui/cityscreen/CityInfoTable.kt
+++ b/core/src/com/unciv/ui/cityscreen/CityInfoTable.kt
@@ -95,7 +95,7 @@ class CityInfoTable(private val cityScreen: CityScreen) : Table(CameraStageBaseS
                                     cityScreen.city.sellBuilding(building.name)
                                     cityScreen.city.cityStats.update()
                                     cityScreen.update()
-                                }, cityScreen)
+                                }, cityScreen).open()
                     }
                     if (cityScreen.city.hasSoldBuildingThisTurn || cityScreen.city.isPuppet
                             || !UncivGame.Current.worldScreen.isPlayersTurn)
diff --git a/core/src/com/unciv/ui/cityscreen/ConstructionsTable.kt b/core/src/com/unciv/ui/cityscreen/ConstructionsTable.kt
index 49fa99f9..142421b0 100644
--- a/core/src/com/unciv/ui/cityscreen/ConstructionsTable.kt
+++ b/core/src/com/unciv/ui/cityscreen/ConstructionsTable.kt
@@ -14,7 +14,6 @@ import com.unciv.models.UncivSound
 import com.unciv.models.stats.Stat
 import com.unciv.models.translations.tr
 import com.unciv.ui.utils.*
-import com.unciv.ui.utils.YesNoPopup
 
 class ConstructionsTable(val cityScreen: CityScreen) : Table(CameraStageBaseScreen.skin) {
     /* -2 = Nothing, -1 = current construction, >= 0 queue entry */
@@ -294,7 +293,7 @@ class ConstructionsTable(val cityScreen: CityScreen) : Table(CameraStageBaseScre
                     }
                     if (!construction.shouldBeDisplayed(cityConstructions)) cityScreen.selectedConstruction = null
                     cityScreen.update()
-                }, cityScreen)
+                }, cityScreen).open()
             }
 
             if (constructionGoldCost > city.civInfo.gold)
diff --git a/core/src/com/unciv/ui/mapeditor/LoadMapScreen.kt b/core/src/com/unciv/ui/mapeditor/LoadMapScreen.kt
index 231c0bb6..6daa7700 100644
--- a/core/src/com/unciv/ui/mapeditor/LoadMapScreen.kt
+++ b/core/src/com/unciv/ui/mapeditor/LoadMapScreen.kt
@@ -12,7 +12,6 @@ import com.unciv.models.translations.tr
 import com.unciv.ui.pickerscreens.PickerScreen
 import com.unciv.ui.saves.Gzip
 import com.unciv.ui.utils.*
-import com.unciv.ui.utils.YesNoPopup
 
 class LoadMapScreen(previousMap: TileMap?) : PickerScreen(){
     var chosenMap = ""
@@ -43,7 +42,7 @@ class LoadMapScreen(previousMap: TileMap?) : PickerScreen(){
 
         val downloadMapButton = TextButton("Download map".tr(), skin)
         downloadMapButton.onClick {
-            MapDownloadPopup(this)
+            MapDownloadPopup(this).open()
         }
         rightSideTable.add(downloadMapButton).row()
 
@@ -70,7 +69,7 @@ class LoadMapScreen(previousMap: TileMap?) : PickerScreen(){
             YesNoPopup("Are you sure you want to delete this map?", {
                 MapSaver().deleteMap(chosenMap)
                 UncivGame.Current.setScreen(LoadMapScreen(previousMap))
-            }, this)
+            }, this).open()
         }
         deleteMapButton.disable()
         deleteMapButton.color = Color.RED
diff --git a/core/src/com/unciv/ui/mapeditor/MapDownloadPopup.kt b/core/src/com/unciv/ui/mapeditor/MapDownloadPopup.kt
index 7ec9bb47..6469f2a8 100644
--- a/core/src/com/unciv/ui/mapeditor/MapDownloadPopup.kt
+++ b/core/src/com/unciv/ui/mapeditor/MapDownloadPopup.kt
@@ -8,9 +8,9 @@ import com.unciv.UncivGame
 import com.unciv.logic.MapSaver
 import com.unciv.ui.saves.Gzip
 import com.unciv.ui.utils.CameraStageBaseScreen
+import com.unciv.ui.utils.Popup
 import com.unciv.ui.utils.onClick
 import com.unciv.ui.worldscreen.mainmenu.DropBox
-import com.unciv.ui.utils.Popup
 import kotlin.concurrent.thread
 
 class MapDownloadPopup(loadMapScreen: LoadMapScreen): Popup(loadMapScreen) {
@@ -39,6 +39,7 @@ class MapDownloadPopup(loadMapScreen: LoadMapScreen): Popup(loadMapScreen) {
                                 val couldNotDownloadMapPopup = Popup(screen)
                                 couldNotDownloadMapPopup.addGoodSizedLabel("Could not download map!").row()
                                 couldNotDownloadMapPopup.addCloseButton()
+                                couldNotDownloadMapPopup.open()
                             }
                         }
                     }
@@ -50,6 +51,5 @@ class MapDownloadPopup(loadMapScreen: LoadMapScreen): Popup(loadMapScreen) {
             addGoodSizedLabel("Could not get list of maps!").row()
         }
         addCloseButton()
-        open()
     }
 }
diff --git a/core/src/com/unciv/ui/mapeditor/MapEditorMenuPopup.kt b/core/src/com/unciv/ui/mapeditor/MapEditorMenuPopup.kt
index ed6ff130..5c5b6ddb 100644
--- a/core/src/com/unciv/ui/mapeditor/MapEditorMenuPopup.kt
+++ b/core/src/com/unciv/ui/mapeditor/MapEditorMenuPopup.kt
@@ -11,9 +11,9 @@ import com.unciv.logic.map.MapType
 import com.unciv.logic.map.RoadStatus
 import com.unciv.models.translations.tr
 import com.unciv.ui.saves.Gzip
+import com.unciv.ui.utils.Popup
 import com.unciv.ui.utils.onClick
 import com.unciv.ui.worldscreen.mainmenu.DropBox
-import com.unciv.ui.utils.Popup
 import kotlin.concurrent.thread
 
 class MapEditorMenuPopup(mapEditorScreen: MapEditorScreen): Popup(mapEditorScreen){
@@ -95,7 +95,5 @@ class MapEditorMenuPopup(mapEditorScreen: MapEditorScreen): Popup(mapEditorScree
         val closeOptionsButton = TextButton("Close".tr(), skin)
         closeOptionsButton.onClick { close() }
         add(closeOptionsButton).row()
-
-        open()
     }
 }
diff --git a/core/src/com/unciv/ui/mapeditor/MapEditorScreen.kt b/core/src/com/unciv/ui/mapeditor/MapEditorScreen.kt
index be09dcb8..d2b969b7 100644
--- a/core/src/com/unciv/ui/mapeditor/MapEditorScreen.kt
+++ b/core/src/com/unciv/ui/mapeditor/MapEditorScreen.kt
@@ -13,6 +13,7 @@ import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.translations.tr
 import com.unciv.ui.utils.CameraStageBaseScreen
 import com.unciv.ui.utils.onClick
+import com.unciv.ui.utils.popups
 import com.unciv.ui.utils.setFontSize
 
 class MapEditorScreen(): CameraStageBaseScreen() {
@@ -76,9 +77,9 @@ class MapEditorScreen(): CameraStageBaseScreen() {
 
         val optionsMenuButton = TextButton("Menu".tr(), skin)
         optionsMenuButton.onClick {
-            if(stage.actors.any { it is MapEditorMenuPopup })
+            if(popups.any { it is MapEditorMenuPopup })
                 return@onClick // already open
-            MapEditorMenuPopup(this)
+            MapEditorMenuPopup(this).open(force = true)
         }
         optionsMenuButton.label.setFontSize(24)
         optionsMenuButton.labelCell.pad(20f)
diff --git a/core/src/com/unciv/ui/trade/DiplomacyScreen.kt b/core/src/com/unciv/ui/trade/DiplomacyScreen.kt
index b3b36143..55b56922 100644
--- a/core/src/com/unciv/ui/trade/DiplomacyScreen.kt
+++ b/core/src/com/unciv/ui/trade/DiplomacyScreen.kt
@@ -158,7 +158,7 @@ class DiplomacyScreen(val viewingCiv:CivilizationInfo):CameraStageBaseScreen() {
                     tradeLogic.currentTrade.theirOffers.add(TradeOffer(Constants.peaceTreaty, TradeType.Treaty, 30))
                     tradeLogic.acceptTrade()
                     updateLeftSideTable()
-                }, this)
+                }, this).open()
             }
             diplomacyTable.add(peaceButton).row()
             if(isNotPlayersTurn()) peaceButton.disable()
@@ -373,7 +373,7 @@ class DiplomacyScreen(val viewingCiv:CivilizationInfo):CameraStageBaseScreen() {
                 diplomacyManager.declareWar()
                 setRightSideFlavorText(otherCiv, otherCiv.getTranslatedNation().attacked, "Very well.")
                 updateLeftSideTable()
-            }, this)
+            }, this).open()
         }
         return declareWarButton
     }
diff --git a/core/src/com/unciv/ui/utils/CameraStageBaseScreen.kt b/core/src/com/unciv/ui/utils/CameraStageBaseScreen.kt
index b86a51cb..4dc25344 100644
--- a/core/src/com/unciv/ui/utils/CameraStageBaseScreen.kt
+++ b/core/src/com/unciv/ui/utils/CameraStageBaseScreen.kt
@@ -7,21 +7,8 @@ import com.badlogic.gdx.graphics.Color
 import com.badlogic.gdx.graphics.GL20
 import com.badlogic.gdx.graphics.g2d.Batch
 import com.badlogic.gdx.graphics.g2d.SpriteBatch
-import com.badlogic.gdx.scenes.scene2d.Actor
-import com.badlogic.gdx.scenes.scene2d.InputEvent
-import com.badlogic.gdx.scenes.scene2d.InputListener
-import com.badlogic.gdx.scenes.scene2d.Stage
-import com.badlogic.gdx.scenes.scene2d.Touchable
-import com.badlogic.gdx.scenes.scene2d.ui.Button
-import com.badlogic.gdx.scenes.scene2d.ui.Cell
-import com.badlogic.gdx.scenes.scene2d.ui.CheckBox
-import com.badlogic.gdx.scenes.scene2d.ui.Image
-import com.badlogic.gdx.scenes.scene2d.ui.Label
-import com.badlogic.gdx.scenes.scene2d.ui.SelectBox
-import com.badlogic.gdx.scenes.scene2d.ui.Skin
-import com.badlogic.gdx.scenes.scene2d.ui.Table
-import com.badlogic.gdx.scenes.scene2d.ui.TextButton
-import com.badlogic.gdx.scenes.scene2d.ui.TextField
+import com.badlogic.gdx.scenes.scene2d.*
+import com.badlogic.gdx.scenes.scene2d.ui.*
 import com.badlogic.gdx.scenes.scene2d.utils.ClickListener
 import com.badlogic.gdx.utils.viewport.ExtendViewport
 import com.unciv.JsonParser
@@ -83,9 +70,6 @@ open class CameraStageBaseScreen : Screen {
         tutorialController.showTutorial(tutorial)
     }
 
-    fun hasOpenPopups(): Boolean =
-            stage.actors.any { it is Popup }
-
     companion object {
         var skin = Skin(Gdx.files.internal("skin/flat-earth-ui.json"))
 
diff --git a/core/src/com/unciv/ui/utils/Popup.kt b/core/src/com/unciv/ui/utils/Popup.kt
index 14b82c78..1dbee4b6 100644
--- a/core/src/com/unciv/ui/utils/Popup.kt
+++ b/core/src/com/unciv/ui/utils/Popup.kt
@@ -18,20 +18,30 @@ open class Popup(val screen: CameraStageBaseScreen): Table(CameraStageBaseScreen
 
         this.pad(20f)
         this.defaults().pad(5f)
+
+        this.isVisible = false
     }
 
     /**
-     * Displays the Popup on the screen. Will not open the popup if another one is already open.
+     * Displays the Popup on the screen. If another popup is already open, this one will display after the other has
+     * closed. Use [force] = true if you want to open this popup above the other one anyway.
      */
-    fun open() {
-        if (screen.hasOpenPopups()) return;
+    fun open(force: Boolean = false) {
+        if (force || !screen.hasOpenPopups()) {
+            this.isVisible = true
+        }
+
+        screen.stage.addActor(this)
         pack()
         center(screen.stage)
-        screen.stage.addActor(this)
+
     }
 
     open fun close() {
         remove()
+        if (screen.popups.isNotEmpty()) {
+            screen.popups[0].isVisible = true;
+        }
     }
 
     fun addGoodSizedLabel(text: String, size:Int=18): Cell<Label> {
@@ -58,3 +68,8 @@ open class Popup(val screen: CameraStageBaseScreen): Table(CameraStageBaseScreen
     fun addCloseButton() = addButton("Close") { close() }
 }
 
+fun CameraStageBaseScreen.hasOpenPopups(): Boolean = stage.actors.any { it is Popup && it.isVisible }
+
+val CameraStageBaseScreen.popups: List<Popup>
+    get() = stage.actors.filter{ it is Popup }.map{ it as Popup }
+
diff --git a/core/src/com/unciv/ui/utils/YesNoPopup.kt b/core/src/com/unciv/ui/utils/YesNoPopup.kt
index ca285726..02a33de4 100644
--- a/core/src/com/unciv/ui/utils/YesNoPopup.kt
+++ b/core/src/com/unciv/ui/utils/YesNoPopup.kt
@@ -10,6 +10,5 @@ class YesNoPopup(question:String, action:()->Unit,
         add(question.toLabel()).colspan(2).row()
         add(TextButton("No".tr(), skin).onClick { close(); restoredefault() })
         add(TextButton("Yes".tr(), skin).onClick { close(); action() })
-        open()
     }
 }
diff --git a/core/src/com/unciv/ui/worldscreen/AlertPopup.kt b/core/src/com/unciv/ui/worldscreen/AlertPopup.kt
index 4f8d7c1b..026da480 100644
--- a/core/src/com/unciv/ui/worldscreen/AlertPopup.kt
+++ b/core/src/com/unciv/ui/worldscreen/AlertPopup.kt
@@ -178,7 +178,6 @@ class AlertPopup(val worldScreen: WorldScreen, val popupAlert: PopupAlert): Popu
                 add(getCloseButton("Close"))
             }
         }
-        open()
     }
 
     override fun close(){
diff --git a/core/src/com/unciv/ui/worldscreen/TradePopup.kt b/core/src/com/unciv/ui/worldscreen/TradePopup.kt
index 0d3bd100..0513263b 100644
--- a/core/src/com/unciv/ui/worldscreen/TradePopup.kt
+++ b/core/src/com/unciv/ui/worldscreen/TradePopup.kt
@@ -9,17 +9,17 @@ import com.unciv.logic.trade.TradeLogic
 import com.unciv.logic.trade.TradeType
 import com.unciv.models.translations.tr
 import com.unciv.ui.trade.DiplomacyScreen
+import com.unciv.ui.utils.Popup
 import com.unciv.ui.utils.addSeparator
 import com.unciv.ui.utils.toLabel
-import com.unciv.ui.utils.Popup
 import kotlin.math.max
 import kotlin.math.min
 
 class TradePopup(worldScreen: WorldScreen): Popup(worldScreen){
-    init{
-        val viewingCiv = worldScreen.viewingCiv
-        val tradeRequest = viewingCiv.tradeRequests.first()
+    val viewingCiv = worldScreen.viewingCiv
+    val tradeRequest = viewingCiv.tradeRequests.first()
 
+    init{
         val requestingCiv = worldScreen.gameInfo.getCivilization(tradeRequest.requestingCiv)
         val translatedNation = requestingCiv.getTranslatedNation()
         val otherCivLeaderName = "[${translatedNation.leaderName}] of [${translatedNation.getNameTranslation()}]".tr()
@@ -50,7 +50,6 @@ class TradePopup(worldScreen: WorldScreen): Popup(worldScreen){
             val tradeLogic = TradeLogic(viewingCiv, requestingCiv)
             tradeLogic.currentTrade.set(trade)
             tradeLogic.acceptTrade()
-            viewingCiv.tradeRequests.remove(tradeRequest)
             close()
             Popup(worldScreen).apply {
                 add(otherCivLeaderName.toLabel()).colspan(2)
@@ -67,8 +66,6 @@ class TradePopup(worldScreen: WorldScreen): Popup(worldScreen){
             requestingCiv.addNotification("[${viewingCiv.civName}] has accepted your trade request", Color.GOLD)
         }
         addButton("Not this time.".tr()){
-            viewingCiv.tradeRequests.remove(tradeRequest)
-
             val diplomacyManager = requestingCiv.getDiplomacyManager(viewingCiv)
             if(trade.ourOffers.all { it.type==TradeType.Luxury_Resource } && trade.theirOffers.all { it.type==TradeType.Luxury_Resource })
                 diplomacyManager.setFlag(DiplomacyFlags.DeclinedLuxExchange,20) // offer again in 20 turns
@@ -82,7 +79,6 @@ class TradePopup(worldScreen: WorldScreen): Popup(worldScreen){
             worldScreen.shouldUpdate=true
         }
         addButton("How about something else...".tr()){
-            viewingCiv.tradeRequests.remove(tradeRequest)
             close()
 
             val diplomacyScreen= DiplomacyScreen(viewingCiv)
@@ -92,6 +88,10 @@ class TradePopup(worldScreen: WorldScreen): Popup(worldScreen){
             worldScreen.game.setScreen(diplomacyScreen)
             worldScreen.shouldUpdate=true
         }
-        open()
+    }
+
+    override fun close() {
+        viewingCiv.tradeRequests.remove(tradeRequest)
+        super.close()
     }
 }
diff --git a/core/src/com/unciv/ui/worldscreen/WorldScreen.kt b/core/src/com/unciv/ui/worldscreen/WorldScreen.kt
index 5a398b6e..7fd7491a 100644
--- a/core/src/com/unciv/ui/worldscreen/WorldScreen.kt
+++ b/core/src/com/unciv/ui/worldscreen/WorldScreen.kt
@@ -209,8 +209,8 @@ class WorldScreen(val viewingCiv:CivilizationInfo) : CameraStageBaseScreen() {
                 !gameInfo.oneMoreTurnMode && gameInfo.civilizations.any { it.victoryManager.hasWon() } -> game.setScreen(VictoryScreen())
                 viewingCiv.policies.freePolicies > 0 && viewingCiv.policies.canAdoptPolicy() -> game.setScreen(PolicyPickerScreen(this))
                 viewingCiv.greatPeople.freeGreatPeople > 0 -> game.setScreen(GreatPersonPickerScreen(viewingCiv))
-                viewingCiv.popupAlerts.any() -> AlertPopup(this, viewingCiv.popupAlerts.first())
-                viewingCiv.tradeRequests.isNotEmpty() -> TradePopup(this)
+                viewingCiv.popupAlerts.any() -> AlertPopup(this, viewingCiv.popupAlerts.first()).open()
+                viewingCiv.tradeRequests.isNotEmpty() -> TradePopup(this).open()
             }
         }
         updateNextTurnButton(hasOpenPopups()) // This must be before the notifications update, since its position is based on it
diff --git a/core/src/com/unciv/ui/worldscreen/WorldScreenTopBar.kt b/core/src/com/unciv/ui/worldscreen/WorldScreenTopBar.kt
index da9c3a1e..341f5f7f 100644
--- a/core/src/com/unciv/ui/worldscreen/WorldScreenTopBar.kt
+++ b/core/src/com/unciv/ui/worldscreen/WorldScreenTopBar.kt
@@ -95,8 +95,8 @@ class WorldScreenTopBar(val worldScreen: WorldScreen) : Table() {
                 .apply { setSize(50f, 50f) }
         menuButton.color = Color.WHITE
         menuButton.onClick {
-            if(worldScreen.stage.actors.none { it is WorldScreenMenuPopup })
-                WorldScreenMenuPopup(worldScreen)
+            if(worldScreen.popups.none { it is WorldScreenMenuPopup })
+                WorldScreenMenuPopup(worldScreen).open(force = true)
         }
         menuButton.centerY(this)
         menuButton.x = menuButton.y
diff --git a/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenMenuPopup.kt b/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenMenuPopup.kt
index e24819ad..42855456 100644
--- a/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenMenuPopup.kt
+++ b/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenMenuPopup.kt
@@ -23,25 +23,25 @@ class WorldScreenMenuPopup(val worldScreen: WorldScreen) : Popup(worldScreen) {
         val height = 30f
         addSquareButton("Map editor".tr()){
             openMapEditorPopup()
-            remove()
+            close()
         }.size(width,height)
         addSeparator()
 
         addSquareButton("Civilopedia".tr()){
             UncivGame.Current.setScreen(CivilopediaScreen(worldScreen.gameInfo.ruleSet))
-            remove()
+            close()
         }.size(width,height)
         addSeparator()
 
         addSquareButton("Load game".tr()){
             UncivGame.Current.setScreen(LoadGameScreen())
-            remove()
+            close()
         }.size(width,height)
         addSeparator()
 
         addSquareButton("Save game".tr()) {
             UncivGame.Current.setScreen(SaveGameScreen())
-            remove()
+            close()
         }.size(width,height)
         addSeparator()
 
@@ -56,22 +56,20 @@ class WorldScreenMenuPopup(val worldScreen: WorldScreen) : Popup(worldScreen) {
         addSeparator()
 
         addSquareButton("Options".tr()){
-            UncivGame.Current.worldScreen.stage.addActor(WorldScreenOptionsPopup(worldScreen))
-            remove()
+            WorldScreenOptionsPopup(worldScreen).open()
+            close()
         }.size(width,height)
         addSeparator()
 
         addSquareButton("Community"){
-            WorldScreenCommunityPopup(worldScreen)
-            remove()
+            WorldScreenCommunityPopup(worldScreen).open()
+            close()
         }.size(width,height)
         addSeparator()
 
         addSquareButton("Close"){
             close()
         }.size(width,height)
-
-        open()
     }
 
 
@@ -159,16 +157,14 @@ class WorldScreenCommunityPopup(val worldScreen: WorldScreen) : Popup(worldScree
     init{
         addButton("Discord"){
             Gdx.net.openURI("https://discord.gg/bjrB4Xw")
-            remove()
+            close()
         }
 
         addButton("Github"){
             Gdx.net.openURI("https://github.com/yairm210/UnCiv")
-            remove()
+            close()
         }
 
         addCloseButton()
-
-        open()
     }
 }
diff --git a/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenOptionsPopup.kt b/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenOptionsPopup.kt
index 69580009..39a1b4fc 100644
--- a/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenOptionsPopup.kt
+++ b/core/src/com/unciv/ui/worldscreen/mainmenu/WorldScreenOptionsPopup.kt
@@ -26,7 +26,6 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
     init {
         UncivGame.Current.settings.addCompletedTutorialTask("Open the options table")
         update()
-        open()
     }
 
 
@@ -35,44 +34,43 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         settings.save()
         clear()
 
-        val innerTable = Popup(screen) // cheating, to get the old code to fit inside a Scroll =)
-        innerTable.background = null
+        val innerTable = Table(CameraStageBaseScreen.skin)
 
         innerTable.add("Display options".toLabel(fontSize = 24)).colspan(2).row()
 
         innerTable.add("Show worked tiles".toLabel())
-        innerTable.addButton(if (settings.showWorkedTiles) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showWorkedTiles) "Yes".tr() else "No".tr()) {
             settings.showWorkedTiles= !settings.showWorkedTiles
             update()
         }
 
         innerTable.add("Show resources and improvements".toLabel())
-        innerTable.addButton(if (settings.showResourcesAndImprovements) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showResourcesAndImprovements) "Yes".tr() else "No".tr()) {
             settings.showResourcesAndImprovements = !settings.showResourcesAndImprovements
             update()
         }
 
 
         innerTable.add("Show tutorials".toLabel())
-        innerTable.addButton(if (settings.showTutorials) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showTutorials) "Yes".tr() else "No".tr()) {
             settings.showTutorials = !settings.showTutorials
             update()
         }
 
         innerTable.add("Show minimap".toLabel())
-        innerTable.addButton(if (settings.showMinimap) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showMinimap) "Yes".tr() else "No".tr()) {
             settings.showMinimap = !settings.showMinimap
             update()
         }
 
         innerTable.add("Show pixel units".toLabel())
-        innerTable.addButton(if (settings.showPixelUnits) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showPixelUnits) "Yes".tr() else "No".tr()) {
             settings.showPixelUnits = !settings.showPixelUnits
             update()
         }
 
         innerTable.add("Show pixel improvements".toLabel())
-        innerTable.addButton(if (settings.showPixelImprovements) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.showPixelImprovements) "Yes".tr() else "No".tr()) {
             settings.showPixelImprovements = !settings.showPixelImprovements
             update()
         }
@@ -85,7 +83,7 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
 
         // Do not add to template.properties yet please.
         innerTable.add("Continuous rendering\n(HIGHLY EXPERIMENTAL)".toLabel())
-        innerTable.addButton(if (settings.continuousRendering) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.continuousRendering) "Yes".tr() else "No".tr()) {
             settings.continuousRendering = !settings.continuousRendering
             Gdx.graphics.isContinuousRendering = settings.continuousRendering
             update()
@@ -95,32 +93,32 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
 
 
         innerTable.add("Check for idle units".toLabel())
-        innerTable.addButton(if (settings.checkForDueUnits) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.checkForDueUnits) "Yes".tr() else "No".tr()) {
             settings.checkForDueUnits = !settings.checkForDueUnits
             update()
         }
 
         innerTable.add("Move units with a single tap".toLabel())
-        innerTable.addButton(if (settings.singleTapMove) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.singleTapMove) "Yes".tr() else "No".tr()) {
             settings.singleTapMove = !settings.singleTapMove
             update()
         }
 
         innerTable.add("Auto-assign city production".toLabel())
-        innerTable.addButton(if (settings.autoAssignCityProduction) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.autoAssignCityProduction) "Yes".tr() else "No".tr()) {
             settings.autoAssignCityProduction = !settings.autoAssignCityProduction
             update()
         }
 
         innerTable.add("Auto-build roads".toLabel())
-        innerTable.addButton(if (settings.autoBuildingRoads) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.autoBuildingRoads) "Yes".tr() else "No".tr()) {
             settings.autoBuildingRoads = !settings.autoBuildingRoads
             update()
         }
 
 
         innerTable.add("Enable nuclear weapons".toLabel())
-        innerTable.addButton(if (settings.nuclearWeaponEnabled) "Yes".tr() else "No".tr()) {
+        addButton(innerTable, if (settings.nuclearWeaponEnabled) "Yes".tr() else "No".tr()) {
             settings.nuclearWeaponEnabled = !settings.nuclearWeaponEnabled
             update()
         }
@@ -150,8 +148,14 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         UncivGame.Current.worldScreen.shouldUpdate = true
     }
 
+    private fun addButton(table: Table, text: String, action: () -> Unit): Cell<TextButton> {
+        val button = TextButton(text.tr(), skin).apply { color = ImageGetter.getBlue() }
+        button.onClick(action)
+        return table.add(button).apply { row() }
+    }
 
-    private fun addSoundEffectsVolumeSlider(innerTable: Popup) {
+
+    private fun addSoundEffectsVolumeSlider(innerTable: Table) {
         innerTable.add("Sound effects volume".tr())
 
         val soundEffectsVolumeSlider = Slider(0f, 1.0f, 0.1f, false, skin)
@@ -166,7 +170,7 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         innerTable.add(soundEffectsVolumeSlider).row()
     }
 
-    private fun addMusicVolumeSlider(innerTable: Popup) {
+    private fun addMusicVolumeSlider(innerTable: Table) {
         val musicLocation =Gdx.files.local(UncivGame.Current.musicLocation)
         if(musicLocation.exists()) {
             innerTable.add("Music volume".tr())
@@ -208,7 +212,7 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         }
     }
 
-    private fun addResolutionSelectBox(innerTable: Popup) {
+    private fun addResolutionSelectBox(innerTable: Table) {
         innerTable.add("Resolution".toLabel())
 
         val resolutionSelectBox = SelectBox<String>(skin)
@@ -224,12 +228,12 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
                 UncivGame.Current.settings.save()
                 UncivGame.Current.worldScreen = WorldScreen(worldScreen.viewingCiv)
                 UncivGame.Current.setWorldScreen()
-                WorldScreenOptionsPopup(UncivGame.Current.worldScreen)
+                WorldScreenOptionsPopup(UncivGame.Current.worldScreen).open()
             }
         })
     }
 
-    private fun addTileSetSelectBox(innerTable: Popup) {
+    private fun addTileSetSelectBox(innerTable: Table) {
         innerTable.add("Tileset".toLabel())
 
         val tileSetSelectBox = SelectBox<String>(skin)
@@ -247,12 +251,12 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
                 UncivGame.Current.settings.save()
                 UncivGame.Current.worldScreen = WorldScreen(worldScreen.viewingCiv)
                 UncivGame.Current.setWorldScreen()
-                WorldScreenOptionsPopup(UncivGame.Current.worldScreen)
+                WorldScreenOptionsPopup(UncivGame.Current.worldScreen).open()
             }
         })
     }
 
-    private fun addAutosaveTurnsSelectBox(innerTable: Popup) {
+    private fun addAutosaveTurnsSelectBox(innerTable: Table) {
         innerTable.add("Turns between autosaves".toLabel())
 
         val autosaveTurnsSelectBox = SelectBox<Int>(skin)
@@ -272,7 +276,7 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         })
     }
 
-    private fun addLanguageSelectBox(innerTable: Popup) {
+    private fun addLanguageSelectBox(innerTable: Table) {
         val languageSelectBox = SelectBox<Language>(skin)
         val languageArray = Array<Language>()
         UncivGame.Current.translations.percentCompleteOfLanguages
@@ -306,6 +310,6 @@ class WorldScreenOptionsPopup(val worldScreen:WorldScreen) : Popup(worldScreen){
         CameraStageBaseScreen.resetFonts() // to load chinese characters if necessary
         UncivGame.Current.worldScreen = WorldScreen(worldScreen.viewingCiv)
         UncivGame.Current.setWorldScreen()
-        WorldScreenOptionsPopup(UncivGame.Current.worldScreen)
+        WorldScreenOptionsPopup(UncivGame.Current.worldScreen).open()
     }
 }
diff --git a/core/src/com/unciv/ui/worldscreen/unit/UnitActions.kt b/core/src/com/unciv/ui/worldscreen/unit/UnitActions.kt
index 9ccedb69..80ecabde 100644
--- a/core/src/com/unciv/ui/worldscreen/unit/UnitActions.kt
+++ b/core/src/com/unciv/ui/worldscreen/unit/UnitActions.kt
@@ -15,8 +15,8 @@ import com.unciv.models.ruleset.Building
 import com.unciv.models.translations.tr
 import com.unciv.ui.pickerscreens.ImprovementPickerScreen
 import com.unciv.ui.pickerscreens.PromotionPickerScreen
-import com.unciv.ui.worldscreen.WorldScreen
 import com.unciv.ui.utils.YesNoPopup
+import com.unciv.ui.worldscreen.WorldScreen
 
 class UnitActions {
 
@@ -314,7 +314,7 @@ class UnitActions {
                     val disbandText = if (unit.currentTile.getOwner() == unit.civInfo)
                         "Disband this unit for [${unit.baseUnit.getDisbandGold()}] gold?".tr()
                     else "Do you really want to disband this unit?".tr()
-                    YesNoPopup(disbandText, { unit.disband(); worldScreen.shouldUpdate = true })
+                    YesNoPopup(disbandText, { unit.disband(); worldScreen.shouldUpdate = true }).open()
                 })
 
         return actionList