Make ChangeAttachmentChanges save a copy of the previous value. (#10483)

Make ChangeAttachmentChanges save a copy of the previous value of the property. The previous value is used when inverting the change (e.g. when going back in history and reverting changes). By copying the properties, this fixes several issues: * Ensuring that the inverted change is actually correct, as we don't want to be reverting to a "live view" of the current state. * Fixing exceptions caused by my recent change where property getters return unmodifiable collections (we don't want to set the property to an unmodifiable collection, since future changes will fail). Additionally, switches to improved syntax for some locking in a few related places and some small clean ups.
triplea-game · May 23, 2022 · b2a9ada · b2a9ada
1 parent eba93d5
commit b2a9ada
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 50 deletions.
diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java b/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java
@@ -25,7 +25,7 @@ public ChangeAttachmentChange(
         attachment.getAttachedTo(),
         attachment.getName(),
         newValue,
-        attachment.getPropertyOrThrow(property).getValue(),
+        DefaultAttachment.copyPropertyValue(attachment.getPropertyOrThrow(property).getValue()),
         property,
         false);
   }
@@ -43,7 +43,7 @@ public ChangeAttachmentChange(
         attachment.getAttachedTo(),
         attachment.getName(),
         newValue,
-        attachment.getPropertyOrThrow(property).getValue(),
+        DefaultAttachment.copyPropertyValue(attachment.getPropertyOrThrow(property).getValue()),
         property,
         clearFirst);
   }

diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java b/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java
@@ -6,9 +6,9 @@
 import com.google.common.collect.Iterables;
 import games.strategy.engine.data.gameparser.GameParseException;
 import games.strategy.triplea.Constants;
-import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -79,16 +79,6 @@ protected static int getInt(final String value) {
     }
   }
 
-  /** Throws an error if format is invalid. */
-  protected static BigDecimal getBigDecimal(final String value) {
-    try {
-      return new BigDecimal(value);
-    } catch (final NumberFormatException e) {
-      throw new IllegalArgumentException(
-          "Attachments: " + value + " is not a valid decimal value", e);
-    }
-  }
-
   /** Throws an error if format is invalid. Must be either true or false ignoring case. */
   protected static boolean getBool(final String value) {
     if (value.equalsIgnoreCase(Constants.PROPERTY_TRUE)) {
@@ -216,4 +206,18 @@ protected static <T> IntegerMap<T> getIntegerMapProperty(@Nullable IntegerMap<T>
     }
     return IntegerMap.unmodifiableViewOf(value);
   }
+
+  @SuppressWarnings({"unchecked", "rawtypes"})
+  public static Object copyPropertyValue(Object value) {
+    if (value instanceof List) {
+      return new ArrayList((List) value);
+    } else if (value instanceof IntegerMap) {
+      return new IntegerMap((IntegerMap) value);
+    } else if (value instanceof Set) {
+      return new HashSet((Set) value);
+    } else if (value instanceof Map) {
+      return new HashMap((Map) value);
+    }
+    return value;
+  }
 }
diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java b/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java
@@ -526,7 +526,7 @@ public void performChange(final Change change) {
     try (Unlocker ignored = acquireWriteLock()) {
       change.perform(this);
     }
-    dataChangeListeners.forEach(dataChangelistener -> dataChangelistener.gameDataChanged(change));
+    dataChangeListeners.forEach(listener -> listener.gameDataChanged(change));
     GameDataEvent.lookupEvent(change).ifPresent(this::fireGameDataEvent);
   }
 

diff --git a/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java b/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java
@@ -4,7 +4,7 @@
 import games.strategy.engine.data.GameData;
 import games.strategy.engine.data.GamePlayer;
 import games.strategy.engine.history.EventChild;
