GP-768 - Function Graph - condense before edge routing

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/clipboard/CodeBrowserClipboardProvider.java

@@ -24,6 +24,8 @@ import java.util.concurrent.CopyOnWriteArraySet;
 import javax.swing.event.ChangeEvent;
 import javax.swing.event.ChangeListener;
 
+import org.apache.commons.lang3.StringUtils;
+
 import docking.ActionContext;
 import docking.ComponentProvider;
 import docking.dnd.GenericDataFlavor;
@@ -303,12 +305,9 @@ public class CodeBrowserClipboardProvider extends ByteCopier
 	private Transferable copyAddress() {
 
 		AddressSetView addressSet = getSelectedAddresses();
-		StringBuilder buffy = new StringBuilder();
 		AddressIterator it = addressSet.getAddresses(true);
-		while (it.hasNext()) {
-			buffy.append(it.next()).append('\n');
-		}
-		return createStringTransferable(buffy.toString());
+		String joined = StringUtils.join((Iterator<Address>) it, "\n");
+		return createStringTransferable(joined);
 	}
 
 	protected Transferable copyCode(TaskMonitor monitor) {
@@ -377,8 +376,8 @@ public class CodeBrowserClipboardProvider extends ByteCopier
 	private boolean pasteLabelsComments(Transferable pasteData, boolean pasteLabels,
 			boolean pasteComments) {
 		try {
-			List<?> list = (List<?>) pasteData.getTransferData(
-				CodeUnitInfoTransferable.localDataTypeFlavor);
+			List<?> list =
+				(List<?>) pasteData.getTransferData(CodeUnitInfoTransferable.localDataTypeFlavor);
 			List<CodeUnitInfo> infos = CollectionUtils.asList(list, CodeUnitInfo.class);
 			Command cmd = new CodeUnitInfoPasteCmd(currentLocation.getAddress(), infos, pasteLabels,
 				pasteComments);
@@ -451,8 +450,8 @@ public class CodeBrowserClipboardProvider extends ByteCopier
 			String oldName = symbol.getName();
 			Namespace namespace = symbol.getParentNamespace();
 			Address symbolAddress = symbol.getAddress();
-			RenameLabelCmd cmd = new RenameLabelCmd(symbolAddress, oldName, labelName,
-				namespace, SourceType.USER_DEFINED);
+			RenameLabelCmd cmd = new RenameLabelCmd(symbolAddress, oldName, labelName, namespace,
+				SourceType.USER_DEFINED);
 			return tool.execute(cmd, currentProgram);
 		}
 

Ghidra/Features/FunctionGraphDecompilerExtension/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/DecompilerNestedLayout.java

@@ -38,6 +38,7 @@ import ghidra.app.plugin.core.functiongraph.graph.jung.renderer.DNLArticulatedEd
 import ghidra.app.plugin.core.functiongraph.graph.vertex.FGVertex;
 import ghidra.app.plugin.core.functiongraph.graph.vertex.GroupedFunctionGraphVertex;
 import ghidra.graph.VisualGraph;
+import ghidra.graph.viewer.GraphViewerUtils;
 import ghidra.graph.viewer.layout.*;
 import ghidra.graph.viewer.vertex.VisualGraphVertexShapeTransformer;
 import ghidra.program.model.address.Address;
@@ -262,11 +263,8 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 				DecompilerBlock loop = block.getParentLoop();
 
 				if (loop != null) {
-					Set<FGVertex> vertices = loop.getVertices();
-
-					Column outermostCol = getOutermostCol(layoutToGridMap, vertices);
-					Column loopEndColumn = layoutToGridMap.nextColumn(outermostCol);
-					List<Point2D> articulations = routeLoopEdge(start, end, loopEndColumn);
+					List<Point2D> articulations =
+						routeUpwardLoop(layoutToGridMap, vertex2dFactory, start, end, loop);
 					newEdgeArticulations.put(e, articulations);
 					continue;
 				}
@@ -331,6 +329,70 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		return newEdgeArticulations;
 	}
 
+	private List<Point2D> routeUpwardLoop(LayoutLocationMap<FGVertex, FGEdge> layoutToGridMap,
+			Vertex2dFactory vertex2dFactory, Vertex2d start, Vertex2d end, DecompilerBlock loop) {
+		Set<FGVertex> loopVertices = loop.getVertices();
+		FGVertex rightmostLoopVertex =
+			getRightmostVertex(layoutToGridMap, vertex2dFactory, loopVertices);
+
+		int startRow = start.rowIndex;
+		int endRow = end.rowIndex;
+		int startColumn = Math.min(start.columnIndex, end.columnIndex);
+		int endColumn = Math.max(start.columnIndex, end.columnIndex);
+
+		Column rightmostLoopColumn = layoutToGridMap.col(rightmostLoopVertex);
+		endColumn = Math.max(endColumn, rightmostLoopColumn.index);
+
+		// Look for any vertices that are no part of the loop, but are placed inside
+		// of the loop bounds.  This can happen in a graph when the decompiler uses
+		// goto statements.   Use the loop's rightmost vertex to establish the loops
+		// right edge and then use that to check for any stray non-loop vertices.
+		List<Vertex2d> interlopers =
+			getVerticesInBounds(vertex2dFactory, startRow, endRow, startColumn, endColumn);
+
+		// place the right x position to the right of the rightmost vertex, not
+		// extending past the next column
+		FGVertex rightmostVertex = getRightmostVertex(interlopers);
+		Column rightmostColumn = layoutToGridMap.col(rightmostVertex);
+		Column nextColumn = layoutToGridMap.nextColumn(rightmostColumn);
+		Vertex2d rightmostV2d = vertex2dFactory.get(rightmostVertex);
+
+		// the padding used for these two lines is somewhat arbitrary and may be changed
+		double rightSide = rightmostV2d.getRight() + GraphViewerUtils.EXTRA_LAYOUT_COLUMN_SPACING;
+		double x = Math.min(rightSide,
+			nextColumn.x - GraphViewerUtils.EXTRA_LAYOUT_COLUMN_SPACING_CONDENSED);
+
+		List<Point2D> articulations = routeLoopEdge(start, end, x);
+		return articulations;
+	}
+
+	private List<Vertex2d> getVerticesInBounds(Vertex2dFactory vertex2dFactory, int startRow,
+			int endRow, int startColumn, int endColumn) {
+
+		if (startRow > endRow) { // going upwards
+			int temp = endRow;
+			endRow = startRow;
+			startRow = temp;
+		}
+
+		List<Vertex2d> toCheck = new LinkedList<>();
+		for (int row = startRow; row < endRow + 1; row++) {
+
+			for (int col = startColumn; col < endColumn + 1; col++) {
+
+				// assume any other vertex in our column can clip (it will not clip when
+				// the 'spacing' above pushes the edge away from this column, like for
+				// large row delta values)
+				Vertex2d otherVertex = vertex2dFactory.get(row, col);
+				if (otherVertex != null) {
+					toCheck.add(otherVertex);
+				}
+			}
+		}
+
+		return toCheck;
+	}
+
 	private void routeToTheRightGoingUpwards(Vertex2d start, Vertex2d end,
 			Vertex2dFactory vertex2dFactory, List<Point2D> articulations) {
 
@@ -347,11 +409,9 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		}
 		int distanceSpacing = delta * multiplier;
 
-		// Condensing Note: we have guilty knowledge that our parent class my condense the 
-		// vertices and edges towards the center of the graph after we calculate positions.
-		// To prevent the edges from moving to far behind the vertices, we will compensate a
-		// bit for that effect using this offset value.   The getEdgeOffset() method is 
-		// updated for the condense factor.
+		// Condensing is when the graph will pull nodes closer together on the x axis to
+		// reduce whitespace and make the entire graph easier to see.   In this case, update
+		// the offset to avoid running into the moved vertices.
 		int exaggerationFactor = 1;
 		if (isCondensedLayout()) {
 			exaggerationFactor = 2; // determined by trial-and-error; can be made into an option
@@ -556,19 +616,6 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 	private void routeAroundColumnVertices(Vertex2d start, Vertex2d end,
 			Vertex2dFactory vertex2dFactory, List<Point2D> articulations, double edgeX) {
 
-		Column column = vertex2dFactory.getColumn(edgeX);
-		int columnIndex = 0;
-		if (column != null) {
-			// a null column happens with a negative x value that is outside of any column
-			columnIndex = column.index;
-		}
-
-		routeAroundColumnVertices(start, end, columnIndex, vertex2dFactory, articulations, edgeX);
-	}
-
-	private void routeAroundColumnVertices(Vertex2d start, Vertex2d end, int column,
-			Vertex2dFactory vertex2dFactory, List<Point2D> articulations, double edgeX) {
-
 		if (useSimpleRouting()) {
 			return;
 		}
@@ -582,14 +629,34 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			startRow = end.rowIndex;
 		}
 
+		int startColumn = Math.min(start.columnIndex, end.columnIndex);
+		int endColumn = Math.max(start.columnIndex, end.columnIndex);
+		if (goingDown) {
+			endRow -= 1;
+			endColumn -= 1;
+
+			if (start.columnIndex <= end.columnIndex) {
+				startRow += 1;
+			}
+		}
+		else {
+			// going up we swing out to the right; grab the column that is out to the right
+			Column rightColumn = vertex2dFactory.getColumn(edgeX);
+			endColumn = rightColumn.index;
+		}
+
 		List<Vertex2d> toCheck = new LinkedList<>();
-		for (int row = startRow + 1; row < endRow; row++) {
-			// assume any other vertex in our column can clip (it will not clip when
-			// the 'spacing' above pushes the edge away from this column, like for
-			// large row delta values)
-			Vertex2d otherVertex = vertex2dFactory.get(row, column);
-			if (otherVertex != null) {
-				toCheck.add(otherVertex);
+		for (int row = startRow; row < endRow + 1; row++) {
+
+			for (int col = startColumn; col < endColumn + 1; col++) {
+
+				// assume any other vertex in our column can clip (it will not clip when
+				// the 'spacing' above pushes the edge away from this column, like for
+				// large row delta values)
+				Vertex2d otherVertex = vertex2dFactory.get(row, col);
+				if (otherVertex != null) {
+					toCheck.add(otherVertex);
+				}
 			}
 		}
 
@@ -605,11 +672,9 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			int padding = VERTEX_TO_EDGE_AVOIDANCE_PADDING;
 			int distanceSpacing = padding + delta; // adding the delta makes overlap less likely
 
-			// Condensing Note: we have guilty knowledge that our parent class my condense the 
-			// vertices and edges towards the center of the graph after we calculate positions.
-			// To prevent the edges from moving to far behind the vertices, we will compensate a
-			// bit for that effect using this offset value.   The getEdgeOffset() method is 
-			// updated for the condense factor.
+			// Condensing is when the graph will pull nodes closer together on the x axis to
+			// reduce whitespace and make the entire graph easier to see.   In this case, update
+			// the offset to avoid running into the moved vertices.
 			int vertexToEdgeOffset = otherVertex.getEdgeOffset();
 			int exaggerationFactor = 1;
 			if (isCondensedLayout()) {
@@ -696,15 +761,11 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		return !getLayoutOptions().useEdgeRoutingAroundVertices();
 	}
 
-	private List<Point2D> routeLoopEdge(Vertex2d start, Vertex2d end, Column loopEndColumn) {
+	private List<Point2D> routeLoopEdge(Vertex2d start, Vertex2d end, double x) {
 
 		// going backwards
 		List<Point2D> articulations = new ArrayList<>();
 
-		// loop first point - same y coord as the vertex; x is the middle of the next col
-		int halfWidth = loopEndColumn.getPaddedWidth(isCondensedLayout()) >> 1;
-		double x = loopEndColumn.x + halfWidth; // middle of the column
-
 		int startRow = start.rowIndex;
 		int endRow = end.rowIndex;
 		if (startRow > endRow) { // going upwards			
@@ -739,21 +800,37 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		e.setDefaultAlpha(.25);
 	}
 
-	private Column getOutermostCol(LayoutLocationMap<FGVertex, FGEdge> layoutLocations,
-			Set<FGVertex> vertices) {
+	private FGVertex getRightmostVertex(LayoutLocationMap<FGVertex, FGEdge> layoutLocations,
+			Vertex2dFactory vertex2dFactory, Set<FGVertex> vertices) {
 
-		Column outermost = null;
+		List<Vertex2d> points = new ArrayList<>();
 		for (FGVertex v : vertices) {
-			Column col = layoutLocations.col(v);
-			if (outermost == null) {
-				outermost = col;
+			Vertex2d v2d = vertex2dFactory.get(v);
+			points.add(v2d);
+		}
+
+		FGVertex v = getRightmostVertex(points);
+		return v;
+	}
+
+	private FGVertex getRightmostVertex(Collection<Vertex2d> points) {
+
+		Vertex2d rightmost = null;
+		for (Vertex2d v2d : points) {
+			if (rightmost == null) {
+				rightmost = v2d;
 			}
-			else if (col.x > outermost.x) {
-				outermost = col;
+			else {
+				// the rightmost is that which extends furthest to the right
+				double current = rightmost.getRight();
+				double other = v2d.getRight();
+				if (other > current) {
+					rightmost = v2d;
+				}
 			}
 		}
 
-		return outermost;
+		return rightmost.v;
 	}
 
 	@Override
@@ -840,8 +917,11 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			BlockCopy copy = (BlockCopy) child;
 
 			StringBuilder buffy = new StringBuilder();
-			buffy.append(printDepth(depth, depth + 1)).append(' ').append(ID).append(
-				" plain - ").append(copy.getRef());
+			buffy.append(printDepth(depth, depth + 1))
+					.append(' ')
+					.append(ID)
+					.append(" plain - ")
+					.append(copy.getRef());
 
 			debug(buffy.toString());
 		}
@@ -1207,8 +1287,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 
 			int row = startRow;
 
-			for (int i = 0; i < allChildren.size(); i++) {
-				DecompilerBlock block = allChildren.get(i);
+			for (DecompilerBlock block : allChildren) {
 				if (block instanceof DecompilerBlockGraph) {
 					row = ((DecompilerBlockGraph) block).setRows(row);
 				}
@@ -1229,8 +1308,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		String getChildrenString(int depth) {
 			StringBuilder buffy = new StringBuilder();
 			int childCount = 0;
-			for (int i = 0; i < allChildren.size(); i++) {
-				DecompilerBlock block = allChildren.get(i);
+			for (DecompilerBlock block : allChildren) {
 				if (block instanceof DecompilerBlockGraph) {
 
 					String blockName = block.getName();
@@ -1447,9 +1525,8 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			// The 'list' structure for children's nesting:
 			// -all nodes are at the same level
 			//
-			for (int i = 0; i < allChildren.size(); i++) {
+			for (DecompilerBlock block : allChildren) {
 				int column = col;
-				DecompilerBlock block = allChildren.get(i);
 				block.setCol(column);
 			}
 
@@ -1477,8 +1554,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			// -each successive condition is another level nested
 			//
 			int column = col;
-			for (int i = 0; i < allChildren.size(); i++) {
-				DecompilerBlock block = allChildren.get(i);
+			for (DecompilerBlock block : allChildren) {
 				block.setCol(column);
 				column++;
 			}
@@ -1518,8 +1594,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 			// -all blocks nested
 			//
 			int column = col + 1;
-			for (int i = 0; i < allChildren.size(); i++) {
-				DecompilerBlock block = allChildren.get(i);
+			for (DecompilerBlock block : allChildren) {
 				block.setCol(column);
 			}
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/layout/AbstractVisualGraphLayout.java

@@ -313,14 +313,7 @@ public abstract class AbstractVisualGraphLayout<V extends VisualVertex,
 		Map<V, Point2D> vertexLayoutLocations =
 			positionVerticesInLayoutSpace(transformer, vertices, layoutLocations);
 
-		Map<E, List<Point2D>> edgeLayoutArticulationLocations =
-			positionEdgeArticulationsInLayoutSpace(transformer, vertexLayoutLocations, edges,
-				layoutLocations);
-
-		// DEGUG triggers grid lines to be printed; useful for debugging
-//		VisualGraphRenderer.DEBUG_ROW_COL_MAP.put((Graph<?, ?>) visualGraph,
-//			layoutLocations.copy());
-
+		Map<E, List<Point2D>> edgeLayoutArticulationLocations = new HashMap<>();
 		Rectangle graphBounds =
 			getTotalGraphSize(vertexLayoutLocations, edgeLayoutArticulationLocations, transformer);
 		double centerX = graphBounds.getCenterX();
@@ -332,6 +325,12 @@ public abstract class AbstractVisualGraphLayout<V extends VisualVertex,
 				centerX, centerY);
 		}
 
+		edgeLayoutArticulationLocations = positionEdgeArticulationsInLayoutSpace(transformer,
+			vertexLayoutLocations, edges, layoutLocations);
+
+		// DEGUG triggers grid lines to be printed; useful for debugging
+		// VisualGraphRenderer.DEBUG_ROW_COL_MAP.put(this, layoutLocations.copy());
+
 		layoutLocations.dispose();
 		gridLocations.dispose();
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/renderer/VisualGraphRenderer.java

@@ -22,8 +22,8 @@ import java.util.*;
 import com.google.common.base.Function;
 
 import edu.uci.ics.jung.algorithms.layout.Layout;
-import edu.uci.ics.jung.graph.Graph;
 import edu.uci.ics.jung.visualization.*;
+import edu.uci.ics.jung.visualization.layout.ObservableCachingLayout;
 import edu.uci.ics.jung.visualization.renderers.Renderer;
 import edu.uci.ics.jung.visualization.transform.shape.GraphicsDecorator;
 import ghidra.graph.viewer.*;
@@ -44,7 +44,8 @@ public class VisualGraphRenderer<V extends VisualVertex, E extends VisualEdge<V>
 	/**
 	 * Used for displaying grid information for graph layouts
 	 */
-	public static Map<Graph<?, ?>, LayoutLocationMap<?, ?>> DEBUG_ROW_COL_MAP = new HashMap<>();
+	public static Map<VisualGraphLayout<?, ?>, LayoutLocationMap<?, ?>> DEBUG_ROW_COL_MAP =
+		new HashMap<>();
 
 	private Renderer.EdgeLabel<V, E> edgeLabelRenderer = new BasicEdgeLabelRenderer<>();
 
@@ -122,11 +123,15 @@ public class VisualGraphRenderer<V extends VisualVertex, E extends VisualEdge<V>
 		edgeLabelRenderer.labelEdge(rc, layout, e, xform.apply(e));
 	}
 
+	@SuppressWarnings({ "unchecked", "rawtypes" }) // the types in the cast matter not
 	private void paintLayoutGridCells(RenderContext<V, E> renderContext, Layout<V, E> layout) {
 
 		// to enable this debug, search java files for commented-out uses of 'DEBUG_ROW_COL_MAP'
-		Graph<V, E> graph = layout.getGraph();
-		LayoutLocationMap<?, ?> locationMap = DEBUG_ROW_COL_MAP.get(graph);
+		Layout<V, E> key = layout;
+		if (layout instanceof ObservableCachingLayout) {
+			key = ((ObservableCachingLayout) layout).getDelegate();
+		}
+		LayoutLocationMap<?, ?> locationMap = DEBUG_ROW_COL_MAP.get(key);
 		if (locationMap == null) {
 			return;
 		}