GT-2947 - updated Edit Function dialog to put focus in the signature

field by default; added undo/redo support

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionEditorDialog.java

@@ -27,8 +27,7 @@ import javax.swing.border.CompoundBorder;
 import javax.swing.event.*;
 import javax.swing.table.TableCellEditor;
 
-import docking.DialogComponentProvider;
-import docking.DockingUtils;
+import docking.*;
 import docking.widgets.OptionDialog;
 import docking.widgets.checkbox.GCheckBox;
 import docking.widgets.combobox.GComboBox;
@@ -73,7 +72,10 @@ public class FunctionEditorDialog extends DialogComponentProvider implements Mod
 	private JCheckBox storageCheckBox;
 	private JScrollPane scroll;
 	private JPanel previewPanel;
+
 	private FunctionSignatureTextField signatureTextField;
+	private UndoRedoKeeper signatureFieldUndoRedoKeeper;
+
 	private MyGlassPane glassPane;
 	private JPanel centerPanel;
 
@@ -94,7 +96,6 @@ public class FunctionEditorDialog extends DialogComponentProvider implements Mod
 		addCancelButton();
 		glassPane = new MyGlassPane();
 		dataChanged();
-		setFocusComponent(nameField);
 	}
 
 	private static String createTitle(Function function) {
@@ -122,6 +123,23 @@ public class FunctionEditorDialog extends DialogComponentProvider implements Mod
 		return strBuilder.toString();
 	}
 