-import java.util.ConcurrentModificationException;
+import games.strategy.engine.history.HistoryWriter;
 import javax.swing.SwingUtilities;
 
 /**
@@ -14,10 +14,8 @@
  */
 public class HistorySynchronizer {
   // Note the GameData here and the game are not the same we are keeping gameData in sync with the
-  // history of the game
-  // by listening for changes we do this because our data can change depending where in the history
-  // we are we want to be
-  // able to do this without changing the data for the game
+  // history of the game by listening for changes. We do this because our data can change depending
+  // where we are in history. We want to be able to do this without changing the data for the game.
   private final GameData gameData;
   private int currentRound;
   private final IGame game;
@@ -27,16 +25,8 @@ public class HistorySynchronizer {
         public void gameDataChanged(final Change change) {
           SwingUtilities.invokeLater(
               () -> {
-                try {
-                  final Change localizedChange = (Change) translateIntoMyData(change);
-                  gameData.getHistory().getHistoryWriter().addChange(localizedChange);
-                } catch (ConcurrentModificationException e) {
-                  // Temporary instrumentation to diagnose root cause of:
-                  // https://github.com/triplea-game/triplea/issues/10274
-                  // Include the Change subclass name in the error.
-                  throw new ConcurrentModificationException(
-                      "Translating " + change.getClass().getName(), e);
-                }
+                final Change localizedChange = (Change) translateIntoMyData(change);
+                gameData.getHistory().getHistoryWriter().addChange(localizedChange);
               });
         }
 
@@ -88,14 +78,12 @@ public void stepChanged(
           }
           SwingUtilities.invokeLater(
               () -> {
+                final HistoryWriter historyWriter = gameData.getHistory().getHistoryWriter();
                 if (currentRound != round) {
                   currentRound = round;
-                  gameData.getHistory().getHistoryWriter().startNextRound(currentRound);
+                  historyWriter.startNextRound(currentRound);
                 }
-                gameData
-                    .getHistory()
-                    .getHistoryWriter()
-                    .startNextStep(stepName, delegateName, player, displayName);
+                historyWriter.startNextStep(stepName, delegateName, player, displayName);
               });
         }
 
@@ -112,11 +100,8 @@ public HistorySynchronizer(final GameData data, final IGame game) {
     }
     gameData = data;
     gameData.forceChangesOnlyInSwingEventThread();
-    data.acquireReadLock();
-    try {
+    try (GameData.Unlocker ignored = data.acquireReadLock()) {
       currentRound = data.getSequence().getRound();
-    } finally {
-      data.releaseReadLock();
     }
     this.game = game;
     this.game

diff --git a/...game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java b/...game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java
@@ -125,7 +125,13 @@ private void setMovementCostModifier(final String value) throws GameParseExcepti
           "movementCostModifier must have a count and at least one unitType" + thisErrorMsg());
     }
     final Iterator<String> iter = List.of(s).iterator();
-    final BigDecimal effect = getBigDecimal(iter.next());
+    final BigDecimal effect;
+    try {
+      effect = new BigDecimal(iter.next());
+    } catch (final NumberFormatException e) {
+      throw new IllegalArgumentException(
+          "Attachments: " + s[0] + " is not a valid decimal value", e);
+    }
     while (iter.hasNext()) {
       if (movementCostModifier == null) {
         movementCostModifier = new HashMap<>();

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java
@@ -175,18 +175,14 @@ private void territoryChanged(final @Nullable Territory territory) {
     final String unitsLabel;
 
     // Get the unit information under lock as otherwise they may change on the game thread causing a
-    // ConcurrentModificationException. Use local variable to ensure we lock/unlock the same object.
-    final GameData gameData = this.gameData;
-    gameData.acquireReadLock();
-    try {
+    // ConcurrentModificationException.
+    try (GameData.Unlocker ignored = gameData.acquireReadLock()) {
       unitsList = UnitSeparator.getSortedUnitCategories(territory, uiContext.getMapData());
       unitsLabel =
           "Units: "
               + territory.getUnits().stream()
                   .filter(u -> uiContext.getMapData().shouldDrawUnit(u.getType().getName()))
                   .count();
-    } finally {
-      gameData.releaseReadLock();
     }
 
     unitInfo.setText(unitsLabel);

diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java
@@ -182,7 +182,7 @@ public final class TripleAFrame extends JFrame implements QuitHandler {
   private final StatPanel statsPanel;
   private final EconomyPanel economyPanel;
   private final Runnable clientLeftGame;
-  private ObjectivePanel objectivePanel;
+  private @Nullable ObjectivePanel objectivePanel;
   @Getter private final TerritoryDetailPanel territoryDetails;
   private final JPanel historyComponent = new JPanel();
   private HistoryPanel historyPanel;
@@ -1759,8 +1759,7 @@ private void showHistory() {
     inGame.set(false);
     setWidgetActivation();
     final GameData clonedGameData;
-    data.acquireReadLock();
-    try {
+    try (GameData.Unlocker ignored = data.acquireReadLock()) {
       // we want to use a clone of the data, so we can make changes to it as we walk up and down the
       // history
       final var cloneOptions = GameDataManager.Options.builder().withHistory(true).build();
@@ -1774,8 +1773,6 @@ private void showHistory() {
       }
       historySyncher = new HistorySynchronizer(clonedGameData, game);
       clonedGameData.addDataChangeListener(dataChangeListener);
-    } finally {
-      data.releaseReadLock();
     }
     statsPanel.setGameData(clonedGameData);
     economyPanel.setGameData(clonedGameData);