ogallagher/fxgraph

A JavaFX graph editor

fxgraph's Issues

Graph node z order is inconsistent

Depending on the order that they’re added to the graph, edges can sometimes display in front of cells instead of always being behind them (see demo video).

This could be fixed by specializing the cartesian graph and its canvas to group cells and edges separately, or by modifying those base classes directly to do so. I plan to do the latter, updating Graph and PannableCanvas to separate edges and cells into distinct groups, with a customizable z order.

BasicGraphDemo zoom scroll not centered on cursor

The zoom scrolling doesn’t quite change the scale relative to the cursor location; it looks like something should be changed here:

fxgraph/src/main/java/com/fxgraph/graph/ViewportGestures.java

Lines 102 to 143 in 4643e37

	private final EventHandler<ScrollEvent> onScrollEventHandler = new EventHandler<ScrollEvent>() {
	@Override
	public void handle(ScrollEvent event) {
	double scale = canvas.getScale(); // currently we only use Y, same value is used for X
	final double oldScale = scale;
	if(event.getDeltaY() < 0) {
	scale /= getZoomSpeed();
	} else if (event.getDeltaY() > 0) {
	scale *= getZoomSpeed();
	}
	scale = clamp(scale, minScaleProperty.get(), maxScaleProperty.get());
	final double f = (scale / oldScale) - 1;
	// maxX = right overhang, maxY = lower overhang
	final double maxX = canvas.getBoundsInParent().getMaxX() - canvas.localToParent(canvas.getPrefWidth(), canvas.getPrefHeight()).getX();
	final double maxY = canvas.getBoundsInParent().getMaxY() - canvas.localToParent(canvas.getPrefWidth(), canvas.getPrefHeight()).getY();
	// minX = left overhang, minY = upper overhang
	final double minX = canvas.localToParent(0, 0).getX() - canvas.getBoundsInParent().getMinX();
	final double minY = canvas.localToParent(0, 0).getY() - canvas.getBoundsInParent().getMinY();
	// adding the overhangs together, as we only consider the width of canvas itself
	final double subX = maxX + minX;
	final double subY = maxY + minY;
	// subtracting the overall overhang from the width and only the left and upper overhang from the upper left point
	final double dx = (event.getSceneX() - ((canvas.getBoundsInParent().getWidth() - subX) / 2 + (canvas.getBoundsInParent().getMinX() + minX)));
	final double dy = (event.getSceneY() - ((canvas.getBoundsInParent().getHeight() - subY) / 2 + (canvas.getBoundsInParent().getMinY() + minY)));
	canvas.setScale(scale);
	// note: pivot value must be untransformed, i. e. without scaling
	canvas.setPivot(f * dx, f * dy);
	event.consume();
	}
	};

CartesianPoint node gesture drag translation is inconsistent

I can be difficult to get the node dragging to work with cartesian point cells. See demo video.

Redundant CorneredEdge, DoubleCorneredEdge, CorneredLoopEdge methods

The CorneredEdge.EdgeGraphic class defines getGraphic and getText methods that were already defined the same way in the abstract superclass AbstractEdgeGraphic. The other cornered edge graphic classes (double, loop) also have similar methods.

Rename Graph.beginUpdate, endUpdate

See Col-E#3 (comment). RenameGraph.beginUpdate and Graph.endUpdate to more clearly reflect these methods’ purposes.

Scale cartesian graph points to fit initial viewport

In a cartesian graph, between the data coordinates for a point (raw x,y) and the viewport coordinates (camera pan, zoom), there should be a transform for display coordinates, given the desired viewport space versus the data space. For example, this transform could optionally flip the y axis and scale the data to fit in the initial viewport (pan=0,0 zoom=1) when displayed.

Use the FitToContentLayout class for this, and make a difference in the placement of a point between the point’s coordinates and the displayed location (CartesianPoint.getGraphic).

Create CartesianAxis class

Create a specialized CartesianAxis with the following features:

• Remains static in the graph canvas viewport (always visible at the edge)
• Is displayed at one of the four viewport edges, representing the x or y axis
• Displays optional tick marks
• Has an optional axis label
• Tick mark locations are dynamic according to data coordinates

Add hover node gesture

Branch: nodegesture-hover

In addition to having the drag translation node gesture (and drag handle resize cell gesture), a hover node gesture could be used to make graph nodes (cells or messages) hoverable.

One application for such a gesture would be sequence diagram message edge details, as suggested at sirolf2009/fxgraph #10.

Another would be for Cartesian plot points, which could show x,y coordinates on hover.

