UI: Fix options popup "spilling" in cramped screen conditions (#11235)

* Remove unused "Closing page" feature from TabbedPager

* Minor linting

* Ensure Popup's innerTable doesn't exceed its Cell within Popup

* Change a few defaults and remove a misguided comment

* Move OptionPopup's close button to top right - another Red X

core/src/com/unciv/ui/components/extensions/Scene2dExtensions.kt

@@ -266,7 +266,7 @@ fun String.toImageButton(iconSize: Float, circleSize: Float, circleColor: Color,
  *  Automatically binds the BACK key to the [action].
  */
 fun getCloseButton(
-    size: Float,
+    size: Float = 50f,
     iconSize: Float = size - 20f,
     circleColor: Color = BaseScreen.skinStrings.skinConfig.baseColor,
     overColor: Color = Color.RED,

core/src/com/unciv/ui/components/widgets/TabbedPager.kt

@@ -55,6 +55,15 @@ import com.unciv.ui.screens.basescreen.BaseScreen
  * maximum is not specified, that coordinate will grow with content up to screen size, and layout
  * max-W/H will always report the same as pref-W/H.
  */
+
+/*
+   Notes - this is implemented as 2x3 Table with 4 Cells.
+   First row is (scrolling header, fixed decoration) - second cell empty and size 0 until you use decorateHeader.
+   Second row / third Cell is "fixed" content, colspan 2. Used for pages that implement IPageExtensions.getFixedContent, otherwise size zero.
+   Third row / fourth Cell is scrolling content, colspan 2. Can scroll in both axes, while "fixed" only scrolls horizontally -
+   horizontal scroll is synchronized (for that to look good, both content parts must match in size and visual layout).
+*/
+
 //region Fields
 @Suppress("MemberVisibilityCanBePrivate", "unused")  // All members are part of our API
 open class TabbedPager(
@@ -290,12 +299,6 @@ open class TabbedPager(
         }
     }
 
-    private class EmptyClosePage(private val action: ()->Unit) : Actor(), IPageExtensions {
-        override fun activated(index: Int, caption: String, pager: TabbedPager) {
-            action()
-        }
-    }
-
     //endregion
     //region Initialization
 
@@ -582,18 +585,6 @@ open class TabbedPager(
         return addAndShowPage(page, insertBefore)
     }
 
-    /**
-     * Add a "Close" button tho the Tab headers, with empty content which will invoke [action] when clicked
-     */
-    fun addClosePage(
-        insertBefore: Int = -1,
-        color: Color = Color(0.75f, 0.1f, 0.1f, 1f),
-        action: ()->Unit
-    ) {
-        val index = addPage(Constants.close, EmptyClosePage(action), insertBefore = insertBefore)
-        pages[index].button.color = color
-    }
-
     /**
      * Activate any [secret][addPage] pages by asking for the password.
      *
@@ -633,14 +624,13 @@ open class TabbedPager(
      *  Must be called _after_ all pages are final, otherwise effects not guaranteed.
      *
      *  Notes:
-     *  *  Using [fixed] will make the widths of content and header Scrollpanes different. Synchronized scrolling will look off.
      *  *  [fixed] decorations have predefined cells, thus decorateHeader will replace any previous decoration. Non-[fixed] are cumulative and cannot be removed.
      *  *  [leftSide] and [fixed] both true is not implemented.
      *
      *  @param leftSide If `true` then [actor] is inserted on the left, otherwise on the right of the page buttons.
      *  @param fixed If `true` [actor] is outside the header ScrollPane and thus always shown.
      */
-    fun decorateHeader(actor: Actor, leftSide: Boolean, fixed: Boolean = false) {
+    fun decorateHeader(actor: Actor, leftSide: Boolean = false, fixed: Boolean = true) {
         if (fixed) headerDecorationRightCell.pad(headerPadding).setActor(actor)
         else insertHeaderCellAt(if (leftSide) 0 else -1).setActor(actor)
         invalidate()

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

@@ -43,7 +43,8 @@ import com.unciv.ui.screens.basescreen.UncivStage
  *
  * @property stageToShowOn The stage that will be used for [open], measurements or finding other instances
  * @param scrollable Controls how content can scroll if too large - see [Scrollability]
- * @param maxSizePercentage Causes [topTable] to limit its height - useful if `scrollable` is on. Will be multiplied by stageToShowOn.height.
+ * @param maxSizePercentage Causes [topTable] to limit its width/height - useful if `scrollable` is on. Will be multiplied by stageToShowOn sizes.
+ *        Note this can be overridden if a larger minWidth is reported by [innerTable].
  */
 @Suppress("MemberVisibilityCanBePrivate")
 open class Popup(
@@ -63,7 +64,7 @@ open class Popup(
      * With scrolling enabled, the ScrollPane can be accessed via [getScrollPane].
      * @property None No scrolling
      * @property All Entire content wrapped in an [AutoScrollPane] so it can scroll if larger than maximum dimensions
-     * @property WithoutButtons content separated into scrollable upper part and static lower part containing the buttons
+     * @property WithoutButtons Content separated into scrollable upper part and static lower part containing the buttons
      */
     enum class Scrollability { None, All, WithoutButtons }
 
@@ -75,6 +76,8 @@ open class Popup(
      *
      *  Note you seldom need to interact directly with it, Popup has many Table method proxies
      *  that pass through, like [add], [row], [defaults], [addSeparator] or [clear].
+     *
+     *  For [Scrollability.WithoutButtons], this wraps [topTable] and [bottomTable], otherwise both are aliases.
      */
     /* Hierarchy:
         Scrollability.None:
@@ -98,11 +101,14 @@ open class Popup(
     */
     protected val innerTable = Table(BaseScreen.skin)
 
-    /** This contains most of the Popup content (except the closing buttons which go in [bottomTable]) */
+    /** This contains most of the Popup content.
+     *  For [Scrollability.WithoutButtons], that excludes the closing buttons which go in [bottomTable].
+     *  In other modes, this is an alias for [innerTable]. */
     private val topTable: Table
     private val topTableCell: Cell<WidgetGroup>
 
-    /** This contains the bottom row buttons and does not participate in scrolling */
+    /** This contains the bottom row buttons and does not participate in scrolling
+     *  (for [Scrollability.WithoutButtons], otherwise alias for [innerTable]). */
     protected val bottomTable: Table
 
     /** Callbacks that will be called whenever this Popup is shown */
@@ -180,6 +186,13 @@ open class Popup(
         innerTable.invalidate()
     }
 
+    private fun recalculateInnerTableMaxWidth() {
+        val minWidth = innerTable.minWidth.coerceAtMost(stageToShowOn.width)
+        if (minWidth < maxPopupWidth) return
+        topTableCell.maxWidth(minWidth)
+        innerTable.invalidate()
+    }
+
 
     /**
      * Displays the Popup on the screen. If another popup is already open, this one will display after the other has
@@ -189,6 +202,7 @@ open class Popup(
         stageToShowOn.addActor(this)
         recalculateInnerTableMaxHeight()
         innerTable.pack()
+        recalculateInnerTableMaxWidth()
         pack()
         center(stageToShowOn)
         events.receive(UncivStage.VisibleAreaChanged::class) {
@@ -240,18 +254,18 @@ open class Popup(
        Note the Kdoc mentions innerTable when under Scrollability.WithoutButtons it's actually topTable,
        but metioning that distinction seems overkill. innerTable has the clearer Kdoc for "where the Actors go".
     */
-    /** Popup proxy redirects [add][com.badlogic.gdx.scenes.scene2d.ui.Table.add] to [innerTable] */
+    /** Popup proxy redirects [add][com.badlogic.gdx.scenes.scene2d.ui.Table.add] to [topTable] */
     final override fun <T : Actor?> add(actor: T): Cell<T> = topTable.add(actor)
-    /** Popup proxy redirects [add][com.badlogic.gdx.scenes.scene2d.ui.Table.add] to [innerTable] */
+    /** Popup proxy redirects [add][com.badlogic.gdx.scenes.scene2d.ui.Table.add] to [topTable] */
     final override fun add(): Cell<Actor?> = topTable.add()
-    /** Popup proxy redirects [row][com.badlogic.gdx.scenes.scene2d.ui.Table.row] to [innerTable] */
+    /** Popup proxy redirects [row][com.badlogic.gdx.scenes.scene2d.ui.Table.row] to [topTable] */
     override fun row(): Cell<Actor> = topTable.row()
-    /** Popup proxy redirects [defaults][com.badlogic.gdx.scenes.scene2d.ui.Table.defaults] to [innerTable] */
+    /** Popup proxy redirects [defaults][com.badlogic.gdx.scenes.scene2d.ui.Table.defaults] to [topTable] */
     override fun defaults(): Cell<Actor> = topTable.defaults()
-    /** Popup proxy redirects [addSeparator][com.unciv.ui.components.extensions.addSeparator] to [innerTable] */
+    /** Popup proxy redirects [addSeparator][com.unciv.ui.components.extensions.addSeparator] to [topTable] */
     fun addSeparator(color: Color = Color.WHITE, colSpan: Int = 0, height: Float = 2f) =
         topTable.addSeparator(color, colSpan, height)
-    /** Proxy redirects [add][com.badlogic.gdx.scenes.scene2d.ui.Table.clear] to clear content:
+    /** Proxy redirects [clear][com.badlogic.gdx.scenes.scene2d.ui.Table.clear] to clear all content:
      *  [innerTable] or if [Scrollability.WithoutButtons] was used [topTable] and [bottomTable] */
     override fun clear() {
         topTable.clear()

core/src/com/unciv/ui/popups/options/AdvancedTab.kt

@@ -90,7 +90,7 @@ private fun addCutoutCheckbox(table: Table, optionsPopup: OptionsPopup) {
     {
         optionsPopup.settings.androidCutout = it
         Display.setCutout(it)
-        optionsPopup.reopenAfterDiplayLayoutChange()
+        optionsPopup.reopenAfterDisplayLayoutChange()
     }
 }
 
@@ -99,7 +99,7 @@ private fun addHideSystemUiCheckbox(table: Table, optionsPopup: OptionsPopup) {
     {
         optionsPopup.settings.androidHideSystemUi = it
         Display.setSystemUiVisibility(hide = it)
-        optionsPopup.reopenAfterDiplayLayoutChange()
+        optionsPopup.reopenAfterDisplayLayoutChange()
     }
 }
 
@@ -116,7 +116,7 @@ private fun addOrientationSelectBox(table: Table, optionsPopup: OptionsPopup) {
         val orientation = selectBox.selected
         settings.displayOrientation = orientation
         Display.setOrientation(orientation)
-        optionsPopup.reopenAfterDiplayLayoutChange()
+        optionsPopup.reopenAfterDisplayLayoutChange()
     }
 
     table.add(selectBox).minWidth(optionsPopup.selectBoxMinWidth).pad(10f).row()

core/src/com/unciv/ui/popups/options/OptionsPopup.kt

@@ -9,6 +9,7 @@ import com.unciv.models.metadata.BaseRuleset
 import com.unciv.models.ruleset.RulesetCache
 import com.unciv.ui.components.extensions.areSecretKeysPressed
 import com.unciv.ui.components.extensions.center
+import com.unciv.ui.components.extensions.getCloseButton
 import com.unciv.ui.components.extensions.toCheckBox
 import com.unciv.ui.components.widgets.TabbedPager
 import com.unciv.ui.images.ImageGetter
@@ -65,7 +66,7 @@ class OptionsPopup(
             selectBoxMinWidth = if (stage.width < 600f) 200f else 240f
             tabMaxWidth = if (isPortrait()) stage.width - 10f else 0.8f * stage.width
             tabMinWidth = 0.6f * stage.width
-            tabMaxHeight = (if (isPortrait()) 0.7f else 0.8f) * stage.height
+            tabMaxHeight = 0.8f * stage.height
         }
         tabs = TabbedPager(
             tabMinWidth, tabMaxWidth, 0f, tabMaxHeight,
@@ -123,13 +124,13 @@ class OptionsPopup(
             tabs.addPage("Debug", debugTab(this), ImageGetter.getImage("OtherIcons/SecretOptions"), 24f)
         }
 
-        addCloseButton {
+        tabs.decorateHeader(getCloseButton {
             screen.game.musicController.onChange(null)
             center(screen.stage)
             keyBindingsTab?.save()
             settings.save()
             onClose()
-        }.padBottom(10f)
+        })
 
         if (GUI.keyboardAvailable) {
             showOrHideKeyBindings()  // Do this late because it looks for the page to insert before
@@ -172,7 +173,7 @@ class OptionsPopup(
      *  Does nothing if any Popup (which can only be this one) is still open after a short delay and context yield.
      *  Reason: A resize might relaunch the parent screen ([MainMenuScreen] is [RecreateOnResize]) and thus close this Popup.
      */
-    fun reopenAfterDiplayLayoutChange() {
+    internal fun reopenAfterDisplayLayoutChange() {
         Concurrency.run("Reload from options") {
             delay(100)
             withGLContext {
@@ -183,7 +184,7 @@ class OptionsPopup(
         }
     }
 
-    fun addCheckbox(table: Table, text: String, initialState: Boolean, updateWorld: Boolean = false, newRow: Boolean = true, action: ((Boolean) -> Unit)) {
+    internal fun addCheckbox(table: Table, text: String, initialState: Boolean, updateWorld: Boolean = false, newRow: Boolean = true, action: ((Boolean) -> Unit)) {
         val checkbox = text.toCheckBox(initialState) {
             action(it)
             val worldScreen = GUI.getWorldScreenIfActive()
@@ -193,7 +194,7 @@ class OptionsPopup(
         else table.add(checkbox).left()
     }
 
-    fun addCheckbox(
+    internal fun addCheckbox(
         table: Table,
         text: String,
         settingsProperty: KMutableProperty0<Boolean>,

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

@@ -71,8 +71,8 @@ class EmpireOverviewScreen(
             }
         }
 
-        val closeButton = getCloseButton(50f) { game.popScreen() }
+        val closeButton = getCloseButton { game.popScreen() }
-        tabbedPager.decorateHeader(closeButton, leftSide = false, fixed = true)
+        tabbedPager.decorateHeader(closeButton)
 
         tabbedPager.setFillParent(true)
         stage.addActor(tabbedPager)

core/src/com/unciv/ui/screens/victoryscreen/VictoryScreen.kt

@@ -135,10 +135,12 @@ class VictoryScreen(
         val difficultyLabel = "{Difficulty}: {${gameInfo.difficulty}}".toLabel()
         val neededSpace = topRightPanel.width.coerceAtLeast(difficultyLabel.width) * 2 + tabs.getHeaderPrefWidth()
         if (neededSpace > stage.width) {
-            tabs.decorateHeader(difficultyLabel, true)
+            // Let additions take part in TabbedPager's header scrolling
-            tabs.decorateHeader(topRightPanel, false)
+            tabs.decorateHeader(difficultyLabel, leftSide = true, fixed = false)
+            tabs.decorateHeader(topRightPanel, leftSide = false, fixed = false)
             tabs.headerScroll.fadeScrollBars = false
         } else {
+            // Let additions float in the corners
             val panelY = stage.height - tabs.getRowHeight(0) * 0.5f
             stage.addActor(topRightPanel)
             topRightPanel.setPosition(stage.width - 10f, panelY, Align.right)