Address TODOs

Signed-off-by: Ulrich Schöpp <schoepp@fortiss.org>
Issue-Ref: 4181

org.fortiss.tooling.common.ui/src/org/fortiss/tooling/common/ui/javafx/lwfxef/.ratings

 DiagramCoordinate.java 6b00aec99054d4cd19003a72bd4e5e774ac6a641 GREEN
 DiagramLayers.java aa1f95dbae290c8b00202abe4385b01b8f36e5ab GREEN
-DiagramViewer.java 504ae4708be551d72bf4dee5ef9fbfbdbadef18f RED
+DiagramViewer.java f57cb00ccedd1ef37e5ce33f2df04989b81329a4 YELLOW
 DiagramViewerDefaultTags.java 6230763252409c60009ab8887b4ef582cf883229 GREEN
 DiagramViewerFeatures.java 3dd78d9c117fc156924a151c6f8d770c53c103bc GREEN
 DiagramViewerSelection.java e833f592543bc97077907d980a39b123fc4044e6 GREEN
 EDragGesture.java 5cfa098d3877db11981c2750e5e103156d62fc5e GREEN
 FeedbackChange.java b088fa89af648f1674f2f9c1f7f99d585ce801ca GREEN
-GridCanvasVisual.java d8499f9458032ececff706c170d5f747daf20050 RED
+GridCanvasVisual.java e7c83211e0fce2b0d55aac77d0203950286646b9 YELLOW
 MVCBundleManager.java 18667b4ed98da124b7c1bc7a103e95232df9ad49 GREEN
 MouseState.java 3d9993f799d5d74bc74ac03b46e4a1857c4d267e GREEN
 SVGExporter.java cbbd1eceb2910fd5c1693e05c5303a193127b9db GREEN

org.fortiss.tooling.common.ui/src/org/fortiss/tooling/common/ui/javafx/lwfxef/DiagramViewer.java

@@ -13,6 +13,8 @@ import static java.lang.Math.abs;
 import static java.lang.Math.max;
 import static java.lang.Math.min;
 import static java.util.Objects.requireNonNull;
+import static javafx.geometry.Orientation.HORIZONTAL;
+import static javafx.geometry.Orientation.VERTICAL;
 import static javafx.geometry.VPos.TOP;
 import static javafx.scene.layout.GridPane.setHgrow;
 import static javafx.scene.layout.GridPane.setVgrow;
