From 94027bf3a6b01ff96ed6523265554bcf35408a3a Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Wed, 27 Jul 2022 16:33:13 -0400 Subject: [PATCH 1/2] GP-2328 - Fixed exception in listing when xrefs are grouped by function and the number of xrefs to display is 1; fixed xrefs '...' text so that it will show the xrefs dialog when double-clicked --- .../util/viewer/field/XRefFieldFactory.java | 25 ++++++++++++++----- .../viewer/field/XRefFieldMouseHandler.java | 21 +++++++++++++--- .../CompositeVerticalLayoutTextField.java | 5 ++-- .../fieldpanel/field/StrutFieldElement.java | 23 ++++++++++++++--- .../field/VerticalLayoutTextField.java | 3 +-- 5 files changed, 59 insertions(+), 18 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldFactory.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldFactory.java index 1358606747..add67b32f2 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldFactory.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldFactory.java @@ -385,7 +385,7 @@ public class XRefFieldFactory extends FieldFactory { int availableLines = maxLines - functionRows.size(); if (tooMany) { // save room for the "more" field at the end - availableLines -= 1; + availableLines = Math.max(1, availableLines--); } // @@ -394,7 +394,7 @@ public class XRefFieldFactory extends FieldFactory { // // Note: the objects we build here want the 'data' row as a parameter, not the screen row. - // Out screen rows are what we are building to display; a data row we are here + // Our screen rows are what we are building to display; a data row we are here // defining to be a single xref. This is a somewhat arbitrary decision. int dataRow = totalXrefs - noFunction.size(); TextField noFunctionXrefsField = @@ -802,10 +802,14 @@ public class XRefFieldFactory extends FieldFactory { RowColLocation loc = field.screenToDataLocation(row, col); if (element instanceof XrefFieldElement) { XrefFieldElement xrefElement = (XrefFieldElement) element; - Reference xref = xrefElement.getXref(); - Address refAddr = xref.getFromAddress(); - return new XRefFieldLocation(cu.getProgram(), cu.getMinAddress(), cpath, refAddr, row, - loc.col()); + return getXRefLocation(xrefElement, cu, cpath, loc, row); + } + else if (element instanceof StrutFieldElement) { + FieldElement baseElement = ((StrutFieldElement) element).getBaseType(); + if (baseElement instanceof XrefFieldElement) { + XrefFieldElement xrefElement = (XrefFieldElement) baseElement; + return getXRefLocation(xrefElement, cu, cpath, loc, row); + } } String text = element.getText(); @@ -817,6 +821,15 @@ public class XRefFieldFactory extends FieldFactory { return null; } + private XRefFieldLocation getXRefLocation(XrefFieldElement xrefElement, CodeUnit cu, + int[] cpath, + RowColLocation loc, int row) { + Reference xref = xrefElement.getXref(); + Address refAddr = xref.getFromAddress(); + return new XRefFieldLocation(cu.getProgram(), cu.getMinAddress(), cpath, refAddr, row, + loc.col()); + } + protected String getBlockName(Program pgm, Address addr) { Memory mem = pgm.getMemory(); MemoryBlock block = mem.getBlock(addr); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldMouseHandler.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldMouseHandler.java index 5f4b52f883..045db6883f 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldMouseHandler.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/viewer/field/XRefFieldMouseHandler.java @@ -18,8 +18,7 @@ package ghidra.app.util.viewer.field; import java.awt.event.MouseEvent; import java.util.Set; -import docking.widgets.fieldpanel.field.FieldElement; -import docking.widgets.fieldpanel.field.TextField; +import docking.widgets.fieldpanel.field.*; import ghidra.app.nav.Navigatable; import ghidra.app.services.GoToService; import ghidra.app.util.XReferenceUtils; @@ -59,8 +58,7 @@ public class XRefFieldMouseHandler implements FieldMouseHandlerExtension { } Address referencedAddress = getFromReferenceAddress(location); - String clickedText = getText(clickedObject); - boolean isInvisibleXRef = XRefFieldFactory.MORE_XREFS_STRING.equals(clickedText); + boolean isInvisibleXRef = isInvisibleXRef(clickedObject); if (isInvisibleXRef) { showXRefDialog(sourceNavigatable, location, serviceProvider); return true; @@ -69,6 +67,21 @@ public class XRefFieldMouseHandler implements FieldMouseHandlerExtension { return goTo(sourceNavigatable, referencedAddress, goToService); } + private boolean isInvisibleXRef(Object clickedObject) { + + String clickedText = getText(clickedObject); + if (XRefFieldFactory.MORE_XREFS_STRING.equals(clickedText)) { + return true; + } + + if (clickedObject instanceof StrutFieldElement) { + // this implies that the xrefs field has been clipped and has used a struct to trigger + // clipping + return true; + } + return false; + } + protected boolean isXREFHeaderLocation(ProgramLocation location) { return location instanceof XRefHeaderFieldLocation; } diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/CompositeVerticalLayoutTextField.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/CompositeVerticalLayoutTextField.java index b28d4e97c9..58096b17c5 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/CompositeVerticalLayoutTextField.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/CompositeVerticalLayoutTextField.java @@ -146,7 +146,7 @@ public class CompositeVerticalLayoutTextField implements TextField { FieldElement[] elements = new FieldElement[] { element, - new StrutFieldElement(500) + new StrutFieldElement(element, 500) }; FieldElement compositeElement = new CompositeFieldElement(elements); return new ClippingTextField(startX, width, compositeElement, hlFactory); @@ -284,14 +284,13 @@ public class CompositeVerticalLayoutTextField implements TextField { int startY = myStartY; int translatedY = 0; - for (int i = 0; i < fieldRows.size(); i++) { + for (FieldRow fieldRow : fieldRows) { // if past clipping region we are done if (startY > clipEndY) { break; } - FieldRow fieldRow = fieldRows.get(i); TextField field = fieldRow.field; int subFieldHeight = fieldRow.field.getHeight(); int endY = startY + subFieldHeight; diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/StrutFieldElement.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/StrutFieldElement.java index c74069d6ae..9cc3ad8838 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/StrutFieldElement.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/StrutFieldElement.java @@ -28,12 +28,29 @@ import docking.widgets.fieldpanel.support.RowColLocation; */ public class StrutFieldElement implements FieldElement { + private final FieldElement baseElement; private final int width; - public StrutFieldElement(int width) { + /** + * Constructor. Clients may choose to pass + * + * @param baseElement the base type replaced by this strut; may be null if no type is being + * replaced + * @param width the width of this strut class + */ + public StrutFieldElement(FieldElement baseElement, int width) { + this.baseElement = baseElement; this.width = width; } + /** + * Returns the base type replaced by this strut; may be null + * @return the base type replaced by this strut; may be null + */ + public FieldElement getBaseType() { + return baseElement; + } + @Override public char charAt(int index) { return ' '; @@ -101,12 +118,12 @@ public class StrutFieldElement implements FieldElement { @Override public FieldElement substring(int start) { - return new StrutFieldElement(0); + return new StrutFieldElement(baseElement, 0); } @Override public FieldElement substring(int start, int end) { - return new StrutFieldElement(0); + return new StrutFieldElement(baseElement, 0); } @Override diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/VerticalLayoutTextField.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/VerticalLayoutTextField.java index 527553165e..11606f3a5b 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/VerticalLayoutTextField.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/fieldpanel/field/VerticalLayoutTextField.java @@ -446,7 +446,7 @@ public class VerticalLayoutTextField implements TextField { if (tooManyLines && (i == maxLines - 1)) { FieldElement[] elements = new FieldElement[2]; elements[0] = element; - elements[1] = new StrutFieldElement(500); + elements[1] = new StrutFieldElement(element, 500); element = new CompositeFieldElement(elements); } TextField field = createFieldForLine(element); @@ -459,7 +459,6 @@ public class VerticalLayoutTextField implements TextField { } isClipped |= tooManyLines; - return newSubFields; } From 53476edf4770bc03aff9071123c2cf151ea03744 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Thu, 28 Jul 2022 18:35:57 -0400 Subject: [PATCH 2/2] GP-2371 - Fixed tool restoring exception --- .../java/docking/ComponentPlaceholder.java | 10 ++++-- .../main/java/docking/DetachedWindowNode.java | 30 +++++++++++++--- .../main/java/docking/PlaceholderManager.java | 35 +++++++++++-------- .../src/main/java/docking/RootNode.java | 14 +++++--- 4 files changed, 65 insertions(+), 24 deletions(-) diff --git a/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java b/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java index 9008c949ae..9d4fcafc7d 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/ComponentPlaceholder.java @@ -35,6 +35,12 @@ import utilities.util.reflection.ReflectionUtilities; * Class to hold information about a dockable component with respect to its position within the * windowing system. It also holds identification information about the provider so that its * location can be reused when the provider is re-opened. + *

+ * The placeholder will be used to link previously saved position information. The tool will + * initially construct plugins and their component providers with default position information. + * Then, any existing xml data will be restored, which may have provider position information. + * The restoring of the xml will create placeholders with this saved information. Finally, the + * restored placeholders will be linked with existing component providers. */ public class ComponentPlaceholder { private String name; @@ -68,7 +74,7 @@ public class ComponentPlaceholder { * @param name the name of the component * @param owner the owner of the component * @param group the window group - * @param title the title + * @param title the title * @param show whether or not the component is showing * @param node componentNode that has this placeholder * @param instanceID the instance ID @@ -116,7 +122,7 @@ public class ComponentPlaceholder { if (node != null && disposed) { // // TODO Hack Alert! (When this is removed, also update ComponentNode) - // + // // This should not happen! We have seen this bug recently Msg.debug(this, "Found disposed component that was not removed from the hierarchy " + "list: " + this, ReflectionUtilities.createJavaFilteredThrowable()); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/DetachedWindowNode.java b/Ghidra/Framework/Docking/src/main/java/docking/DetachedWindowNode.java index 3d655a03a2..e15055d5cd 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/DetachedWindowNode.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/DetachedWindowNode.java @@ -242,13 +242,13 @@ class DetachedWindowNode extends WindowNode { /** * Creates a list of titles from the given component providers and placeholders. The utility - * of this method is that it will group like component providers into one title value + * of this method is that it will group like component providers into one title value * instead of having one value for each placeholder. */ private List generateTitles(List placeholders) { // - // Decompose the given placeholders into a mapping of provider names to placeholders + // Decompose the given placeholders into a mapping of provider names to placeholders // that share that name. This lets us group placeholders that are multiple instances of // the same provider. // @@ -368,7 +368,7 @@ class DetachedWindowNode extends WindowNode { } /** - * Ensures the bounds of this window have a valid location and size + * Ensures the bounds of this window have a valid location and size */ private void adjustBounds() { @@ -474,6 +474,16 @@ class DetachedWindowNode extends WindowNode { @Override void dispose() { + disposeWindow(); + + if (child != null) { + child.parent = null; + child.dispose(); + child = null; + } + } + + private void disposeWindow() { if (dropTargetHandler != null) { dropTargetHandler.dispose(); } @@ -485,12 +495,24 @@ class DetachedWindowNode extends WindowNode { window.dispose(); window = null; } + } + /** + * An oddly named method for an odd use case. This method is meant to take an existing window, + * such as that created by default when loading plugins, and hide it. Clients cannot simply + * call dispose(), as that would also dispose the entire child hierarchy of this class. This + * method is intended to keep all children not disposed while allowing this window node to go + * away. + */ + void disconnect() { + + // note: do not call child.dispose() here if (child != null) { child.parent = null; - child.dispose(); child = null; } + + disposeWindow(); } @Override diff --git a/Ghidra/Framework/Docking/src/main/java/docking/PlaceholderManager.java b/Ghidra/Framework/Docking/src/main/java/docking/PlaceholderManager.java index 93d1a9c8e5..349bb3faa7 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/PlaceholderManager.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/PlaceholderManager.java @@ -43,17 +43,22 @@ class PlaceholderManager { } ComponentPlaceholder replacePlaceholder(ComponentProvider provider, - ComponentPlaceholder oldPlaceholder) { + ComponentPlaceholder defaultPlaceholder) { - ComponentPlaceholder newPlaceholder = createOrRecyclePlaceholder(provider, oldPlaceholder); + // Note: the 'restoredPlaceholder' is from xml; the 'defaultPlaceholder' is that which was + // created by a plugin as it was constructed. If there is no placeholder in the xml, + // then the original 'defaultPlaceholder' will be returned from + // createOrRecyclePlaceholder(). + ComponentPlaceholder restoredPlaceholder = + createOrRecyclePlaceholder(provider, defaultPlaceholder); - moveActions(oldPlaceholder, newPlaceholder); - if (!oldPlaceholder.isHeaderShowing()) { - newPlaceholder.showHeader(false); + moveActions(defaultPlaceholder, restoredPlaceholder); + if (!defaultPlaceholder.isHeaderShowing()) { + restoredPlaceholder.showHeader(false); } - if (oldPlaceholder.isShowing() != newPlaceholder.isShowing()) { - if (newPlaceholder.isShowing()) { + if (defaultPlaceholder.isShowing() != restoredPlaceholder.isShowing()) { + if (restoredPlaceholder.isShowing()) { provider.componentShown(); } else { @@ -61,11 +66,13 @@ class PlaceholderManager { } } - if (newPlaceholder != oldPlaceholder) { - oldPlaceholder.dispose(); - removePlaceholder(oldPlaceholder); + // if we have found a replacement placeholder, then remove the default placeholder + if (restoredPlaceholder != defaultPlaceholder) { + defaultPlaceholder.dispose(); + removePlaceholder(defaultPlaceholder); } - return newPlaceholder; + + return restoredPlaceholder; } /** @@ -240,8 +247,8 @@ class PlaceholderManager { // 1) share the same owner (plugin), and // 2) are in the same group, or related groups // - // If there are not other providers that share the same owner as the one we are given, - // then we will search all providers. This allows different plugins to share + // If there are not other providers that share the same owner as the one we are given, + // then we will search all providers. This allows different plugins to share // window arrangement. // Set buddies = activePlaceholders; @@ -275,7 +282,7 @@ class PlaceholderManager { * provider. * @param activePlaceholders the set of currently showing placeholders. * @param newInfo the placeholder for the new provider to be shown. - * @return an existing matching placeholder or null. + * @return an existing matching placeholder or null. */ private ComponentPlaceholder findBestPlaceholderToStackUpon( Set activePlaceholders, ComponentPlaceholder newInfo) { diff --git a/Ghidra/Framework/Docking/src/main/java/docking/RootNode.java b/Ghidra/Framework/Docking/src/main/java/docking/RootNode.java index 89c4b38967..03a9d5a589 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/RootNode.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/RootNode.java @@ -54,7 +54,7 @@ class RootNode extends WindowNode { * Constructs a new root node for the given DockingWindowsManager. * @param mgr the DockingWindowsManager * @param toolName the name of the tool to be displayed in all the top-level windows. - * @param factory a factory for creating drop targets for this nodes windows; may be null + * @param factory a factory for creating drop targets for this nodes windows; may be null */ RootNode(DockingWindowManager mgr, String toolName, List images, boolean isModal, DropTargetFactory factory) { @@ -445,7 +445,13 @@ class RootNode extends WindowNode { /** * Restores the component hierarchy from the given XML JDOM element. - * @param root the XML from which to restore the state. + *

+ * The process of restoring from xml will create new {@link ComponentPlaceholder}s that will be + * used to replace any existing matching placeholders. This allows the already loaded default + * placeholders to be replaced by the previously saved configuration. + * + * @param rootNodeElement the XML from which to restore the state. + * @return the newly created placeholders */ List restoreFromXML(Element rootNodeElement) { invalid = true; @@ -455,7 +461,7 @@ class RootNode extends WindowNode { detachedWindows.clear(); for (DetachedWindowNode windowNode : copy) { notifyWindowRemoved(windowNode); - windowNode.dispose(); + windowNode.disconnect(); } int x = Integer.parseInt(rootNodeElement.getAttributeValue("X_POS")); @@ -591,7 +597,7 @@ class RootNode extends WindowNode { //================================================================================================== // Inner Classes -//================================================================================================== +//================================================================================================== /** Interface to wrap JDialog and JFrame so that they can be used by one handle */ private interface SwingWindowWrapper {