GP-4740 Added undo/redo support to composite editor. Switched actions to use isEnabledForContext. Transitioned VT FilterFormattedTestField to GFormattedTextField and use for editor text entry fields. Cleanup of old datatype tree actions no longer in use. Lots of changes to improve handling of data type dependency changes and restored original DTM.

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java

@@ -16,6 +16,7 @@
 package ghidra.program.database.data;
 
 import java.io.IOException;
+import java.util.Objects;
 
 import db.DBRecord;
 import ghidra.docking.settings.Settings;
@@ -212,6 +213,9 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		lock.acquire();
 		try {
 			checkDeleted();
+			if (Objects.equals(desc, record.getString(CompositeDBAdapter.COMPOSITE_COMMENT_COL))) {
+				return;
+			}
 			record.setString(CompositeDBAdapter.COMPOSITE_COMMENT_COL, desc);
 			compositeAdapter.updateRecord(record, true);
 			dataMgr.dataTypeChanged(this, false);
@@ -391,13 +395,17 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		return record.getLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL);
 	}
 
+	void doSetLastChangeTime(long lastChangeTime) throws IOException {
+		record.setLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL, lastChangeTime);
+		compositeAdapter.updateRecord(record, false);
+	}
+
 	@Override
 	public void setLastChangeTime(long lastChangeTime) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			record.setLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL, lastChangeTime);
-			compositeAdapter.updateRecord(record, false);
+			doSetLastChangeTime(lastChangeTime);
 			dataMgr.dataTypeChanged(this, false);
 		}
 		catch (IOException e) {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeComponentDB.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -138,7 +138,11 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 		if (id == -1) {
 			return DataType.DEFAULT;
 		}
-		return dataMgr.getDataType(id);
+		DataType dt = dataMgr.getDataType(id);
+		if (dt == null) {
+			return BadDataType.dataType;
+		}
+		return dt;
 	}
 
 	@Override
@@ -191,6 +195,7 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 
 	@Override
 	public Settings getDefaultSettings() {
+
 		if (!hasSettings()) {
 			return SettingsImpl.NO_SETTINGS;
 		}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java

@@ -1693,8 +1693,12 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		// (preference is given to similar kind of datatype when checking existing conflict types)
 		DataType existingDataType = findDataTypeSameLocation(dataType);
 		if (existingDataType == null) {
-			return createDataType(dataType, getUnusedConflictName(dataType), sourceArchive,
-				currentHandler);
+			// create non-existing datatype - keep original name unless it is already used
+			String name = dataType.getName();
+			if (getDataType(dataType.getCategoryPath(), name) != null) {
+				name = getUnusedConflictName(dataType);
+			}
+			return createDataType(dataType, name, sourceArchive, currentHandler);
 		}
 
 		// So we have a dataType with the same path and name, but not equivalent, so use
@@ -2310,7 +2314,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		if (id <= 0) { // removal of certain special types not permitted
 			return false;
 		}
-		idsToDelete.add(Long.valueOf(id));
+		idsToDelete.add(id);
 		removeQueuedDataTypes();
 		return true;
 	}
@@ -3130,8 +3134,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 			structDB.doReplaceWith(struct, false);
 
-			// doReplaceWith may have updated the last change time so set it back to what we want.
-			structDB.setLastChangeTime(struct.getLastChangeTime());
+			// doReplaceWith may have updated the last change time so set it back to what we want
+			// without triggering change notification
+			structDB.doSetLastChangeTime(struct.getLastChangeTime());
 
 			return structDB;
 		}
@@ -3198,8 +3203,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 			unionDB.doReplaceWith(union, false);
 
-			// doReplaceWith updated the last change time so set it back to what we want.
-			unionDB.setLastChangeTime(union.getLastChangeTime());
+			// doReplaceWith may have updated the last change time so set it back to what we want
+			// without triggering change notification
+			unionDB.doSetLastChangeTime(union.getLastChangeTime());
 
 			return unionDB;
 		}
