Issues 12447 (Standardized Sorting of UnitCategory) #12722

frigoref · 2024-07-13T22:12:42Z

Issue 12447 was mentioning the sorting of units being different in different UI areas.
This PR tries to unify the sorting (based on UnitCategory) used before adding UI components to ensure a standardized sorting.
Test was done with UnitChooser, TerritoryPanel, Battle Calculator and the UnitScoller for map 1941 Global Command Decision:

One thing I was not so sure about is the way gameData is selected in UnitChooser.java by picking the first entry of a list.
Maybe there is a better way to do that which can be suggested by the reviewer.

The commit comment describes the changes:

AbstractUndoableMovesPanel.java
-unitCategory sorting with gameData

AvatarPanelFactory.java
-passing territory
-replace sorting by unitRenderingOrder with new generic sorting with territory and player 

BattleCalculatorPanel.java
-rename method to getUnitCategories

BattleDisplay.java (2x)
BattleModel.java
MapPanel.java
OrderOfLossesInputPanel.java
PlayerUnitsPanel.java
SimpleUnitPanel.java
UnitChooser.java
-unitCategory sorting with gameData

UnitScroller.java
-passing territory

UnitSeparator.java
-core Comparator for sorting by UnitCategory: getComparatorUnitCategories
-several overloaded methods getComparatorUnitCategories

PS: I will try next time to chop it into smaller pieces. It just grew somehow.

AbstractUndoableMovesPanel.java -unitCategory sorting with gameData AvatarPanelFactory.java -passing territory -replace sorting by unitRenderingOrder with new generic sorting with territory and player BattleCalculatorPanel.java -rename method to getUnitCategories BattleDisplay.java (2x) BattleModel.java MapPanel.java OrderOfLossesInputPanel.java PlayerUnitsPanel.java SimpleUnitPanel.java UnitChooser.java -unitCategory sorting with gameData UnitScroller.java -passing territory UnitSeparator.java -core Comparator for sorting by UnitCategory: getComparatorUnitCategories -several overloaded methods getComparatorUnitCategories

AvatarPanelFactory.java -remove now unused method unitRenderingOrder

AbstractUndoableMovesPanel.java -Insert <> for ArrayList instantiation -fix format violations with game-app:game-headed:spotlessApply

AbstractUndoableMovesPanel.java -fix format violations with game-app:game-headed:spotlessApply

DanVanAtta · 2024-07-21T17:17:43Z

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

@@ -1,5 +1,7 @@
 package games.strategy.triplea.odds.calculator;

+import static games.strategy.triplea.util.UnitSeparator.getComparatorUnitCategories;


style nitpick, avoid static method imports unless in test code and importing things like assertions. The reasoning is that when inspecting the code, the method call looks like a call to a local method or to a super class.

It might be even cleaner to have a static method in UnitSeparato to do the sorting, rather than returning the comparator to then let the client code to the sorting. EG: UnitSeparator.sort( <unit-type-list> )

DanVanAtta · 2024-07-21T17:19:56Z

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

