Fix13580 select in battle calculator

[frigoref]

Should fix issue 13580 (Can´t select opponent in battle calculator)

Notes to Reviewer

BattleCalculatorPanel.java

• new member determineUnitsOnComboSelectionChange
• Constructor: Add ItemListener for comboBoxes; For swapSidesButton ensure new defender units are for defense

Rest: Comments added

[DanVanAtta]

What's the rationale behind this variable and its purpose?

[frigoref]

It is to determine the units displayed when the combo selection changes, i.e., the user has change the attacking or defending player in one of the top combo field.
I thought I finally found a good name for it -.-

[DanVanAtta]

Did a git bisect to 2046ca7 to find when this problem started. Doing a selective revert of the offending code is this diff, it looks like this patch would solve the issue:

--- a/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java
+++ b/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java
@@ -372,7 +372,19 @@ class BattleCalculatorPanel extends JPanel {
     final JButton closeButton = new JButton("Close");
     buttons.add(closeButton);
     add(buttons);
-
+    defenderCombo.addActionListener(
+        e -> {
+          setDefendingUnits(
+              defendingUnitsPanel.getUnits().stream().anyMatch(Matches.unitIsOwnedBy(getDefender()))
+                  ? defendingUnitsPanel.getUnits()
+                  : List.of());
+          setWidgetActivation();
+        });
+    attackerCombo.addActionListener(
+        e -> {
+          setAttackingUnits(List.of());
+          setWidgetActivation();
+        });
     amphibiousCheckBox.addActionListener(e -> setWidgetActivation());
     landBattleCheckBox.addActionListener(
         e -> {

I suggest we first do a surgical update to get us back to baseline functionality. Then, consider any future changes (which helps mitigate risk in case those updates have issues. It's a bad situation when fixes contain new problems)

[DanVanAtta]

Went ahead and submitted a minimal fix: #13610
@frigoref please rebase and we can discuss the enhancements you have made

[frigoref]

Did a git bisect to 2046ca7 to find when this problem started. Doing a selective revert of the offending code is this diff, it looks like this patch would solve the issue:

--- a/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java
+++ b/game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java
@@ -372,7 +372,19 @@ class BattleCalculatorPanel extends JPanel {
     final JButton closeButton = new JButton("Close");
     buttons.add(closeButton);
     add(buttons);
-
+    defenderCombo.addActionListener(
+        e -> {
+          setDefendingUnits(
+              defendingUnitsPanel.getUnits().stream().anyMatch(Matches.unitIsOwnedBy(getDefender()))
+                  ? defendingUnitsPanel.getUnits()
+                  : List.of());
+          setWidgetActivation();
+        });
+    attackerCombo.addActionListener(
+        e -> {
+          setAttackingUnits(List.of());
+          setWidgetActivation();
+        });
     amphibiousCheckBox.addActionListener(e -> setWidgetActivation());
     landBattleCheckBox.addActionListener(
         e -> {

I suggest we first do a surgical update to get us back to baseline functionality. Then, consider any future changes (which helps mitigate risk in case those updates have issues. It's a bad situation when fixes contain new problems)

I recall that there was an issue with the switch concerning the unit types did not switch as well properly, leaving extra icons. When I see this old code you found, I think it is odd as the listener would be called every time (also during initial setup or other buttons that affect the combo button). That's why I introduced variable determineUnitsOnComboSelectionChange, so the listener would only react on user changes.
The listener code itself also seems to not match the one I referred to from the swapSidesButton:

swapSidesButton.addActionListener(
        e -> {
          attackerOrderOfLosses = null;
          defenderOrderOfLosses = null;
          final GamePlayer newAttacker = getDefender();
          final List<Unit> newAttackerUnits =
              CollectionUtils.getMatches(
                  defendingUnitsPanel.getUnits(),
                  Matches.unitIsOwnedBy(getDefender())
                      .and(
                          Matches.unitCanBeInBattle(
                              true,
                              isLandBattle(),
                              1,
                              hasMaxRounds(isLandBattle(), data),
                              true,
                              List.of())));
          final GamePlayer newDefender = getAttacker();
          final List<Unit> newDefenderUnits =
              CollectionUtils.getMatches(
                  attackingUnitsPanel.getUnits(),
                  Matches.unitCanBeInBattle(false, isLandBattle(), 1, true));
          setAttackerWithUnits(newAttacker, newAttackerUnits);
          setDefenderWithUnits(newDefender, newDefenderUnits);
          setWidgetActivation();
        });

All in all, I still thing the PR makes sense the way it is.

[DanVanAtta]

The determineUnitsOnComboSelectionChange gives me a lot of pause. I'd like to do that in a cleaner way (or at least with some polish if we really can't clean it up).

First though, what concrete fixes are achieving? I think we should step back and look at the high level objective, there might be an easier approach.