Clear transportedBy on fighters on allied carriers when battle doesn'…

…t happen. (#12041)

What was happening was that transportedBy was being set when the battle was created, but then if the units moved out still during combat move, it was not being cleared. This caused those fighters later on to have incorrect state and to not be able to participate in future combats.

Fixes: #10294
(Although the existing save will not be fixed, it will just make the issue not happen in new games.)

This is a pre-2.6 bug.

Includes a test and some related code/comment clean ups.

game-app/game-core/src/main/java/games/strategy/triplea/delegate/MovePerformer.java

@@ -103,7 +103,8 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
                 final Route routeUnitUsedToMove =
                     moveDelegate.getRouteUsedToMoveInto(unit, route.getStart());
                 if (battle != null) {
-                  battle.removeAttack(routeUnitUsedToMove, Set.of(unit));
+                  Change change = battle.removeAttack(routeUnitUsedToMove, Set.of(unit));
+                  bridge.addChange(change);
                 }
               }
             }
@@ -145,8 +146,8 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
 
           @Override
           public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
-            // if any non enemy territories on route or if any enemy units on route the battles
-            // on (note water could have enemy but its not owned)
+            // if any non-enemy territories on route or if any enemy units on route the battles
+            // on (note water could have enemy, but it's not owned)
             final GameData data = bridge.getData();
             final Predicate<Territory> mustFightThrough = getMustFightThroughMatch(gamePlayer);
             final Collection<Unit> arrived =