Create classes for a Cartesian graph

I originally found this graphing library looking to use it for Cartesian graphs/plots and not for making a a diagramming editor, but it has enough functionality to have my purposes in mind as well.

In order to follow the current direction of the project, I'll incorporate optional interaction (editing) features as I go.

Below are classes to be created to enable basic Cartesian graphs:

• CartesianGraph A graph that plots points in three major display modes: points (disconnected/scattered points), lines (connected lines), or connected points. Other display types could be added later as well (ex. bar heights), perhaps in subclasses. This graph has axes (directed perpendicular edges) with potential unit marks/ticks. When scroll panning the graph viewport, the axes should remain, but their ticks change to reflect the current viewport position.
• FitToContentLayout Modifies the viewport of the graph so that all points are visible, and according to the placement of the graph axes.
• MultiplotModel Organizes cells and edges into plots, such that each cell represents a point in an ordered sequence by x-value, and each edge connects adjacent cells in a plot.
• CartesianPoint A plot point with x,y coordinates, a text to show its coordinates, and a bullet (default is small circle). Ideally I would want points to not be hierarchical as they exist in sequence rather than a tree, but I could probably use the child-parent members to reference leading and following points, where the "tree level" of a point is determined with its x coordinate.
• SimpleEdge A line between cells, extends AbstractEdge. It will have no text, be a straight diagonal, and still have optional direction.
• CartesianAxis An x or y axis at along the edge of the viewport, with a text and optional unit marks.
• CartesianGraphDemo A test driver app that creates and updates a cartesian graph.

MessageEdge.EdgeGraphic defines redundant text member

fxgraph/src/main/java/com/fxgraph/edges/AbstractEdgeGraphic.java

