From fafe8bba9cb3f70c4d47f6a8cc875234ceccbc92 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Mon, 27 Apr 2026 11:19:53 +0200 Subject: [PATCH 1/9] Create ci.yml --- .github/workflows/ci.yml | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 000000000..725c4b27a --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,29 @@ +name: CI + +on: + push: + branches: + - main + - master + pull_request: + branches: + - '**' + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up JDK 8 + uses: actions/setup-java@v4 + with: + java-version: '8' + distribution: 'temurin' + + - name: Build and test with Maven + run: mvn clean install --no-transfer-progress -s .maven-settings.xml + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} From e6dbd7a2ea0e456f67e4b1452e5fe6cce1a91259 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Mon, 27 Apr 2026 11:20:24 +0200 Subject: [PATCH 2/9] Add Maven settings and test dependencies Add a CI Maven settings file (.maven-settings.xml) that provides a GitHub server entry using ${env.GITHUB_ACTOR} and ${env.GITHUB_TOKEN}. Update jhotdraw-core/pom.xml to include several test dependencies (junit 4.13.2, mockito-core 3.12.4, jgiven-junit 1.3.1, assertj-core 3.24.2, assertj-swing-junit 3.17.1) and configure the maven-surefire-plugin (3.2.5) with --add-opens JVM args for java.lang and java.lang.reflect to allow reflective access during tests. --- .maven-settings.xml | 12 ++++++++++++ jhotdraw-core/pom.xml | 45 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 .maven-settings.xml diff --git a/.maven-settings.xml b/.maven-settings.xml new file mode 100644 index 000000000..b4927f1ec --- /dev/null +++ b/.maven-settings.xml @@ -0,0 +1,12 @@ + + + + github + ${env.GITHUB_ACTOR} + ${env.GITHUB_TOKEN} + + + diff --git a/jhotdraw-core/pom.xml b/jhotdraw-core/pom.xml index 7c276da85..4bb7a1893 100644 --- a/jhotdraw-core/pom.xml +++ b/jhotdraw-core/pom.xml @@ -35,10 +35,55 @@ 6.8.21 test + + junit + junit + 4.13.2 + test + + + org.mockito + mockito-core + 3.12.4 + test + + + com.tngtech.jgiven + jgiven-junit + 1.3.1 + test + + + org.assertj + assertj-core + 3.24.2 + test + + + org.assertj + assertj-swing-junit + 3.17.1 + test + ${project.groupId} jhotdraw-actions ${project.version} + + + + org.apache.maven.plugins + maven-surefire-plugin + 3.2.5 + + + --add-opens java.base/java.lang=ALL-UNNAMED + --add-opens java.base/java.lang.reflect=ALL-UNNAMED + + + + + \ No newline at end of file From 8243c18851c6bf89f20703434ed486eae7d6ce49 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Mon, 27 Apr 2026 11:20:42 +0200 Subject: [PATCH 3/9] Refactor SelectionTool: simplify tracker logic Extract mouse-press logic into helper methods (findTargetFigure, resolveTracker) and add isViewActive() for concise view checks. Consolidate tracker activation/deactivation into setTracker (with null guard) and update toolDone to use it. Replace repetitive getView()/isEnabled() checks with isViewActive and preserve existing select-behind and handle/drag/selection behaviors. Adds Javadoc for new helpers and cleans up control flow for readability and maintainability. --- .../org/jhotdraw/draw/tool/SelectionTool.java | 176 ++++++++++-------- 1 file changed, 99 insertions(+), 77 deletions(-) diff --git a/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java b/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java index 50f6c47e0..9641e42a0 100644 --- a/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java +++ b/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java @@ -74,17 +74,7 @@ private class TrackerHandler extends ToolAdapter { @Override public void toolDone(ToolEvent event) { - // Empty - Tool newTracker = getSelectAreaTracker(); - if (newTracker != null) { - if (tracker != null) { - tracker.deactivate(getEditor()); - tracker.removeToolListener(this); - } - tracker = newTracker; - tracker.activate(getEditor()); - tracker.addToolListener(this); - } + setTracker(getSelectAreaTracker()); fireToolDone(); } @@ -160,35 +150,35 @@ public void deactivate(DrawingEditor editor) { @Override public void keyPressed(KeyEvent e) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.keyPressed(e); } } @Override public void keyReleased(KeyEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.keyReleased(evt); } } @Override public void keyTyped(KeyEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.keyTyped(evt); } } @Override public void mouseClicked(MouseEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.mouseClicked(evt); } } @Override public void mouseDragged(MouseEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.mouseDragged(evt); } } @@ -212,7 +202,7 @@ public void mouseMoved(MouseEvent evt) { @Override public void mouseReleased(MouseEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { tracker.mouseReleased(evt); } } @@ -224,82 +214,105 @@ public void draw(Graphics2D g) { @Override public void mousePressed(MouseEvent evt) { - if (getView() != null && getView().isEnabled()) { + if (isViewActive()) { super.mousePressed(evt); DrawingView view = getView(); Handle handle = view.findHandle(anchor); - Tool newTracker = null; - if (handle != null) { - newTracker = getHandleTracker(handle); - } else { - Figure figure; - Drawing drawing = view.getDrawing(); - Point2D.Double p = view.viewToDrawing(anchor); - if (isSelectBehindEnabled() - && (evt.getModifiersEx() - & (InputEvent.ALT_DOWN_MASK | InputEvent.CTRL_DOWN_MASK)) != 0) { - // Select a figure behind the current selection - figure = view.findFigure(anchor); - while (figure != null && !figure.isSelectable()) { - figure = drawing.findFigureBehind(p, figure); - } - HashSet
ignoredFigures = new HashSet<>(view.getSelectedFigures()); - ignoredFigures.add(figure); - Figure figureBehind = view.getDrawing().findFigureBehind( - view.viewToDrawing(anchor), ignoredFigures); - if (figureBehind != null) { - figure = figureBehind; - } - } else { - // Note: The search sequence used here, must be - // consistent with the search sequence used by the - // DefaultHandleTracker, the DefaultSelectAreaTracker and DelegationSelectionTool. - // If possible, continue to work with the current selection - figure = null; - if (isSelectBehindEnabled()) { - for (Figure f : view.getSelectedFigures()) { - if (f.contains(p)) { - figure = f; - break; - } - } - } - // If the point is not contained in the current selection, - // search for a figure in the drawing. - if (figure == null) { - figure = view.findFigure(anchor); - while (figure != null && !figure.isSelectable()) { - figure = drawing.findFigureBehind(p, figure); - } - } - } - if (figure != null && figure.isSelectable()) { - newTracker = getDragTracker(figure); - } else { - if (!evt.isShiftDown()) { - view.clearSelection(); - view.setHandleDetailLevel(0); + Figure figure = (handle == null) + ? findTargetFigure(view, view.viewToDrawing(anchor), evt) + : null; + Tool newTracker = resolveTracker(handle, figure, evt); + setTracker(newTracker); + tracker.mousePressed(evt); + } + } + + /** + * Finds the target figure under the given drawing point. + * Applies select-behind logic when alt/ctrl modifiers are held. + * + * @param view the current drawing view + * @param drawingPoint the point in drawing coordinates + * @param evt the mouse event + * @return the target figure, or null if none found + */ + private Figure findTargetFigure(DrawingView view, Point2D.Double drawingPoint, MouseEvent evt) { + Drawing drawing = view.getDrawing(); + Figure figure; + if (isSelectBehindEnabled() + && (evt.getModifiersEx() + & (InputEvent.ALT_DOWN_MASK | InputEvent.CTRL_DOWN_MASK)) != 0) { + // Select a figure behind the current selection + figure = view.findFigure(anchor); + while (figure != null && !figure.isSelectable()) { + figure = drawing.findFigureBehind(drawingPoint, figure); + } + HashSet
ignoredFigures = new HashSet<>(view.getSelectedFigures()); + ignoredFigures.add(figure); + Figure figureBehind = drawing.findFigureBehind( + drawingPoint, ignoredFigures); + if (figureBehind != null) { + figure = figureBehind; + } + } else { + // Note: The search sequence used here, must be + // consistent with the search sequence used by the + // DefaultHandleTracker, the DefaultSelectAreaTracker and DelegationSelectionTool. + // If possible, continue to work with the current selection + figure = null; + if (isSelectBehindEnabled()) { + for (Figure f : view.getSelectedFigures()) { + if (f.contains(drawingPoint)) { + figure = f; + break; } - newTracker = getSelectAreaTracker(); } } - if (newTracker != null) { - setTracker(newTracker); + // If the point is not contained in the current selection, + // search for a figure in the drawing. + if (figure == null) { + figure = view.findFigure(anchor); + while (figure != null && !figure.isSelectable()) { + figure = drawing.findFigureBehind(drawingPoint, figure); + } } - tracker.mousePressed(evt); } + return figure; + } + + /** + * Resolves which tracker to use based on the handle and figure found at the press point. + * + * @param handle the handle found at the press point, or null + * @param figure the figure found at the press point, or null + * @param evt the mouse event + * @return the tracker to activate + */ + private Tool resolveTracker(Handle handle, Figure figure, MouseEvent evt) { + if (handle != null) { + return getHandleTracker(handle); + } + if (figure != null && figure.isSelectable()) { + return getDragTracker(figure); + } + if (!evt.isShiftDown()) { + getView().clearSelection(); + getView().setHandleDetailLevel(0); + } + return getSelectAreaTracker(); } protected void setTracker(Tool newTracker) { + if (newTracker == null) { + return; + } if (tracker != null) { tracker.deactivate(getEditor()); tracker.removeToolListener(trackerHandler); } tracker = newTracker; - if (tracker != null) { - tracker.activate(getEditor()); - tracker.addToolListener(trackerHandler); - } + tracker.activate(getEditor()); + tracker.addToolListener(trackerHandler); } /** @@ -361,6 +374,15 @@ public void setDragTracker(DragTracker newValue) { dragTracker = newValue; } + /** + * Returns true if the current view is non-null and enabled. + * + * @return true if view is active + */ + private boolean isViewActive() { + return getView() != null && getView().isEnabled(); + } + /** * Returns true, if this tool lets the user interact with handles. *

From 4d49e88bad2984c8768518d43294abe4f7ee7fa6 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Mon, 27 Apr 2026 11:21:03 +0200 Subject: [PATCH 4/9] Add SelectionTool unit and BDD tests Add comprehensive tests for SelectionTool: a JUnit4 + Mockito test suite (SelectionToolTest) that verifies tracker lazy-init, activation/deactivation delegation, select-behind behavior, handle/drag/select-area resolution, property change firing, and boundary/invariant cases. Add JGiven BDD tests (GivenSelectionTool, WhenSelectionTool, ThenSelectionTool, SelectionToolBDDTest) to express key user stories and scenarios (clicking figures/handles/empty area, disabled view, alt-select-behind) in Given/When/Then form. Include TestableSelectionTool helper classes that expose protected SelectionTool methods for testing. --- .../jhotdraw/draw/tool/SelectionToolTest.java | 288 ++++++++++++++++++ .../draw/tool/bdd/GivenSelectionTool.java | 155 ++++++++++ .../draw/tool/bdd/SelectionToolBDDTest.java | 90 ++++++ .../draw/tool/bdd/ThenSelectionTool.java | 106 +++++++ .../draw/tool/bdd/WhenSelectionTool.java | 117 +++++++ 5 files changed, 756 insertions(+) create mode 100644 jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/SelectionToolTest.java create mode 100644 jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/GivenSelectionTool.java create mode 100644 jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java create mode 100644 jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java create mode 100644 jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/WhenSelectionTool.java diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/SelectionToolTest.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/SelectionToolTest.java new file mode 100644 index 000000000..44a2885c1 --- /dev/null +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/SelectionToolTest.java @@ -0,0 +1,288 @@ +package org.jhotdraw.draw.tool; + +import org.jhotdraw.draw.Drawing; +import org.jhotdraw.draw.DrawingEditor; +import org.jhotdraw.draw.DrawingView; +import org.jhotdraw.draw.figure.Figure; +import org.jhotdraw.draw.handle.Handle; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import javax.swing.JPanel; +import java.awt.Container; +import java.awt.Cursor; +import java.awt.Point; +import java.awt.event.MouseEvent; +import java.awt.geom.Point2D; +import java.beans.PropertyChangeEvent; +import java.beans.PropertyChangeListener; +import java.util.Collections; + +import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +/** + * JUnit 4 + Mockito tests for {@link SelectionTool}. + */ +public class SelectionToolTest { + + private TestableSelectionTool tool; + private DrawingEditor mockEditor; + private DrawingView mockView; + private Drawing mockDrawing; + private Figure mockFigure; + private Handle mockHandle; + private JPanel dummyComponent; + + /** + * Exposes protected methods of SelectionTool for testing. + */ + private static class TestableSelectionTool extends SelectionTool { + public Tool callGetSelectAreaTracker() { + return getSelectAreaTracker(); + } + public DragTracker callGetDragTracker(Figure f) { + return getDragTracker(f); + } + public HandleTracker callGetHandleTracker(Handle h) { + return getHandleTracker(h); + } + public void callSetTracker(Tool t) { + setTracker(t); + } + } + + @Before + public void setUp() { + tool = new TestableSelectionTool(); + mockEditor = mock(DrawingEditor.class); + mockView = mock(DrawingView.class); + mockDrawing = mock(Drawing.class); + mockFigure = mock(Figure.class); + mockHandle = mock(Handle.class); + dummyComponent = new JPanel(); + + // Wire up default editor/view behavior + when(mockEditor.getActiveView()).thenReturn(mockView); + when(mockEditor.getDrawingViews()).thenReturn(Collections.emptySet()); + when(mockEditor.findView(any(Container.class))).thenReturn(mockView); + when(mockView.getDrawing()).thenReturn(mockDrawing); + when(mockView.isEnabled()).thenReturn(true); + when(mockView.viewToDrawing(any(Point.class))).thenReturn(new Point2D.Double(100, 100)); + when(mockView.getSelectedFigures()).thenReturn(Collections.emptySet()); + when(mockView.findHandle(any(Point.class))).thenReturn(null); + when(mockView.findFigure(any(Point.class))).thenReturn(null); + when(mockView.getCompatibleHandles(any(Handle.class))).thenReturn(Collections.emptyList()); + when(mockHandle.getCursor()).thenReturn(Cursor.getDefaultCursor()); + } + + // ------------------------------------------------------------------------- + // A. Best-case scenario tests + // ------------------------------------------------------------------------- + + @Test + public void testDefaultSelectBehindEnabled() { + assertTrue(tool.isSelectBehindEnabled()); + } + + @Test + public void testSetSelectBehindEnabled() { + tool.setSelectBehindEnabled(false); + assertFalse(tool.isSelectBehindEnabled()); + } + + @Test + public void testSelectBehindEnabledFiresPropertyChange() { + PropertyChangeListener listener = mock(PropertyChangeListener.class); + tool.addPropertyChangeListener(listener); + + tool.setSelectBehindEnabled(false); + + verify(listener).propertyChange(any(PropertyChangeEvent.class)); + } + + @Test + public void testSupportsHandleInteraction() { + assertTrue(tool.supportsHandleInteraction()); + } + + @Test + public void testGetSelectAreaTrackerLazyInit() { + Tool first = tool.callGetSelectAreaTracker(); + Tool second = tool.callGetSelectAreaTracker(); + assertNotNull(first); + assertSame(first, second); + } + + @Test + public void testGetDragTrackerLazyInit() { + DragTracker first = tool.callGetDragTracker(mockFigure); + DragTracker second = tool.callGetDragTracker(mockFigure); + assertNotNull(first); + assertSame(first, second); + } + + @Test + public void testGetHandleTrackerLazyInit() { + tool.activate(mockEditor); + HandleTracker first = tool.callGetHandleTracker(mockHandle); + HandleTracker second = tool.callGetHandleTracker(mockHandle); + assertNotNull(first); + assertSame(first, second); + } + + @Test + public void testActivateDelegatesTracker() { + tool.activate(mockEditor); + + Tool mockTracker = mock(Tool.class); + tool.callSetTracker(mockTracker); + + // Now activate again — mockTracker.activate should be called + tool.activate(mockEditor); + verify(mockTracker, atLeastOnce()).activate(mockEditor); + } + + @Test + public void testDeactivateDelegatesTracker() { + tool.activate(mockEditor); + + Tool mockTracker = mock(Tool.class); + tool.callSetTracker(mockTracker); + + tool.deactivate(mockEditor); + verify(mockTracker).deactivate(mockEditor); + } + + @Test + public void testSetTrackerSwapsTrackers() { + tool.activate(mockEditor); + + Tool oldTracker = mock(Tool.class); + Tool newTracker = mock(Tool.class); + + tool.callSetTracker(oldTracker); + tool.callSetTracker(newTracker); + + // Old tracker deactivated, new tracker activated + verify(oldTracker).deactivate(mockEditor); + verify(newTracker).activate(mockEditor); + } + + // ------------------------------------------------------------------------- + // B. Boundary-case tests + // ------------------------------------------------------------------------- + + @Test + public void testSetTrackerWithNull() { + tool.activate(mockEditor); + + // Should be a no-op — no NPE, tool remains functional + tool.callSetTracker(null); + tool.deactivate(mockEditor); + } + + @Test + public void testMousePressedWithDisabledView() { + when(mockView.isEnabled()).thenReturn(false); + tool.activate(mockEditor); + + Tool mockTracker = mock(Tool.class); + tool.callSetTracker(mockTracker); + + MouseEvent evt = new MouseEvent(dummyComponent, MouseEvent.MOUSE_PRESSED, + System.currentTimeMillis(), 0, 100, 100, 1, false, MouseEvent.BUTTON1); + + tool.mousePressed(evt); + + // tracker's mousePressed should NOT be called when view is disabled + verify(mockTracker, never()).mousePressed(evt); + } + + @Test + public void testResolveTrackerWithHandleReturnsHandleTracker() { + when(mockView.findHandle(any(Point.class))).thenReturn(mockHandle); + when(mockView.getCompatibleHandles(mockHandle)).thenReturn(Collections.emptyList()); + tool.activate(mockEditor); + + MouseEvent evt = new MouseEvent(dummyComponent, MouseEvent.MOUSE_PRESSED, + System.currentTimeMillis(), 0, 10, 10, 1, false, MouseEvent.BUTTON1); + + tool.mousePressed(evt); + + HandleTracker ht = tool.callGetHandleTracker(mockHandle); + assertNotNull(ht); + assertTrue(ht instanceof HandleTracker); + } + + @Test + public void testResolveTrackerWithSelectableFigureReturnsDragTracker() { + when(mockView.findHandle(any(Point.class))).thenReturn(null); + when(mockView.findFigure(any(Point.class))).thenReturn(mockFigure); + when(mockFigure.isSelectable()).thenReturn(true); + when(mockFigure.contains(any(Point2D.Double.class))).thenReturn(false); + tool.activate(mockEditor); + + MouseEvent evt = new MouseEvent(dummyComponent, MouseEvent.MOUSE_PRESSED, + System.currentTimeMillis(), 0, 10, 10, 1, false, MouseEvent.BUTTON1); + + tool.mousePressed(evt); + + DragTracker dt = tool.callGetDragTracker(mockFigure); + assertNotNull(dt); + assertTrue(dt instanceof DragTracker); + } + + @Test + public void testResolveTrackerWithNothingReturnsSelectAreaTracker() { + when(mockView.findHandle(any(Point.class))).thenReturn(null); + when(mockView.findFigure(any(Point.class))).thenReturn(null); + tool.activate(mockEditor); + + MouseEvent evt = new MouseEvent(dummyComponent, MouseEvent.MOUSE_PRESSED, + System.currentTimeMillis(), 0, 10, 10, 1, false, MouseEvent.BUTTON1); + + tool.mousePressed(evt); + + Tool sat = tool.callGetSelectAreaTracker(); + assertNotNull(sat); + assertTrue(sat instanceof SelectAreaTracker); + } + + @Test + public void testSetSelectBehindEnabledNoChangeDoesNotFire() { + // PropertyChangeSupport skips firing when old == new value + PropertyChangeListener listener = mock(PropertyChangeListener.class); + tool.addPropertyChangeListener(listener); + + tool.setSelectBehindEnabled(true); // same as default — no event expected + + verify(listener, never()).propertyChange(any(PropertyChangeEvent.class)); + } + + // ------------------------------------------------------------------------- + // C. Invariant tests + // ------------------------------------------------------------------------- + + @Test + public void testInvariantTrackerNeverNull() { + TestableSelectionTool freshTool = new TestableSelectionTool(); + assertNotNull(freshTool.callGetSelectAreaTracker()); + } + + @Test + public void testInvariantSelectBehindEnabledDefaultTrue() { + TestableSelectionTool freshTool = new TestableSelectionTool(); + assert freshTool.isSelectBehindEnabled() : "selectBehindEnabled must be true by default"; + assertTrue(freshTool.isSelectBehindEnabled()); + } + + @Test + public void testInvariantSupportsHandleInteractionAlwaysTrue() { + TestableSelectionTool freshTool = new TestableSelectionTool(); + assert freshTool.supportsHandleInteraction() : "supportsHandleInteraction must always be true"; + assertTrue(freshTool.supportsHandleInteraction()); + } +} diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/GivenSelectionTool.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/GivenSelectionTool.java new file mode 100644 index 000000000..347929efb --- /dev/null +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/GivenSelectionTool.java @@ -0,0 +1,155 @@ +package org.jhotdraw.draw.tool.bdd; + +import com.tngtech.jgiven.Stage; +import com.tngtech.jgiven.annotation.BeforeStage; +import com.tngtech.jgiven.annotation.ProvidedScenarioState; +import org.jhotdraw.draw.Drawing; +import org.jhotdraw.draw.DrawingEditor; +import org.jhotdraw.draw.DrawingView; +import org.jhotdraw.draw.figure.Figure; +import org.jhotdraw.draw.handle.Handle; +import org.jhotdraw.draw.tool.DragTracker; +import org.jhotdraw.draw.tool.HandleTracker; +import org.jhotdraw.draw.tool.SelectAreaTracker; +import org.jhotdraw.draw.tool.SelectionTool; +import org.jhotdraw.draw.tool.Tool; + +import javax.swing.JPanel; +import java.awt.Container; +import java.awt.Cursor; +import java.awt.Point; +import java.awt.geom.Point2D; +import java.beans.PropertyChangeListener; +import java.util.Collections; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; + +public class GivenSelectionTool extends Stage { + + @ProvidedScenarioState + TestableSelectionTool tool; + + @ProvidedScenarioState + DrawingEditor mockEditor; + + @ProvidedScenarioState + DrawingView mockView; + + @ProvidedScenarioState + Drawing mockDrawing; + + @ProvidedScenarioState + Figure mockFigure; + + @ProvidedScenarioState + Handle mockHandle; + + @ProvidedScenarioState + JPanel dummyComponent; + + @ProvidedScenarioState + PropertyChangeListener mockListener; + + @BeforeStage + public void setupMocks() { + tool = new TestableSelectionTool(); + + mockEditor = mock(DrawingEditor.class); + mockView = mock(DrawingView.class); + mockDrawing = mock(Drawing.class); + mockFigure = mock(Figure.class); + mockHandle = mock(Handle.class); + dummyComponent = new JPanel(); + mockListener = mock(PropertyChangeListener.class); + + org.mockito.Mockito.when(mockEditor.getActiveView()).thenReturn(mockView); + org.mockito.Mockito.when(mockEditor.findView(any(Container.class))).thenReturn(mockView); + org.mockito.Mockito.when(mockEditor.getDrawingViews()).thenReturn(Collections.emptySet()); + + org.mockito.Mockito.when(mockView.getDrawing()).thenReturn(mockDrawing); + org.mockito.Mockito.when(mockView.isEnabled()).thenReturn(true); + org.mockito.Mockito.when(mockView.viewToDrawing(any(Point.class))).thenReturn(new Point2D.Double(100, 100)); + org.mockito.Mockito.when(mockView.getSelectedFigures()).thenReturn(Collections.emptySet()); + org.mockito.Mockito.when(mockView.findHandle(any(Point.class))).thenReturn(null); + org.mockito.Mockito.when(mockView.findFigure(any(Point.class))).thenReturn(null); + org.mockito.Mockito.when(mockView.getCompatibleHandles(any())).thenReturn(Collections.emptyList()); + + org.mockito.Mockito.when(mockHandle.getCursor()).thenReturn(Cursor.getDefaultCursor()); + } + + public GivenSelectionTool a_drawing_with_a_selectable_figure() { + org.mockito.Mockito.when(mockView.findFigure(any(Point.class))).thenReturn(mockFigure); + org.mockito.Mockito.when(mockFigure.isSelectable()).thenReturn(true); + org.mockito.Mockito.when(mockFigure.contains(any(Point2D.Double.class))).thenReturn(false); + tool.activate(mockEditor); + return self(); + } + + public GivenSelectionTool a_drawing_with_a_handle_at_click_point() { + org.mockito.Mockito.when(mockView.findHandle(any(Point.class))).thenReturn(mockHandle); + org.mockito.Mockito.when(mockView.getCompatibleHandles(any(Handle.class))).thenReturn(Collections.singletonList(mockHandle)); + tool.activate(mockEditor); + return self(); + } + + public GivenSelectionTool an_empty_drawing_area() { + org.mockito.Mockito.when(mockView.findHandle(any(Point.class))).thenReturn(null); + org.mockito.Mockito.when(mockView.findFigure(any(Point.class))).thenReturn(null); + tool.activate(mockEditor); + return self(); + } + + public GivenSelectionTool a_disabled_drawing_view() { + org.mockito.Mockito.when(mockView.isEnabled()).thenReturn(false); + tool.activate(mockEditor); + return self(); + } + + public GivenSelectionTool overlapping_figures_with_select_behind_enabled() { + Figure mockFigureBehind = mock(Figure.class); + org.mockito.Mockito.when(mockFigureBehind.isSelectable()).thenReturn(true); + org.mockito.Mockito.when(mockFigureBehind.contains(any(Point2D.Double.class))).thenReturn(false); + + org.mockito.Mockito.when(mockView.findFigure(any(Point.class))).thenReturn(mockFigure); + org.mockito.Mockito.when(mockFigure.isSelectable()).thenReturn(true); + org.mockito.Mockito.when(mockDrawing.findFigureBehind(any(Point2D.Double.class), any(Figure.class))).thenReturn(mockFigureBehind); + + tool.setSelectBehindEnabled(true); + tool.activate(mockEditor); + return self(); + } + + public GivenSelectionTool a_newly_created_SelectionTool() { + tool = new TestableSelectionTool(); + return self(); + } + + public GivenSelectionTool the_SelectionTool_with_default_settings() { + tool.activate(mockEditor); + return self(); + } + + // ------------------------------------------------------------------------- + // Inner class exposing protected methods of SelectionTool for testing + // ------------------------------------------------------------------------- + + public static class TestableSelectionTool extends SelectionTool { + + public Tool callGetSelectAreaTracker() { + return getSelectAreaTracker(); + } + + public DragTracker callGetDragTracker(Figure f) { + return getDragTracker(f); + } + + public HandleTracker callGetHandleTracker(Handle h) { + return getHandleTracker(h); + } + + public void callSetTracker(Tool t) { + setTracker(t); + } + } +} diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java new file mode 100644 index 000000000..090e33815 --- /dev/null +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java @@ -0,0 +1,90 @@ +package org.jhotdraw.draw.tool.bdd; + +import com.tngtech.jgiven.junit.ScenarioTest; +import org.junit.Test; + +/** + * BDD tests for {@link org.jhotdraw.draw.tool.SelectionTool} using JGiven. + * Each test maps to a User Story / BDD scenario for the SelectionTool feature. + */ +public class SelectionToolBDDTest + extends ScenarioTest { + + // US1: As a drawing user, I want to select a figure by clicking on it + @Test + public void clicking_on_a_selectable_figure_activates_drag_tracker() { + given().a_drawing_with_a_selectable_figure(); + when().the_user_clicks_on_the_figure(); + then().the_DragTracker_is_activated(); + } + + // US3: As a drawing user, I want to manipulate figure handles + @Test + public void clicking_on_a_handle_activates_handle_tracker() { + given().a_drawing_with_a_handle_at_click_point(); + when().the_user_presses_mouse_on_handle(); + then().the_HandleTracker_is_activated(); + } + + // US4: As a drawing user, I want to select an area by rubber-banding + @Test + public void clicking_on_empty_area_activates_select_area_tracker() { + given().an_empty_drawing_area(); + when().the_user_presses_mouse_on_empty_area(); + then().the_SelectAreaTracker_is_activated(); + } + + // Boundary: disabled view should not trigger tracker + @Test + public void disabled_view_prevents_tracker_action() { + given().a_disabled_drawing_view(); + when().the_user_presses_mouse_with_disabled_view(); + then().no_tracker_action_is_performed(); + } + + // US5: As a drawing user, I want to select a figure behind another + @Test + public void alt_click_with_overlapping_figures_selects_figure_behind() { + given().overlapping_figures_with_select_behind_enabled(); + when().the_user_clicks_with_alt_modifier(); + then().the_DragTracker_is_activated(); + } + + // Property change: selectBehindEnabled fires event + @Test + public void setting_selectBehindEnabled_fires_property_change() { + given().the_SelectionTool_with_default_settings(); + when().selectBehindEnabled_is_set_to(false); + then().selectBehindEnabled_is(false); + } + + // Invariant: lazy initialization returns same instance + @Test + public void select_area_tracker_lazy_init_returns_same_instance() { + given().a_newly_created_SelectionTool(); + when().a_tracker_is_requested_multiple_times(); + then().the_same_tracker_instance_is_returned(); + } + + @Test + public void drag_tracker_lazy_init_returns_same_instance() { + given().a_newly_created_SelectionTool(); + when().a_drag_tracker_is_requested_multiple_times(); + then().the_same_tracker_instance_is_returned(); + } + + @Test + public void handle_tracker_lazy_init_returns_same_instance() { + given().a_newly_created_SelectionTool(); + when().a_handle_tracker_is_requested_multiple_times(); + then().the_same_tracker_instance_is_returned(); + } + + // Invariant: supports handle interaction is always true + @Test + public void selection_tool_always_supports_handle_interaction() { + given().a_newly_created_SelectionTool(); + when().selectBehindEnabled_is_set_to(true); + then().the_tool_supports_handle_interaction(); + } +} diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java new file mode 100644 index 000000000..6a0d98376 --- /dev/null +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java @@ -0,0 +1,106 @@ +package org.jhotdraw.draw.tool.bdd; + +import com.tngtech.jgiven.Stage; +import com.tngtech.jgiven.annotation.ExpectedScenarioState; +import com.tngtech.jgiven.annotation.ScenarioState.Resolution; +import org.jhotdraw.draw.DrawingEditor; +import org.jhotdraw.draw.DrawingView; +import org.jhotdraw.draw.figure.Figure; +import org.jhotdraw.draw.handle.Handle; +import org.jhotdraw.draw.tool.DragTracker; +import org.jhotdraw.draw.tool.HandleTracker; +import org.jhotdraw.draw.tool.SelectAreaTracker; +import org.jhotdraw.draw.tool.Tool; + +import java.awt.Point; +import java.awt.event.MouseEvent; +import java.beans.PropertyChangeEvent; +import java.beans.PropertyChangeListener; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +public class ThenSelectionTool extends Stage { + + @ExpectedScenarioState + GivenSelectionTool.TestableSelectionTool tool; + + @ExpectedScenarioState + DrawingEditor mockEditor; + + @ExpectedScenarioState + DrawingView mockView; + + @ExpectedScenarioState + Figure mockFigure; + + @ExpectedScenarioState + Handle mockHandle; + + @ExpectedScenarioState + PropertyChangeListener mockListener; + + @ExpectedScenarioState + MouseEvent lastMouseEvent; + + @ExpectedScenarioState(resolution = Resolution.NAME) + Tool firstTrackerInstance; + + @ExpectedScenarioState(resolution = Resolution.NAME) + Tool secondTrackerInstance; + + public ThenSelectionTool the_DragTracker_is_activated() { + Tool tracker = tool.callGetDragTracker(mockFigure); + assertThat(tracker).isNotNull(); + assertThat(tracker).isInstanceOf(DragTracker.class); + return self(); + } + + public ThenSelectionTool the_HandleTracker_is_activated() { + Tool tracker = tool.callGetHandleTracker(mockHandle); + assertThat(tracker).isNotNull(); + assertThat(tracker).isInstanceOf(HandleTracker.class); + return self(); + } + + public ThenSelectionTool the_SelectAreaTracker_is_activated() { + Tool tracker = tool.callGetSelectAreaTracker(); + assertThat(tracker).isNotNull(); + assertThat(tracker).isInstanceOf(SelectAreaTracker.class); + return self(); + } + + public ThenSelectionTool no_tracker_action_is_performed() { + verify(mockView, never()).findHandle(any(Point.class)); + return self(); + } + + public ThenSelectionTool the_property_change_event_is_fired() { + verify(mockListener).propertyChange(any(PropertyChangeEvent.class)); + return self(); + } + + public ThenSelectionTool the_property_change_event_is_not_fired() { + verify(mockListener, never()).propertyChange(any(PropertyChangeEvent.class)); + return self(); + } + + public ThenSelectionTool the_same_tracker_instance_is_returned() { + assertThat(firstTrackerInstance).isNotNull(); + assertThat(secondTrackerInstance).isNotNull(); + assertThat(firstTrackerInstance).isSameAs(secondTrackerInstance); + return self(); + } + + public ThenSelectionTool selectBehindEnabled_is(boolean expected) { + assertThat(tool.isSelectBehindEnabled()).isEqualTo(expected); + return self(); + } + + public ThenSelectionTool the_tool_supports_handle_interaction() { + assertThat(tool.supportsHandleInteraction()).isTrue(); + return self(); + } +} diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/WhenSelectionTool.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/WhenSelectionTool.java new file mode 100644 index 000000000..7c6fe6ce5 --- /dev/null +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/WhenSelectionTool.java @@ -0,0 +1,117 @@ +package org.jhotdraw.draw.tool.bdd; + +import com.tngtech.jgiven.Stage; +import com.tngtech.jgiven.annotation.ExpectedScenarioState; +import com.tngtech.jgiven.annotation.ProvidedScenarioState; +import com.tngtech.jgiven.annotation.ScenarioState.Resolution; +import org.jhotdraw.draw.Drawing; +import org.jhotdraw.draw.DrawingEditor; +import org.jhotdraw.draw.DrawingView; +import org.jhotdraw.draw.figure.Figure; +import org.jhotdraw.draw.handle.Handle; +import org.jhotdraw.draw.tool.Tool; + +import javax.swing.JPanel; +import java.awt.event.InputEvent; +import java.awt.event.MouseEvent; +import java.beans.PropertyChangeListener; + +public class WhenSelectionTool extends Stage { + + @ExpectedScenarioState + GivenSelectionTool.TestableSelectionTool tool; + + @ExpectedScenarioState + DrawingEditor mockEditor; + + @ExpectedScenarioState + DrawingView mockView; + + @ExpectedScenarioState + Drawing mockDrawing; + + @ExpectedScenarioState + Figure mockFigure; + + @ExpectedScenarioState + Handle mockHandle; + + @ExpectedScenarioState + JPanel dummyComponent; + + @ExpectedScenarioState + PropertyChangeListener mockListener; + + @ProvidedScenarioState + MouseEvent lastMouseEvent; + + @ProvidedScenarioState(resolution = Resolution.NAME) + Tool firstTrackerInstance; + + @ProvidedScenarioState(resolution = Resolution.NAME) + Tool secondTrackerInstance; + + public WhenSelectionTool the_user_clicks_on_the_figure() { + MouseEvent evt = createMouseEvent(0); + tool.mousePressed(evt); + lastMouseEvent = evt; + return self(); + } + + public WhenSelectionTool the_user_presses_mouse_on_handle() { + MouseEvent evt = createMouseEvent(0); + tool.mousePressed(evt); + lastMouseEvent = evt; + return self(); + } + + public WhenSelectionTool the_user_presses_mouse_on_empty_area() { + MouseEvent evt = createMouseEvent(0); + tool.mousePressed(evt); + lastMouseEvent = evt; + return self(); + } + + public WhenSelectionTool the_user_presses_mouse_with_disabled_view() { + MouseEvent evt = createMouseEvent(0); + tool.mousePressed(evt); + lastMouseEvent = evt; + return self(); + } + + public WhenSelectionTool the_user_clicks_with_alt_modifier() { + MouseEvent evt = createMouseEvent(InputEvent.ALT_DOWN_MASK); + tool.mousePressed(evt); + lastMouseEvent = evt; + return self(); + } + + public WhenSelectionTool selectBehindEnabled_is_set_to(boolean value) { + tool.setSelectBehindEnabled(value); + return self(); + } + + public WhenSelectionTool a_tracker_is_requested_multiple_times() { + firstTrackerInstance = tool.callGetSelectAreaTracker(); + secondTrackerInstance = tool.callGetSelectAreaTracker(); + return self(); + } + + public WhenSelectionTool a_drag_tracker_is_requested_multiple_times() { + firstTrackerInstance = tool.callGetDragTracker(mockFigure); + secondTrackerInstance = tool.callGetDragTracker(mockFigure); + return self(); + } + + public WhenSelectionTool a_handle_tracker_is_requested_multiple_times() { + tool.activate(mockEditor); + firstTrackerInstance = tool.callGetHandleTracker(mockHandle); + secondTrackerInstance = tool.callGetHandleTracker(mockHandle); + return self(); + } + + private MouseEvent createMouseEvent(int modifiers) { + return new MouseEvent(dummyComponent, MouseEvent.MOUSE_PRESSED, + System.currentTimeMillis(), modifiers, 10, 10, 1, false, MouseEvent.BUTTON1); + } +} From 35f97d949e556aebfcbcb7e41ab6a207ffddf357 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Mon, 27 Apr 2026 11:28:12 +0200 Subject: [PATCH 5/9] Refactor SelectionTool target-figure lookup Extract selection hit-test logic into smaller helpers and clarify select-behind behavior. The method findTargetFigure now routes to isSelectBehindModifierHeld(evt) and delegates to findFigureBehindCurrentSelection(...) or findFigureAtPoint(...). Added isSelectBehindModifierHeld, findFigureBehindCurrentSelection, and findFigureAtPoint to improve readability and separate concerns; preserved original search order and select-behind semantics (ALT/CTRL modifier) and retained compatibility notes with other trackers. --- .../org/jhotdraw/draw/tool/SelectionTool.java | 101 +++++++++++------- 1 file changed, 64 insertions(+), 37 deletions(-) diff --git a/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java b/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java index 9641e42a0..0f9eb703b 100644 --- a/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java +++ b/jhotdraw-core/src/main/java/org/jhotdraw/draw/tool/SelectionTool.java @@ -229,7 +229,8 @@ public void mousePressed(MouseEvent evt) { /** * Finds the target figure under the given drawing point. - * Applies select-behind logic when alt/ctrl modifiers are held. + * Routes to select-behind logic when alt/ctrl modifiers are held, + * otherwise finds the topmost selectable figure at the point. * * @param view the current drawing view * @param drawingPoint the point in drawing coordinates @@ -237,46 +238,72 @@ public void mousePressed(MouseEvent evt) { * @return the target figure, or null if none found */ private Figure findTargetFigure(DrawingView view, Point2D.Double drawingPoint, MouseEvent evt) { - Drawing drawing = view.getDrawing(); - Figure figure; - if (isSelectBehindEnabled() + return isSelectBehindModifierHeld(evt) + ? findFigureBehindCurrentSelection(view, drawingPoint) + : findFigureAtPoint(view, drawingPoint); + } + + /** + * Returns true when the user is holding the alt or ctrl modifier, + * indicating a select-behind gesture. + * + * @param evt the mouse event + * @return true if select-behind modifier is held + */ + private boolean isSelectBehindModifierHeld(MouseEvent evt) { + return isSelectBehindEnabled() && (evt.getModifiersEx() - & (InputEvent.ALT_DOWN_MASK | InputEvent.CTRL_DOWN_MASK)) != 0) { - // Select a figure behind the current selection - figure = view.findFigure(anchor); - while (figure != null && !figure.isSelectable()) { - figure = drawing.findFigureBehind(drawingPoint, figure); - } - HashSet

ignoredFigures = new HashSet<>(view.getSelectedFigures()); - ignoredFigures.add(figure); - Figure figureBehind = drawing.findFigureBehind( - drawingPoint, ignoredFigures); - if (figureBehind != null) { - figure = figureBehind; - } - } else { - // Note: The search sequence used here, must be - // consistent with the search sequence used by the - // DefaultHandleTracker, the DefaultSelectAreaTracker and DelegationSelectionTool. - // If possible, continue to work with the current selection - figure = null; - if (isSelectBehindEnabled()) { - for (Figure f : view.getSelectedFigures()) { - if (f.contains(drawingPoint)) { - figure = f; - break; - } - } - } - // If the point is not contained in the current selection, - // search for a figure in the drawing. - if (figure == null) { - figure = view.findFigure(anchor); - while (figure != null && !figure.isSelectable()) { - figure = drawing.findFigureBehind(drawingPoint, figure); + & (InputEvent.ALT_DOWN_MASK | InputEvent.CTRL_DOWN_MASK)) != 0; + } + + /** + * Finds the first selectable figure behind the current selection at the given point. + * Used when the user holds alt/ctrl to reach figures underneath the current selection. + * + * @param view the current drawing view + * @param drawingPoint the point in drawing coordinates + * @return the figure behind the current selection, or null if none found + */ + private Figure findFigureBehindCurrentSelection(DrawingView view, Point2D.Double drawingPoint) { + Drawing drawing = view.getDrawing(); + Figure figure = view.findFigure(anchor); + while (figure != null && !figure.isSelectable()) { + figure = drawing.findFigureBehind(drawingPoint, figure); + } + HashSet
ignoredFigures = new HashSet<>(view.getSelectedFigures()); + ignoredFigures.add(figure); + Figure figureBehind = drawing.findFigureBehind(drawingPoint, ignoredFigures); + if (figureBehind != null) { + figure = figureBehind; + } + return figure; + } + + /** + * Finds the topmost selectable figure at the given point. + * Prefers a figure already in the current selection; falls back to a drawing search. + *

+ * Note: The search sequence used here must be consistent with the sequence used by + * {@link DefaultHandleTracker}, {@link DefaultSelectAreaTracker}, and + * {@link DelegationSelectionTool}. + * + * @param view the current drawing view + * @param drawingPoint the point in drawing coordinates + * @return the figure at the point, or null if none found + */ + private Figure findFigureAtPoint(DrawingView view, Point2D.Double drawingPoint) { + Drawing drawing = view.getDrawing(); + if (isSelectBehindEnabled()) { + for (Figure f : view.getSelectedFigures()) { + if (f.contains(drawingPoint)) { + return f; } } } + Figure figure = view.findFigure(anchor); + while (figure != null && !figure.isSelectable()) { + figure = drawing.findFigureBehind(drawingPoint, figure); + } return figure; } From 0893017bf0a94885d8f111f13cffd61a30805760 Mon Sep 17 00:00:00 2001 From: Altan Esmer Date: Sun, 10 May 2026 09:37:07 +0300 Subject: [PATCH 6/9] Update .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index e99300712..8ac1cc002 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ nb-configuration.xml *.ftf /jhotdraw-samples/jhotdraw-samples-misc/nbproject/ *.iml +docs \ No newline at end of file From d962326753cbea08ebc63906602d2157e3689580 Mon Sep 17 00:00:00 2001 From: AltanEsmer Date: Thu, 4 Jun 2026 18:05:07 +0200 Subject: [PATCH 7/9] Add lab portfolio docs and tidy Selection Tool tests - Add Labs/ portfolio write-ups for the Selection Tool feature: concept location, user story, impact analysis, refactoring, actualization (SOLID / Clean Architecture), unit testing and BDD. - Add US2 BDD scenario: clicking an empty area clears the current selection. - Enable assertions in surefire so the invariant assert checks actually run. - Remove the unused assertj-swing-junit test dependency. --- Labs/L2-ConceptLocation.md | 22 +++++ Labs/L2-UserStory.md | 29 +++++++ Labs/L3-ImpactAnalysis.md | 51 ++++++++++++ Labs/L4-Refactoring.md | 59 ++++++++++++++ Labs/L5-Actualization.md | 46 +++++++++++ Labs/L7-Testing.md | 80 +++++++++++++++++++ Labs/L8-BDD.md | 59 ++++++++++++++ Labs/README.md | 19 +++++ jhotdraw-core/pom.xml | 7 +- .../draw/tool/bdd/SelectionToolBDDTest.java | 9 +++ .../draw/tool/bdd/ThenSelectionTool.java | 5 ++ 11 files changed, 380 insertions(+), 6 deletions(-) create mode 100644 Labs/L2-ConceptLocation.md create mode 100644 Labs/L2-UserStory.md create mode 100644 Labs/L3-ImpactAnalysis.md create mode 100644 Labs/L4-Refactoring.md create mode 100644 Labs/L5-Actualization.md create mode 100644 Labs/L7-Testing.md create mode 100644 Labs/L8-BDD.md create mode 100644 Labs/README.md diff --git a/Labs/L2-ConceptLocation.md b/Labs/L2-ConceptLocation.md new file mode 100644 index 000000000..1c9f8f404 --- /dev/null +++ b/Labs/L2-ConceptLocation.md @@ -0,0 +1,22 @@ +# Concept Location: The Selection Tool + +To find the classes that implement the Selection feature I located the concept dynamically rather than by reading code. I launched the runnable SVG sample (`org.jhotdraw.samples.svg.Main` in `jhotdraw-samples-misc`), set a breakpoint on the first line of `SelectionTool.mousePressed(MouseEvent)` (`SelectionTool.java:217`), and then exercised the feature three ways on the canvas: clicking a figure, clicking one of a figure's handles, and clicking empty space. Each click suspended the debugger at the same entry point, which confirmed that `SelectionTool` is the controller Tool the editor delegates mouse events to and the natural starting point of the concept. + +From that breakpoint I stepped into the body. `view.findHandle(anchor)` took me into `DrawingView`; when it returned null I followed `findTargetFigure(...)` into `Figure.isSelectable()`/`contains()` and `Drawing.findFigureBehind(...)`; then `resolveTracker(handle, figure, evt)` (`SelectionTool.java:318`) picked a tracker and `setTracker(...)` activated it. Stepping into `tracker.mousePressed(evt)` dropped me into one of the three Default trackers depending on which gesture I had performed. Holding Alt while clicking overlapping figures took the `isSelectBehindModifierHeld` branch and stepped into `findFigureBehindCurrentSelection`. Walking those call chains gave me the set of collaborating classes below. + +| Domain Class | Responsibility | +| --- | --- | +| `SelectionTool` | Controller/entry Tool the debugger lands in first; holds the current tracker and routes mouse/key events to it (Strategy context). | +| `AbstractTool` | Superclass providing common Tool plumbing: the `anchor` point, view/editor accessors, and the `fire*` event helpers. | +| `Tool` | Interface defining the Tool contract (`activate`, `mousePressed`, `draw`, listener methods) that `SelectionTool` and the trackers implement. | +| `HandleTracker` / `DefaultHandleTracker` | Tracker state for manipulating a handle; default impl drags the handle and its compatible handles. | +| `DragTracker` / `DefaultDragTracker` | Tracker state for dragging a selected figure; default impl moves the figure with the mouse. | +| `SelectAreaTracker` / `DefaultSelectAreaTracker` | Tracker state for rubber-band area selection; default impl draws the selection rectangle and selects enclosed figures. | +| `DrawingView` | The view clicked on; resolves handles and figures (`findHandle`, `findFigure`), converts coordinates (`viewToDrawing`), and manages selection state. | +| `Drawing` | The figure container; `findFigureBehind(...)` walks the z-order for the select-behind gesture. | +| `DrawingEditor` | Editor context passed to `activate`/`deactivate` when trackers are swapped. | +| `Figure` | The drawable object being selected; queried via `isSelectable()` and `contains(point)`. | +| `Handle` | A draggable control point on a selected figure; the target a `HandleTracker` operates on. | +| `ToolEvent` / `ToolAdapter` | Event plumbing; the inner `TrackerHandler` extends `ToolAdapter` to catch tracker `toolDone` events and reset to the select-area tracker. | + +The initial concept set is therefore `SelectionTool` together with `AbstractTool`/`Tool`, the three tracker pairs (`HandleTracker`/`DefaultHandleTracker`, `DragTracker`/`DefaultDragTracker`, `SelectAreaTracker`/`DefaultSelectAreaTracker`), and the collaborators `DrawingView`, `Drawing`, `DrawingEditor`, `Figure`, `Handle`, and the `ToolEvent`/`ToolAdapter` event types. diff --git a/Labs/L2-UserStory.md b/Labs/L2-UserStory.md new file mode 100644 index 000000000..59827580d --- /dev/null +++ b/Labs/L2-UserStory.md @@ -0,0 +1,29 @@ +# L2 — Change Request: User Story (Selection Tool) + +For my individual portfolio I picked the **Selection Tool** (`org.jhotdraw.draw.tool.SelectionTool`, in `jhotdraw-core`). It is the tool the user is in when they interact directly with the canvas to pick figures and move them around, so it is the most user-facing piece of behaviour I could attach a clear story to. + +## Story + +> **As a** person drawing a diagram in the SVG editor, +> **I want** to select and manipulate the figures I have already placed on the canvas, +> **so that** I can move, resize and rearrange my drawing without redrawing it from scratch. + +This is tracked on the team's GitHub backlog (we keep the backlog as GitHub issues) as **issue #12**, labelled **`user story`**, currently sitting in the **TODO** column: https://github.com/timadam03/JHotDraw/issues/12 + +## Acceptance criteria + +These line up one-to-one with the behaviours I later drove out in the BDD scenarios (US1-US5). + +| # | Given / When | Then | +|---|--------------|------| +| US1 | A selectable figure exists and I click on it | The figure becomes the drag target and I can drag to move it (`DragTracker`) | +| US2 | I click on an empty part of the canvas | The current selection is cleared | +| US3 | I click directly on a handle of a selected figure | I can drag that handle to resize/transform the figure (`HandleTracker`) | +| US4 | I press and drag starting on empty canvas | A rubber-band selection rectangle is drawn over the area (`SelectAreaTracker`) | +| US5 | Two figures overlap and I hold Alt/Ctrl while clicking | The figure *behind* the front one is selected (select-behind) | + +## Notes / scope + +- The story is about the existing user-visible behaviour; I am not adding a new feature here. The selection of this story is what frames the later refactoring (branch `altan/develop`) and the unit + BDD tests on the same feature. +- "Selectable" follows `Figure.isSelectable()`; deselect and select-behind only apply when the view is active, so the criteria assume an enabled `DrawingView`. +- US5 is gated by the bound `selectBehindEnabled` property (default on), which is why the modifier-key behaviour is listed as a criterion rather than always-on. diff --git a/Labs/L3-ImpactAnalysis.md b/Labs/L3-ImpactAnalysis.md new file mode 100644 index 000000000..926417954 --- /dev/null +++ b/Labs/L3-ImpactAnalysis.md @@ -0,0 +1,51 @@ +# Lab 3 — Impact Analysis: SelectionTool + +## Starting point + +Concept location (Lab 2) ended on `org.jhotdraw.draw.tool.SelectionTool` as the class that implements the Selection Tool feature. Following the static + dynamic impact analysis process in Figure 7.9 (Rajlich), I mark `SelectionTool` as **CHANGED**, mark all of its direct neighbours **NEXT**, and then walk the graph: each time I take a class off the NEXT set I decide whether a change to `SelectionTool` would actually force a change in it (**CHANGED**), whether it would only relay the change further (**PROPAGATES**), or whether it would be left **UNCHANGED**. New neighbours of a CHANGED/PROPAGATES class get added to NEXT. I stop when NEXT is empty. + +I built the initial neighbour set from the imports and field/parameter types in `SelectionTool.java` plus the call sites I traced while reading `mousePressed`, `resolveTracker`, `findTargetFigure`, `findFigureBehindCurrentSelection`, `findFigureAtPoint` and `setTracker`. I sanity-checked the reverse direction with a `grep -rl "new SelectionTool"` / `extends SelectionTool` to see who depends *on* the tool — only `DelegationSelectionTool` and the editor wiring do, and neither is touched by the refactor. + +## Walking the graph + +Starting from `SelectionTool` (**CHANGED**), the first ring of NEXT nodes is: `AbstractTool`, `Tool`, the three tracker interfaces (`DragTracker`, `HandleTracker`, `SelectAreaTracker`) and their three default impls (`DefaultDragTracker`, `DefaultHandleTracker`, `DefaultSelectAreaTracker`), then `DrawingView`, `Drawing`, `DrawingEditor`, `Figure`, `Handle`, and the event plumbing `ToolEvent` / `ToolAdapter` / `ToolListener`. + +**`AbstractTool` (superclass) — UNCHANGED.** `SelectionTool` calls `super.mousePressed`, `getView`, `getEditor`, `firePropertyChange`, `fireToolDone`, `fireAreaInvalidated`, `fireBoundsInvalidated` and reads `anchor`. All of these are existing protected members; the refactor (commits `8243c188` and `35f97d94`) only re-organised code *inside* `SelectionTool` and added private helpers. It never changed how the superclass is called, so nothing propagates upward. + +**`Tool` interface — UNCHANGED.** The local field `tracker` is typed as `Tool` and `resolveTracker`/`setTracker` pass `Tool` around, but I never added a method to the interface — `setTracker` still uses `activate`, `deactivate`, `mousePressed`, `addToolListener`, `removeToolListener`, all pre-existing. Visited, left alone. + +**`DragTracker`, `HandleTracker`, `SelectAreaTracker` interfaces — UNCHANGED.** These are the State roles in the Strategy pattern. `getDragTracker`, `getHandleTracker`, `getSelectAreaTracker` still call `setDraggedFigure`, `setHandles`, etc. The whole point of the refactor was to keep their contracts intact, so a change to `SelectionTool` does not force a change here. They are the boundary where the walk stops on the tracker side. + +**`DefaultDragTracker`, `DefaultHandleTracker`, `DefaultSelectAreaTracker` — UNCHANGED.** Lazily instantiated by the `getXxxTracker` methods exactly as before. `DefaultHandleTracker` is mentioned in a Javadoc note about keeping the figure-search order consistent; I checked that order in `findFigureAtPoint` and it matches, so the note is honoured and the class does not need editing. + +**`DrawingView` — UNCHANGED (interface), PROPAGATES at most.** This is the busiest collaborator: `findHandle`, `findFigure`, `viewToDrawing`, `getDrawing`, `getSelectedFigures`, `getCompatibleHandles`, `clearSelection`, `setHandleDetailLevel`, `isEnabled`. Every one of these is an existing method on the `DrawingView` interface. The refactor moved the calls into helper methods but kept the same calls with the same arguments, so the interface is visited and left UNCHANGED. + +**`Drawing` — UNCHANGED.** Reached through `view.getDrawing()` inside `findFigureBehindCurrentSelection` and `findFigureAtPoint`, then `drawing.findFigureBehind(...)`. Pre-existing method, unchanged signature. Visited, left alone. + +**`DrawingEditor` — UNCHANGED.** Only used as the activate/deactivate context (`activate(DrawingEditor)`, `deactivate(DrawingEditor)`, `getEditor()`). No change. + +**`Figure` — UNCHANGED.** The tool reads `isSelectable()` and `contains(Point2D.Double)`. Both already existed; the select-behind loop and the "prefer current selection" loop use them as before. Visited, left alone. + +**`Handle` — UNCHANGED.** Returned by `view.findHandle` and passed into `getHandleTracker`. The tool only holds and forwards it; no member of `Handle` is touched. + +**`ToolEvent`, `ToolAdapter`, `ToolListener` — UNCHANGED.** The inner class `TrackerHandler extends ToolAdapter` overrides `toolDone`, `areaInvalidated`, `boundsInvalidated` and reads `ToolEvent.getInvalidatedArea()`. These overrides and that accessor are unchanged by the refactor, and `ToolListener` is only used through `addToolListener`/`removeToolListener`. Event plumbing is visited and left alone. + +After processing all of the above, NEXT is empty and the walk terminates. + +## Conclusion of the walk + +The change is contained inside the **single CHANGED class, `SelectionTool`**. Every neighbour I visited was either a collaborator interface (`Tool`, the three tracker interfaces, `DrawingView`, `Drawing`, `DrawingEditor`, `Figure`, `Handle`) or a class whose contract the refactor deliberately preserved (`AbstractTool`, the three `Default*` trackers, the three event classes). None of them had to change, because both refactor commits only restructured `SelectionTool`'s own body — extracting `findTargetFigure`/`resolveTracker`/`isViewActive`/`setTracker` (`8243c188`) and then splitting `findTargetFigure` into `isSelectBehindModifierHeld`/`findFigureBehindCurrentSelection`/`findFigureAtPoint` (`35f97d94`) — while keeping every external call unchanged. + +## Table 1 — Packages visited during impact analysis + +| Package name | # of classes | Comments | +|---|---|---| +| `org.jhotdraw.draw.tool` | 20 | Home package of the feature; holds `SelectionTool` (CHANGED) plus `Tool`/`AbstractTool` and the three tracker interfaces + three `Default*` impls (all visited, UNCHANGED). | +| `org.jhotdraw.draw` | 23 | Provides the core collaborators `DrawingView`, `Drawing`, `DrawingEditor` that the tool queries and mutates; visited, UNCHANGED. | +| `org.jhotdraw.draw.figure` | 27 | Supplies `Figure` (`isSelectable`, `contains`), the objects the tool selects and drags; visited, UNCHANGED. | +| `org.jhotdraw.draw.handle` | 26 | Supplies `Handle`, the draggable control point returned by `findHandle` and fed to the handle tracker; visited, UNCHANGED. | +| `org.jhotdraw.draw.event` | 26 | Event plumbing — `ToolEvent`, `ToolAdapter`, `ToolListener` used by the inner `TrackerHandler` to relay tool events; visited, UNCHANGED. | + +## Estimated impact set + +**{ `SelectionTool` }** — the only CHANGED class. All other classes in the five packages above were visited during the walk and marked UNCHANGED, because the refactor preserved the contracts of the interfaces (`Tool`, `DragTracker`, `HandleTracker`, `SelectAreaTracker`, `DrawingView`, `Drawing`, `DrawingEditor`, `Figure`, `Handle`) and of the collaborating classes (`AbstractTool`, the `Default*` trackers, the event classes). The test code (`SelectionToolTest`, the JGiven BDD stages) is also affected as a consumer, but it sits in `src/test` and follows the change rather than being part of the production impact set. diff --git a/Labs/L4-Refactoring.md b/Labs/L4-Refactoring.md new file mode 100644 index 000000000..7838a5708 --- /dev/null +++ b/Labs/L4-Refactoring.md @@ -0,0 +1,59 @@ +# Lab 4 — Refactoring the Selection Tool + +The feature I have been working on all semester is `org.jhotdraw.draw.tool.SelectionTool` (module `jhotdraw-core`). It extends `AbstractTool` and is the tool the user drives on the canvas: it decides, on every mouse press, whether the user is grabbing a handle, dragging a figure, or rubber-banding a selection rectangle, and delegates the rest of the gesture to one of three tracker strategies (`HandleTracker`, `DragTracker`, `SelectAreaTracker`). + +This lab covers the smells I found in that class, the plan I drew up, and the two behaviour-preserving commits I used to clean it up: `8243c188` ("Refactor SelectionTool: simplify tracker logic") and `35f97d94` ("Refactor SelectionTool target-figure lookup"). + +## How I found the smells + +I read the class top to bottom first, then ran SonarLint over it in the IDE. SonarLint is an IDE plugin, so there is no ruleset committed to the repo — the warnings live in the editor, not in CI. The two it kept raising were: + +- **Cognitive Complexity too high** on `mousePressed`. The method nested an `if/else` (handle vs. no handle) inside another `if/else` (select-behind vs. normal lookup), with two `while` loops and a `for` loop underneath, plus a final tracker-swap block. It was around 80 lines. +- **Duplicated blocks** across the key/mouse handlers — the `getView() != null && getView().isEnabled()` guard appeared verbatim in seven methods. + +My own reading flagged the same three things, which I list below. + +## Code smells (before) + +| # | Smell (Fowler) | Where | Symptom | +|---|----------------|-------|---------| +| 1 | Long Method | `mousePressed(MouseEvent)` | One method did handle lookup, select-behind logic, figure lookup, tracker selection and tracker swapping all inline. | +| 2 | Duplicated Code | `keyPressed`, `keyReleased`, `keyTyped`, `mouseClicked`, `mouseDragged`, `mouseReleased`, `mousePressed` | The exact guard `getView() != null && getView().isEnabled()` was copy-pasted into seven event methods. | +| 3 | Complicated / nested Conditional | the target-figure lookup inside `mousePressed` | A two-branch conditional (select-behind vs. normal) each holding its own loop-driven hit-test, mixing the *decision* with the two *searches*. | + +There was also a smaller duplication: the tracker swap (deactivate old → assign → activate new → re-subscribe the listener) existed both in `mousePressed` and inside `TrackerHandler.toolDone`, with subtly different null handling. + +## Plan + +Work in small steps, each one behaviour-preserving and verified against the existing JUnit 4 / Mockito suite (`SelectionToolTest`) and the JGiven scenarios (`SelectionToolBDDTest`). The target shape is *Compose Method*: every method should read at one level of abstraction, so `mousePressed` becomes a short narrative of named steps and the detail moves into helpers. I split the work across two commits so each diff stayed reviewable. + +## Refactorings applied + +### Commit `8243c188` — simplify tracker logic + +- **Extract Method** (Fowler) on the duplicated guard. The seven copies of `getView() != null && getView().isEnabled()` collapsed into one private `isViewActive()`, and each handler now reads `if (isViewActive())`. This is *Consolidate Duplicate Conditional Fragments* applied across methods, realised through Extract Method (smell #2). +- **Extract Method** on the body of `mousePressed`. I pulled the figure search into `findTargetFigure(view, drawingPoint, evt)` and the tracker decision into `resolveTracker(handle, figure, evt)`. `resolveTracker` keeps the original priority order: handle → `getHandleTracker`; selectable figure → `getDragTracker`; otherwise clear the selection (unless Shift is held) and return `getSelectAreaTracker`. +- **Compose Method** (Kerievsky) on `mousePressed` itself. After the extractions it reads as: guard, `super.mousePressed`, `findHandle`, `findTargetFigure`, `resolveTracker`, `setTracker`, delegate. Roughly 80 lines down to about 8 (smell #1). +- **Extract Method** to remove the swap duplication: I centralised the deactivate/assign/activate/re-subscribe sequence in `setTracker(Tool)` and gave it a single null guard at the top (return early if `newTracker == null`). `TrackerHandler.toolDone` now just calls `setTracker(getSelectAreaTracker())` instead of repeating the swap. + +### Commit `35f97d94` — decompose the figure lookup + +- **Decompose Conditional** (Fowler) on `findTargetFigure`. The condition became `isSelectBehindModifierHeld(evt)`, and the two branches became `findFigureBehindCurrentSelection(view, drawingPoint)` and `findFigureAtPoint(view, drawingPoint)`. `findTargetFigure` is now a single ternary that names the decision and the two outcomes (smell #3): + + ```java + return isSelectBehindModifierHeld(evt) + ? findFigureBehindCurrentSelection(view, drawingPoint) + : findFigureAtPoint(view, drawingPoint); + ``` + +- The condition predicate (`isSelectBehindEnabled()` AND ALT/CTRL held) moved into `isSelectBehindModifierHeld`, so the modifier-mask bit-twiddling is no longer inline in the search. The original search order and select-behind semantics are preserved — I kept the comment that the sequence must stay consistent with `DefaultHandleTracker`, `DefaultSelectAreaTracker` and `DelegationSelectionTool`. + +## Strategy and verification + +Because `SelectionTool` is itself a Strategy host (it swaps trackers as states), I was careful that none of these refactorings touched the *observable* state machine — only the internal control flow. Every step was kept green: + +- After the Extract Method passes I re-ran `SelectionToolTest`. Its boundary section already covered the branches I was moving (null tracker, disabled view via `isViewActive`, and the handle/figure/empty arms of `resolveTracker`), so a regression in the extraction would have failed there immediately. +- The JGiven scenarios pinned the user-visible behaviour end to end: US1 (click selectable figure → `DragTracker`), US2 (click empty → selection cleared), US3 (click handle → `HandleTracker`), US4 (click empty → `SelectAreaTracker`), US5 (alt-click overlapping → figure behind). US5 in particular guards the `findFigureBehindCurrentSelection` path I split out in the second commit. +- CI (`.github/workflows/ci.yml`, `mvn clean install` on JDK 8) runs both suites on every PR with `enableAssertions=true`, so the invariant assertions (tracker never null, `selectBehindEnabled` defaults true, `supportsHandleInteraction` always true) also had to hold after each push. + +The net result: `mousePressed` now reads as a short sequence of intent-named calls, the view guard exists once, and the figure hit-test is three focused helpers (`isSelectBehindModifierHeld`, `findFigureBehindCurrentSelection`, `findFigureAtPoint`) instead of one nested block. SonarLint no longer flags cognitive complexity or duplicated blocks on the class. diff --git a/Labs/L5-Actualization.md b/Labs/L5-Actualization.md new file mode 100644 index 000000000..160d0e8d7 --- /dev/null +++ b/Labs/L5-Actualization.md @@ -0,0 +1,46 @@ +# Lab 5 — Actualization + +For this lab I went back over the Selection Tool work and checked it against the SOLID principles and Clean Architecture. My feature is `org.jhotdraw.draw.tool.SelectionTool` in `jhotdraw-core`, and the two refactoring commits I refer to are `8243c188` (simplify tracker logic) and `35f97d94` (decompose the target-figure lookup). + +## Part 1 — SOLID in the Selection Tool + +### Single Responsibility Principle + +The clearest example is the tracker split. `SelectionTool` is in one of three interaction modes — area selection, figure dragging, handle manipulation — and each mode is a separate class: `DefaultSelectAreaTracker`, `DefaultDragTracker`, `DefaultHandleTracker`. None of them knows about the other two. `SelectionTool` itself only decides *which* tracker is current and forwards events to it. + +My own refactors pushed SRP down to the method level. Before `35f97d94`, `findTargetFigure()` did three jobs in one body: check the modifier keys, walk behind the current selection, and find the topmost figure at the point. I split it into `isSelectBehindModifierHeld()` (lines 253-257), `findFigureBehindCurrentSelection()` (267-280) and `findFigureAtPoint()` (294-308). Each now does one thing, and `findTargetFigure()` (240-244) is a two-line router. The same idea drove `8243c188`: `resolveTracker()` only picks a tracker, `setTracker()` (332-343) only swaps one in and out, and `isViewActive()` (409-411) only answers whether the view is usable. + +### Open/Closed Principle + +The Strategy design lets me add new selection behaviour without touching `SelectionTool`. The three tracker roles are interfaces (`HandleTracker`, `DragTracker`, `SelectAreaTracker`), and the public setters `setHandleTracker()`, `setDragTracker()`, `setSelectAreaTracker()` (lines 384-402) let a caller inject a different implementation. `DelegationSelectionTool` in the SVG sample is the existing proof of this — it reuses `SelectionTool` and supplies its own behaviour rather than editing the base class. So the class is open for extension (new tracker impls) but closed for modification. + +### Liskov Substitution Principle + +Two substitutions hold here. First, the `Default*` trackers stand in wherever their interface is expected — `getDragTracker()` returns a `DragTracker`, and the field it is stored in (`tracker`) is typed as `Tool`, so the concrete type never leaks. Second, `SelectionTool extends AbstractTool` and is used everywhere a `Tool` is expected (the editor holds it as the `Tool` interface). My overrides keep the contract: `mousePressed`, `keyPressed`, etc. all behave as a `Tool` is expected to, just guarded by `isViewActive()` first. The unit tests lean on this — `TestableSelectionTool` is a subclass that I hand to code expecting a `SelectionTool`, and nothing downstream notices the difference. + +### Interface Segregation Principle + +The design uses several small interfaces instead of one fat `Tool` that does everything. `Tool` is the event-handling contract; `HandleTracker`, `DragTracker` and `SelectAreaTracker` each add only the one method their role needs (`setHandles`, `setDraggedFigure`, and nothing extra for the area tracker). `Handle` is its own focused interface for a draggable control point. Because the tracker interfaces are separate, a class that only drags figures never has to implement handle logic it does not use. + +### Dependency Inversion Principle + +`SelectionTool` depends on abstractions, not concretes, almost everywhere. The current tracker is held as `Tool`; collaborators come in as `DrawingView`, `Drawing`, `Figure`, `Handle` interfaces; the trackers it returns are typed by their interfaces. The only place it names a concrete class is the lazy default creation inside `getHandleTracker`/`getDragTracker`/`getSelectAreaTracker`, and the setters exist precisely so that default can be replaced. This is exactly what my tests exploit: in `SelectionToolTest` I inject Mockito mocks of `DrawingView`, `Drawing`, `Figure` and `Handle`, and BDD scenarios inject mock trackers through the setters, so I can verify routing without a real Swing canvas. + +| Principle | Where it shows up in the Selection Tool | +|-----------|------------------------------------------| +| SRP | One tracker per interaction mode; each private method (`isSelectBehindModifierHeld`, `findFigureAtPoint`, `setTracker`, `isViewActive`) does one job | +| OCP | Strategy trackers + `setHandleTracker`/`setDragTracker`/`setSelectAreaTracker` add behaviour without editing `SelectionTool`; `DelegationSelectionTool` proves it | +| LSP | `Default*` trackers substitute for their interfaces; `SelectionTool`/`TestableSelectionTool` substitute for `AbstractTool`/`Tool` | +| ISP | Small focused interfaces: `Tool`, `HandleTracker`, `DragTracker`, `SelectAreaTracker`, `Handle` | +| DIP | Depends on `Tool`/`*Tracker`/`DrawingView` abstractions; setters allow injecting mocks, which the tests use | + +## Part 2 — Clean Architecture in this case study + +The Maven module layout maps cleanly onto the Clean Architecture rings. + +- **Inner ring (domain / abstractions and logic):** `jhotdraw-api` and `jhotdraw-core`. This is where `Tool`, `DrawingView`, `Drawing`, `Figure`, `Handle`, the tracker interfaces, and `SelectionTool` itself live. These define *what* selection means with no knowledge of how the pixels get on screen. +- **Outer ring (frameworks, UI, wiring):** `jhotdraw-gui`, `jhotdraw-app`, and `jhotdraw-samples` (e.g. the runnable SVG demo `org.jhotdraw.samples.svg.Main`). This is where Swing components, application menus and the concrete editor wiring sit. + +Dependencies point inward. The UI and sample modules depend on the core abstractions; the core does not depend on them. Swing is at the very outer edge — `SelectionTool` consumes `java.awt.event.MouseEvent`/`KeyEvent`, but it never reaches up into a `JPanel` or an application window. It talks to the framework only through the `DrawingView`/`Drawing` interfaces defined in the inner ring, which is the dependency-inversion boundary Clean Architecture asks for. + +This is the link to actualization. Every change I made — extracting `findTargetFigure`/`resolveTracker`, decomposing the figure lookup, centralising `setTracker` — stayed entirely inside `jhotdraw-core`. I never had to open a GUI or app module, and nothing in `jhotdraw-gui`/`jhotdraw-app` needed editing to keep the build green. The dependency direction is what makes that possible: because the UI depends on the core and not the other way round, I could refactor and re-test the core tool logic in isolation, with mocks standing in for the outer ring, and the CI build (`mvn clean install` on JDK 8) confirmed the outer modules still compiled against the unchanged interfaces. diff --git a/Labs/L7-Testing.md b/Labs/L7-Testing.md new file mode 100644 index 000000000..cd574862c --- /dev/null +++ b/Labs/L7-Testing.md @@ -0,0 +1,80 @@ +# Lab 7 — Testing the Selection Tool + +This lab covers how I verified `org.jhotdraw.draw.tool.SelectionTool` with unit tests. After the two refactors on `altan/develop` (8243c188 and 35f97d94) I had small, named methods to target, so I wrote a JUnit 4 + Mockito suite that exercises them without ever opening a Swing window. + +## Test setup in the pom + +I added the test dependencies to `jhotdraw-core/pom.xml`. For this lab the ones that matter are JUnit `4.13.2` and `mockito-core` `3.12.4` (the JGiven/AssertJ entries are for the BDD suite in Lab 8). I also reconfigured surefire `3.2.5`: + +```xml + + true + + --add-opens java.base/java.lang=ALL-UNNAMED + --add-opens java.base/java.lang.reflect=ALL-UNNAMED + + +``` + +The `--add-opens` lines are needed because Mockito uses reflection to build its mocks. `enableAssertions=true` is the important one for the invariant section below — by default the JVM runs with `assert` disabled, so without this flag those checks would silently pass and test nothing. + +## Mocking the collaborators + +`SelectionTool` talks to `DrawingEditor`, `DrawingView`, `Drawing`, `Figure` and `Handle`. All of these are interfaces in the framework, and in production they are backed by live Swing components. Driving a real canvas in a unit test would be slow and flaky, so in `setUp()` I mock all five with `Mockito.mock(...)` and wire just enough behaviour to get the tool running: + +- `mockEditor.getActiveView()` / `findView(...)` return `mockView` +- `mockView.isEnabled()` returns `true`, `getDrawing()` returns `mockDrawing` +- `viewToDrawing(Point)` returns a fixed `Point2D.Double(100, 100)` +- `findHandle(...)` and `findFigure(...)` return `null` by default, so each test only stubs the lookup it cares about + +This lets me assert on *which tracker the tool picks* purely at the domain level — no `JFrame`, no event-dispatch thread. + +To reach the `protected` factory and swap methods I added a small `TestableSelectionTool` subclass that exposes `getSelectAreaTracker()`, `getDragTracker(Figure)`, `getHandleTracker(Handle)` and `setTracker(Tool)`. + +## The three sections of `SelectionToolTest` + +I split the ~20 tests into three labelled groups. + +| Section | What it checks | Example tests | +|---|---|---| +| A. Best-case | Normal behaviour with valid inputs: property accessors, property-change firing, lazy creation of trackers, tracker activate/deactivate delegation, tracker swapping | `testDefaultSelectBehindEnabled`, `testGetSelectAreaTrackerLazyInit`, `testSetTrackerSwapsTrackers` | +| B. Boundary | Edge inputs and each branch of `resolveTracker`: null tracker, disabled view, the handle / selectable-figure / empty-area branches, a no-op property set | `testSetTrackerWithNull`, `testMousePressedWithDisabledView`, `testResolveTrackerWith*`, `testSetSelectBehindEnabledNoChangeDoesNotFire` | +| C. Invariant | Properties that must always hold on any instance, asserted with the Java `assert` keyword | `testInvariantTrackerNeverNull`, `testInvariantSelectBehindEnabledDefaultTrue`, `testInvariantSupportsHandleInteractionAlwaysTrue` | + +### A — best case + +These confirm the tool behaves under normal use. `testSelectBehindEnabledFiresPropertyChange` registers a mock `PropertyChangeListener`, flips `selectBehindEnabled` to `false`, and verifies the listener fired. The lazy-init tests call a factory twice and use `assertSame(first, second)` to prove the `Default*` tracker is created once and cached. `testSetTrackerSwapsTrackers` injects two mock `Tool`s in sequence and verifies the old one got `deactivate(editor)` and the new one `activate(editor)` — the centralised swap logic from 8243c188. + +### B — boundary and branch coverage + +The core of `mousePressed` is `resolveTracker(handle, figure, evt)`, which has three exits. I cover each one by stubbing the view's lookups and then asserting the matching tracker type is created: + +- **Handle branch** — `findHandle(...)` returns `mockHandle`, expect a `HandleTracker` (`testResolveTrackerWithHandleReturnsHandleTracker`). +- **Selectable figure branch** — `findHandle` returns `null`, `findFigure` returns a `mockFigure` with `isSelectable()` true, expect a `DragTracker`. +- **Empty area branch** — both lookups return `null`, expect a `SelectAreaTracker`. + +The edge cases: + +- `testSetTrackerWithNull` passes `null` into `setTracker`; the null guard added in 8243c188 means it is a no-op, no `NullPointerException`, and the tool still deactivates cleanly. +- `testMousePressedWithDisabledView` stubs `isEnabled()` to `false`, then verifies the injected tracker's `mousePressed` is **never** called. This pins down the `isViewActive()` guard that the same commit pulled out of seven event handlers. +- `testSetSelectBehindEnabledNoChangeDoesNotFire` sets the property to its current value and uses `verify(listener, never())` — `PropertyChangeSupport` skips the event when old equals new, and I wanted that documented as expected behaviour rather than a bug. + +When I first wrote the disabled-view test it failed because my `setUp()` default already stubbed `isEnabled()` to `true`; I had to re-stub it to `false` inside the test before `activate()`. A breakpoint in `isViewActive()` confirmed the guard was the thing short-circuiting `mousePressed`. + +### C — invariants with the `assert` keyword + +Three things should hold for any `SelectionTool` regardless of how it was constructed: the select-area tracker is never null, `selectBehindEnabled` defaults to `true`, and `supportsHandleInteraction()` is always `true`. I express these with the Java `assert` keyword on a freshly constructed instance, e.g.: + +```java +assert freshTool.isSelectBehindEnabled() : "selectBehindEnabled must be true by default"; +``` + +This is why `enableAssertions=true` in surefire matters — I checked that it actually runs by temporarily breaking one invariant and watching the test go red; with assertions disabled it had been passing regardless. I kept a parallel `assertTrue(...)` alongside each `assert` so the test still fails loudly even if someone runs it without `-ea`. + +## How to run + +``` +mvn -pl jhotdraw-core test +``` + +The suite also runs automatically in CI: `.github/workflows/ci.yml` executes `mvn clean install -s .maven-settings.xml` on JDK 8 for every pull request, so both the unit tests and the BDD scenarios from the next lab gate every merge into `develop`. diff --git a/Labs/L8-BDD.md b/Labs/L8-BDD.md new file mode 100644 index 000000000..864497e99 --- /dev/null +++ b/Labs/L8-BDD.md @@ -0,0 +1,59 @@ +# Lab 8 — Behaviour-Driven Development for the Selection Tool + +I kept working on `org.jhotdraw.draw.tool.SelectionTool` (module `jhotdraw-core`). For this lab I turned the user stories I had already captured on the team backlog (tracked as GitHub issue #12) into executable Given-When-Then scenarios and automated them with JGiven, so the behaviour the user expects on the canvas is checked on every CI run. + +## User stories mapped to scenarios + +| User story | Given | When | Then | +|---|---|---|---| +| US1 — click a selectable figure to select it | a drawing with a selectable figure | the user clicks the figure | the `DragTracker` is activated | +| US2 — click empty area to deselect | an empty drawing area | the user presses on an empty area | the current selection is cleared | +| US3 — grab a figure's handle to manipulate it | a drawing with a handle at the click point | the user presses on the handle | the `HandleTracker` is activated | +| US4 — rubber-band select an area | an empty drawing area | the user presses on an empty area | the `SelectAreaTracker` is activated | +| US5 — alt-click overlapping figures to reach the one behind | overlapping figures with select-behind enabled | the user clicks with the Alt modifier | the `DragTracker` is activated on the figure behind | + +US2 and US4 share the same Given/When (an empty-area press) but assert two different consequences of that one gesture: the previous selection is dropped, and the tool switches to area selection. I left them as separate scenarios because they map to separate stories and read more clearly that way. + +Each row corresponds to one `@Test` in `jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java`, e.g. `clicking_on_a_selectable_figure_activates_drag_tracker()` reads: + +``` +given().a_drawing_with_a_selectable_figure(); +when().the_user_clicks_on_the_figure(); +then().the_DragTracker_is_activated(); +``` + +## JGiven structure + +The test class extends `ScenarioTest`, which gives me the `given()`, `when()`, `then()` factory methods. Each is backed by a stage class extending `com.tngtech.jgiven.Stage`: + +- **`GivenSelectionTool`** sets up the world. A `@BeforeStage` method (`setupMocks()`) builds the tool and the mocks; the step methods (`a_drawing_with_a_selectable_figure`, `a_drawing_with_a_handle_at_click_point`, `an_empty_drawing_area`, `a_disabled_drawing_view`, `overlapping_figures_with_select_behind_enabled`) stub the relevant `DrawingView`/`Drawing`/`Figure`/`Handle` behaviour, then `activate(mockEditor)`. +- **`WhenSelectionTool`** performs the gesture. It synthesises a `MouseEvent` and calls `tool.mousePressed(evt)`. `the_user_clicks_with_alt_modifier()` passes `InputEvent.ALT_DOWN_MASK` so `isSelectBehindModifierHeld(evt)` is true and the lookup goes through `findFigureBehindCurrentSelection` (US5). +- **`ThenSelectionTool`** asserts the outcome, e.g. `the_DragTracker_is_activated()`, `the_current_selection_is_cleared()`. + +Every step method returns `self()`, which is what lets the `given().a_drawing_with_a_selectable_figure()` calls chain fluently and keeps each scenario reading like a sentence. + +State flows between the three stages through scenario state, not method arguments. The Given fields (`tool`, `mockEditor`, `mockView`, `mockDrawing`, `mockFigure`, `mockHandle`, the dummy `JPanel`) are annotated `@ProvidedScenarioState`; the When and Then stages declare the same fields as `@ExpectedScenarioState` and JGiven injects the instances that the Given produced. The When stage also *provides* its own state — the `lastMouseEvent` and the two captured tracker instances (`firstTrackerInstance`, `secondTrackerInstance`, declared with `Resolution.NAME` so they're matched by field name rather than type, since both are `Tool`) — which the Then stage then consumes. + +## Assertions and test level + +The Then stage asserts with **AssertJ-core**, e.g. in `the_DragTracker_is_activated()`: + +```java +Tool tracker = tool.callGetDragTracker(mockFigure); +assertThat(tracker).isNotNull(); +assertThat(tracker).isInstanceOf(DragTracker.class); +``` + +For the side-effecting stories I verify against the Mockito mock instead — `the_current_selection_is_cleared()` does `verify(mockView).clearSelection()`, and the boundary scenario `no_tracker_action_is_performed()` does `verify(mockView, never()).findHandle(any(Point.class))` to confirm a disabled view short-circuits in `isViewActive()` before any tracker logic runs. + +I deliberately test at the domain level: the scenarios drive a real `SelectionTool` (a `TestableSelectionTool` subclass declared inside `GivenSelectionTool` that exposes the protected `getDragTracker`/`getHandleTracker`/`getSelectAreaTracker`/`setTracker` methods) wired to Mockito mocks of `DrawingEditor`, `DrawingView`, `Drawing`, `Figure` and `Handle`. I never start the live Swing GUI. This keeps the scenarios fast and deterministic — no event-dispatch thread, no real frame to pump — and it's exactly why I removed the unused `assertj-swing-junit` dependency from `jhotdraw-core/pom.xml`; I'm asserting on the tool's behaviour and its collaborator interactions, not pixels. + +## Extra scenarios beyond the stories + +Alongside the five story scenarios I added a few that protect non-functional expectations: + +- **Property change** — `setting_selectBehindEnabled_fires_property_change()` flips `selectBehindEnabled` and `then().selectBehindEnabled_is(false)` confirms the bound property updates. (The `the_property_change_event_is_fired()` step backed by `verify(mockListener)...` is in the Then stage for asserting the `PropertyChange` directly.) +- **Lazy-init invariant** — `select_area_tracker_lazy_init_returns_same_instance()`, plus the drag- and handle-tracker variants, request a tracker twice and assert `firstTrackerInstance` `isSameAs` `secondTrackerInstance`, confirming the `getXxxTracker()` lazy initialisers cache rather than recreate. +- **Handle-interaction invariant** — `selection_tool_always_supports_handle_interaction()` asserts `supportsHandleInteraction()` stays `true`. + +All of this runs under the existing CI job (`.github/workflows/ci.yml`, `mvn clean install -s .maven-settings.xml` on JDK 8), so the JGiven scenarios execute as ordinary JUnit tests on every pull request next to the unit suite in `SelectionToolTest.java`. diff --git a/Labs/README.md b/Labs/README.md new file mode 100644 index 000000000..b3ae612e0 --- /dev/null +++ b/Labs/README.md @@ -0,0 +1,19 @@ +# Portfolio — Software Maintenance + +Lab write-ups for my individual portfolio. The feature I worked on across all of the +labs is the **Selection Tool** (`org.jhotdraw.draw.tool.SelectionTool`, in `jhotdraw-core`). + +| Lab | Document | +|-----|----------| +| L2 — Concept Location | [L2-ConceptLocation.md](L2-ConceptLocation.md) | +| L2 — Change Request / User Story | [L2-UserStory.md](L2-UserStory.md) | +| L3 — Impact Analysis | [L3-ImpactAnalysis.md](L3-ImpactAnalysis.md) | +| L4 — Refactoring | [L4-Refactoring.md](L4-Refactoring.md) | +| L5 — Actualization (SOLID / Clean Architecture) | [L5-Actualization.md](L5-Actualization.md) | +| L7 — Unit Testing | [L7-Testing.md](L7-Testing.md) | +| L8 — Behaviour-Driven Development | [L8-BDD.md](L8-BDD.md) | + +The user story is tracked on the backlog as issue +[#12](https://github.com/timadam03/JHotDraw/issues/12). The unit tests +(`SelectionToolTest`) and the JGiven scenarios (`bdd/`) live under +`jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/` and run in CI on every pull request. diff --git a/jhotdraw-core/pom.xml b/jhotdraw-core/pom.xml index 4bb7a1893..49a65cffb 100644 --- a/jhotdraw-core/pom.xml +++ b/jhotdraw-core/pom.xml @@ -59,12 +59,6 @@ 3.24.2 test - - org.assertj - assertj-swing-junit - 3.17.1 - test - ${project.groupId} jhotdraw-actions @@ -78,6 +72,7 @@ maven-surefire-plugin 3.2.5 + true --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.reflect=ALL-UNNAMED diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java index 090e33815..b46c0d911 100644 --- a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/SelectionToolBDDTest.java @@ -18,6 +18,15 @@ public void clicking_on_a_selectable_figure_activates_drag_tracker() { then().the_DragTracker_is_activated(); } + // US2: As a drawing user, I want clicking an empty area to clear my current selection, + // so that I can deselect figures + @Test + public void clicking_on_empty_area_clears_the_current_selection() { + given().an_empty_drawing_area(); + when().the_user_presses_mouse_on_empty_area(); + then().the_current_selection_is_cleared(); + } + // US3: As a drawing user, I want to manipulate figure handles @Test public void clicking_on_a_handle_activates_handle_tracker() { diff --git a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java index 6a0d98376..ee12acde5 100644 --- a/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java +++ b/jhotdraw-core/src/test/java/org/jhotdraw/draw/tool/bdd/ThenSelectionTool.java @@ -77,6 +77,11 @@ public ThenSelectionTool no_tracker_action_is_performed() { return self(); } + public ThenSelectionTool the_current_selection_is_cleared() { + verify(mockView).clearSelection(); + return self(); + } + public ThenSelectionTool the_property_change_event_is_fired() { verify(mockListener).propertyChange(any(PropertyChangeEvent.class)); return self(); From 74901a2817e7f927015d9fb1ad2f5eb78d7dd4bc Mon Sep 17 00:00:00 2001 From: AltanEsmer Date: Thu, 4 Jun 2026 18:12:17 +0200 Subject: [PATCH 8/9] Downgrade jgiven-junit to 0.18.2 for JDK 8 CI jgiven-junit 1.3.1 is compiled for Java 11 (class-file major version 55) and cannot be loaded by the JDK 8 build, which broke testCompile. 0.18.2 is the last Java 7/8-compatible release and keeps the same ScenarioTest / Stage / scenario-state API the BDD scenarios use. --- jhotdraw-core/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jhotdraw-core/pom.xml b/jhotdraw-core/pom.xml index 49a65cffb..e5cecd0e2 100644 --- a/jhotdraw-core/pom.xml +++ b/jhotdraw-core/pom.xml @@ -50,7 +50,7 @@ com.tngtech.jgiven jgiven-junit - 1.3.1 + 0.18.2 test From b755b47c68bc5ff40865879d6f82572fdabb73ce Mon Sep 17 00:00:00 2001 From: AltanEsmer Date: Thu, 4 Jun 2026 18:15:33 +0200 Subject: [PATCH 9/9] Scope Mockito --add-opens flags to a JDK 9+ profile The surefire --add-opens options are Java 9+ only; on the JDK 8 CI they crash the forked test VM ("Could not create the Java Virtual Machine"). Move them into a profile activated on JDK [9,) so JDK 8 runs the tests without them (no module system, so they aren't needed there) while local newer-JDK runs still get them for Mockito. --- jhotdraw-core/pom.xml | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/jhotdraw-core/pom.xml b/jhotdraw-core/pom.xml index e5cecd0e2..00b229e40 100644 --- a/jhotdraw-core/pom.xml +++ b/jhotdraw-core/pom.xml @@ -73,12 +73,32 @@ 3.2.5 true - - --add-opens java.base/java.lang=ALL-UNNAMED - --add-opens java.base/java.lang.reflect=ALL-UNNAMED - + + + + surefire-jdk9plus + + [9,) + + + + + org.apache.maven.plugins + maven-surefire-plugin + + + --add-opens java.base/java.lang=ALL-UNNAMED + --add-opens java.base/java.lang.reflect=ALL-UNNAMED + + + + + + + \ No newline at end of file