Merge remote-tracking branch 'origin/GP-2042-dragonmacher-table-row-update-issue--SQUASHED'

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/functionwindow/FunctionWindowProvider.java

@@ -18,15 +18,17 @@ package ghidra.app.plugin.core.functionwindow;
 import java.awt.BorderLayout;
 import java.awt.Dimension;
 import java.awt.event.MouseEvent;
+import java.util.HashSet;
+import java.util.Set;
 
 import javax.swing.*;
-import javax.swing.table.JTableHeader;
+import javax.swing.table.*;
 
 import docking.ActionContext;
 import ghidra.app.services.GoToService;
 import ghidra.framework.plugintool.ComponentProviderAdapter;
-import ghidra.program.model.listing.Function;
-import ghidra.program.model.listing.Program;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.listing.*;
 import ghidra.program.util.ProgramSelection;
 import ghidra.util.HelpLocation;
 import ghidra.util.table.*;
@@ -161,16 +163,44 @@ public class FunctionWindowProvider extends ComponentProviderAdapter {
 	}
 
 	private void setFunctionTableRenderer() {
-		functionTable.getColumnModel()
-				.getColumn(FunctionTableModel.LOCATION_COL)
-				.setPreferredWidth(
-					FunctionTableModel.LOCATION_COL_WIDTH);
+		TableColumnModel columnModel = functionTable.getColumnModel();
+		TableColumn column = columnModel.getColumn(FunctionTableModel.LOCATION_COL);
+		column.setPreferredWidth(FunctionTableModel.LOCATION_COL_WIDTH);
 	}
 
 	void update(Function function) {
-		if (isVisible()) {
-			functionModel.update(function);
+		if (!isVisible()) {
+			return;
 		}
+
+		Set<Function> functions = getRelatedFunctions(function);
+		for (Function f : functions) {
+			functionModel.update(f);
+		}
+	}
+
+	/**
+	 * Gathers this function and any functions that thunk it
+	 * @param f the function
+	 * @return the related functions
+	 */
+	private Set<Function> getRelatedFunctions(Function f) {
+
+		Program program = f.getProgram();
+		FunctionManager functionManager = program.getFunctionManager();
+		Set<Function> functions = new HashSet<>();
+		Address[] addresses = f.getFunctionThunkAddresses(true);
+		if (addresses != null) {
+			for (Address a : addresses) {
+				Function thunk = functionManager.getFunctionAt(a);
+				if (thunk != null) {
+					functions.add(thunk);
+				}
+			}
+		}
+
+		functions.add(f);
+		return functions;
 	}
 
 	void functionAdded(Function function) {

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/SymbolTableModel.java

@@ -60,6 +60,8 @@ class SymbolTableModel extends AddressBasedTableModel<Symbol> {
 	private ReferenceManager refMgr;
 	private Symbol lastSymbol;
 	private SymbolFilter filter;
+
+	// TODO this can be removed after GP-2030 is finished
 	private TableAddRemoveStrategy<Symbol> deletedDbObjectAddRemoveStrategy =
 		new SymbolTableAddRemoveStrategy();
 

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/CoalescingAddRemoveStrategy.java

@@ -0,0 +1,264 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package docking.widgets.table;
+
+import static docking.widgets.table.AddRemoveListItem.Type.*;
+
+import java.util.*;
+
+import docking.widgets.table.threaded.*;
+import ghidra.util.exception.CancelledException;
+import ghidra.util.task.TaskMonitor;
+
+/**
+ * The {@link ThreadedTableModel} does not correctly function with data that can change outside of 
+ * the table.  For example, if a table uses db objects as row objects, these db objects can be 
+ * changed by the user and by analysis while table has already been loaded.   The problem with this 
+ * is that the table's sort can be broken when new items are to be added, removed or re-inserted, 
+ * as this process requires a binary search, which will be broken if the criteria used to sort the 
+ * data has changed.   Effectively, a row object change can break the binary search if that item 
+ * stays in a previously sorted position, but has updated data that would put the symbol in a new 
+ * position if sorted again.  For example, if the table is sorted on name and the name of an item 
+ * changes, then future uses of the binary search will be broken while that item is still in the 
+ * position that matches its old name.
+ * <p>
+ * This issue has been around for quite some time.  To completely fix this issue, each row object
+ * of the table would need to be immutable, at least on the sort criteria.   We could fix this in 
+ * the future if the *mostly correct* sorting behavior is not good enough.  For now, the
+ * client can trigger a re-sort (e.g., by opening and closing the table) to fix the slightly 
+ * out-of-sort data.
+ * <p>
+ * The likelihood of the sort being inconsistent now relates directly to how many changed items are 
+ * in the table at the time of an insert.   The more changed items, the higher the chance of a 
+ * stale/misplaced item being used during a binary search, thus producing an invalid insert 
+ * position.  
+ * <p>
+ * This strategy is setup to mitigate the number of invalid items in the table at the time the 
+ * inserts are applied.   The basic workflow of this algorithm is:
+ * <pre>
+ * 1) condense the add / remove requests to remove duplicate efforts
+ * 2) process all removes first
+ *    --all pure removes
+ *    --all removes as part of a re-insert
+ * 3) process all items that failed to remove due to the sort data changing
+ * 4) process all adds (this step will fail if the data contains mis-sorted items)
+ *    --all adds as part of a re-insert
+ *    --all pure adds
+ * </pre>
+ * 
+ * Step 3, processing failed removals, is done to avoid a brute force lookup at each removal 
+ * request.   
+ * 
+ * <P>This strategy allows for the use of client proxy objects.   The proxy objects should be coded 
+ * such that the {@code hashCode()} and {@code equals()} methods will match those methods of the 
+ * data's real objects.  These proxy objects allow clients to search for an item without having a
+ * reference to the actual item.  In this sense, the proxy object is equal to the existing row
+ * object in the table model, but is not the <b>same</b> instance as the row object.
+ * 
+ * @param <T> the row type
+ */
+public class CoalescingAddRemoveStrategy<T> implements TableAddRemoveStrategy<T> {
+
+	@Override
+	public void process(List<AddRemoveListItem<T>> addRemoveList, TableData<T> tableData,
+			TaskMonitor monitor) throws CancelledException {
+
+		Set<AddRemoveListItem<T>> items = coalesceAddRemoveItems(addRemoveList);
+
+		//
+		// Hash map the existing values so that we can use any object inside the add/remove list
+		// as a key into this map to get the matching existing value.  Using the existing value
+		// enables the binary search to work when the add/remove item is a proxy object, but the
+		// existing item still has the data used to sort it.  If the sort data has changed, then
+		// even this step will not allow the TableData to find the item in a search.
+		//
+		Map<T, T> hashed = new HashMap<>();
+		for (T t : tableData) {
+			hashed.put(t, t);
+		}
+
+		Set<T> failedToRemove = new HashSet<>();
+
+		int n = items.size();
+		monitor.setMessage("Removing " + n + " items...");
+		monitor.initialize(n);
+
+		Iterator<AddRemoveListItem<T>> it = items.iterator();
+		while (it.hasNext()) {
+			AddRemoveListItem<T> item = it.next();
+			T value = item.getValue();
+			if (item.isChange()) {
+				T toRemove = hashed.remove(value);
+				remove(tableData, toRemove, failedToRemove);
+				monitor.incrementProgress(1);
+			}
+			else if (item.isRemove()) {
+				T toRemove = hashed.remove(value);
+				remove(tableData, toRemove, failedToRemove);
+				it.remove();
+			}
+			monitor.checkCanceled();
+		}
+
+		if (!failedToRemove.isEmpty()) {
+			int size = failedToRemove.size();
+			String message = size == 1 ? "1 old symbol..." : size + " old symbols...";
+			monitor.setMessage("Removing " + message);
+
+			tableData.process((data, sortContext) -> {
+				return expungeLostItems(failedToRemove, data, sortContext);
+			});
+		}
+
+		n = items.size();
+		monitor.setMessage("Adding " + n + " items...");
+		it = items.iterator();
+		while (it.hasNext()) {
+			AddRemoveListItem<T> item = it.next();
+			T value = item.getValue();
+			if (item.isChange()) {
+				tableData.insert(value);
+				hashed.put(value, value);
+			}
+			else if (item.isAdd()) {
+				tableData.insert(value);
+				hashed.put(value, value);
+			}
+			monitor.checkCanceled();
+			monitor.incrementProgress(1);
+		}
+
+		monitor.setMessage("Done adding/removing");
+	}
+
+	private Set<AddRemoveListItem<T>> coalesceAddRemoveItems(
+			List<AddRemoveListItem<T>> addRemoveList) {
+
+		Map<T, AddRemoveListItem<T>> map = new HashMap<>();
+
+		for (AddRemoveListItem<T> item : addRemoveList) {
+
+			if (item.isChange()) {
+				handleChange(item, map);
+			}
+			else if (item.isAdd()) {
+				handleAdd(item, map);
+			}
+			else {
+				handleRemove(item, map);
+			}
+		}
+
+		return new HashSet<>(map.values());
+	}
+
+	private void handleAdd(AddRemoveListItem<T> item, Map<T, AddRemoveListItem<T>> map) {
+
+		T rowObject = item.getValue();
+		AddRemoveListItem<T> existing = map.get(rowObject);
+		if (existing == null) {
+			map.put(rowObject, item);
+			return;
+		}
+
+		if (existing.isChange()) {
+			return; // change -> add; keep the change
+		}
+		if (existing.isAdd()) {
+			return; // already an add
+		}
+
+		// remove -> add; make a change
+		map.put(rowObject, new AddRemoveListItem<>(CHANGE, existing.getValue()));
+	}
+
+	private void handleRemove(AddRemoveListItem<T> item, Map<T, AddRemoveListItem<T>> map) {
+
+		T rowObject = item.getValue();
+		AddRemoveListItem<T> existing = map.get(rowObject);
+		if (existing == null) {
+			map.put(rowObject, item);
+			return;
+		}
+
+		if (existing.isChange()) {
+			map.put(rowObject, item); // change -> remove; just do the remove
+			return;
+		}
+		if (existing.isRemove()) {
+			return;
+		}
+
+		// add -> remove; do no work
+		map.remove(rowObject);
+	}
+
+	private void handleChange(AddRemoveListItem<T> item, Map<T, AddRemoveListItem<T>> map) {
+
+		T rowObject = item.getValue();
+		AddRemoveListItem<T> existing = map.get(rowObject);
+		if (existing == null) {
+			map.put(rowObject, item);
+			return;
+		}
+
+		if (!existing.isChange()) {
+			// either add or remove followed by a change; keep the change
+			map.put(rowObject, item);
+		}
+
+		// otherwise, we had a change followed by a change; keep just 1 change
+	}
+
+	private void remove(TableData<T> tableData, T t, Set<T> failedToRemove) {
+		if (t == null) {
+			return;
+		}
+
+		if (!tableData.remove(t)) {
+			failedToRemove.add(t);
+		}
+	}
+
+	/*
+	 * Removes the given set of items that were unsuccessfully removed from the table as part of
+	 * the add/remove process.   These items could not be removed because some part of their state 
+	 * has changed such that the binary search performed during the normal remove process cannot 
+	 * locate the item in the table data.   This algorithm will check the given set of items 
+	 * against the entire list of table data, locating the item to be removed.
+	 */
+	private List<T> expungeLostItems(Set<T> toRemove, List<T> data,
+			TableSortingContext<T> sortContext) {
+
+		if (sortContext.isUnsorted()) {
+			// this can happen if the data is unsorted and we were asked to remove an item that
+			// was never in the table for some reason
+			return data;
+		}
+
+		// Copy to a new list those items that are not marked for removal.  This saves the
+		// list move its items every time a remove takes place
+		List<T> newList = new ArrayList<>(data.size() - toRemove.size());
+		for (int i = 0; i < data.size(); i++) {
+			T rowObject = data.get(i);
+			if (!toRemove.contains(rowObject)) {
+				newList.add(rowObject);
+			}
+		}
+
+		return newList;
+	}
+}

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/RowObject.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,15 +15,15 @@
  */
 package docking.widgets.table;
 
-import ghidra.util.SystemUtilities;
-
 import java.util.ArrayList;
 import java.util.List;
 
 import javax.swing.table.TableModel;
 
+import ghidra.util.SystemUtilities;
+
 /**
- * An object that represents a row in a table.  Most tables used in the system create tables that
+ * An object that represents a row in a table.  Most tables used in the system create models that
  * use their own row objects (see {@link AbstractSortedTableModel}).  This class exists to 
  * compensate for those models that do not do this, but instead rely on the classic Java 
  * {@link TableModel} method {@link TableModel#getValueAt(int, int)}.
@@ -36,14 +35,18 @@ import javax.swing.table.TableModel;
  * row objects that created for non-{@link AbstractSortedTableModel}s will not be equal to 
  * those created before the data change.  This causes some features to break, such as selection
  * restoration after user edits.
+ * 
+ * @deprecated this class is no longer used and will be removed
  */
+@Deprecated(forRemoval = true, since = "10.1")
 public class RowObject {
 
 	/**
 	 * Factory method to create and initialize a row object.
 	 * 
 	 * @param model the model required to gather data for the row object.
-	 * @param row the row for which to create a row object	 * @return
+	 * @param row the row for which to create a row object	
+	 * @return the row object
 	 */
 	public static RowObject createRowObject(TableModel model, int row) {
 		RowObject rowObject = new RowObject();
@@ -54,7 +57,7 @@ public class RowObject {
 		return rowObject;
 	}
 
-	List<Object> values = new ArrayList<Object>();
+	List<Object> values = new ArrayList<>();
 	int hash = -1;
 
 	void addElement(Object object) {

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/DefaultAddRemoveStrategy.java

@@ -1,95 +0,0 @@
-/* ###
- * IP: GHIDRA
- *
- * 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.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package docking.widgets.table.threaded;
-
-import java.util.*;
-
-import docking.widgets.table.AddRemoveListItem;
-import docking.widgets.table.TableSortingContext;
-import ghidra.util.exception.CancelledException;
-import ghidra.util.task.TaskMonitor;
-
-/**
- * A strategy that uses the table's sort state to perform a binary search of items to be added
- * and removed.   
- *
- * <P>This strategy is aware that some items may not be correctly removed, such as database items
- * that have been deleted externally to the table.   These items may require a brute force update
- * to achieve removal.
- *
- * @param <T> the row type
- */
-public class DefaultAddRemoveStrategy<T> implements TableAddRemoveStrategy<T> {
-
-	@Override
-	public void process(List<AddRemoveListItem<T>> addRemoveList, TableData<T> tableData,
-			TaskMonitor monitor) throws CancelledException {
-
-		int n = addRemoveList.size();
-		monitor.setMessage("Adding/Removing " + n + " items...");
-		monitor.initialize(n);
-
-		Set<T> failedToRemove = new HashSet<>();
-
-		// Note: this class does not directly perform a binary search, but instead relies on that
-		//       work to be done by the call to TableData.remove()
-		for (int i = 0; i < n; i++) {
-			AddRemoveListItem<T> item = addRemoveList.get(i);
-			T value = item.getValue();
-			if (item.isChange()) {
-				tableData.remove(value);
-				tableData.insert(value);
-			}
-			else if (item.isRemove()) {
-				if (!tableData.remove(value)) {
-					failedToRemove.add(value);
-				}
-			}
-			else if (item.isAdd()) {
-				tableData.insert(value);
-			}
-			monitor.checkCanceled();
-			monitor.setProgress(i);
-		}
-
-		if (!failedToRemove.isEmpty()) {
-			int size = failedToRemove.size();
-			String message = size == 1 ? "1 deleted item..." : size + " deleted items...";
-			monitor.setMessage("Removing " + message);
-
-			tableData.process((data, sortContext) -> {
-				return expungeLostItems(failedToRemove, data, sortContext);
-			});
-		}
-
-		monitor.setMessage("Done adding/removing");
-	}
-
-	private List<T> expungeLostItems(Set<T> toRemove, List<T> data,
-			TableSortingContext<T> sortContext) {
-
-		List<T> newList = new ArrayList<>(data.size() - toRemove.size());
-		for (int i = 0; i < data.size(); i++) {
-			T rowObject = data.get(i);
-			if (!toRemove.contains(rowObject)) {
-				newList.add(rowObject);
-			}
-		}
-
-		return newList;
-	}
-
-}

Ghidra/Framework/Docking/src/main/java/docking/widgets/table/threaded/ThreadedTableModel.java

@@ -93,7 +93,7 @@ public abstract class ThreadedTableModel<ROW_OBJECT, DATA_SOURCE>
 	private int minUpdateDelayMillis;
 	private int maxUpdateDelayMillis;
 	private TableAddRemoveStrategy<ROW_OBJECT> binarySearchAddRemoveStrategy =
-		new DefaultAddRemoveStrategy<>();
+		new CoalescingAddRemoveStrategy<>();
 
 	protected ThreadedTableModel(String modelName, ServiceProvider serviceProvider) {
 		this(modelName, serviceProvider, null);

Ghidra/Framework/Docking/src/test/java/docking/widgets/table/CoalescingAddRemoveStrategyTest.java

@@ -0,0 +1,396 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package docking.widgets.table;
+
+import static docking.widgets.table.AddRemoveListItem.Type.*;
+import static org.junit.Assert.*;
+
+import java.util.*;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import docking.widgets.table.threaded.TestTableData;
+import ghidra.util.task.TaskMonitor;
+
+public class CoalescingAddRemoveStrategyTest {
+
+	private CoalescingAddRemoveStrategy<TestRowObject> strategy;
+	private SpyTableData spyTableData;
+	private List<TestRowObject> modelData;
+
+	@Before
+	public void setUp() throws Exception {
+		strategy = new CoalescingAddRemoveStrategy<>();
+		modelData = createModelData();
+		spyTableData = createTableData();
+	}
+
+	private SpyTableData createTableData() {
+		Comparator<TestRowObject> comparator = (s1, s2) -> {
+			return s1.getName().compareTo(s2.getName());
+		};
+		TableSortState sortState = TableSortState.createDefaultSortState(0);
+		TableSortingContext<TestRowObject> sortContext =
+			new TableSortingContext<>(sortState, comparator);
+
+		modelData.sort(comparator);
+
+		return new SpyTableData(modelData, sortContext);
+	}
+
+	private List<TestRowObject> createModelData() {
+		List<TestRowObject> data = new ArrayList<>();
+		data.add(new TestRowObject(1));
+		data.add(new TestRowObject(2));
+		data.add(new TestRowObject(3));
+		data.add(new TestRowObject(4));
+		return data;
+	}
+
+	@Test
+	public void testRemove_DifferentInstance_SameId() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = new TestRowObject(1);
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(0, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testInsert_NewSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject newSymbol = new TestRowObject(10);
+		addRemoves.add(new AddRemoveListItem<>(ADD, newSymbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+		assertEquals(0, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testInsertAndRemove_NewSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject newSymbol = new TestRowObject(10);
+		addRemoves.add(new AddRemoveListItem<>(ADD, newSymbol));
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, newSymbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// no work was done, since the insert was followed by a remove
+		assertEquals(0, spyTableData.getRemoveCount());
+		assertEquals(0, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testChange_NewSymbol() throws Exception {
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject newSymbol = new TestRowObject(10);
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, newSymbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// no remove, since the symbol was not in the table
+		assertEquals(0, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testChange_ExisingSymbol() throws Exception {
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// no remove, since the symbol was not in the table
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testRemoveAndInsert_NewSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject newSymbol = new TestRowObject(10);
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, newSymbol));
+		addRemoves.add(new AddRemoveListItem<>(ADD, newSymbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the remove does not happen, since the time was not in the table
+		assertEquals(0, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testRemoveAndInsert_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+		addRemoves.add(new AddRemoveListItem<>(ADD, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the remove does not happen, since the time was not in the table
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testChangeAndInsert_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+		addRemoves.add(new AddRemoveListItem<>(ADD, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the insert portions get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testChangeAndRemove_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the remove portions get coalesced; no insert takes place
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(0, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testChangeAndChange_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the changes get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testRemoveAndRemove_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the removes get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(0, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testInsertAndInsert_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(ADD, ro));
+		addRemoves.add(new AddRemoveListItem<>(ADD, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the inserts get coalesced
+		assertEquals(0, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testInsertAndChange_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(ADD, ro));
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the insert portions get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testRemoveAndChange_ExistingSymbol() throws Exception {
+
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject ro = modelData.get(0);
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, ro));
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, ro));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the remove portions get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testLostItems_Remove() throws Exception {
+
+		// 
+		// Test that symbols get removed when the data up on which they are sorted changes before
+		// the removal takes place
+		//
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject symbol = modelData.get(0);
+		symbol.setName("UpdatedName");
+		addRemoves.add(new AddRemoveListItem<>(REMOVE, symbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the insert portions get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(0, spyTableData.getInsertCount());
+	}
+
+	@Test
+	public void testLostItems_Change() throws Exception {
+
+		// 
+		// Test that symbols get removed when the data up on which they are sorted changes before
+		// the removal takes place
+		//
+		List<AddRemoveListItem<TestRowObject>> addRemoves = new ArrayList<>();
+
+		TestRowObject symbol = modelData.get(0);
+		symbol.setName("UpdatedName");
+		addRemoves.add(new AddRemoveListItem<>(CHANGE, symbol));
+
+		strategy.process(addRemoves, spyTableData, TaskMonitor.DUMMY);
+
+		// the insert portions get coalesced
+		assertEquals(1, spyTableData.getRemoveCount());
+		assertEquals(1, spyTableData.getInsertCount());
+	}
+
+//==================================================================================================
+// Inner Classes
+//==================================================================================================	
+
+	private class SpyTableData extends TestTableData<TestRowObject> {
+
+		private int removeCount;
+		private int insertCount;
+
+		SpyTableData(List<TestRowObject> data, TableSortingContext<TestRowObject> sortContext) {
+			super(data, sortContext);
+		}
+
+		@Override
+		public boolean remove(TestRowObject t) {
+			removeCount++;
+			return super.remove(t);
+		}
+
+		@Override
+		public void insert(TestRowObject value) {
+			insertCount++;
+			super.insert(value);
+		}
+
+		int getRemoveCount() {
+			return removeCount;
+		}
+
+		int getInsertCount() {
+			return insertCount;
+		}
+	}
+
+	private class TestRowObject {
+
+		private String name;
+		private long id;
+
+		TestRowObject(long id) {
+			this.id = id;
+			this.name = Long.toString(id);
+		}
+
+		String getName() {
+			return name;
+		}
+
+		void setName(String newName) {
+			this.name = newName;
+		}
+
+		@Override
+		public int hashCode() {
+			return (int) id;
+		}
+
+		@Override
+		public boolean equals(Object obj) {
+			if (this == obj) {
+				return true;
+			}
+			if (obj == null) {
+				return false;
+			}
+			if (getClass() != obj.getClass()) {
+				return false;
+			}
+
+			TestRowObject other = (TestRowObject) obj;
+			if (id != other.id) {
+				return false;
+			}
+			return true;
+		}
+
+	}
+}

Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/dialog/ExtensionTableModel.java

@@ -18,8 +18,7 @@ package ghidra.framework.plugintool.dialog;
 import java.awt.Color;
 import java.awt.Component;
 import java.io.File;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.*;
 
 import docking.widgets.table.*;
 import docking.widgets.table.threaded.ThreadedTableModel;
@@ -52,7 +51,7 @@ import utilities.util.FileUtilities;
  * is a checkbox allowing users to install/uninstall a particular extension. 
  * 
  */
-class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<ExtensionDetails>> {
+class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, Object> {
 
 	/** We don't care about the ordering of other columns, but the install/uninstall checkbox should be 
 	 the first one and the name col is our initial sort column. */
@@ -60,7 +59,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	final static int NAME_COL = 1;
 
 	/** This is the data source for the model. Whatever is here will be displayed in the table. */
-	private List<ExtensionDetails> extensions = new ArrayList<>();
+	private Set<ExtensionDetails> extensions;
 
 	/** Indicates if the model has changed due to an install or uninstall. */
 	private boolean modelChanged = false;
@@ -78,7 +77,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	protected TableColumnDescriptor<ExtensionDetails> createTableColumnDescriptor() {
 
 		TableColumnDescriptor<ExtensionDetails> descriptor =
-			new TableColumnDescriptor<ExtensionDetails>();
+			new TableColumnDescriptor<>();
 
 		descriptor.addVisibleColumn(new ExtensionInstalledColumn(), INSTALLED_COL, true);
 		descriptor.addVisibleColumn(new ExtensionNameColumn(), NAME_COL, true);
@@ -178,20 +177,33 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	}
 
 	@Override
-	public List<ExtensionDetails> getDataSource() {
-		return extensions;
+	public Object getDataSource() {
+		return null;
 	}
 
 	@Override
 	protected void doLoad(Accumulator<ExtensionDetails> accumulator, TaskMonitor monitor)
 			throws CancelledException {
+		if (extensions != null) {
+			accumulator.addAll(extensions);
+			return;
+		}
+
+		try {
+			extensions = ExtensionUtils.getExtensions();
+		}
+		catch (ExtensionException e) {
+			Msg.error(this, "Error loading extensions", e);
+			return;
+		}
+
 		accumulator.addAll(extensions);
 	}
 
 	/**
-	 * Returns true if the model has changed as a result of installing or uninstalling an extension.
+	 * Returns true if the model has changed as a result of installing or uninstalling an extension
 	 * 
-	 * @return true if the model has changed as a result of installing or uninstalling an extension.
+	 * @return true if the model has changed as a result of installing or uninstalling an extension
 	 */
 	public boolean hasModelChanged() {
 		return modelChanged;
@@ -203,7 +215,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * @param model the list to use as the model
 	 */
 	public void setModelData(List<ExtensionDetails> model) {
-		extensions = model;
+		extensions = new HashSet<>(model);
 		reload();
 	}
 
@@ -211,12 +223,8 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Gets a new set of extensions and reloads the table.
 	 */
 	public void refreshTable() {
-		try {
-			setModelData(new ArrayList<ExtensionDetails>(ExtensionUtils.getExtensions()));
-		}
-		catch (ExtensionException e) {
-			Msg.error(this, "Error loading extensions", e);
-		}
+		extensions = null;
+		reload();
 	}
 
 	/**
@@ -236,7 +244,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension name.
 	 */
 	private class ExtensionNameColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, String, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, String, Object> {
 
 		private ExtVersionRenderer renderer = new ExtVersionRenderer();
 
@@ -252,7 +260,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public String getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 			return rowObject.getName();
 		}
 
@@ -266,7 +274,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension description.
 	 */
 	private class ExtensionDescriptionColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, String, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, String, Object> {
 
 		private ExtVersionRenderer renderer = new ExtVersionRenderer();
 
@@ -282,7 +290,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public String getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 			return rowObject.getDescription();
 		}
 
@@ -296,7 +304,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension description.
 	 */
 	private class ExtensionVersionColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, String, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, String, Object> {
 
 		private ExtVersionRenderer renderer = new ExtVersionRenderer();
 
@@ -312,7 +320,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public String getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 
 			String version = rowObject.getVersion();
 
@@ -335,7 +343,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension installation status.
 	 */
 	private class ExtensionInstalledColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, Boolean, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, Boolean, Object> {
 
 		@Override
 		public String getColumnName() {
@@ -349,7 +357,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public Boolean getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 			return rowObject.isInstalled();
 		}
 	}
@@ -358,7 +366,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension installation directory.
 	 */
 	private class ExtensionInstallationDirColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, String, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, String, Object> {
 
 		@Override
 		public String getColumnName() {
@@ -372,7 +380,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public String getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 			return rowObject.getInstallPath();
 		}
 	}
@@ -381,7 +389,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 	 * Table column for displaying the extension archive file.
 	 */
 	private class ExtensionArchiveFileColumn
-			extends AbstractDynamicTableColumn<ExtensionDetails, String, List<ExtensionDetails>> {
+			extends AbstractDynamicTableColumn<ExtensionDetails, String, Object> {
 
 		@Override
 		public String getColumnName() {
@@ -395,7 +403,7 @@ class ExtensionTableModel extends ThreadedTableModel<ExtensionDetails, List<Exte
 
 		@Override
 		public String getValue(ExtensionDetails rowObject, Settings settings,
-				List<ExtensionDetails> data, ServiceProvider sp) throws IllegalArgumentException {
+				Object data, ServiceProvider sp) throws IllegalArgumentException {
 			return rowObject.getArchivePath();
 		}
 	}