Fix selecting step nodes in history view not having an effect. (#10439)

Fix selecting step nodes in history view not having an effect.

This was broken with #10429, which made the logically correct change, but ran into a bug where we weren't actually setting the change index on step nodes. Introduce a fix as well as handling of history from saved games that didn't set it.

Additionally, when non-leaf nodes are selected from the UI, we actually don't want to go to the state after that node, since that breaks chronological property of elements of the tree (i.e. Round 1 non-leaf node would be before first leaf node of Round 1), so this PR adds logic to go to previous child node instead.

game-app/game-core/src/main/java/games/strategy/engine/history/History.java

@@ -1,5 +1,6 @@
 package games.strategy.engine.history;
 
+import com.google.common.base.Preconditions;
 import games.strategy.engine.data.Change;
 import games.strategy.engine.data.CompositeChange;
 import games.strategy.engine.data.GameData;
@@ -80,7 +81,7 @@ private HistoryNode getLastChildInternal(final HistoryNode node) {
   }
 
   private int getLastChange(final HistoryNode node) {
-    final int lastChangeIndex;
+    int lastChangeIndex;
     if (node == getRoot()) {
       lastChangeIndex = 0;
     } else if (node instanceof Event) {
@@ -89,6 +90,11 @@ private int getLastChange(final HistoryNode node) {
       lastChangeIndex = ((Event) node.getParent()).getChangeEndIndex();
     } else if (node instanceof IndexedHistoryNode) {
       lastChangeIndex = ((IndexedHistoryNode) node).getChangeEndIndex();
+      // If this node is still current, or comes from an old save game where we didn't set it, get
+      // the last change index from its last child node.
+      if (lastChangeIndex == -1 && node.getChildCount() > 0) {
+        lastChangeIndex = getLastChange((HistoryNode) node.getLastChild());
+      }
     } else {
       lastChangeIndex = 0;
     }
@@ -113,6 +119,9 @@ public Change getDelta(final HistoryNode start, final HistoryNode end) {
 
   /** Changes the game state to reflect the historical state at {@code node}. */
   public synchronized void gotoNode(final HistoryNode node) {
+    // Setting node to null causes problems, because we'll restore the state to the start, but then
+    // next gotoNode() call will reset currentNode to getLastNode() causing an invalid delta.
+    Preconditions.checkNotNull(node);
     assertCorrectThread();
     gameData.acquireWriteLock();
     try {

game-app/game-core/src/main/java/games/strategy/engine/history/HistoryWriter.java

@@ -88,6 +88,7 @@ private void closeCurrent() {
           parent.remove(current);
           history.nodesWereRemoved(parent, new int[] {index}, new Object[] {current});
         }
+        ((Step) current).setChangeEndIndex(history.getChanges().size());
         current = parent;
         return;
       }

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

@@ -19,6 +19,7 @@
 import java.util.Collection;
 import java.util.Deque;
 import java.util.Enumeration;
+import java.util.Optional;
 import javax.swing.ImageIcon;
 import javax.swing.JButton;
 import javax.swing.JPanel;
@@ -220,7 +221,6 @@ private void previous() {
     }
     final TreePath path = tree.getSelectionPath();
     final TreeNode selected = (TreeNode) path.getLastPathComponent();
-    @SuppressWarnings("unchecked")
     final Enumeration<TreeNode> nodeEnum =
         ((DefaultMutableTreeNode) tree.getModel().getRoot()).depthFirstEnumeration();
     TreeNode previous = null;
@@ -263,7 +263,6 @@ private void next() {
     }
     final TreePath path = tree.getSelectionPath();
     final TreeNode selected = (TreeNode) path.getLastPathComponent();
-    @SuppressWarnings("unchecked")
     final Enumeration<TreeNode> nodeEnum =
         ((DefaultMutableTreeNode) tree.getModel().getRoot()).preorderEnumeration();
     TreeNode next = null;
@@ -294,7 +293,16 @@ private void gotoNode(final HistoryNode node) {
     if (details != null) {
       details.render(node);
     }
-    data.getHistory().gotoNode(node);
+    // If this is not a leaf node, set the game state to the last leaf node before it. This way,
+    // selecting something like "Round 3" shows the state at the start of the round, which ensures
+    // chronological order when moving up/down the nodes using arrow keys even if some are expanded.
+    if (!node.isLeaf()) {
+      final TreeNode leaf = node.getPreviousLeaf();
+      // If no previous leaf, select the root node.
+      data.getHistory().gotoNode((HistoryNode) Optional.ofNullable(leaf).orElse(node.getRoot()));
+    } else {
+      data.getHistory().gotoNode(node);
+    }
   }
 
   public HistoryNode getCurrentNode() {
@@ -445,7 +453,7 @@ public Component getTreeCellRendererComponent(
       if (value instanceof Step) {
         final GamePlayer player = ((Step) value).getPlayerId();
         if (player != null) {
-          final String text = value.toString() + " (" + player.getName() + ")";
+          final String text = value + " (" + player.getName() + ")";
           if (uiContext != null) {
             super.getTreeCellRendererComponent(tree, value, sel, expanded, leaf, row, haveFocus);
             icon.setImage(uiContext.getFlagImageFactory().getSmallFlag(player));