Issue 12447 part2 (#12783)

* Reapply "Issues 12447 (Standardized Sorting of UnitCategory) (#12722)" (#12762)

This reverts commit e73fdec.

* Issue_12447_Part2 #1

PlayerUnitsPanel.java
- ensure uniqueness in returned list in method getAllUnitCategories
- reduce complexity by use of new method getProducibleUnitTypes
ProductionFrontier.java
- new method getProducibleUnitTypes

game-app/game-core/src/main/java/games/strategy/engine/data/ProductionFrontier.java

@@ -3,6 +3,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
@@ -50,6 +51,21 @@ public List<ProductionRule> getRules() {
     return cachedRules;
   }
 
+  /**
+   * @return Collection of <code>UnitType</code> that can be produced by this frontier
+   */
+  public Collection<UnitType> getProducibleUnitTypes() {
+    Collection<UnitType> producibleUnitTypes = new ArrayList<>();
+    for (final ProductionRule rule : this) {
+      for (final NamedAttachable type : rule.getResults().keySet()) {
+        if (type instanceof UnitType) {
+          producibleUnitTypes.add((UnitType) type);
+        }
+      }
+    }
+    return producibleUnitTypes;
+  }
+
   @Override
   public Iterator<ProductionRule> iterator() {
     return getRules().iterator();

game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java

@@ -417,12 +417,17 @@ class BattleCalculatorPanel extends JPanel {
                   Matches.unitIsOwnedBy(getDefender())
                       .and(
                           Matches.unitCanBeInBattle(
-                              true, isLand(), 1, hasMaxRounds(isLand(), data), true, List.of())));
+                              true,
+                              isLandBattle(),
+                              1,
+                              hasMaxRounds(isLandBattle(), data),
+                              true,
+                              List.of())));
           final GamePlayer newDefender = getAttacker();
           final List<Unit> newDefenders =
               CollectionUtils.getMatches(
                   attackingUnitsPanel.getUnits(),
-                  Matches.unitCanBeInBattle(true, isLand(), 1, true));
+                  Matches.unitCanBeInBattle(true, isLandBattle(), 1, true));
           setAttacker(newAttacker);
           setDefender(newDefender);
           setAttackingUnits(newAttackers);