@@ -3717,7 +3723,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
-	void removeParentChildRecord(long parentID, long childID) {
+	protected void removeParentChildRecord(long parentID, long childID) {
 
 		if (isBulkRemoving) {
 			// we are in the process of bulk removing the given child; no need to call
@@ -3733,6 +3739,26 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
+	protected Set<Long> getChildIds(long parentID) {
+		try {
+			return parentChildAdapter.getChildIds(parentID);
+		}
+		catch (IOException e) {
+			dbError(e);
+		}
+		return Set.of();
+	}
+
+	protected boolean hasParent(long childID) {
+		try {
+			return parentChildAdapter.hasParent(childID);
+		}
+		catch (IOException e) {
+			dbError(e);
+		}
+		return false;
+	}
+
 	List<DataType> getParentDataTypes(long dataTypeId) {
 		lock.acquire();
 		try {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ParentChildAdapter.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -80,6 +80,16 @@ abstract class ParentChildAdapter {
 
 	abstract void removeRecord(long parentID, long childID) throws IOException;
 
+	/**
+	 * Get the unique set of child IDs associated with the specified parent ID.
+	 * Since a parent may have duplicate parent-child records, this method
+	 * avoids returning the same child more than once.
+	 * @param parentID parent datatype ID
+	 * @return set of child datatype IDs
+	 * @throws IOException if a DB IO error occurs
+	 */
+	abstract Set<Long> getChildIds(long parentID) throws IOException;
+
 	/**
 	 * Get the unique set of parent ID associated with the specified childID.
 	 * Since composite parents may have duplicate parent-child records, this method
@@ -90,6 +100,14 @@ abstract class ParentChildAdapter {
 	 */
 	abstract Set<Long> getParentIds(long childID) throws IOException;
 
+	/** 
+	 * Determine if there is one or more parents associated with the specified childID.
+	 * @param childID child datatype ID
+	 * @return true if a parent was identified, else false
+	 * @throws IOException if a DB IO error occurs
+	 */
+	abstract boolean hasParent(long childID) throws IOException;
+
 	abstract void removeAllRecordsForParent(long parentID) throws IOException;
 
 	abstract void removeAllRecordsForChild(long childID) throws IOException;

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ParentChildDBAdapterNoTable.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -43,11 +43,21 @@ class ParentChildDBAdapterNoTable extends ParentChildAdapter {
 		throw new UnsupportedOperationException();
 	}
 
+	@Override
+	Set<Long> getChildIds(long parentID) throws IOException {
+		return Set.of();
+	}
+
 	@Override
 	Set<Long> getParentIds(long childID) throws IOException {
 		return Set.of();
 	}
 
+	@Override
+	boolean hasParent(long childID) throws IOException {
+		return false;
+	}
+
 	@Override
 	boolean needsInitializing() {
 		return false;

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ParentChildDBAdapterV0.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -88,17 +88,33 @@ class ParentChildDBAdapterV0 extends ParentChildAdapter {
 		}
 	}
 
+	@Override
+	Set<Long> getChildIds(long parentID) throws IOException {
+		Field[] ids = table.findRecords(new LongField(parentID), PARENT_COL);
+		Set<Long> childIds = new HashSet<>(ids.length);
+		for (Field id : ids) {
+			DBRecord rec = table.getRecord(id);
+			childIds.add(rec.getLongValue(CHILD_COL));
+		}
+		return childIds;
+	}
+
 	@Override
 	Set<Long> getParentIds(long childID) throws IOException {
 		Field[] ids = table.findRecords(new LongField(childID), CHILD_COL);
 		Set<Long> parentIds = new HashSet<>(ids.length);
-		for (int i = 0; i < ids.length; i++) {
-			DBRecord rec = table.getRecord(ids[i]);
+		for (Field id : ids) {
+			DBRecord rec = table.getRecord(id);
 			parentIds.add(rec.getLongValue(PARENT_COL));
 		}
 		return parentIds;
 	}
 
+	@Override
+	boolean hasParent(long childID) throws IOException {
+		return table.hasRecord(new LongField(childID), CHILD_COL);
+	}
+
 	public void setNeedsInitializing() {
 		needsInitializing = true;
 	}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/PointerDBAdapter.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -111,9 +111,6 @@ abstract class PointerDBAdapter implements RecordTranslator {
 		}
 	}
 
-	/**
-	 *
-	 */
 	abstract void deleteTable(DBHandle handle) throws IOException;
 
 	/**

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java

@@ -2301,7 +2301,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 				checkAncestry(replacementDt);
 			}
 			catch (Exception e) {
-				// TODO: should we flag bad replacement
+				// Handle bad replacement with use of undefined component
 				replacementDt = isPackingEnabled() ? Undefined1DataType.dataType : DataType.DEFAULT;
 			}
 

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataUtilities.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -15,6 +15,8 @@
  */
 package ghidra.program.model.data;
 
+import org.apache.commons.lang3.StringUtils;
+
 import ghidra.program.model.address.*;
 import ghidra.program.model.listing.*;
 import ghidra.program.model.mem.*;
@@ -36,7 +38,7 @@ public final class DataUtilities {
 	 * @return true if name is valid, else false
 	 */
 	public static boolean isValidDataTypeName(String name) {
-		if (name == null || name.length() == 0) {
+		if (StringUtils.isBlank(name)) {
 			return false;
 		}
 

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StandAloneDataTypeManager.java

@@ -872,6 +872,14 @@ public class StandAloneDataTypeManager extends DataTypeManagerDB implements Clos
 		return transaction.intValue();
 	}
 
+	/**
+	 * Get the number of active transactions
+	 * @return number of active transactions
+	 */
+	protected int getTransactionCount() {
+		return transactionCount;
+	}
+
 	@Override
 	public void endTransaction(int transactionID, boolean commit) {
 		boolean restored = false;
@@ -953,6 +961,8 @@ public class StandAloneDataTypeManager extends DataTypeManagerDB implements Clos
 	protected synchronized void clearUndo() {
 		undoList.clear();
 		redoList.clear();
+
+		// Flatten all checkpoints then restore undo stack size
 		dbHandle.setMaxUndos(0);
 		dbHandle.setMaxUndos(NUM_UNDOS);
 	}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StructureDataType.java

@@ -1640,13 +1640,13 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 	 * @param dataType the data type of the new component
 	 * @param newOffset offset of replacement component which must fall within origComponents bounds
 	 * @param length the length of the new component
-	 * @param name the field name of the new component
+	 * @param fieldName the field name of the new component
 	 * @param comment the comment for the new component
 	 * @return the new component or null if only a clear operation was performed.
 	 * @throws IllegalArgumentException if unable to identify/make sufficient space 
 	 */
 	private DataTypeComponent replaceComponents(LinkedList<DataTypeComponentImpl> origComponents,
-			DataType dataType, int newOffset, int length, String name, String comment)
+			DataType dataType, int newOffset, int length, String fieldName, String comment)
 			throws IllegalArgumentException {
 
 		boolean clearOnly = false;
@@ -1721,8 +1721,8 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 		DataTypeComponentImpl newDtc = null;
 		if (!clearOnly) {
 			// insert new component
-			newDtc = new DataTypeComponentImpl(dataType, this, length, newOrdinal, newOffset, name,
-				comment);
+			newDtc = new DataTypeComponentImpl(dataType, this, length, newOrdinal, newOffset,
+				fieldName, comment);
 			components.add(index, newDtc);
 		}
 

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/data/StructureDBTest.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -32,7 +32,8 @@ import ghidra.util.task.TaskMonitorAdapter;
 public class StructureDBTest extends AbstractGenericTest {
 
 	private StructureDB struct;
-	private DataTypeManagerDB dataMgr;
+	private StandAloneDataTypeManager dataMgr;
+	private int txId;
 
 	@Before
 	public void setUp() throws Exception {
@@ -42,7 +43,7 @@ public class StructureDBTest extends AbstractGenericTest {
 		// default data organization is little-endian
 		// default BitFieldPackingImpl uses gcc conventions with type alignment enabled
 
-		dataMgr.startTransaction("Test");
+		txId = dataMgr.startTransaction("Test");
 
 		struct = createStructure("Test", 0);
 		struct.add(new ByteDataType(), "field1", "Comment1");
@@ -52,6 +53,14 @@ public class StructureDBTest extends AbstractGenericTest {
 
 	}
 
+	@After
+	public void tearDown() {
+		if (dataMgr != null) {
+			dataMgr.endTransaction(txId, true);
+			dataMgr.close();
+		}
+	}
+
 	private void transitionToBigEndian() {
 
 		Structure structClone = struct.clone(null);
@@ -1442,7 +1451,45 @@ public class StructureDBTest extends AbstractGenericTest {
 	}
 
 	@Test
-	public void testDeleteMany() throws InvalidDataTypeException {
+	public void testDeleteMany() {
+
+		struct.growStructure(20);
+		struct.insertAtOffset(12, WordDataType.dataType, -1, "A", null);
+		struct.insertAtOffset(16, WordDataType.dataType, -1, "B", null);
+
+		assertEquals(32, struct.getLength());
+		assertEquals(26, struct.getNumComponents());
+		assertEquals(6, struct.getNumDefinedComponents());
+
+		struct.delete(Sets.newHashSet(1, 4, 5));
+
+		assertEquals(28, struct.getLength());
+		assertEquals(23, struct.getNumComponents());
+		assertEquals(5, struct.getNumDefinedComponents());
+
+		DataTypeComponent[] comps = struct.getDefinedComponents();
+		assertEquals(WordDataType.class, comps[3].getDataType().getClass());
+		assertEquals(5, comps[3].getOrdinal());
+		assertEquals(8, comps[3].getOffset());
+
+		// Verify that records were properly updated by comitting and performing an undo/redo
+		dataMgr.endTransaction(txId, true);
+		dataMgr.undo();
+		dataMgr.redo();
+		txId = dataMgr.startTransaction("Continue Test");
+
+		assertEquals(28, struct.getLength());
+		assertEquals(23, struct.getNumComponents());
+		assertEquals(5, struct.getNumDefinedComponents());
+
+		comps = struct.getDefinedComponents();
+		assertEquals(WordDataType.class, comps[3].getDataType().getClass());
+		assertEquals(5, comps[3].getOrdinal());
+		assertEquals(8, comps[3].getOffset());
+	}
+
+	@Test
+	public void testDeleteManyBF() throws InvalidDataTypeException {
 
 		struct.insertBitFieldAt(2, 4, 0, IntegerDataType.dataType, 3, "bf1", "bf1Comment");
 		struct.insertBitFieldAt(2, 4, 3, IntegerDataType.dataType, 3, "bf2", "bf2Comment");

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/data/UnionDBTest.java

@@ -4,9 +4,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -30,8 +30,9 @@ import ghidra.util.task.TaskMonitor;
  */
 public class UnionDBTest extends AbstractGenericTest {
 
-	private DataTypeManager dataMgr;
+	private StandAloneDataTypeManager dataMgr;
 	private UnionDB union;
+	private int txId;
 
 	@Before
 	public void setUp() throws Exception {
@@ -41,7 +42,7 @@ public class UnionDBTest extends AbstractGenericTest {
 		// default data organization is little-endian
 		// default BitFieldPackingImpl uses gcc conventions
 
-		dataMgr.startTransaction("Test");
+		txId = dataMgr.startTransaction("Test");
 
 		union = createUnion("TestUnion");
 		union.add(new ByteDataType(), "field1", "Comment1");
@@ -50,6 +51,14 @@ public class UnionDBTest extends AbstractGenericTest {
 		union.add(new ByteDataType(), "field4", "Comment4");
 	}
 
+	@After
+	public void tearDown() {
+		if (dataMgr != null) {
+			dataMgr.endTransaction(txId, true);
+			dataMgr.close();
+		}
+	}
+
 	private void transitionToBigEndian() {
 
 		Union unionClone = union.clone(null);
@@ -380,6 +389,7 @@ public class UnionDBTest extends AbstractGenericTest {
 		union.delete(Sets.newHashSet(2, 4));
 
 		assertEquals(2, union.getLength());
+
 		//@formatter:off
 		CompositeTestUtils.assertExpectedComposite(this, "/TestUnion\n" + 
 			"pack(disabled)\n" + 
@@ -390,6 +400,33 @@ public class UnionDBTest extends AbstractGenericTest {
 			"}\n" + 
 			"Length: 2 Alignment: 1", union);
 		//@formatter:on
+
+		DataTypeComponent[] comps = union.getDefinedComponents();
+		assertEquals(ByteDataType.class, comps[2].getDataType().getClass());
+		assertEquals(2, comps[2].getOrdinal());
+
+		// Verify that records were properly updated by comitting and performing an undo/redo
+		dataMgr.endTransaction(txId, true);
+		dataMgr.undo();
+		dataMgr.redo();
+		txId = dataMgr.startTransaction("Continue Test");
+
+		assertEquals(2, union.getLength());
+
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/TestUnion\n" + 
+			"pack(disabled)\n" + 
+			"Union TestUnion {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   0   word   2      \"Comment2\"\n" + 
+			"   0   byte   1   field4   \"Comment4\"\n" + 
+			"}\n" + 
+			"Length: 2 Alignment: 1", union);
+		//@formatter:on
+
+		comps = union.getDefinedComponents();
+		assertEquals(ByteDataType.class, comps[2].getDataType().getClass());
+		assertEquals(2, comps[2].getOrdinal());
 	}
 
 	@Test