Data Type Manager - Fixed broken forward history button

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/datamgr/DataTypesProvider.java

@@ -426,9 +426,18 @@ public class DataTypesProvider extends ComponentProviderAdapter {
 		archiveGTree.addGTreeSelectionListener(e -> {
 
 			TreePath path = e.getNewLeadSelectionPath();
-			DataType dataType = getDataTypeFrom(path);
+			DataType selectedDt = getDataTypeFrom(path);
+			DataTypeUrl dtUrl = navigationHistory.getCurrentHistoryItem();
+			if (dtUrl != null) {
+				DataType historyDt = dtUrl.getDataType(plugin);
+				if (Objects.equals(historyDt, selectedDt)) {
+					// Don't add an item to the history that is already there.  This can happen when
+					// the user interacts with the navigation buttons.
+					return;
+				}
+			}
 
-			dataTypeSelected(e.getEventOrigin(), dataType);
+			dataTypeSelected(e.getEventOrigin(), selectedDt);
 		});
 
 		buildPreviewPane();
@@ -783,8 +792,9 @@ public class DataTypesProvider extends ComponentProviderAdapter {
 			return;
 		}
 
+		TreePath treePath = dataTypeNode.getTreePath();
 		gTree.setSelectedNode(dataTypeNode);
-		gTree.scrollPathToVisible(dataTypeNode.getTreePath());
+		gTree.scrollPathToVisible(treePath);
 		contextChanged();
 	}
 

Ghidra/Framework/Generic/src/main/java/util/HistoryList.java

@@ -21,6 +21,7 @@ import java.util.function.Consumer;
 
 import org.apache.commons.lang3.StringUtils;
 
+import ghidra.util.Msg;
 import ghidra.util.SystemUtilities;
 import ghidra.util.datastruct.FixedSizeStack;
 
@@ -38,8 +39,10 @@ import ghidra.util.datastruct.FixedSizeStack;
  * item is cleared, then client is expected to call {@link #add(Object)} with value of 
  * null.  (This is safe to do, regardless of whether null are allowed).  When nulls are allowed
  * and a null value is received, then current item is placed onto the history stack as the 
- * previous item.  This way, when the user presses the back button, the last visible item 
+ * previous item, thus using the null value as a flag.  After using null this way, when the user 
- * will be activated.  
+ * presses the back button, the last visible item will be activated.   
+ * 
+ * <p>If nulls are not allowed, then any calls to {@link #add(Object)} a null value will be ignored.
  * 
  * <p>Note: when nulls are allowed, only a single null value will be stored.  Further, 
  * if new, non-null items are added, then the null value is dropped.  
@@ -191,8 +194,14 @@ public class HistoryList<T> {
 			return;
 		}
 
+		doGoToIndex(historyIndex - 1);
+	}
+
+	private void doGoToIndex(int index) {
+		historyIndex = index;
 		T leaving = getCurrentHistoryItem();
-		T t = historyStack.get(--historyIndex);
+		T t = historyStack.get(index);
+
 		dropNull();
 		broadcast(t, leaving);
 	}
@@ -204,9 +213,21 @@ public class HistoryList<T> {
 	 * @param t the item
 	 */
 	public void goBackTo(T t) {
-		while (!getCurrentHistoryItem().equals(t) && hasPrevious()) {
+		int index = historyIndex - 1; // assume we want to ignore the current item
-			goBack();
+		while (index >= 0) {
+			T other = historyStack.get(index);
+			if (Objects.equals(t, other)) {
+				break;
+			}
+			index--;
 		}
+
+		if (index < 0) {
+			Msg.error(this, "Item not in backward history: " + t);
+			return;
+		}
+
+		doGoToIndex(index);
 	}
 
 	/**
@@ -220,9 +241,7 @@ public class HistoryList<T> {
 			return;
 		}
 
-		T leaving = getCurrentHistoryItem();
+		doGoToIndex(historyIndex + 1);
-		T t = historyStack.get(++historyIndex);
-		broadcast(t, leaving);
 	}
 
 	/**
@@ -232,9 +251,21 @@ public class HistoryList<T> {
 	 * @param t the item
 	 */
 	public void goForwardTo(T t) {
-		while (!getCurrentHistoryItem().equals(t) && hasNext()) {
+		int index = historyIndex + 1; // assume we want to ignore the current item
-			goForward();
+		while (index < historyStack.size()) {
+			T other = historyStack.get(index);
+			if (Objects.equals(t, other)) {
+				break;
+			}
+			index++;
 		}
+
+		if (index < 0) {
+			Msg.error(this, "Item not in forward history: " + t);
+			return;
+		}
+
+		doGoToIndex(index);
 	}
 
 	/**

Ghidra/Framework/Generic/src/test/java/util/HistoryListTest.java

@@ -16,6 +16,7 @@
 package util;
 
 import static org.hamcrest.CoreMatchers.*;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.*;
 
 import java.util.LinkedList;
@@ -455,9 +456,10 @@ public class HistoryListTest {
 // Private Methods
 //==================================================================================================
 
-	private void assertCurrentItem(String item) {
+	private void assertCurrentItem(String expected) {
-		assertThat("History list is not pointing to the expected item", item,
+		assertThat("History list is not pointing to the expected item",
-			is(historyList.getCurrentHistoryItem()));
+			historyList.getCurrentHistoryItem(),
+			is(expected));
 	}
 
 	private void assertNotified(String item) {