@@ -145,19 +147,18 @@ public class DiagramViewer {
 		// viewer pane
 		viewerPane = new Pane();
 		layers.boundsInLocalProperty().addListener((obs, ov, nv) -> {
-			updateHorizontalScrollbar();
-			updateVerticalScrollbar();
+			updateScrollBars();
 		});
 		layers.getTransforms().add(transform);
 
 		viewerPane.getChildren().add(layers);
 		viewerPane.widthProperty().addListener(evt -> {
 			updateGridCanvasSize();
-			updateHorizontalScrollbar();
+			updateScrollBars();
 		});
 		viewerPane.heightProperty().addListener(evt -> {
 			updateGridCanvasSize();
-			updateVerticalScrollbar();
+			updateScrollBars();
 		});
 		// focus event handling
 		viewerPane.focusedProperty().addListener((obs, ov, nv) -> {
@@ -177,29 +178,11 @@ public class DiagramViewer {
 		setHgrow(viewerPane, ALWAYS);
 		setVgrow(viewerPane, ALWAYS);
 
-		// TODO factorize to method createScrollBar(javafx.geometry.Orientation)
-		// vertical scrollbar
-		verticalScrollbar = new ScrollBar();
-		verticalScrollbar.setOrientation(Orientation.VERTICAL);
+		verticalScrollbar = createContentScrollBar(VERTICAL);
 		scrolledPane.add(verticalScrollbar, 2, 1);
-		verticalScrollbar.valueProperty().addListener((obs, ov, nv) -> {
-			hideContextMenu();
-			// TODO (SB): Use "speaking" variable name
-			Point2D p1 = layers.parentToLocal(0, 0);
-			transform.appendTranslation(0, -nv.doubleValue() + p1.getY());
-			viewerManager.gridCanvasVisual.updateNodes(layers);
-		});
 
-		// TODO Use createScrollbar(javafx.geometry.Orientation)
-		// horizontal scrollbar
-		horizontalScrollbar = new ScrollBar();
+		horizontalScrollbar = createContentScrollBar(HORIZONTAL);
 		scrolledPane.add(horizontalScrollbar, 1, 2);
-		horizontalScrollbar.valueProperty().addListener((obs, ov, nv) -> {
-			hideContextMenu();
-			Point2D p1 = layers.parentToLocal(0, 0);
-			transform.appendTranslation(-nv.doubleValue() + p1.getX(), 0);
-			viewerManager.gridCanvasVisual.updateNodes(layers);
-		});
 
 		viewerPane.getChildren().add(debugLabel);
 		debugLabel.setLayoutX(20);
@@ -235,6 +218,26 @@ public class DiagramViewer {
 		contentDragController.registerEventFilters(viewerPane);
 	}
 
+	/** Constructs a scrollbar controlling the content */
+	private ScrollBar createContentScrollBar(Orientation orientation) {
+		ScrollBar scrollbar = new ScrollBar();
+		scrollbar.setOrientation(orientation);
+		scrollbar.valueProperty().addListener((obs, ov, nv) -> {
+			hideContextMenu();
+			Point2D diagramTranslation = layers.parentToLocal(0, 0);
+			switch(orientation) {
+				case HORIZONTAL:
+					transform.appendTranslation(-nv.doubleValue() + diagramTranslation.getX(), 0);
+					break;
+				case VERTICAL:
+					transform.appendTranslation(0, -nv.doubleValue() + diagramTranslation.getY());
+					break;
+			}
+			viewerManager.gridCanvasVisual.updateNodes(layers);
+		});
+		return scrollbar;
+	}
+
 	/** Returns viewerPane. */
 	/* package */ Pane getViewerPane() {
 		return viewerPane;
@@ -286,55 +289,43 @@ public class DiagramViewer {
 		return exporter.export();
 	}
 
-	// TOOD (SB): Factorize commonalities with updateHorizontalScrollbar()
-	/** Updates the vertical scrollbar. */
-	private void updateVerticalScrollbar() {
+	/** Updates the scrollbars. */
+	private void updateScrollBars() {
 		// The size of the content (in diagram coordinates)
 		Bounds contentBounds = layers.getBoundsInLocal();
 		double contentHeight = contentBounds.getHeight();
+		double contentWidth = contentBounds.getWidth();
 		// The size of the visible content (in diagram coordinates)
 		Bounds visibleBounds = layers.parentToLocal(viewerPane.getLayoutBounds());
+		double visibleWidth = visibleBounds.getWidth();
 		double visibleHeight = visibleBounds.getHeight();
 
 		verticalScrollbar.setMin(0);
+		horizontalScrollbar.setMin(0);
 		verticalScrollbar.setMax(contentHeight);
-
-		Point2D o = layers.parentToLocal(0, 0);
-		verticalScrollbar.setValue(o.getY());
-
-		// Including blank space to the bottom, the height of the
-		// content is (contentHeight + visibleHeight), of which
-		// visibleHeight is visible. Since the scrollbar
-		// goes from 0 to contentHeight, this means we need:
-		// contentHeight / visibleAmount == (contentHeight + visibleHeight) / visibleHeight
-		// TODO (SB): create local variable visibleAmount (and use it later for
-		// verticalScrollbar.setBlockIncrement)
-		verticalScrollbar
-				.setVisibleAmount(contentHeight * visibleHeight / (contentHeight + visibleHeight));
+		horizontalScrollbar.setMax(contentWidth);
+
+		Point2D diagramTopLeft = layers.parentToLocal(0, 0);
+		verticalScrollbar.setValue(diagramTopLeft.getY());
+		horizontalScrollbar.setValue(diagramTopLeft.getX());
+
+		// Including blank space to the bottom, the height of the content
+		// is (contentHeight + visibleHeight), of which visibleHeight is
+		// visible. Since the vertical scrollbar goes from 0 to contentHeight,
+		// this means we need:
+		// contentHeight / verticalVisibleAmount
+		// == (contentHeight + visibleHeight) / visibleHeight
+		// Hence:
+		double verticalVisibleAmount =
+				contentHeight * visibleHeight / (contentHeight + visibleHeight);
+		double horizontalVisibleAmount =
+				contentWidth * visibleWidth / (contentWidth + visibleWidth);
+		verticalScrollbar.setVisibleAmount(verticalVisibleAmount);
+		horizontalScrollbar.setVisibleAmount(horizontalVisibleAmount);
 
 		verticalScrollbar.setUnitIncrement(1);
-		verticalScrollbar.setBlockIncrement(verticalScrollbar.getVisibleAmount());
-	}
-
-	/** Updates the horizontal scrollbar. */
-	private void updateHorizontalScrollbar() {
-		// The size of the content (in diagram coordinates)
-		Bounds contentBounds = layers.getBoundsInLocal();
-		double contentWidth = contentBounds.getWidth();
-		// The size of the visible content (in diagram coordinates)
-		Bounds visibleBounds = layers.parentToLocal(viewerPane.getLayoutBounds());
-		double visibleWidth = visibleBounds.getWidth();
-
-		horizontalScrollbar.setMin(0);
-		horizontalScrollbar.setMax(contentBounds.getWidth());
-
-		Point2D o = layers.parentToLocal(0, 0);
-		horizontalScrollbar.setValue(o.getX());
-
-		horizontalScrollbar
-				.setVisibleAmount(contentWidth * visibleWidth / (contentWidth + visibleWidth));
-
 		horizontalScrollbar.setUnitIncrement(1);
+		verticalScrollbar.setBlockIncrement(verticalScrollbar.getVisibleAmount());
 		horizontalScrollbar.setBlockIncrement(horizontalScrollbar.getVisibleAmount());
 	}
 
@@ -473,8 +464,7 @@ public class DiagramViewer {
 	void appendContentTransform(Transform t) {
 		transform.append(t);
 		enforceBounds();
-		updateHorizontalScrollbar();
-		updateVerticalScrollbar();
+		updateScrollBars();
 		viewerManager.gridCanvasVisual.updateNodes(layers);
 	}
 

org.fortiss.tooling.common.ui/src/org/fortiss/tooling/common/ui/javafx/lwfxef/GridCanvasVisual.java

@@ -10,7 +10,6 @@
 package org.fortiss.tooling.common.ui.javafx.lwfxef;
 
 import static java.lang.Double.doubleToLongBits;
-import static java.lang.Math.abs;
 
 import java.util.Objects;
 
@@ -144,12 +143,10 @@ final class GridCanvasVisual implements IVisual {
 
 	/** Returns the zoom factor from current content transformation. */
 	private double getZoomFactor() {
-		Point2D p0 = viewer.getLayers().sceneToLocal(pane.localToScene(0, 0));
-		Point2D p1 = viewer.getLayers().sceneToLocal(pane.localToScene(1, 1));
-
-		// TODO (SB): Add check against division by zero. (assert() if we are sure that it will not
-		// happen, otherwise maybe throw a RuntimeException)
-		return 1 / abs(p1.getX() - p0.getX());
+		Point2D unitVector = new Point2D(1, 1);
+		Point2D transformedVector =
+				viewer.getLayers().getLocalToParentTransform().deltaTransform(unitVector);
+		return transformedVector.getX();
 	}
 
 	/** Structure to represent the current location of the grid. */
@@ -190,7 +187,7 @@ final class GridCanvasVisual implements IVisual {
 		private double vSpacing;
 
 		/** Zoom factor. */
-		private double zf;
+		private double zoomFactor;
 
 		/** Grid indicator size. */
 		private double indicatorSize;
@@ -208,10 +205,14 @@ final class GridCanvasVisual implements IVisual {
 		@Override
 		public int hashCode() {
 			return Objects.hash(backgroundColor, hSpacing, indicatorColor, indicatorSize,
-					indicatorType, vSpacing, zf);
+					indicatorType, vSpacing, zoomFactor);
 		}
 
-		/** {@inheritDoc} */
+		/**
+		 * Auto-generated (by Eclipse) deep equality method.
+		 * 
+		 * {@inheritDoc}
+		 */
 		@Override
 		public boolean equals(Object obj) {
 			if(this == obj) {
@@ -225,16 +226,16 @@ final class GridCanvasVisual implements IVisual {
 			}
 			GridParameters other = (GridParameters)obj;
 
-			// TODO Add comment to explain the condition. In particular, the use of
-			// doubleToLongBits() is not obvious. Also, splitting the expression into several
-			// variables (with self-explanatory names) might improve understandability
+			// Compare all member variables individually for equality.
+			// The equality of doubles is tested using {@code doubleToLongBits},
+			// as explained in {@link java.lang.Double#equals(Object)}.
 			return Objects.equals(backgroundColor, other.backgroundColor) &&
 					doubleToLongBits(hSpacing) == doubleToLongBits(other.hSpacing) &&
 					Objects.equals(indicatorColor, other.indicatorColor) &&
 					doubleToLongBits(indicatorSize) == doubleToLongBits(other.indicatorSize) &&
 					indicatorType == other.indicatorType &&
 					doubleToLongBits(vSpacing) == doubleToLongBits(other.vSpacing) &&
-					abs(zf - other.zf) < 0.00001;
+					doubleToLongBits(zoomFactor) == doubleToLongBits(other.zoomFactor);
 		}
 	}
 
@@ -245,7 +246,7 @@ final class GridCanvasVisual implements IVisual {
 		GridParameters parameters = new GridParameters();
 		parameters.hSpacing = features.getHorizontalSpacing();
 		parameters.vSpacing = features.getVerticalSpacing();
-		parameters.zf = getZoomFactor();
+		parameters.zoomFactor = getZoomFactor();
 		parameters.indicatorType = features.getIndicatorType();
 		parameters.backgroundColor = features.getBackgroundColor();
 		parameters.indicatorColor = features.getIndicatorColor();
@@ -254,8 +255,8 @@ final class GridCanvasVisual implements IVisual {
 		paintCanvas(parameters);
 
 		GridOffset gridOffset = getGridOffset();
-		double zfhs = parameters.zf * parameters.hSpacing;
-		double zfvs = parameters.zf * parameters.vSpacing;
+		double zfhs = parameters.zoomFactor * parameters.hSpacing;
+		double zfvs = parameters.zoomFactor * parameters.vSpacing;
 		gridCanvas.relocate(-gridOffset.insets.getX() * zfhs, -gridOffset.insets.getY() * zfvs);
 
 		pane.getChildren().removeAll(leftOuterBorder, topOuterBorder);
@@ -280,8 +281,8 @@ final class GridCanvasVisual implements IVisual {
 
 	/** Draw gridCanvas with the given parameters. */
 	private void paintCanvas(GridParameters parameters) {
-		double targetCanvasWidth = width + parameters.zf * parameters.hSpacing;
-		double targetCanvasHeight = height + parameters.zf * parameters.vSpacing;
+		double targetCanvasWidth = width + parameters.zoomFactor * parameters.hSpacing;
+		double targetCanvasHeight = height + parameters.zoomFactor * parameters.vSpacing;
 
 		boolean resized = false;
 		resized |= gridCanvas.getWidth() != targetCanvasWidth;
@@ -313,9 +314,9 @@ final class GridCanvasVisual implements IVisual {
 		// draw grid indicators
 		gc.setStroke(parameters.indicatorColor);
 		gc.setFill(parameters.indicatorColor);
-		double zfhs = parameters.zf * parameters.hSpacing;
-		double zfvs = parameters.zf * parameters.vSpacing;
-		double isize = parameters.indicatorSize * parameters.zf;
+		double zfhs = parameters.zoomFactor * parameters.hSpacing;
+		double zfvs = parameters.zoomFactor * parameters.vSpacing;
+		double isize = parameters.indicatorSize * parameters.zoomFactor;
 
 		for(double x = zfhs; x < gridCanvas.getWidth(); x += zfhs) {
 			for(double y = zfvs; y < gridCanvas.getHeight(); y += zfvs) {