Fix random nations pool popup (#8769)

* Fix random nations pool dialog

* Rename randomNations for backwards compat with 4.5.0 or older

core/src/com/unciv/logic/GameStarter.kt

@@ -222,7 +222,7 @@ object GameStarter {
     private fun addCivilizations(newGameParameters: GameParameters, gameInfo: GameInfo, ruleset: Ruleset, existingMap: Boolean) {
         val availableCivNames = Stack<String>()
         if (gameSetupInfo.gameParameters.enableRandomNationsPool) {
-            availableCivNames.addAll(gameSetupInfo.gameParameters.randomNations.shuffled())
+            availableCivNames.addAll(gameSetupInfo.gameParameters.randomNationsPool.shuffled())
         } else
             // CityState or Spectator civs are not available for Random pick
             availableCivNames.addAll(ruleset.nations.filter { it.value.isMajorCiv() }.keys.shuffled())

core/src/com/unciv/models/metadata/GameParameters.kt

@@ -30,7 +30,7 @@ class GameParameters : IsPartOfGameInfoSerialization { // Default values are the
     var numberOfCityStates = 6
 
     var enableRandomNationsPool = false
-    var randomNations = arrayListOf<String>()
+    var randomNationsPool = arrayListOf<String>()
 
     var noCityRazing = false
     var noBarbarians = false
@@ -64,7 +64,7 @@ class GameParameters : IsPartOfGameInfoSerialization { // Default values are the
         parameters.maxNumberOfCityStates = maxNumberOfCityStates
         parameters.numberOfCityStates = numberOfCityStates
         parameters.enableRandomNationsPool = enableRandomNationsPool
-        parameters.randomNations = ArrayList(randomNations)
+        parameters.randomNationsPool = ArrayList(randomNationsPool)
         parameters.noCityRazing = noCityRazing
         parameters.noBarbarians = noBarbarians
         parameters.ragingBarbarians = ragingBarbarians

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

@@ -3,6 +3,7 @@ package com.unciv.ui.screens.newgamescreen
 import com.badlogic.gdx.graphics.Color
 import com.badlogic.gdx.scenes.scene2d.Group
 import com.badlogic.gdx.scenes.scene2d.Touchable
+import com.badlogic.gdx.scenes.scene2d.ui.CheckBox
 import com.badlogic.gdx.scenes.scene2d.ui.ImageButton
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.badlogic.gdx.utils.Align
@@ -23,7 +24,6 @@ import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.components.ExpanderTab
 import com.unciv.ui.components.KeyCharAndCode
 import com.unciv.ui.components.UncivSlider
-import com.unciv.ui.components.extensions.isNarrowerThan4to3
 import com.unciv.ui.components.extensions.keyShortcuts
 import com.unciv.ui.components.extensions.onActivation
 import com.unciv.ui.components.extensions.onChange
@@ -35,15 +35,17 @@ import com.unciv.ui.components.extensions.toLabel
 import com.unciv.ui.components.extensions.toTextButton
 
 class GameOptionsTable(
-    val previousScreen: IPreviousScreen,
+    private val previousScreen: IPreviousScreen,
-    val isPortrait: Boolean = false,
+    private val isPortrait: Boolean = false,
-    val updatePlayerPickerTable: (desiredCiv:String)->Unit
+    private val updatePlayerPickerTable: (desiredCiv:String)->Unit
 ) : Table(BaseScreen.skin) {
     var gameParameters = previousScreen.gameSetupInfo.gameParameters
     val ruleset = previousScreen.ruleset
     var locked = false
     var modCheckboxes: ModCheckboxTable? = null
         private set
+    // Remember this so we can unselect it when the pool dialog returns an empty pool
+    private var randomNationsPoolCheckbox: CheckBox? = null
 
     init {
         getGameOptionsTable()
@@ -125,11 +127,17 @@ class GameOptionsTable(
         pack()
     }
 
-    private fun Table.addCheckbox(text: String, initialState: Boolean, lockable: Boolean = true, onChange: (newValue: Boolean) -> Unit) {
+    private fun Table.addCheckbox(
+        text: String,
+        initialState: Boolean,
+        lockable: Boolean = true,
+        onChange: (newValue: Boolean) -> Unit
+    ): CheckBox {
         val checkbox = text.toCheckBox(initialState) { onChange(it) }
         checkbox.isDisabled = lockable && locked
         checkbox.align(Align.left)
         add(checkbox).colspan(2).row()
+        return checkbox
     }
 
     private fun Table.addNoCityRazingCheckbox() =
@@ -173,16 +181,25 @@ class GameOptionsTable(
         addCheckbox("Enable Espionage", gameParameters.espionageEnabled)
         { gameParameters.espionageEnabled = it }
 
-    private fun Table.addRandomNationsPoolCheckbox() =
+    private fun Table.addRandomNationsPoolCheckbox() {
-            addCheckbox("Set available nations for random pool", gameParameters.enableRandomNationsPool) {
+        randomNationsPoolCheckbox = addCheckbox(
-                gameParameters.enableRandomNationsPool = it
+            "Set available nations for random pool",
-                update()  // To show the button opening the chooser popup
+            gameParameters.enableRandomNationsPool
-            }
+        ) {
+            gameParameters.enableRandomNationsPool = it
+            update()  // To show the button opening the chooser popup
+        }
+    }
 
     private fun Table.addNationsSelectTextButton() {
         val button = "Select nations".toTextButton()
         button.onClick {
-            val popup = RandomNationPickerPopup(previousScreen, gameParameters)
+            val popup = RandomNationPickerPopup(previousScreen, gameParameters) {
+                if (gameParameters.randomNationsPool.isEmpty()) {
+                    gameParameters.enableRandomNationsPool = false
+                    randomNationsPoolCheckbox?.isChecked = false
+                }
+            }
             popup.open()
             popup.update()
         }
@@ -391,7 +408,7 @@ class GameOptionsTable(
         add(victoryConditionsTable).colspan(2).row()
     }
 
-    fun reloadRuleset() {
+    private fun reloadRuleset() {
         ruleset.clear()
         val newRuleset = RulesetCache.getComplexRuleset(gameParameters)
         ruleset.add(newRuleset)
@@ -433,7 +450,8 @@ class GameOptionsTable(
 
 private class RandomNationPickerPopup(
     previousScreen: IPreviousScreen,
-    val gameParameters: GameParameters
+    private val gameParameters: GameParameters,
+    private val onExit: () -> Unit
 ) : Popup(previousScreen as BaseScreen) {
     companion object {
         // These are used for the Close/OK buttons in the lower left/right corners:
@@ -443,51 +461,52 @@ private class RandomNationPickerPopup(
         val buttonsBackColor: Color = Color.BLACK.cpy().apply { a = 0.67f }
     }
 
-    val blockWidth: Float = 0f
-    val civBlocksWidth = if(blockWidth <= 10f) previousScreen.stage.width / 3 - 5f else blockWidth
-
     // This Popup's body has two halves of same size, either side by side or arranged vertically
     // depending on screen proportions - determine height for one of those
-    private val partHeight = stageToShowOn.height * (if (stageToShowOn.isNarrowerThan4to3()) 0.45f else 0.8f)
+    private val isPortrait = (previousScreen as BaseScreen).isPortrait()
-    private val nationListTable = Table()
+    private val civBlocksWidth = stageToShowOn.width / 3 - 5f
-    private val nationListScroll = AutoScrollPane(nationListTable)
+    private val partHeight = stageToShowOn.height * (if (isPortrait) 0.45f else 0.8f)
+    /** Widget offering Nations for the Pool (those that would be excluded) goes on the left/top */
+    private val availableNationsListTable = Table()
+    private val availableNationsListScroll = AutoScrollPane(availableNationsListTable)
+    /** Widget selecting Nations for the Pool (those that could be chosen for a random slot) goes on the right/bottom */
     private val selectedNationsListTable = Table()
     private val selectedNationsListScroll = AutoScrollPane(selectedNationsListTable)
-    private var bannedNations = gameParameters.randomNations
+    /** sorted list of all major nations as ready-made button */
-    var availableNations = arrayListOf<String>()
+    private val allNationTables: ArrayList<NationTable>
-
+    /** backing for the left side - unchosen Nations */
-    private val ruleset = previousScreen.ruleset
+    private var availableNations: MutableSet<String>
+    /** backing for the right side - chosen Nations */
+    private var selectedNations: MutableSet<String>
 
     init {
-        var nationListScrollY = 0f
+        val sortedNations = previousScreen.ruleset.nations.values
-        availableNations += ruleset.nations.values.asSequence()
+                .filter { it.isMajorCiv() }
-            .filter { it.isMajorCiv() }.map { it.name }
+                .sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.name.tr() })
-        nationListScroll.setOverscroll(false, false)
+        allNationTables = ArrayList(
-        // +10, because the nation table has a 5f pad, for a total of +10f
+            sortedNations.map { NationTable(it, civBlocksWidth, 0f) }  // no need for min height
-        if (stageToShowOn.isNarrowerThan4to3()) {
+        )
-            add("Available nations".tr())
+        availableNations = sortedNations.map { it.name }.toMutableSet()
+        selectedNations = gameParameters.randomNationsPool.intersect(availableNations) as MutableSet<String>
+        availableNations.removeAll(selectedNations)
+
+        availableNationsListScroll.setOverscroll(false, false)
+        // size below uses civBlocksWidth +10, because the nation table has a 5f pad, for a total of +10f
+        add("Banned nations".tr())
+        if (isPortrait) {
             row()
-            add(nationListScroll).size( civBlocksWidth + 10f, partHeight )
+            add(availableNationsListScroll).size( civBlocksWidth + 10f, partHeight ).row()
-            row()
+            addSeparator()
-            add("Banned nations".tr()).row()
+            add("Available nations".tr()).row()
         } else {
-            add("Available nations".tr())
+            add("Available nations".tr()).row()
-            add("Banned nations".tr()).row()
+            add(availableNationsListScroll).size( civBlocksWidth + 10f, partHeight )
-            row()
-            add(nationListScroll).size( civBlocksWidth + 10f, partHeight )
         }
         selectedNationsListScroll.setOverscroll(false, false)
-        add(selectedNationsListScroll).size(civBlocksWidth + 10f, partHeight) // Same here, see above
+        add(selectedNationsListScroll).size(civBlocksWidth + 10f, partHeight)
 
         update()
-
-        nationListScroll.layout()
         pack()
-        if (nationListScrollY > 0f) {
-            // center the selected nation vertically, getRowHeight safe because nationListScrollY > 0f ensures at least 1 row
-            nationListScrollY -= (nationListScroll.height - nationListTable.getRowHeight(0)) / 2
-            nationListScroll.scrollY = nationListScrollY.coerceIn(0f, nationListScroll.maxY)
-        }
 
         val closeButton = "OtherIcons/Close".toImageButton(Color.FIREBRICK)
         closeButton.onActivation { close() }
@@ -509,24 +528,23 @@ private class RandomNationPickerPopup(
     }
 
     fun update() {
-        nationListTable.clear()
+        updateNationListTable(availableNationsListTable, availableNations) {
-        bannedNations = gameParameters.randomNations
+            nation -> { addNationToPool(nation) }
-        availableNations -= bannedNations.toSet()
-        availableNations = availableNations.sortedWith(compareBy(UncivGame.Current.settings.getCollatorFromLocale()) { it.tr() }).toMutableList() as ArrayList<String>
-
-        var currentY = 0f
-        for (nationName in availableNations) {
-            val nation = ruleset.nations[nationName] ?: continue
-            val nationTable = NationTable(nation, civBlocksWidth, 0f) // no need for min height
-            val cell = nationListTable.add(nationTable)
-            currentY += cell.padBottom + cell.prefHeight + cell.padTop
-            cell.row()
-            nationTable.onClick {
-                addNationToPool(nation)
-            }
         }
+        updateNationListTable(selectedNationsListTable, selectedNations) {
+            nation -> { removeNationFromPool(nation) }
+        }
+    }
 
-        updateNationListTable()
+    private fun updateNationListTable(table: Table, nations: Set<String>, actionFactory: (Nation)->(()->Unit) ) {
+        for (child in table.children) { child.listeners.clear() }
+        table.clear()
+
+        for (nationTable in allNationTables) {
+            if (nationTable.nation.name !in nations) continue
+            nationTable.onClick(actionFactory(nationTable.nation))
+            table.add(nationTable).row()
+        }
     }
 
     private fun String.toImageButton(overColor: Color): Group {
@@ -540,43 +558,30 @@ private class RandomNationPickerPopup(
         return button.surroundWithCircle(buttonsCircleSize, false, buttonsBackColor)
     }
 
-    private fun updateNationListTable() {
-        selectedNationsListTable.clear()
-
-        for (nationName in bannedNations) {
-            val currentNation = ruleset.nations[nationName] ?: continue
-            val nationTable = NationTable(currentNation, civBlocksWidth, 0f)
-            nationTable.onClick { removeNationFromPool(currentNation) }
-            selectedNationsListTable.add(nationTable).row()
-        }
-    }
-
     private fun addNationToPool(nation: Nation) {
         availableNations.remove(nation.name)
-        bannedNations.add(nation.name)
+        selectedNations.add(nation.name)
 
         update()
     }
 
     private fun removeNationFromPool(nation: Nation) {
         availableNations.add(nation.name)
-        bannedNations.remove(nation.name)
+        selectedNations.remove(nation.name)
 
         update()
     }
 
     private fun returnSelected() {
         close()
-        gameParameters.randomNations = availableNations
+        gameParameters.randomNationsPool = ArrayList(selectedNations)
+        onExit()
     }
 
     private fun switchAllNations() {
         val tempNations = availableNations
-        availableNations = bannedNations
+        availableNations = selectedNations
-        bannedNations = tempNations
+        selectedNations = tempNations
-
-        gameParameters.randomNations = bannedNations
-
         update()
     }
 }