@@ -122,7 +124,8 @@ public void init(final GamePlayer gamePlayer, final List<Unit> units, final bool

    GamePlayer previousPlayer = null;
    JPanel panel = null;
-    for (final UnitCategory category : categories) {
+    unitCategories.sort(getComparatorUnitCategories(data));


There is a sort of unitCategories higher up on line 69. Does this new sort call here, on line 127, overwrite the results of that previous sort?

Good point. Another place where we can remove a "special" sorting with our generic one. Fix will be provided in commit #6.

DanVanAtta · 2024-07-21T17:23:43Z

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

+      if (unitCategories.isEmpty()) {
+        return;
+      }
+      final GameData gameData = unitCategories.get(0).getUnitAttachment().getData();


BattleDisplay:L95 has a reference to GameData - can that be used?

It cannot as the changed code is in private static class CasualtyNotificationPanel for which I could not find GameData.

NP - please double check the nullability of unitAttachment#data. I think grabbing gameData like this should be 'safe', but let's be sure.

What should happen if unitAttachment = null ? For the sorting a different gameData source would be needed, e.g. from unitAttachment#unitType? If all potential sources could be null, we cannot escape this what-if-question.

I looked into this a bit further. You're good here W.R.T gameData. I'll post some more followup in a bit. We can assume that gameData will be non-null. I presume it is already guaranteed for unitAttachment to be non-null as well.

Correction.. tracing this down... Neutral units in 2.5 have null gameData

If you save a game that has neutral players with 2.5, then load that up with 2.6, then find a territory with only neutral units - in that case gameData would seemingly be null.

Happily that nullity concern will go away kinda soon as how the neutral player is saved to disk was changed. Once we no longer need to load saves from 2.5, that gameData would never be null.

DanVanAtta · 2024-07-21T17:25:09Z

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

@@ -120,7 +125,13 @@ public void setUnitsFromRepairRuleMap(
   */
  public void setUnitsFromCategories(final Collection<UnitCategory> categories) {
    removeAll();
-    for (final UnitCategory category : categories) {
+    List<UnitCategory> unitCategories = new ArrayList(categories);
+    if (unitCategories.isEmpty()) {


minor, seems like we can check if categories is empty first & return sooner (ie: this line can be moved up by one, and check categories instead of unitCategories

Agree, I create a fix.

DanVanAtta · 2024-07-21T17:29:05Z

game-app/game-core/src/main/java/games/strategy/triplea/ui/panels/map/MapPanel.java

+      List<UnitCategory> unitCategories = new ArrayList(UnitSeparator.categorize(value));
+      unitCategories.sort(getComparatorUnitCategories(gameData));


There is a little bit of a pattern here. Perhaps this can be cleaned up to be something like:
List<unitCategory> unitCategories = UnitSeparator.sortByUnitType(value)

Which in turn would do the list copy, categorize, and sorting.
FWIW, there is a bit of a feature envy code smell here where UnitSeparator is used to generate a return value, more data is obtained from UnitSeparator to then operate on that same data again. Resolving 'feature envy code smell' is usually done by moving the functionality entirely into UnitSeparator

Agree, I create a fix to introduce another method UnitSeparator.getSortedUnitCategories.

...-app/game-core/src/main/java/games/strategy/triplea/ui/unit/scroller/AvatarPanelFactory.java

DanVanAtta · 2024-07-21T17:32:39Z

game-app/game-core/src/main/java/games/strategy/triplea/util/UnitSeparator.java

+
+  /** Returns <code>Comparator</code> for unit categories of a <code>Territory</code> */
+  private static Comparator<UnitCategory> getComparatorUnitCategories(
+      final Territory t, final GameData gameData, final GamePlayer currentPlayer) {


I notice methods calling this one get GameData from Territory, any reason to not do that here and remove the GameData parameter?

I am going to create a fix to change Territory to Optional<Territory>.

Cool, though - at a style level consider having multiple method signatures instead. Optional is a "bad" type for a method parameter.

Though, maybe we can avoid the dependency on territory? Is the ordering change potentially that significant considering territory? Is that important? Second, it's an interesting question if it's more valuable to have a fully consistent ordering regardless of even territory. Thus, perhaps it's worthwhile to see if we can simplify the comparator to not depend on territory (while at it, maybe player too)

I think we should enable the sorting to allow for some criteria to be optional that should still be considered and territory properties like owner or isWater seem plausble criteria as well as gamePlayer with whether its the same as the unit owner.
For me this comes as a cost of having a general sorting logic that it should incorporate several situations where some criteria might not be present.

properties like owner or isWater seem plausble criteria as well as gamePlayer with whether its the same as the unit owner.

Are these plausible criteria or required? For either case, if you would, can you walk me through some scenarios you considering where player or territory would change the Unit-Category ordering?

Also to consider, we can have a method give a generic ordering of unit category, and then the places that do care about player or territory, they can do their one-off logic locally. That is to say, there is flexibility; just because the top-most level method is not super-flexible, doesn't mean we can't work that flexibility in where needed.

Second thought: just please be sure to remove any method signatures that are not used if any.

Minimizing inputs is a way to have a more consistent sort ordering. We probably don't need to draw out the convo any further than there.

game-app/game-core/src/main/java/games/strategy/triplea/util/UnitSeparator.java

MapPanel.java - use new method UnitSeparator.getSortedUnitCategories SimpleUnitPanel.java - move isEmpty check upward UnitSeparator.java - introduce Optional<Territory> for getComparatorUnitCategories - new method etSortedUnitCategories(Collection<Unit>, GameData, MapData)

AbstractUndoableMovesPanel.java - use UnitSeparator.getSortedUnitCategories AvatarPanelFactory.java - use new method UnitSeparator.getSortedUnitCategories(units,territory,player) BattleCalculatorPanel.java - rename isLand to isLandBattle - pass parameter Territory location to PlayerUnitsPanel.init BattleDisplay.java - use getSortedUnitCategories instead of getComparatorUnitCategories - use new method UnitSeparator.sortUnitCategories BattleModel.java - use UnitSeparator.getSortedUnitCategories MapPanel.java - use UnitSeparator.getSortedUnitCategories OrderOfLossesInputPanel.java - use new method UnitSeparator.sortUnitCategories PlayerUnitsPanel.java - removed duplicated unitCategories.sort - introduce territory parameter to method init - rename isLand to isLandBattle SimpleUnitPanel.java - - use new method UnitSeparator.sortUnitCategories (later rework needed to replace setUnitsFromCategories(Collection<UnitCategory>)) with setUnitsFromCategories(Collection<Unit>)) - use UnitSeparator.getSortedUnitCategories UnitChooser.java - use UnitSeparator.getComparatorUnitCategories instead of import static method UnitSeparator.java - introduce getSortedUnitCategories for units and territory - introduce sortUnitCategories for a list of unit categories and gameData

PlayerUnitsPanel.java - reintroduce usage of getUnitTypes - introduce panel != null check in loop UnitSeparator.java - introduce methode ortUnitCategories(unitCategories, territory, currentPlayer) - comment fix for Javadoc parse error

This reverts commit ab48e6b.

…#12762) This reverts commit ab48e6b.

…-game#12722)" (triplea-game#12762) This reverts commit e73fdec.

* 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

frigoref added 6 commits July 13, 2024 23:57

Merge remote-tracking branch 'origin/issues_12447' into issues_12447

82ef025

Merge branch 'triplea-game:master' into issues_12447

feaa42b

issues_12447 #2

5f44c6a

AvatarPanelFactory.java -remove now unused method unitRenderingOrder

issues_12447 #3

a0506c8

AbstractUndoableMovesPanel.java -Insert <> for ArrayList instantiation -fix format violations with game-app:game-headed:spotlessApply

issues_12447 #4

d7cd8f2

AbstractUndoableMovesPanel.java -fix format violations with game-app:game-headed:spotlessApply

DanVanAtta reviewed Jul 21, 2024

View reviewed changes

frigoref requested a review from DanVanAtta July 22, 2024 12:01

frigoref added 2 commits July 22, 2024 23:29

issues_12447 #7

30c1ed3

PlayerUnitsPanel.java - reintroduce usage of getUnitTypes - introduce panel != null check in loop UnitSeparator.java - introduce methode ortUnitCategories(unitCategories, territory, currentPlayer) - comment fix for Javadoc parse error

DanVanAtta merged commit ab48e6b into triplea-game:master Jul 23, 2024
1 check passed

frigoref deleted the issues_12447 branch July 23, 2024 07:03

DanVanAtta added a commit that referenced this pull request Jul 24, 2024

Revert "Issues 12447 (Standardized Sorting of UnitCategory) (#12722)"

8899eb4

This reverts commit ab48e6b.

DanVanAtta added a commit that referenced this pull request Jul 24, 2024

Revert "Issues 12447 (Standardized Sorting of UnitCategory) (#12722)" (…

e73fdec

…#12762) This reverts commit ab48e6b.

frigoref added a commit to frigoref/triplea that referenced this pull request Jul 27, 2024

Reapply "Issues 12447 (Standardized Sorting of UnitCategory) (triplea…

823bceb

…-game#12722)" (triplea-game#12762) This reverts commit e73fdec.

frigoref mentioned this pull request Jul 27, 2024

Issue 12447 part2 #12783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues 12447 (Standardized Sorting of UnitCategory) #12722

Issues 12447 (Standardized Sorting of UnitCategory) #12722

frigoref commented Jul 13, 2024

DanVanAtta Jul 21, 2024

DanVanAtta Jul 21, 2024

DanVanAtta Jul 21, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 21, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 22, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 22, 2024 •

edited

Loading

DanVanAtta Jul 22, 2024

DanVanAtta Jul 21, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 21, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 21, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 22, 2024

frigoref Jul 22, 2024

DanVanAtta Jul 22, 2024

DanVanAtta Jul 22, 2024

		@@ -1,5 +1,7 @@
		package games.strategy.triplea.odds.calculator;

		import static games.strategy.triplea.util.UnitSeparator.getComparatorUnitCategories;

		List<UnitCategory> unitCategories = new ArrayList(UnitSeparator.categorize(value));
		unitCategories.sort(getComparatorUnitCategories(gameData));

Issues 12447 (Standardized Sorting of UnitCategory) #12722

Issues 12447 (Standardized Sorting of UnitCategory) #12722

Conversation

frigoref commented Jul 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanVanAtta Jul 22, 2024 •

edited

Loading