@@ -206,7 +207,7 @@ public void execute(final ExecutionStack stack, final IDelegateBridge bridge) {
               // if it's all bombers and there's something to bomb
               if (allCanBomb && targetsOrEscort && GameStepPropertiesHelper.isCombatMove(data)) {
                 final boolean bombing = getRemotePlayer().shouldBomberBomb(route.getEnd());
-                // if bombing and there's something to target- ask what to bomb
+                // if bombing and there's something to target - ask what to bomb
                 if (bombing) {
                   // CompositeMatchOr<Unit> unitsToBeBombed = new
                   // CompositeMatchOr<Unit>(Matches.UnitIsFactory,
@@ -353,7 +354,7 @@ private Change markMovementChange(
               unit, moved.add(unit.getAlreadyMoved()), Unit.ALREADY_MOVED));
     }
     // if neutrals were taken over mark land units with 0 movement
-    // if entered a non blitzed conquered territory, mark with 0 movement
+    // if entered a non-blitzed conquered territory, mark with 0 movement
     if (GameStepPropertiesHelper.isCombatMove(data)
         && (!MoveDelegate.getEmptyNeutral(route).isEmpty() || hasConqueredNonBlitzed(route))) {
       for (final Unit unit : CollectionUtils.getMatches(units, Matches.unitIsLand())) {

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AbstractBattle.java

@@ -199,11 +199,6 @@ public final boolean isOver() {
   @Override
   public void cancelBattle(final IDelegateBridge bridge) {}
 
-  @Override
-  public boolean isBombingRun() {
-    return battleType.isBombingRun();
-  }
-
   @Override
   public BattleType getBattleType() {
     return battleType;

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/AirBattle.java

@@ -106,8 +106,9 @@ public Change addAttackChange(
   }
 
   @Override
-  public void removeAttack(final Route route, final Collection<Unit> units) {
+  public Change removeAttack(final Route route, final Collection<Unit> units) {
     attackingUnits.removeAll(units);
+    return new CompositeChange();
   }
 
   @Override
@@ -341,14 +342,11 @@ private void makeBattle(final IDelegateBridge bridge) {
       recordUnitsWereInAirBattle(attackingUnits, bridge);
       recordUnitsWereInAirBattle(defendingUnits, bridge);
     }
-    // so as of right now, Air Battles are created before both normal battles and strategic bombing
-    // raids
-    // once completed, the air battle will create a strategic bombing raid, if that is the purpose
-    // of those aircraft
-    // however, if the purpose is a normal battle, it will have already been created by the battle
-    // tracker / combat move
-    // so we do not have to create normal battles, only bombing raids
-    // setup new battle here
+    // As of right now, Air Battles are created before both normal battles and strategic bombing
+    // raids. Once completed, the air battle will create a strategic bombing raid, if that is the
+    // purpose of those aircraft. However, if the purpose is a normal battle, it will have already
+    // been created by the battle tracker / combat move.
+    // So we do not have to create normal battles, only bombing raids setup new battle here.
     if (isBombingRun) {
       final Collection<Unit> bombers =
           CollectionUtils.getMatches(attackingUnits, Matches.unitIsStrategicBomber());

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/DependentBattle.java

@@ -1,11 +1,14 @@
 package games.strategy.triplea.delegate.battle;
 
+import games.strategy.engine.data.Change;
+import games.strategy.engine.data.CompositeChange;
 import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.Route;
 import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.Unit;
 import games.strategy.triplea.delegate.Matches;
+import games.strategy.triplea.delegate.TransportTracker;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -31,11 +34,12 @@ public abstract class DependentBattle extends AbstractBattle {
   }
 
   @Override
-  public void removeAttack(final Route route, final Collection<Unit> units) {
+  public Change removeAttack(final Route route, final Collection<Unit> units) {
+    // Note: This code is also duplicated in FinishedBattle, which is not a subclass.
     attackingUnits.removeAll(units);
     // the route could be null, in the case of a unit in a territory where a sub is submerged.
     if (route == null) {
-      return;
+      return new CompositeChange();
     }
     final Territory attackingFrom = route.getTerritoryBeforeEnd();
     final Collection<Unit> attackingFromMapUnits =
@@ -49,11 +53,14 @@ public void removeAttack(final Route route, final Collection<Unit> units) {
       // if none of the units is a land unit, the attack from that territory is no longer an
       // amphibious assault
       if (attackingFromMapUnits.stream().noneMatch(Matches.unitIsLand())) {
-        getAmphibiousAttackTerritories().remove(attackingFrom);
+        amphibiousAttackFrom.remove(attackingFrom);
         // do we have any amphibious attacks left?
-        isAmphibious = !getAmphibiousAttackTerritories().isEmpty();
+        isAmphibious = !amphibiousAttackFrom.isEmpty();
       }
     }
+    // Clear transportedBy for allied air on carriers as these are only set during the battle.
+    return TransportTracker.clearTransportedByForAlliedAirOnCarrier(
+        units, battleSite, attacker, gameData);
   }
 
   /** Return attacking from Collection. */

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/FinishedBattle.java

@@ -1,6 +1,7 @@
 package games.strategy.triplea.delegate.battle;
 
 import games.strategy.engine.data.Change;
+import games.strategy.engine.data.CompositeChange;
 import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.Route;
@@ -10,6 +11,7 @@
 import games.strategy.engine.data.changefactory.ChangeFactory;
 import games.strategy.engine.delegate.IDelegateBridge;
 import games.strategy.triplea.delegate.Matches;
+import games.strategy.triplea.delegate.TransportTracker;
 import games.strategy.triplea.delegate.data.BattleRecord;
 import games.strategy.triplea.delegate.data.BattleRecord.BattleResultDescription;
 import games.strategy.triplea.util.TuvUtils;
@@ -90,29 +92,33 @@ public Change addAttackChange(
   }
 
   @Override
-  public void removeAttack(final Route route, final Collection<Unit> units) {
+  public Change removeAttack(final Route route, final Collection<Unit> units) {
+    // Note: This is the same code as in DependentBattle, but this is not a subclass.
     attackingUnits.removeAll(units);
     // the route could be null, in the case of a unit in a territory where a sub is submerged.
     if (route == null) {
-      return;
+      return new CompositeChange();
     }
     final Territory attackingFrom = route.getTerritoryBeforeEnd();
-    Collection<Unit> attackingFromMapUnits = attackingFromMap.get(attackingFrom);
-    // handle possible null pointer
-    if (attackingFromMapUnits == null) {
-      attackingFromMapUnits = new ArrayList<>();
-    }
+    final Collection<Unit> attackingFromMapUnits =
+        attackingFromMap.getOrDefault(attackingFrom, new ArrayList<>());
     attackingFromMapUnits.removeAll(units);
+    if (attackingFromMapUnits.isEmpty()) {
+      this.attackingFromMap.remove(attackingFrom);
+    }
     // deal with amphibious assaults
     if (attackingFrom.isWater()) {
-      // if none of the units is a land unit, the attack from
-      // that territory is no longer an amphibious assault
+      // if none of the units is a land unit, the attack from that territory is no longer an
+      // amphibious assault
       if (attackingFromMapUnits.stream().noneMatch(Matches.unitIsLand())) {
         amphibiousAttackFrom.remove(attackingFrom);
         // do we have any amphibious attacks left?
         isAmphibious = !amphibiousAttackFrom.isEmpty();
       }
     }
+    // Clear transportedBy for allied air on carriers as these are only set during the battle.
+    return TransportTracker.clearTransportedByForAlliedAirOnCarrier(
+        units, battleSite, attacker, gameData);
   }
 
   @Override

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/IBattle.java

@@ -29,7 +29,7 @@ enum WhoWon {
     DEFENDER
   }
 
-  /** The type of a battle. */
+  /** The type of battle. */
   @AllArgsConstructor
   @ToString(of = "type")
   enum BattleType {
@@ -68,14 +68,6 @@ public String toDisplayText() {
    */
   Change addAttackChange(Route route, Collection<Unit> units, Map<Unit, Set<Unit>> targets);
 
-  /**
-   * There are two distinct super-types of battles: Bombing battles, and Fighting battles. There may
-   * be sub-types of each of these.
-   *
-   * @return whether this battle is a bombing run
-   */
-  boolean isBombingRun();
-
   /** The type of battle occurring, example: MustFightBattle, StrategicBombingRaidBattle, etc. */
   BattleType getBattleType();
 
@@ -112,8 +104,9 @@ void unitsLostInPrecedingBattle(
    *
    * @param route - attacking route
    * @param units - attacking units
+   * @return any changes to be performed as a result.
    */
-  void removeAttack(Route route, Collection<Unit> units);
+  Change removeAttack(Route route, Collection<Unit> units);
 
   /** If we need to cancel the battle, we may need to perform some cleanup. */
   void cancelBattle(IDelegateBridge bridge);

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/NonFightingBattle.java

@@ -56,7 +56,7 @@ public void fight(final IDelegateBridge bridge) {
     }
     // create event
     bridge.getHistoryWriter().startEvent("Battle in " + battleSite, battleSite);
-    // if any attacking non air units then win
+    // if any attacking non-air units then win
     final boolean someAttacking = hasAttackingUnits();
     if (someAttacking) {
       whoWon = WhoWon.ATTACKER;

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java

@@ -2,6 +2,7 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import games.strategy.engine.data.Change;
+import games.strategy.engine.data.CompositeChange;
 import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.data.Resource;
@@ -134,8 +135,9 @@ public boolean isEmpty() {
   }
 
   @Override
-  public void removeAttack(final Route route, final Collection<Unit> units) {
+  public Change removeAttack(final Route route, final Collection<Unit> units) {
     removeAttackers(units, true);
+    return new CompositeChange();
   }
 
   private void removeAttackers(final Collection<Unit> units, final boolean removeTarget) {

game-app/game-core/src/main/java/games/strategy/triplea/delegate/battle/steps/retreat/RetreaterGeneral.java

@@ -132,7 +132,7 @@ private Change retreatCombatTransportedItems(
     for (final IBattle dependent : dependentBattles) {
       final Route route = new Route(battleState.getBattleSite(), dependent.getTerritory());
       final Collection<Unit> retreatedUnits = dependent.getDependentUnits(units);
-      dependent.removeAttack(route, retreatedUnits);
+      change.add(dependent.removeAttack(route, retreatedUnits));
       TransportTracker.reloadTransports(units, change);
       change.add(ChangeFactory.moveUnits(dependent.getTerritory(), retreatTo, retreatedUnits));
     }

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/FinishedBattleTest.java

@@ -67,7 +67,7 @@ void testTransportedByClearedAfterDependentBattle() {
     assertEquals(null, tank.getTransportedBy());
   }
 
-  private Unit getSingleUnit(Territory t, UnitType type) {
+  public static Unit getSingleUnit(Territory t, UnitType type) {
     Collection<Unit> units = CollectionUtils.getMatches(t.getUnits(), Matches.unitIsOfType(type));
     assertEquals(1, units.size());
     return CollectionUtils.getAny(units);

game-app/game-core/src/test/java/games/strategy/triplea/delegate/battle/MustFightBattleTest.java

@@ -4,26 +4,33 @@
 import static games.strategy.triplea.delegate.GameDataTestUtil.artillery;
 import static games.strategy.triplea.delegate.GameDataTestUtil.battleDelegate;
 import static games.strategy.triplea.delegate.GameDataTestUtil.britain;
+import static games.strategy.triplea.delegate.GameDataTestUtil.british;
 import static games.strategy.triplea.delegate.GameDataTestUtil.britishArtillery;
 import static games.strategy.triplea.delegate.GameDataTestUtil.britishInfantry;
+import static games.strategy.triplea.delegate.GameDataTestUtil.carrier;
 import static games.strategy.triplea.delegate.GameDataTestUtil.chinese;
 import static games.strategy.triplea.delegate.GameDataTestUtil.fighter;
+import static games.strategy.triplea.delegate.GameDataTestUtil.french;
+import static games.strategy.triplea.delegate.GameDataTestUtil.germans;
 import static games.strategy.triplea.delegate.GameDataTestUtil.infantry;
 import static games.strategy.triplea.delegate.GameDataTestUtil.japan;
 import static games.strategy.triplea.delegate.GameDataTestUtil.japaneseInfantry;
 import static games.strategy.triplea.delegate.GameDataTestUtil.move;
 import static games.strategy.triplea.delegate.GameDataTestUtil.moveDelegate;
 import static games.strategy.triplea.delegate.GameDataTestUtil.removeFrom;
 import static games.strategy.triplea.delegate.GameDataTestUtil.territory;
+import static games.strategy.triplea.delegate.GameDataTestUtil.transport;
 import static games.strategy.triplea.delegate.MockDelegateBridge.advanceToStep;
 import static games.strategy.triplea.delegate.MockDelegateBridge.newDelegateBridge;
 import static games.strategy.triplea.delegate.MockDelegateBridge.thenGetRandomShouldHaveBeenCalled;
 import static games.strategy.triplea.delegate.MockDelegateBridge.whenGetRandom;
 import static games.strategy.triplea.delegate.MockDelegateBridge.withDiceValues;
 import static games.strategy.triplea.delegate.MockDelegateBridge.withValues;
+import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.in;
+import static org.hamcrest.core.Is.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.mockito.ArgumentMatchers.any;
@@ -44,6 +51,7 @@
 import games.strategy.triplea.attachments.UnitSupportAttachment;
 import games.strategy.triplea.delegate.AbstractMoveDelegate;
 import games.strategy.triplea.delegate.GameDataTestUtil;
+import games.strategy.triplea.delegate.MoveDelegate;
 import games.strategy.triplea.settings.AbstractClientSettingTestCase;
 import games.strategy.triplea.xml.TestMapGameData;
 import java.util.Collection;
@@ -213,6 +221,44 @@ void testStepNamesChangeDuringCombat() {
     assertThat(indoChina.getUnits(), containsInAnyOrder(attackers.toArray()));
   }
 
+  @Test
+  void testAlliedCarriedPlanesTransportedByIsResetWhenCancelingBattle() throws Exception {
+    // Note: Test uses germans, british and france since other countries aren't at war on t1.
+    final GameData gameData = TestMapGameData.GLOBAL1940.getGameData();
+
+    // SZ 45 has a german transport.
+    final Territory sz45 = territory("45 Sea Zone", gameData);
+    removeFrom(sz45, sz45.getUnits());
+    addTo(sz45, transport(gameData).create(1, germans(gameData)));
+
+    // SZ 46 has a british carrier with 2 french planes.
+    final Territory sz46 = territory("46 Sea Zone", gameData);
+    removeFrom(sz46, sz46.getUnits());
+    Unit carrier = carrier(gameData).create(1, british(gameData)).get(0);
+    addTo(sz46, List.of(carrier));
+    List<Unit> fighters = fighter(gameData).create(2, french(gameData));
+    addTo(sz46, fighters);
+
+    final Territory sz42 = territory("42 Sea Zone", gameData);
+
+    final IDelegateBridge bridge = newDelegateBridge(british(gameData));
+    advanceToStep(bridge, "CombatMove");
+    MoveDelegate moveDelegate = GameDataTestUtil.moveDelegate(gameData);
+    moveDelegate.setDelegateBridgeAndPlayer(bridge);
+    moveDelegate.start();
+
+    Collection<Unit> units = List.of(carrier, fighters.get(0), fighters.get(1));
+    // For the battle, transportedBy will be set to the carrier so that it's shown in the UI and
+    // the units destroyed if the carrier is sunk.
+    GameDataTestUtil.move(units, new Route(sz46, sz45));
+    assertThat(fighters.get(0).getTransportedBy(), is(carrier));
+    assertThat(fighters.get(1).getTransportedBy(), is(carrier));
+    // But if the units move out, then transportedBy should be cleared.
+    GameDataTestUtil.move(units, new Route(sz45, sz42));
+    assertThat(fighters.get(0).getTransportedBy(), is(nullValue()));
+    assertThat(fighters.get(1).getTransportedBy(), is(nullValue()));
+  }
+
   private static <T> void setPropertyValue(GameData gameData, String propertyName, T value) {
     IEditableProperty<T> property =
         (IEditableProperty<T>)