Lines 16 to 18 in ec8beb4

	public abstract class AbstractEdgeGraphic extends Pane {
	protected final Group group = new Group();
	protected final Text text = new Text();

fxgraph/src/main/java/com/fxgraph/edges/MessageEdge.java

Lines 52 to 62 in ec8beb4

	Text text = new Text(edge.name);
	text.getStyleClass().add("edge-text");
	text.xProperty().bind(sourceX.add(targetX).divide(2).subtract(textWidth.divide(2)));
	text.yProperty().bind(sourceY.add(targetY).divide(2).subtract(textHeight.divide(2)));
	final Runnable recalculateWidth = () -> {
	textWidth.set(text.getLayoutBounds().getWidth());
	textHeight.set(text.getLayoutBounds().getHeight());
	};
	text.parentProperty().addListener((obs, oldVal, newVal) -> recalculateWidth.run());
	text.textProperty().addListener((obs, oldVal, newVal) -> recalculateWidth.run());
	group.getChildren().add(text);

Mouse button customization for node translate vs viewport translate not finished

As discussed in sirolf2009/fxgraph issue #7, the mouse buttons to use for node translation vs the button to use for viewport translation (panning) should be customizable.

However, the implementation is not yet complete, as setting both node translation and viewport pan to the same button causes behavior like so:

fxgraph_translation_button_collide.mp4

AbstractEdgeGraphic widgets could be added to pane children automatically

An com.fxgraph.edges.AbstractEdgeGraphic constructor with some arguments could define which of the default widget components in the edge graphic (group, text, arrow) are automatically added to the Pane.getChildren() superclass member, without every subclass having to redundantly implement this detail.

fxgraph/src/main/java/com/fxgraph/edges/AbstractEdgeGraphic.java

Lines 17 to 19 in ec8beb4

	protected final Group group = new Group();
	protected final Text text = new Text();
	protected final Arrow arrow = new Arrow();

fxgraph/src/main/java/com/fxgraph/edges/Edge.java

Line 75 in ec8beb4

getChildren().add(group);

fxgraph/src/main/java/com/fxgraph/edges/MessageEdge.java

Line 63 in ec8beb4

getChildren().add(group);

fxgraph/src/main/java/com/fxgraph/edges/CorneredEdge.java

Line 110 in ec8beb4

getChildren().add(group);

Return ReadOnlyDoubleProperty from property getters

Given there are also methods to set and get the values of these properties, and the properties themselves are private members, I think it makes sense to wrap these properties in read-only classes when returning them from getter methods.

Examples:

• PannableCanvas.scaleProperty
• Arrow.startXProperty
• Edge.textProperty

In cases where the property should be write-accessible, then the property scope could be public instead of private, though I suppose that does open the member up to reassignment and loss of bindings.

Create constants for edge, cell, arrow implementations

Currently the defaults for arrow length and width on a directed edge are hard-coded into one of the constructors:

fxgraph/src/main/java/com/fxgraph/graph/Arrow.java

Lines 19 to 23 in ec8beb4

	public Arrow(Line line, Line arrow1, Line arrow2) {
	this(line, arrow1, arrow2, 10, 5);
	}
	public Arrow(Line line, Line arrow1, Line arrow2, double arrowLength, double arrowWidth) {

It could be better to create constants in the Arrow class to hold these values instead, so they can be referenced and documented easily.

SequenceDiagram.layout functionally overrides Graph.layout

The way that SequenceDiagram extends Graph overrides Graph.layout implicitly by using a slightly different method signature is confusing, so I propose making the override explicit by having the layout parameter be optional.

Source code documentation with javadoc comments

There isn’t much documentation within the source code, so as I sift through how everything works I’ll be adding javadoc comments. Below is a file list to keep track of progress:

• main/
  • com.fxgraph.cells
    • AbstractCell
    • CellGestures
    • RectangleCell
    • TriangleCell
  • com.fxgraph.edges
    • AbstractEdge
    • AbstractEdgeGraphic
    • CorneredEdge
    • CorneredLoopEdge
    • DoubleCorneredEdge
    • Edge
    • MessageEdge
  • com.fxgraph.graph
    • Arrow
    • Graph
    • ICell
    • IEdge
    • IGraphNode
    • Model
    • NodeGestures
    • PannableCanvas
    • SequenceDiagram
    • ViewportGestures
  • com.fxgraph.layout
    • AbegoTreeLayout
    • Layout
    • RandomLayout
• test/
  • com.fxgraph
    • BasicGraphDemo
    • SequenceDiagramDemo

CellGestures resize via drag handles breaks at viewport zoom != 1

Make cell gestures togglable

The Graph class handles enabling and disabling viewport and node gestures in its constructor, but not cell gestures.

fxgraph/src/main/java/com/fxgraph/graph/Graph.java

Lines 65 to 97 in 80ef7dc

	public Graph(Model model) {
	this.model = model;
	nodeGestures = new NodeGestures(this);
	useNodeGestures = new SimpleBooleanProperty(true);
	useNodeGestures.addListener((obs, oldVal, newVal) -> {
	if (newVal) {
	model.getAllCells().forEach(cell -> nodeGestures.makeDraggable(getGraphic(cell)));
	} else {
	model.getAllCells().forEach(cell -> nodeGestures.makeUndraggable(getGraphic(cell)));
	}
	});
	pannableCanvas = new PannableCanvas();
	viewportGestures = new ViewportGestures(this);
	useViewportGestures = new SimpleBooleanProperty(true);
	useViewportGestures.addListener((obs, oldVal, newVal) -> {
	final Parent parent = pannableCanvas.parentProperty().get();
	if (parent == null) {
	return;
	}
	if (newVal) {
	parent.addEventHandler(MouseEvent.MOUSE_PRESSED, viewportGestures.getOnMousePressedEventHandler());
	parent.addEventHandler(MouseEvent.MOUSE_DRAGGED, viewportGestures.getOnMouseDraggedEventHandler());
	parent.addEventHandler(MouseEvent.MOUSE_RELEASED, viewportGestures.getOnMouseReleasedEventHandler());
	parent.addEventHandler(ScrollEvent.ANY, viewportGestures.getOnScrollEventHandler());
	} else {
	parent.removeEventHandler(MouseEvent.MOUSE_PRESSED, viewportGestures.getOnMousePressedEventHandler());
	parent.removeEventHandler(MouseEvent.MOUSE_DRAGGED, viewportGestures.getOnMouseDraggedEventHandler());
	parent.removeEventHandler(MouseEvent.MOUSE_RELEASED, viewportGestures.getOnMouseReleasedEventHandler());
	parent.removeEventHandler(ScrollEvent.ANY, viewportGestures.getOnScrollEventHandler());
	}
	});

Move ICell, IEdge interfaces to appropriate sub packages

Currently, ICell and IEdge are both defined in the com.fxgraph.graph package, but should probably be in the com.fxgraph.cells and com.fxgraph.edges packages, respectively.

Enable arrow key panning

Incorporate (try merge?) what was changed for arrow key panning here.

Allow ICell to remove parent

The fxgraph.graph.ICell interface can add and remove child cells, but only add parents. I think removal of parents makes sense as well, if they can be added.

CartesianPoint label

Add a coordinate label to the CartesianPoint class, which will display or hide using a hover gesture.