Diplomacy trade layout fix (#8808)

* Diplomacy and Trade - Linting

* Diplomacy and Trade - Fix gradual layout deterioration

core/src/com/unciv/ui/screens/diplomacyscreen/DiplomacyScreen.kt

@@ -56,10 +56,11 @@ import com.unciv.ui.components.AutoScrollPane as ScrollPane
  *
  * When [selectCiv] is given and [selectTrade] is not, that Civilization is selected as if clicked on the left side.
  * When [selectCiv] and [selectTrade] are supplied, that Trade for that Civilization is selected, used for the counter-offer option from `TradePopup`.
+ * Note calling this with [selectCiv] a City State and [selectTrade] supplied is **not allowed**.
  */
 @Suppress("KDocUnresolvedReference")  // Mentioning non-field parameters is flagged, but they work anyway
 class DiplomacyScreen(
-    val viewingCiv: Civilization,
+    private val viewingCiv: Civilization,
     private val selectCiv: Civilization? = null,
     private val selectTrade: Trade? = null
 ): BaseScreen(), RecreateOnResize {
@@ -69,7 +70,7 @@ class DiplomacyScreen(
     }
 
     private val leftSideTable = Table().apply { defaults().pad(nationIconPad) }
-    val leftSideScroll = ScrollPane(leftSideTable)
+    private val leftSideScroll = ScrollPane(leftSideTable)
     private val rightSideTable = Table()
     private val closeButton = Constants.close.toTextButton()
 
@@ -169,20 +170,13 @@ class DiplomacyScreen(
         rightSideTable.clear()
         UncivGame.Current.musicController.chooseTrack(otherCiv.civName,
             MusicMood.peaceOrWar(viewingCiv.isAtWarWith(otherCiv)),MusicTrackChooserFlags.setSelectNation)
-        if (otherCiv.isCityState()) rightSideTable.add(
-            ScrollPane(getCityStateDiplomacyTable(otherCiv))
-        )
-        else rightSideTable.add(ScrollPane(getMajorCivDiplomacyTable(otherCiv)))
-            .height(stage.height)
-    }
-
-    private fun setTrade(civ: Civilization): TradeTable {
-        rightSideTable.clear()
-        val tradeTable = TradeTable(civ, this)
-        rightSideTable.add(tradeTable)
-        return tradeTable
+        rightSideTable.add(ScrollPane(
+            if (otherCiv.isCityState()) getCityStateDiplomacyTable(otherCiv)
+            else getMajorCivDiplomacyTable(otherCiv)
+        )).height(stage.height)
     }
 
+    //region City State Diplomacy
     private fun getCityStateDiplomacyTableHeader(otherCiv: Civilization): Table {
         val otherCivDiplomacyManager = otherCiv.getDiplomacyManager(viewingCiv)
 
@@ -290,7 +284,6 @@ class DiplomacyScreen(
         return diplomacyTable
     }
 
-
     private fun getCityStateDiplomacyTable(otherCiv: Civilization): Table {
         val otherCivDiplomacyManager = otherCiv.getDiplomacyManager(viewingCiv)
 
@@ -635,6 +628,8 @@ class DiplomacyScreen(
         return warTable
     }
 
+    //endregion
+    //region Major Civ Diplomacy
 
     private fun getMajorCivDiplomacyTable(otherCiv: Civilization): Table {
         val otherCivDiplomacyManager = otherCiv.getDiplomacyManager(viewingCiv)
@@ -701,6 +696,13 @@ class DiplomacyScreen(
         return diplomacyTable
     }
 
+    private fun setTrade(civ: Civilization): TradeTable {
+        rightSideTable.clear()
+        val tradeTable = TradeTable(civ, this)
+        rightSideTable.add(tradeTable)
+        return tradeTable
+    }
+
     private fun getNegotiatePeaceMajorCivButton(
         otherCiv: Civilization,
         otherCivDiplomacyManager: DiplomacyManager
@@ -909,6 +911,8 @@ class DiplomacyScreen(
         return declareWarButton
     }
 
+    //endregion
+
     // response currently always gets "Very Well.", but that may expand in the future.
     @Suppress("SameParameterValue")
     private fun setRightSideFlavorText(
@@ -940,6 +944,13 @@ class DiplomacyScreen(
         return goToOnMapButton
     }
 
+    /** Calculate a width for [TradeTable] two-column layout, called from [OfferColumnsTable]
+     *
+     *  _Caller is responsible to not exceed this **including its own padding**_
+     */
+    // Note breaking the rule above will squeeze the leftSideScroll to the left - cumulatively.
+    internal fun getTradeColumnsWidth() = (stage.width - leftSideScroll.width - 3f) / 2  // 3 for SplitPane handle
+
     override fun resize(width: Int, height: Int) {
         super.resize(width, height)
         positionCloseButton()

core/src/com/unciv/ui/screens/diplomacyscreen/OfferColumnsTable.kt

@@ -2,6 +2,7 @@ package com.unciv.ui.screens.diplomacyscreen
 
 import com.badlogic.gdx.scenes.scene2d.ui.Table
 import com.unciv.Constants
+import com.unciv.logic.civilization.Civilization
 import com.unciv.logic.trade.TradeLogic
 import com.unciv.logic.trade.TradeOffer
 import com.unciv.logic.trade.TradeOffersList
@@ -14,8 +15,11 @@ import com.unciv.ui.popups.AskNumberPopup
 import com.unciv.ui.screens.basescreen.BaseScreen
 
 /** This is the class that holds the 4 columns of the offers (ours/theirs/ offered/available) in trade */
-class OfferColumnsTable(private val tradeLogic: TradeLogic, val screen: DiplomacyScreen, val onChange: () -> Unit): Table(
-    BaseScreen.skin) {
+class OfferColumnsTable(
+    private val tradeLogic: TradeLogic,
+    private val screen: DiplomacyScreen,
+    private val onChange: () -> Unit
+): Table(BaseScreen.skin) {
 
     private fun addOffer(offer: TradeOffer, offerList: TradeOffersList, correspondingOfferList: TradeOffersList) {
         offerList.add(offer.copy())
@@ -23,34 +27,31 @@ class OfferColumnsTable(private val tradeLogic: TradeLogic, val screen: Diplomac
         onChange()
     }
 
-    private val ourAvailableOffersTable = OffersListScroll("OurAvail") {
-        when (it.type) {
-            TradeType.Gold -> openGoldSelectionPopup(it, tradeLogic.currentTrade.ourOffers, tradeLogic.ourCivilization.gold)
-            TradeType.Gold_Per_Turn -> openGoldSelectionPopup(it, tradeLogic.currentTrade.ourOffers, tradeLogic.ourCivilization.stats.statsForNextTurn.gold.toInt())
-            else -> addOffer(it, tradeLogic.currentTrade.ourOffers, tradeLogic.currentTrade.theirOffers)
+    private fun offerClickImplementation(
+        offer: TradeOffer,
+        invert: Boolean,
+        list: TradeOffersList,
+        counterList: TradeOffersList,
+        civ: Civilization
+    ) {
+        when (offer.type) {
+            TradeType.Gold -> openGoldSelectionPopup(offer, list, civ.gold)
+            TradeType.Gold_Per_Turn -> openGoldSelectionPopup(offer, list, civ.stats.statsForNextTurn.gold.toInt())
+            else -> addOffer(if (invert) offer.copy(amount = -offer.amount) else offer, list, counterList)
         }
     }
 
+    private val ourAvailableOffersTable = OffersListScroll("OurAvail") {
+        tradeLogic.currentTrade.run { offerClickImplementation(it, false, ourOffers, theirOffers, tradeLogic.ourCivilization) }
+    }
     private val ourOffersTable = OffersListScroll("OurTrade") {
-        when (it.type) {
-            TradeType.Gold -> openGoldSelectionPopup(it, tradeLogic.currentTrade.ourOffers, tradeLogic.ourCivilization.gold)
-            TradeType.Gold_Per_Turn -> openGoldSelectionPopup(it, tradeLogic.currentTrade.ourOffers, tradeLogic.ourCivilization.stats.statsForNextTurn.gold.toInt())
-            else -> addOffer(it.copy(amount = -it.amount), tradeLogic.currentTrade.ourOffers, tradeLogic.currentTrade.theirOffers)
-        }
+        tradeLogic.currentTrade.run { offerClickImplementation(it, true, ourOffers, theirOffers, tradeLogic.ourCivilization) }
     }
     private val theirOffersTable = OffersListScroll("TheirTrade") {
-        when (it.type) {
-            TradeType.Gold -> openGoldSelectionPopup(it, tradeLogic.currentTrade.theirOffers, tradeLogic.otherCivilization.gold)
-            TradeType.Gold_Per_Turn -> openGoldSelectionPopup(it, tradeLogic.currentTrade.theirOffers, tradeLogic.otherCivilization.stats.statsForNextTurn.gold.toInt())
-            else -> addOffer(it.copy(amount = -it.amount), tradeLogic.currentTrade.theirOffers, tradeLogic.currentTrade.ourOffers)
-        }
+        tradeLogic.currentTrade.run { offerClickImplementation(it, true, theirOffers, ourOffers, tradeLogic.otherCivilization) }
     }
     private val theirAvailableOffersTable = OffersListScroll("TheirAvail") {
-        when (it.type) {
-            TradeType.Gold -> openGoldSelectionPopup(it, tradeLogic.currentTrade.theirOffers, tradeLogic.otherCivilization.gold)
-            TradeType.Gold_Per_Turn -> openGoldSelectionPopup(it, tradeLogic.currentTrade.theirOffers, tradeLogic.otherCivilization.stats.statsForNextTurn.gold.toInt())
-            else -> addOffer(it, tradeLogic.currentTrade.theirOffers, tradeLogic.currentTrade.ourOffers)
-        }
+        tradeLogic.currentTrade.run { offerClickImplementation(it, false, theirOffers, ourOffers, tradeLogic.otherCivilization) }
     }
 
     init {
@@ -58,9 +59,10 @@ class OfferColumnsTable(private val tradeLogic: TradeLogic, val screen: Diplomac
 
         val isPortraitMode = screen.isNarrowerThan4to3()
 
-        val columnWidth = (screen.stage.width - screen.leftSideScroll.width) / 2
+        val columnWidth = screen.getTradeColumnsWidth() - 20f // Subtract padding: ours and OffersListScroll's
 
         if (!isPortraitMode) {
+            // In landscape, arrange in 4 panels: ours left / theirs right ; items top / offers bottom.
             add("Our items".tr())
             add("[${tradeLogic.otherCivilization.civName}]'s items".tr()).row()
 
@@ -73,8 +75,9 @@ class OfferColumnsTable(private val tradeLogic: TradeLogic, val screen: Diplomac
             add("[${tradeLogic.otherCivilization.civName}]'s trade offer".tr()).row()
             add(ourOffersTable).size(columnWidth, screen.stage.height / 3)
             add(theirOffersTable).size(columnWidth, screen.stage.height / 3)
-        }
-        else {
+        } else {
+            // In portrait, this will arrange the items lists vertically
+            // and the offers still side-by-side below that
             add("Our items".tr()).colspan(2).row()
             add(ourAvailableOffersTable).height(screen.stage.height / 4f).colspan(2).row()
 

core/src/com/unciv/ui/screens/diplomacyscreen/TradeTable.kt

@@ -10,31 +10,33 @@ import com.unciv.ui.components.extensions.isEnabled
 import com.unciv.ui.components.extensions.onClick
 import com.unciv.ui.components.extensions.toTextButton
 
-class TradeTable(private val otherCivilization: Civilization, stage: DiplomacyScreen): Table(
-    BaseScreen.skin) {
-    val currentPlayerCiv = otherCivilization.gameInfo.getCurrentPlayerCivilization()
-    var tradeLogic = TradeLogic(currentPlayerCiv,otherCivilization)
-    var offerColumnsTable = OfferColumnsTable(tradeLogic, stage) { onChange() }
+class TradeTable(
+    private val otherCivilization: Civilization,
+    diplomacyScreen: DiplomacyScreen
+): Table(BaseScreen.skin) {
+    private val currentPlayerCiv = otherCivilization.gameInfo.getCurrentPlayerCivilization()
+    internal val tradeLogic = TradeLogic(currentPlayerCiv, otherCivilization)
+    internal val offerColumnsTable = OfferColumnsTable(tradeLogic, diplomacyScreen) { onChange() }
     // This is so that after a trade has been traded, we can switch out the offersToDisplay to start anew - this is the easiest way
-    private var offerColumnsTableWrapper = Table()
+    private val offerColumnsTableWrapper = Table()
     private val offerButton = "Offer trade".toTextButton()
 
     private fun isTradeOffered() = otherCivilization.tradeRequests.any { it.requestingCiv == currentPlayerCiv.civName }
 
-    private fun retractOffer(){
+    private fun retractOffer() {
         otherCivilization.tradeRequests.removeAll { it.requestingCiv == currentPlayerCiv.civName }
         currentPlayerCiv.cache.updateCivResources()
         offerButton.setText("Offer trade".tr())
     }
 
-    init{
+    init {
         offerColumnsTableWrapper.add(offerColumnsTable)
         add(offerColumnsTableWrapper).row()
 
         val lowerTable = Table().apply { defaults().pad(10f) }
 
         val existingOffer = otherCivilization.tradeRequests.firstOrNull { it.requestingCiv == currentPlayerCiv.civName }
-        if (existingOffer != null){
+        if (existingOffer != null) {
             tradeLogic.currentTrade.set(existingOffer.trade.reverse())
             offerColumnsTable.update()
         }
@@ -43,12 +45,12 @@ class TradeTable(private val otherCivilization: Civilization, stage: DiplomacySc
         else offerButton.apply { isEnabled = false }.setText("Offer trade".tr())
 
         offerButton.onClick {
-            if(isTradeOffered()) {
+            if (isTradeOffered()) {
                 retractOffer()
                 return@onClick
             }
 
-            otherCivilization.tradeRequests.add(TradeRequest(currentPlayerCiv.civName,tradeLogic.currentTrade.reverse()))
+            otherCivilization.tradeRequests.add(TradeRequest(currentPlayerCiv.civName, tradeLogic.currentTrade.reverse()))
             currentPlayerCiv.cache.updateCivResources()
             offerButton.setText("Retract offer".tr())
         }
@@ -61,7 +63,7 @@ class TradeTable(private val otherCivilization: Civilization, stage: DiplomacySc
         pack()
     }
 
-    private fun onChange(){
+    private fun onChange() {
         offerColumnsTable.update()
         retractOffer()
         offerButton.isEnabled = !(tradeLogic.currentTrade.theirOffers.size == 0 && tradeLogic.currentTrade.ourOffers.size == 0)