@@ -435,8 +440,8 @@ class BattleCalculatorPanel extends JPanel {
               new OrderOfLossesInputPanel(
                   attackerOrderOfLosses,
                   defenderOrderOfLosses,
-                  attackingUnitsPanel.getCategories(),
-                  defendingUnitsPanel.getCategories(),
+                  attackingUnitsPanel.getUnitCategories(),
+                  defendingUnitsPanel.getUnitCategories(),
                   landBattleCheckBox.isSelected(),
                   uiContext,
                   data);
@@ -559,12 +564,12 @@ private void updateStats() {
           try {
             final Territory location = findPotentialBattleSite();
             if (location == null) {
-              throw new IllegalStateException("No territory found that is land:" + isLand());
+              throw new IllegalStateException("No territory found that is land:" + isLandBattle());
             }
             final List<Unit> defending = defendingUnitsPanel.getUnits();
             final List<Unit> attacking = attackingUnitsPanel.getUnits();
             List<Unit> bombarding = new ArrayList<>();
-            if (isLand()) {
+            if (isLandBattle()) {
               bombarding =
                   CollectionUtils.getMatches(attacking, Matches.unitCanBombard(getAttacker()));
               attacking.removeAll(bombarding);
@@ -622,7 +627,7 @@ private void updateStats() {
     attackerWin.setText(formatPercentage(results.getAttackerWinPercent()));
     defenderWin.setText(formatPercentage(results.getDefenderWinPercent()));
     draw.setText(formatPercentage(results.getDrawPercent()));
-    final boolean isLand = isLand();
+    final boolean isLand = isLandBattle();
     final List<Unit> mainCombatAttackers =
         CollectionUtils.getMatches(
             attackers.get(), Matches.unitCanBeInBattle(true, isLand, 1, true));
@@ -658,9 +663,9 @@ private void updateStats() {
 
   private Territory findPotentialBattleSite() {
     Territory location = null;
-    if (this.location == null || this.location.isWater() == isLand()) {
+    if (this.location == null || this.location.isWater() == isLandBattle()) {
       for (final Territory t : data.getMap()) {
-        if (t.isWater() == !isLand()) {
+        if (t.isWater() == !isLandBattle()) {
           location = t;
           break;
         }
@@ -693,8 +698,9 @@ private void setAttackingUnits(final List<Unit> initialUnits) {
         CollectionUtils.getMatches(
             units,
             Matches.unitCanBeInBattle(
-                true, isLand(), 1, hasMaxRounds(isLand(), data), false, List.of())),
-        isLand());
+                true, isLandBattle(), 1, hasMaxRounds(isLandBattle(), data), false, List.of())),
+        isLandBattle(),
+        location);
   }
 
   void addDefendingUnits(final List<Unit> unitsToAdd) {
@@ -708,8 +714,10 @@ private void setDefendingUnits(final List<Unit> initialUnits) {
     final List<Unit> units = Optional.ofNullable(initialUnits).orElseGet(List::of);
     defendingUnitsPanel.init(
         getDefender(),
-        CollectionUtils.getMatches(units, Matches.unitCanBeInBattle(false, isLand(), 1, false)),
-        isLand());
+        CollectionUtils.getMatches(
+            units, Matches.unitCanBeInBattle(false, isLandBattle(), 1, false)),
+        isLandBattle(),
+        location);
   }
 
   public boolean hasAttackingUnitsAdded() {
@@ -720,7 +728,7 @@ public boolean hasDefendingUnitsAdded() {
     return !defendingUnitsPanel.isEmpty();
   }
 
-  private boolean isLand() {
+  private boolean isLandBattle() {
     return landBattleCheckBox.isSelected();
   }
 
@@ -742,7 +750,7 @@ private void setResultsToBlank() {
   private void setWidgetActivation() {
     keepOneAttackingLandUnitCheckBox.setEnabled(landBattleCheckBox.isSelected());
     amphibiousCheckBox.setEnabled(landBattleCheckBox.isSelected());
-    final boolean isLand = isLand();
+    final boolean isLand = isLandBattle();
     try (GameData.Unlocker ignored = data.acquireReadLock()) {
       final List<Unit> attackers =
           CollectionUtils.getMatches(

game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/OrderOfLossesInputPanel.java

@@ -12,6 +12,7 @@
 import games.strategy.triplea.image.UnitImageFactory.ImageKey;
 import games.strategy.triplea.ui.UiContext;
 import games.strategy.triplea.util.UnitCategory;
+import games.strategy.triplea.util.UnitSeparator;
 import java.awt.Color;
 import java.awt.Component;
 import java.util.ArrayList;
@@ -252,12 +253,13 @@ private void layoutComponents() {
   }
 
   private JPanel getUnitButtonPanel(
-      final List<UnitCategory> categories, final JTextField textField) {
+      final List<UnitCategory> unitCategories, final JTextField textField) {
     final JPanel panel = new JPanel();
     panel.setLayout(new BoxLayout(panel, BoxLayout.X_AXIS));
-    if (categories != null) {
+    if (unitCategories != null) {
+      UnitSeparator.sortUnitCategories(unitCategories, data);
       final Set<UnitType> typesUsed = new HashSet<>();
-      for (final UnitCategory category : categories) {
+      for (final UnitCategory category : unitCategories) {
         // no duplicates or infrastructure allowed. no sea if land, no land if sea.
         if (typesUsed.contains(category.getType())
             || Matches.unitTypeIsInfrastructure().test(category.getType())

game-app/game-core/src/main/java/games/strategy/triplea/odds/calculator/PlayerUnitsPanel.java

@@ -2,13 +2,10 @@
 
 import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
-import games.strategy.engine.data.NamedAttachable;
 import games.strategy.engine.data.ProductionFrontier;
-import games.strategy.engine.data.ProductionRule;
 import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.Unit;
 import games.strategy.engine.data.UnitType;
-import games.strategy.triplea.attachments.UnitAttachment;
 import games.strategy.triplea.delegate.Matches;
 import games.strategy.triplea.ui.UiContext;
 import games.strategy.triplea.util.TuvCostsCalculator;
@@ -34,8 +31,8 @@ public class PlayerUnitsPanel extends JPanel {
   private final GameData data;
   private final UiContext uiContext;
   private final boolean defender;
-  private boolean isLand = true;
-  @Getter private List<UnitCategory> categories = null;
+  private boolean isLandBattle = true;
+  @Getter private List<UnitCategory> unitCategories = null;
   private final List<Runnable> listeners = new ArrayList<>();
   private final List<UnitPanel> unitPanels = new ArrayList<>();
 
@@ -59,54 +56,17 @@ public List<Unit> getUnits() {
   }
 
   /** Sets up components to an initial state. */
-  public void init(final GamePlayer gamePlayer, final List<Unit> units, final boolean land) {
-    isLand = land;
+  public void init(
+      final GamePlayer gamePlayer,
+      final List<Unit> units,
+      final boolean isLandBattle,
+      final Territory territory) {
+    this.isLandBattle = isLandBattle;
     unitPanels.clear();
-    categories = new ArrayList<>(categorize(gamePlayer, units));
-
-    categories.sort(
-        (c1, c2) -> {
-          if (!c1.isOwnedBy(c2.getOwner())) {
-            if (c1.isOwnedBy(gamePlayer)) {
-              return -1;
-            } else if (c2.isOwnedBy(gamePlayer)) {
-              return 1;
-            } else {
-              return c1.getOwner().getName().compareTo(c2.getOwner().getName());
-            }
-          }
-          final UnitType ut1 = c1.getType();
-          final UnitType ut2 = c2.getType();
-          final UnitAttachment u1 = ut1.getUnitAttachment();
-          final UnitAttachment u2 = ut2.getUnitAttachment();
-          // For land battles, sort by land, air, can't combat move (AA), bombarding
-          if (land) {
-            if (u1.getIsSea() != u2.getIsSea()) {
-              return u1.getIsSea() ? 1 : -1;
-            }
-            final boolean u1CanNotCombatMove =
-                Matches.unitTypeCanNotMoveDuringCombatMove().test(ut1)
-                    || !Matches.unitTypeCanMove(gamePlayer).test(ut1);
-            final boolean u2CanNotCombatMove =
-                Matches.unitTypeCanNotMoveDuringCombatMove().test(ut2)
-                    || !Matches.unitTypeCanMove(gamePlayer).test(ut2);
-            if (u1CanNotCombatMove != u2CanNotCombatMove) {
-              return u1CanNotCombatMove ? 1 : -1;
-            }
-            if (u1.getIsAir() != u2.getIsAir()) {
-              return u1.getIsAir() ? 1 : -1;
-            }
-          } else {
-            if (u1.getIsSea() != u2.getIsSea()) {
-              return u1.getIsSea() ? -1 : 1;
-            }
-          }
-          return u1.getName().compareTo(u2.getName());
-        });
 
     removeAll();
     final Predicate<UnitType> predicate;
-    if (land) {
+    if (isLandBattle) {
       if (defender) {
         predicate = Matches.unitTypeIsNotSea();
       } else {
@@ -120,9 +80,11 @@ public void init(final GamePlayer gamePlayer, final List<Unit> units, final bool
       costs = new TuvCostsCalculator().getCostsForTuv(gamePlayer);
     }
 
+    unitCategories = getAllUnitCategories(gamePlayer, units);
+    UnitSeparator.sortUnitCategories(unitCategories, territory, gamePlayer);
     GamePlayer previousPlayer = null;
     JPanel panel = null;
-    for (final UnitCategory category : categories) {
+    for (final UnitCategory category : unitCategories) {
       if (predicate.test(category.getType())) {
         if (!category.isOwnedBy(previousPlayer)) {
           panel = new JPanel();
@@ -131,10 +93,12 @@ public void init(final GamePlayer gamePlayer, final List<Unit> units, final bool
           add(panel);
           previousPlayer = category.getOwner();
         }
-        final var unitPanel = new UnitPanel(uiContext, category, costs);
-        unitPanel.addChangeListener(this::notifyListeners);
-        panel.add(unitPanel);
-        unitPanels.add(unitPanel);
+        if (panel != null) {
+          final var unitPanel = new UnitPanel(uiContext, category, costs);
+          unitPanel.addChangeListener(this::notifyListeners);
+          panel.add(unitPanel);
+          unitPanels.add(unitPanel);
+        }
       }
     }
 
@@ -150,10 +114,11 @@ public void init(final GamePlayer gamePlayer, final List<Unit> units, final bool
    * production frontier and then any unit types the player owns on the map. Then populate the list
    * of units into the categories.
    */
-  private Set<UnitCategory> categorize(final GamePlayer gamePlayer, final List<Unit> units) {
+  private List<UnitCategory> getAllUnitCategories(
+      final GamePlayer gamePlayer, final List<Unit> units) {
 
     // Get player unit types from production frontier and unit types on the map
-    final Set<UnitCategory> categories = new LinkedHashSet<>();
+    final List<UnitCategory> categories = new ArrayList<>();
     for (final UnitType t : getUnitTypes(gamePlayer)) {
       final UnitCategory category = new UnitCategory(t, gamePlayer);
       categories.add(category);
@@ -172,35 +137,32 @@ private Set<UnitCategory> categorize(final GamePlayer gamePlayer, final List<Uni
     }
 
     // Populate units into each category then add any remaining categories (damaged units, etc)
-    final Set<UnitCategory> unitCategories = UnitSeparator.categorize(units);
-    for (final UnitCategory category : categories) {
-      for (final UnitCategory unitCategory : unitCategories) {
-        if (category.equals(unitCategory)) {
-          category.getUnits().addAll(unitCategory.getUnits());
-        }
+    final Set<UnitCategory> unitCategoriesWithUnits = UnitSeparator.categorize(units);
+    for (final UnitCategory unitCategoryWithUnits : unitCategoriesWithUnits) {
+      int categoryIndex = categories.indexOf(unitCategoryWithUnits);
+      if (categoryIndex > 0) {
+        categories.get(categoryIndex).getUnits().addAll(unitCategoryWithUnits.getUnits());
+      } else {
+        categories.add(unitCategoryWithUnits);
       }
     }
-    categories.addAll(unitCategories);
 
     return categories;
   }
 
   /**
    * Return all the unit types available for the given player. A unit type is available if the unit
-   * can be purchased or if a player has one on the map.
+   * can be produced/purchased or if a player has one on the map.
    */
   private Collection<UnitType> getUnitTypes(final GamePlayer player) {
     Collection<UnitType> unitTypes = new LinkedHashSet<>();
+
     final ProductionFrontier frontier = player.getProductionFrontier();
     if (frontier != null) {
-      for (final ProductionRule rule : frontier) {
-        for (final NamedAttachable type : rule.getResults().keySet()) {
-          if (type instanceof UnitType) {
-            unitTypes.add((UnitType) type);
-          }
-        }
-      }
+      unitTypes.addAll(frontier.getProducibleUnitTypes());
     }
+
+    // get any unit on the map and check
     for (final Territory t : data.getMap()) {
       for (final Unit u : t.getUnitCollection()) {
         if (u.isOwnedBy(player)) {
@@ -216,10 +178,10 @@ private Collection<UnitType> getUnitTypes(final GamePlayer player) {
             unitTypes,
             Matches.unitTypeCanBeInBattle(
                 !defender,
-                isLand,
+                isLandBattle,
                 player,
                 1,
-                BattleCalculatorPanel.hasMaxRounds(isLand, data),
+                BattleCalculatorPanel.hasMaxRounds(isLandBattle, data),
                 false,
                 List.of()));
 

game-app/game-core/src/main/java/games/strategy/triplea/ui/BattleDisplay.java

@@ -47,6 +47,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
 import javax.annotation.Nullable;
 import javax.swing.AbstractAction;
 import javax.swing.Action;
@@ -230,10 +232,10 @@ private Collection<Unit> updateKilledUnits(
     for (final Collection<Unit> dependentCollection : dependentsMap.values()) {
       dependentUnitsReturned.addAll(dependentCollection);
     }
-    for (final UnitCategory category :
-        UnitSeparator.categorize(
-            killedUnits,
-            UnitSeparator.SeparatorCategories.builder().dependents(dependentsMap).build())) {
+
+    List<UnitCategory> unitCategories =
+        UnitSeparator.getSortedUnitCategories(killedUnits, gameData, uiContext.getMapData());
+    for (final UnitCategory category : unitCategories) {
       final JPanel panel = new JPanel();
       JLabel unit = uiContext.newUnitImageLabel(category.getType(), category.getOwner());
       panel.add(unit);
@@ -775,7 +777,14 @@ void setNotificationShort(
 
     private void categorizeUnits(
         final Iterable<UnitCategory> categoryIter, final boolean damaged, final boolean disabled) {
-      for (final UnitCategory category : categoryIter) {
+      final List<UnitCategory> unitCategories =
+          StreamSupport.stream(categoryIter.spliterator(), false).collect(Collectors.toList());
+      if (unitCategories.isEmpty()) {
+        return;
+      }
+      final GameData gameData = unitCategories.get(0).getUnitAttachment().getData();
+      UnitSeparator.sortUnitCategories(unitCategories, gameData);
+      for (final UnitCategory category : unitCategories) {
         final JPanel panel = new JPanel();
         final ImageIcon unitImage =
             uiContext