+	@Override
+	protected void dialogShown() {
+
+		// put user focus in the signature field, ready to take keyboard input
+		signatureTextField.requestFocus();
+		Swing.runLater(() -> {
+			int start = model.getFunctionNameStartPosition();
+			int end = model.getNameString().length();
+			signatureTextField.setCaretPosition(end);
+			signatureTextField.setSelectionStart(start);
+			signatureTextField.setSelectionEnd(start + end);
+
+			// reset any edits that happened before the user interacted with the field
+			signatureFieldUndoRedoKeeper.clear();
+		});
+	}
+
 	@Override
 	protected void okCallback() {
 		if (model.isInParsingMode()) {
@@ -223,6 +241,9 @@ public class FunctionEditorDialog extends DialogComponentProvider implements Mod
 	private JComponent createSignatureTextPanel() {
 		JPanel panel = new JPanel(new BorderLayout());
 		signatureTextField = new FunctionSignatureTextField();
+
+		signatureFieldUndoRedoKeeper = DockingUtils.installUndoRedo(signatureTextField);
+
 		Font font = signatureTextField.getFont();
 		signatureTextField.setFont(font.deriveFont(18.0f));
 		panel.add(signatureTextField);
@@ -559,7 +580,13 @@ public class FunctionEditorDialog extends DialogComponentProvider implements Mod
 	private void updatePreviewField() {
 		String preview = model.getFunctionSignatureTextFromModel();
 		int caretPosition = signatureTextField.getCaretPosition();
-		signatureTextField.setText(preview);
+
+		// don't cause undo/redo updates if the text has not changed
+		String oldText = signatureTextField.getText();
+		if (!preview.equals(oldText)) {
+			signatureTextField.setText(preview);
+		}
+
 		if (!model.hasValidName()) {
 			signatureTextField.setError(model.getFunctionNameStartPosition(),
 				model.getNameString().length());

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/function/editor/FunctionSignatureTextField.java

@@ -28,6 +28,7 @@ import javax.swing.event.*;
 import javax.swing.text.*;
 
 import docking.actions.KeyBindingUtils;
+import ghidra.util.Swing;
 
 class FunctionSignatureTextField extends JTextPane {
 	private static final String ENTER_ACTION_NAME = "ENTER";
@@ -138,30 +139,22 @@ class FunctionSignatureTextField extends JTextPane {
 	}
 
 	private void updateColors() {
-		SwingUtilities.invokeLater(new Runnable() {
-			@Override
-			public void run() {
-				String text = getText();
-				List<ColorField> computeColors = computeColors(text);
-				if (computeColors != null) {
-					doc.setCharacterAttributes(0, text.length(), defaultAttributes, true);
-					for (ColorField colorField : computeColors) {
-						doc.setCharacterAttributes(colorField.start, colorField.length(),
-							colorField.attributes, true);
-					}
+		Swing.runLater(() -> {
+			String text = getText();
+			List<ColorField> computeColors = computeColors(text);
+			if (computeColors != null) {
+				doc.setCharacterAttributes(0, text.length(), defaultAttributes, true);
+				for (ColorField colorField : computeColors) {
+					doc.setCharacterAttributes(colorField.start, colorField.length(),
+						colorField.attributes, true);
 				}
-				notifyChange();
 			}
+			notifyChange();
 		});
 	}
 
 	void clearAttributes(final int start, final int length) {
-		SwingUtilities.invokeLater(new Runnable() {
-			@Override
-			public void run() {
-				doc.setCharacterAttributes(start, length, defaultAttributes, true);
-			}
-		});
+		Swing.runLater(() -> doc.setCharacterAttributes(start, length, defaultAttributes, true));
 	}
 
 	void notifyChange() {
@@ -175,7 +168,7 @@ class FunctionSignatureTextField extends JTextPane {
 	}
 
 	List<ColorField> computeColors(String text) {
-		List<ColorField> list = new ArrayList<ColorField>();
+		List<ColorField> list = new ArrayList<>();
 		int functionRightParenIndex = text.lastIndexOf(')');
 		int functionLeftParenIndex = findMatchingLeftParenIndex(text, functionRightParenIndex);
 		if (functionLeftParenIndex < 0) {
@@ -190,13 +183,12 @@ class FunctionSignatureTextField extends JTextPane {
 
 		SubString substring = new SubString(text, 0, functionLeftParenIndex).trim();
 		SubString functionName = getLastWord(substring);
-//		SubString returnTypeString = getAllButLastWord(substring);
 		if (functionName == null) {
 			return null;
 		}
 
-		list.add(new ColorField(functionName.getStart(), functionName.getEnd(),
-			functionNameAttributes));
+		list.add(
+			new ColorField(functionName.getStart(), functionName.getEnd(), functionNameAttributes));
 		for (int i = 0; i < paramStartStopIndexes.size() - 1; i++) {
 			int start = paramStartStopIndexes.get(i) + 1;
 			int end = paramStartStopIndexes.get(i + 1);
@@ -233,16 +225,8 @@ class FunctionSignatureTextField extends JTextPane {
 		return string.substring(lastIndexOf + 1);
 	}
 
-//	private SubString getAllButLastWord(SubString string) {
-//		int lastIndexOf = string.lastIndexOf(' ');
-//		if (lastIndexOf < 0) {
-//			return null;
-//		}
-//		return string.substring(0, lastIndexOf).trim();
-//	}
-
 	private List<Integer> findParamStartStopindexes(String text, int startIndex, int endIndex) {
-		List<Integer> commaIndexes = new ArrayList<Integer>();
+		List<Integer> commaIndexes = new ArrayList<>();
 		int templateCount = 0;
 		commaIndexes.add(startIndex);
 		for (int i = startIndex + 1; i < endIndex; i++) {
@@ -299,7 +283,7 @@ class FunctionSignatureTextField extends JTextPane {
 
 	public static void main(String[] args) {
 		JFrame jFrame = new JFrame();
-		jFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
+		jFrame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
 		FunctionSignatureTextField field = new FunctionSignatureTextField();
 		JPanel panel = new JPanel(new BorderLayout());
 		panel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
@@ -309,7 +293,7 @@ class FunctionSignatureTextField extends JTextPane {
 		jFrame.setVisible(true);
 	}
 
-	class SubString {
+	private class SubString {
 		private String text;
 		private int subStringStart;
 		private int subStringEnd;
@@ -340,24 +324,11 @@ class FunctionSignatureTextField extends JTextPane {
 			return new SubString(text, subStringStart + start, subStringEnd);
 		}
 
-		public SubString substring(int start, int end) {
-			return new SubString(text, subStringStart + start, subStringStart + end);
-		}
-
 		@Override
 		public String toString() {
 			return text.substring(subStringStart, subStringEnd);
 		}
 
-		public int indexOf(char c) {
-			for (int i = subStringStart; i < subStringEnd; i++) {
-				if (text.charAt(i) == c) {
-					return i - subStringStart;
-				}
-			}
-			return -1;
-		}
-
 		public int lastIndexOf(char c) {
 			for (int i = subStringEnd - 1; i >= subStringStart; i--) {
 				if (text.charAt(i) == c) {
@@ -385,12 +356,6 @@ class FunctionSignatureTextField extends JTextPane {
 	}
 
 	void setError(final int position, final int length) {
-		SwingUtilities.invokeLater(new Runnable() {
-
-			@Override
-			public void run() {
-				doc.setCharacterAttributes(position, length, errorAttributes, true);
-			}
-		});
+		Swing.runLater(() -> doc.setCharacterAttributes(position, length, errorAttributes, true));
 	}
 }

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/script/GhidraScriptComponentProvider.java

@@ -1059,6 +1059,12 @@ public class GhidraScriptComponentProvider extends ComponentProviderAdapter {
 		}
 	}
 
+	@Override
+	public void componentActivated() {
+		// put the user focus in the filter field, as often the user wishes to search for a script
+		tableFilterPanel.requestFocus();
+	}
+
 	@Override
 	public ActionContext getActionContext(MouseEvent event) {
 		Object source = scriptTable;

Ghidra/Framework/Docking/src/main/java/docking/DockingUtils.java

@@ -87,7 +87,7 @@ import resources.ResourceManager;
  * 	<tr><td>{@link JList}</td><td>{@link GList}</td></tr>
  * 	<tr><td>{@link ListCellRenderer}<br>{@link DefaultListCellRenderer}</td><td>{@link GListCellRenderer}</td></tr>
  * 	<tr><td>{@link TableCellRenderer}</td><td>{@link GTableCellRenderer}</td></tr>
- * 	<tr><td>{@link TreeCellRenderer}<br>{@link DefaultTreeCellRenderer}</td><td>{@link GTreeRenderer}<br>{@link DnDTreeCellRenderer}</td></tr>
+ * 	<tr><td>{@link TreeCellRenderer}<br>{@link DefaultTreeCellRenderer}</td><td>{@link GTreeRenderer}<br><code>DnDTreeCellRenderer</code></td></tr>
  * 	<tr><td>{@link JRadioButton}</td><td>{@link GRadioButton}</td></tr>
  * 	<tr><td>{@link JButton}</td><td>???tbd???</td></tr>
  * </table>
@@ -179,15 +179,6 @@ public class DockingUtils {
 		final UndoRedoKeeper undoRedoKeeper = new UndoRedoKeeper();
 		document.addUndoableEditListener(e -> {
 			UndoableEdit edit = e.getEdit();
-
-// TODO We are now handed a wrapper class and not the event for the 'edit'.  It is not clear
-//		which use case caused this code to be added.  If/when we find out, we can revisit how 
-//		to filter these types of updates.
-//			DefaultDocumentEvent defaultDocumentEvent = (DefaultDocumentEvent) edit;
-//			if (defaultDocumentEvent.getType() == EventType.CHANGE) {
-//				return; // this happens for style updates
-//			}
-
 			undoRedoKeeper.addUndo(edit);
 		});
 

Ghidra/Framework/Docking/src/main/java/docking/UndoRedoKeeper.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,31 +15,85 @@
  */
 package docking;
 
-import ghidra.util.datastruct.FixedSizeStack;
-
+import javax.swing.JTextPane;
+import javax.swing.undo.CompoundEdit;
 import javax.swing.undo.UndoableEdit;
 
+import ghidra.util.datastruct.FixedSizeStack;
+
+/**
+ * Handles tracking undo and redo events.   Clients may wish to hold on to this class in order
+ * to clear the undo/redo queue.
+ * 
+ * <p><b><u>Style Edits</u></b><br>
+ * {@link JTextPane}s allow for styles (color, bold, etc) to be applied to their text.  The
+ * default undo/redo events may arrive singly, not in bulk.   Thus, when the user presses undo, 
+ * each style change is undo, one at a time.   This is intuitive when the user controls the 
+ * application of style.  However, when style is applied programmatically, it can be odd to 
+ * see that the user-type text does not change, but just the coloring applied to that text.
+ * <p>
+ * To address this issue, this class takes the approach of combining all style edits into a 
+ * single bulk edit.  Then, as the user presses undo, all style edits can be removed together, as
+ * well as any neighboring text edits.   <b>Put simply, this class tracks style edits such 
+ * that an undo operation will undo all style changes, as well as a single text edit.</b>
+ */
 public class UndoRedoKeeper {
 
 	private static final int MAX_UNDO_REDO_SIZE = 50;
+	private static final String STYLE_EDIT_KEY = "style";
 
-	private FixedSizeStack<UndoableEdit> undoStack = new FixedSizeStack<UndoableEdit>(
-		MAX_UNDO_REDO_SIZE);
-	private FixedSizeStack<UndoableEdit> redoStack = new FixedSizeStack<UndoableEdit>(
-		MAX_UNDO_REDO_SIZE);
+	private FixedSizeStack<UndoableEdit> undoStack = new FixedSizeStack<>(MAX_UNDO_REDO_SIZE);
+	private FixedSizeStack<UndoableEdit> redoStack = new FixedSizeStack<>(MAX_UNDO_REDO_SIZE);
+
+	private StyleCompoundEdit lastStyleUndo;
 
 	void addUndo(UndoableEdit edit) {
+
+		String name = edit.getPresentationName();
+		if (name.contains(STYLE_EDIT_KEY)) {
+			// (see header note about style edits)
+			addStyleEdit(edit);
+			return;
+		}
+
+		endOutstandingStyleEdits();
+
 		undoStack.push(edit);
-		redoStack.clear();
+		redoStack.clear(); // new edit added; clear redo
+	}
+
+	private void endOutstandingStyleEdits() {
+		if (lastStyleUndo != null) {
+			lastStyleUndo.end();
+			lastStyleUndo = null;
+		}
+	}
+
+	private void addStyleEdit(UndoableEdit edit) {
+		if (lastStyleUndo == null) {
+			lastStyleUndo = new StyleCompoundEdit();
+			undoStack.push(lastStyleUndo);
+		}
+
+		lastStyleUndo.addEdit(edit);
+		redoStack.clear(); // new edit added; clear redo
 	}
 
 	void undo() {
 		if (undoStack.isEmpty()) {
 			return;
 		}
+
+		endOutstandingStyleEdits();
+
 		UndoableEdit item = undoStack.pop();
 		redoStack.push(item);
 		item.undo();
+
+		// (see header note)
+		if (item instanceof StyleCompoundEdit) {
+			undo(); // call again to get a 'real' edit
+		}
 	}
 
 	void redo() {
@@ -48,13 +101,24 @@ public class UndoRedoKeeper {
 			return;
 		}
 
+		endOutstandingStyleEdits();
+
 		UndoableEdit item = redoStack.pop();
 		undoStack.push(item);
 		item.redo();
+
+		// (see header note)
+		if (item instanceof StyleCompoundEdit) {
+			undo(); // call again to get a 'real' edit
+		}
 	}
 
 	public void clear() {
 		undoStack.clear();
 		redoStack.clear();
 	}
+
+	private static class StyleCompoundEdit extends CompoundEdit {
+		// simple class for us to track internally
+	}
 }