fix scheduling, threading and movement bugs

scheduling:
- cursor mover created a new java.util.timer for every movement and never
  cancelled it, leaking a live thread roughly every 10 seconds; timers are
  replaced by a single shared scheduledexecutorservice with daemon threads,
  shut down when the automation stops.

clicks:
- the double click sequence used thread.sleep() inside a timer task, blocking
  the scheduler thread; it is now scheduled as one-shot actions on the shared
  scheduler, with the same pacing as before.
- stopping the automation now also cancels in-flight movements and pending
  clicks, instead of letting them run to completion.

movement math:
- when the destination shared one coordinate with the starting position, the
  step on the other axis was computed as length/0 = infinity, and the stop
  condition checked one axis with || instead of &&, so a movement could stop
  (and click) before reaching its destination. steps are now guarded and the
  movement stops only when both axes have converged.

ui and lifecycle:
- the app could not be closed: closing the window only hid it, and ESC
  unregistered the hotkey without exiting. the window now exits cleanly on
  close, stopping the automation and unregistering the native hook.
- toggleRunning() was called from the jnativehook thread and touched swing
  components off the EDT; UI updates are now marshalled to the EDT.
- the hotkey thrashed: key repeats re-fired the toggle while ctrl+alt was
  held, and any other key pressed with the combo held toggled it too. it now
  fires exactly once, when the combo is completed.

away-detection (CursorMoveListener) stays intentionally unscheduled: it cannot
tell the user's movements apart from the app's own, and will be reworked in a
future iteration.

src/main/java/wtf/beatrice/autosqueal/Main.java

@@ -7,6 +7,8 @@ import org.apache.logging.log4j.Logger;
 import wtf.beatrice.autosqueal.listener.KeyPressListener;
 import wtf.beatrice.autosqueal.ui.MainWindow;
 
+import javax.swing.*;
+
 public class Main {
 
     private static final Logger LOGGER = LogManager.getLogger(Main.class);
@@ -16,7 +18,9 @@ public class Main {
         LOGGER.info("Hello world!");
 
         registerJNativeHook();
-        mainWindow.init();
+
+        // all Swing components must be created and updated on the EDT
+        SwingUtilities.invokeLater(mainWindow::init);
     }
 
     private static void registerJNativeHook() {
@@ -34,8 +38,19 @@ public class Main {
         }
     }
 
+    public static void unregisterJNativeHook() {
+        try {
+            GlobalScreen.unregisterNativeHook();
+            LOGGER.info("Successfully unregistered jnativehook library!");
+        }
+        catch (NativeHookException ex) {
+            LOGGER.error("There was a problem unregistering the native hook.");
+            LOGGER.error(ex.getMessage());
+        }
+    }
+
     public static MainWindow getMainWindow() {
         return mainWindow;
     }
 
-}
+}

src/main/java/wtf/beatrice/autosqueal/controls/CursorMover.java

@@ -7,66 +7,61 @@ import wtf.beatrice.autosqueal.util.RunnerUtil;
 import java.awt.*;
 import java.security.SecureRandom;
 import java.util.Random;
-import java.util.Timer;
-import java.util.TimerTask;
+import java.util.concurrent.ScheduledExecutorService;
 
-public class CursorMover extends TimerTask
+/**
+ * Periodically starts a new movement of the mouse cursor to a random position,
+ * or to the top-right corner of the screen for a double click.
+ *
+ * The movement itself is executed by a {@link SingleStepMovementTask}, which
+ * re-schedules itself on the shared scheduler until it reaches its destination.
+ */
+public class CursorMover implements Runnable
 {
-
     private static final Logger LOGGER = LogManager.getLogger(CursorMover.class);
-    private final Random random;
 
     private static final int LOOPS_BEFORE_CLICK = 5;
 
+    private final Random random = new SecureRandom();
+    private final ScheduledExecutorService scheduler;
+
     private int iteration = 0;
 
-    public CursorMover() {
-        random = new SecureRandom();
+    public CursorMover(ScheduledExecutorService scheduler) {
+        this.scheduler = scheduler;
     }
 
     @Override
     public void run() {
 
-        int currentX = MouseInfo.getPointerInfo().getLocation().x;
-        int currentY = MouseInfo.getPointerInfo().getLocation().y;
-
-        LOGGER.info("Starting coordinates: {}, {}", currentX, currentY);
+        Point location = MouseInfo.getPointerInfo().getLocation();
+        LOGGER.info("Starting coordinates: {}, {}", location.x, location.y);
 
         int destX;
         int destY;
-
-        SingleStepMovementTask singleStepMovementTask;
+        boolean click;
 
         if (iteration == LOOPS_BEFORE_CLICK) {
             destX = RunnerUtil.SCREEN_WIDTH - 5;
             destY = 5;
-
-            try {
-                singleStepMovementTask = new SingleStepMovementTask(destX, destY, true);
-            } catch (AWTException e) {
-                LOGGER.error(e);
-                return;
-            }
+            click = true;
 
             iteration = 0;
         } else {
             destX = random.nextInt(RunnerUtil.SCREEN_WIDTH);
             destY = random.nextInt(RunnerUtil.SCREEN_HEIGHT);
-
-            try {
-                singleStepMovementTask = new SingleStepMovementTask(destX, destY, false);
-            } catch (AWTException e) {
-                LOGGER.error(e);
-                return;
-            }
+            click = false;
 
             iteration++;
         }
 
         LOGGER.info("Destination coordinates: {}, {}", destX, destY);
 
-        Timer timer = new Timer();
-        timer.schedule(singleStepMovementTask, 0L, 2L);
-
+        try {
+            SingleStepMovementTask movement = new SingleStepMovementTask(scheduler, destX, destY, click);
+            scheduler.execute(movement);
+        } catch (AWTException ex) {
+            LOGGER.error("Could not start movement task", ex);
+        }
     }
-}
+}

src/main/java/wtf/beatrice/autosqueal/controls/SingleStepMovementTask.java

@@ -5,113 +5,163 @@ import org.apache.logging.log4j.Logger;
 
 import java.awt.*;
 import java.awt.event.InputEvent;
-import java.util.TimerTask;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 
-public class SingleStepMovementTask extends TimerTask {
+/**
+ * Moves the mouse cursor towards a destination one small step at a time,
+ * re-scheduling itself on the shared scheduler until the destination is reached.
+ *
+ * When it gets there, it can optionally perform a double click, scheduled as
+ * separate one-shot actions so that the scheduler is never blocked.
+ */
+public class SingleStepMovementTask implements Runnable {
 
     private static final Logger LOGGER = LogManager.getLogger(SingleStepMovementTask.class);
 
-    final int destX;
-    final int destY;
-    final Robot robot;
+    /** Delay between two consecutive cursor steps, in milliseconds. */
+    private static final long STEP_DELAY_MILLISECONDS = 2L;
 
-    float currentX;
-    float currentY;
+    private final ScheduledExecutorService scheduler;
+    private final Robot robot;
+    private final int destX;
+    private final int destY;
+    private final boolean click;
 
-    float stepX = 1;
-    float stepY = 1;
-    boolean isRunning = true;
-    boolean click;
+    private float currentX;
+    private float currentY;
+    private float stepX;
+    private float stepY;
 
+    public SingleStepMovementTask(ScheduledExecutorService scheduler, int destinationX, int destinationY, boolean click) throws AWTException {
 
-    public SingleStepMovementTask(int destinationX, int destinationY, boolean click) throws AWTException {
-
+        this.scheduler = scheduler;
+        this.destX = destinationX;
+        this.destY = destinationY;
         this.click = click;
 
-        currentX = MouseInfo.getPointerInfo().getLocation().x;
-        currentY = MouseInfo.getPointerInfo().getLocation().y;
+        Point location = MouseInfo.getPointerInfo().getLocation();
+        this.currentX = location.x;
+        this.currentY = location.y;
 
-        destX = destinationX;
-        destY = destinationY;
+        int lengthX = Math.round(Math.abs(currentX - destX));
+        int lengthY = Math.round(Math.abs(currentY - destY));
 
-        int lengthX;
-        int lengthY;
-
-        lengthX = Math.round(Math.abs(currentX - destX));
-        lengthY = Math.round(Math.abs(currentY - destY));
-
-        if(lengthX > lengthY) {
-            stepX = lengthX / (float) lengthY;
-        }
-
-        if(lengthY > lengthX) {
-            stepY = lengthY / (float) lengthX;
-        }
+        this.stepX = computeStepX(lengthX, lengthY);
+        this.stepY = computeStepY(lengthX, lengthY);
 
         LOGGER.info("Dest: [{}, {}], Curr: [{}, {}]", destX, destY, currentX, currentY);
-        LOGGER.info("Len: [{}, {}]", lengthX, lengthY);
         LOGGER.info("Step: [{}, {}]", stepX, stepY);
 
-        robot = new Robot();
-
+        this.robot = new Robot();
     }
 
     @Override
     public void run() {
-        if(Math.abs(currentX - destX) < 1) {
-            stepX = currentX - destX;
-        }
 
-        if(Math.abs(currentY - destY) < 1) {
-            stepY = currentY - destY;
-        }
-
-        if(destX == Math.round(currentX) || destY == Math.round(currentY)) {
-
-            if (click) {
-                try {
-                    Thread.sleep(500);
-                    robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
-                    Thread.sleep(200);
-                    robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
-                    Thread.sleep(500);
-                    robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
-                    Thread.sleep(200);
-                    robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
-                    Thread.sleep(200);
-                } catch (InterruptedException e) {
-                    LOGGER.error(e);
-                    Thread.currentThread().interrupt();
-                }
-            }
-
-            LOGGER.info("Reached destination, stopping mover timer");
-            LOGGER.info("Dest: [{}, {}], Curr: [{}, {}]", destX, destY, currentX, currentY);
-            isRunning = false;
-            this.cancel();
+        if (hasReachedDestination()) {
+            onDestinationReached();
             return;
         }
 
-        if(currentX > destX) {
-            currentX -= stepX;
-        }
+        // when less than a full step is left, move exactly what is left,
+        // so that both axes always land exactly on their destination
+        stepX = adjustedStep(currentX, destX, stepX);
+        stepY = adjustedStep(currentY, destY, stepY);
 
-        if(currentY > destY) {
-            currentY -= stepY;
-        }
-
-        if(currentX < destX) {
-            currentX += stepX;
-        }
-
-        if(currentY < destY) {
-            currentY += stepY;
-        }
+        currentX = advance(currentX, destX, stepX);
+        currentY = advance(currentY, destY, stepY);
 
         robot.mouseMove(Math.round(currentX), Math.round(currentY));
+
+        try {
+            scheduler.schedule(this, STEP_DELAY_MILLISECONDS, TimeUnit.MILLISECONDS);
+        } catch (RejectedExecutionException ex) {
+            // the scheduler was shut down mid-movement: the automation was
+            // stopped, so just end the movement chain here
+            LOGGER.debug("Movement interrupted: scheduler is shut down");
+        }
     }
 
-    public boolean isRunning() {
-        return isRunning;
+    private boolean hasReachedDestination() {
+        return Math.round(currentX) == destX && Math.round(currentY) == destY;
     }
-}
+
+    private void onDestinationReached() {
+        LOGGER.info("Reached destination [{}, {}], stopping mover", destX, destY);
+
+        if (click) {
+            scheduleClickSequence();
+        }
+    }
+
+    /**
+     * Schedules a double click with the same pacing the app has always used:
+     * press at +500ms, release at +700ms, press at +1200ms, release at +1400ms.
+     */
+    private void scheduleClickSequence() {
+        scheduleClick(500L, true);
+        scheduleClick(700L, false);
+        scheduleClick(1200L, true);
+        scheduleClick(1400L, false);
+    }
+
+    private void scheduleClick(long delayMillis, boolean press) {
+        scheduler.schedule(() -> {
+            if (press) {
+                robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
+            } else {
+                robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
+            }
+        }, delayMillis, TimeUnit.MILLISECONDS);
+    }
+
+    /**
+     * Step to take on the X axis so that both axes reach their destination
+     * after the same number of steps. Returns 0 when there is no movement to make.
+     */
+    static float computeStepX(int lengthX, int lengthY) {
+        if (lengthX == 0) {
+            return 0.0f;
+        }
+        if (lengthX >= lengthY) {
+            return 1.0f;
+        }
+        return lengthX / (float) lengthY;
+    }
+
+    /**
+     * Step to take on the Y axis so that both axes reach their destination
+     * after the same number of steps. Returns 0 when there is no movement to make.
+     */
+    static float computeStepY(int lengthX, int lengthY) {
+        if (lengthY == 0) {
+            return 0.0f;
+        }
+        if (lengthY > lengthX) {
+            return 1.0f;
+        }
+        return lengthY / (float) lengthX;
+    }
+
+    /**
+     * When less than a whole step is left to travel, move exactly what is left
+     * instead of a whole step, so that the cursor doesn't overshoot the destination.
+     */
+    static float adjustedStep(float current, float destination, float step) {
+        float remaining = Math.abs(current - destination);
+        return remaining < 1.0f ? remaining : step;
+    }
+
+    /** Moves current towards destination by step, in the right direction. */
+    static float advance(float current, float destination, float step) {
+        if (current > destination) {
+            return current - step;
+        }
+        if (current < destination) {
+            return current + step;
+        }
+        return current;
+    }
+}

src/main/java/wtf/beatrice/autosqueal/listener/KeyPressListener.java

@@ -1,7 +1,5 @@
 package wtf.beatrice.autosqueal.listener;
 
-import com.github.kwhat.jnativehook.GlobalScreen;
-import com.github.kwhat.jnativehook.NativeHookException;
 import com.github.kwhat.jnativehook.keyboard.NativeKeyEvent;
 import com.github.kwhat.jnativehook.keyboard.NativeKeyListener;
 import org.apache.logging.log4j.LogManager;
@@ -14,46 +12,40 @@ import java.util.List;
 public class KeyPressListener implements NativeKeyListener
 {
     private static final Logger LOGGER = LogManager.getLogger(KeyPressListener.class);
-    private static final List<Integer> pressedKeysIds = new ArrayList<>();
+    private final List<Integer> pressedKeys = new ArrayList<>();
 
     @Override
     public void nativeKeyPressed(NativeKeyEvent e) {
-        String key = NativeKeyEvent.getKeyText(e.getKeyCode());
-        LOGGER.info("Key Pressed: {}", key);
-
-        if (e.getKeyCode() == NativeKeyEvent.VC_ESCAPE) {
-            try {
-                GlobalScreen.unregisterNativeHook();
-            } catch (NativeHookException nativeHookException) {
-                LOGGER.error("Failed to unregister native hook", nativeHookException);
-            }
+        // the OS fires repeated "press" events while a key is being held down; ignore them,
+        // otherwise holding the toggle combo down would rapidly toggle the app on and off
+        if (pressedKeys.contains(e.getKeyCode())) {
+            return;
         }
 
-        pressedKeysIds.add(e.getKeyCode());
+        pressedKeys.add(e.getKeyCode());
+        LOGGER.info("Key Pressed: {}", NativeKeyEvent.getKeyText(e.getKeyCode()));
 
-        handlePressedKeys();
-    }
+        if (e.getKeyCode() == NativeKeyEvent.VC_ESCAPE) {
+            Main.unregisterJNativeHook();
+        }
 
-    @Override
-    public void nativeKeyReleased(NativeKeyEvent e) {
-        String key = NativeKeyEvent.getKeyText(e.getKeyCode());
-        LOGGER.info("Key Released: {}", key);
-
-        pressedKeysIds.remove((Integer) e.getKeyCode());
-
-        handlePressedKeys();
-    }
-
-    private void handlePressedKeys() {
-        if (pressedKeysIds.contains(NativeKeyEvent.VC_ALT) &&
-        pressedKeysIds.contains(NativeKeyEvent.VC_CONTROL)) {
-
-            StringBuilder keys = new StringBuilder();
-            pressedKeysIds.forEach(keyCode -> keys.append("[").append(NativeKeyEvent.getKeyText(keyCode)).append("]"));
-
-            LOGGER.warn("Received shutdown keystroke: {}", keys);
+        // toggle only when the second key of the combo is pressed down: not only does this
+        // fire exactly once per combo, but it also avoids toggling when any other key is
+        // pressed or released while ctrl+alt happen to be held down
+        if ((e.getKeyCode() == NativeKeyEvent.VC_CONTROL && pressedKeys.contains(NativeKeyEvent.VC_ALT))
+                || (e.getKeyCode() == NativeKeyEvent.VC_ALT && pressedKeys.contains(NativeKeyEvent.VC_CONTROL))) {
+            LOGGER.warn("Received shutdown keystroke: [{}][{}]",
+                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_CONTROL),
+                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_ALT));
 
             Main.getMainWindow().toggleRunning();
         }
     }
-}
+
+    @Override
+    public void nativeKeyReleased(NativeKeyEvent e) {
+        LOGGER.info("Key Released: {}", NativeKeyEvent.getKeyText(e.getKeyCode()));
+
+        pressedKeys.remove((Integer) e.getKeyCode());
+    }
+}

src/main/java/wtf/beatrice/autosqueal/ui/MainWindow.java

@@ -3,15 +3,19 @@ package wtf.beatrice.autosqueal.ui;
 import com.github.kwhat.jnativehook.keyboard.NativeKeyEvent;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import wtf.beatrice.autosqueal.Main;
 import wtf.beatrice.autosqueal.controls.CursorMover;
-import wtf.beatrice.autosqueal.listener.CursorMoveListener;
 import wtf.beatrice.autosqueal.util.RunnerUtil;
 import wtf.beatrice.autosqueal.util.SystemUtil;
 
 import javax.swing.*;
 import java.awt.*;
+import java.awt.event.WindowAdapter;
+import java.awt.event.WindowEvent;
 import java.awt.image.BufferedImage;
-import java.util.Timer;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 
 public class MainWindow
 {
@@ -20,22 +24,37 @@ public class MainWindow
     private static final int WINDOW_HEIGHT = 700;
     private static final int WINDOW_WIDTH = 800;
 
-    private Timer timerRunner = new Timer();
-    private CursorMover cursorMover = new CursorMover();
+    private static final long AUTOMATION_START_DELAY_SECONDS = 1L;
+
+    private final JFrame frame = new JFrame();
     private Button toggleButton;
 
+    private ScheduledExecutorService scheduler;
+    private CursorMover cursorMover;
+
+    /**
+     * Builds and shows the main window, and starts the automation.
+     * Must be called on the EDT.
+     */
     public void init() {
 
-        JFrame frame = new JFrame();
         frame.setSize(new Dimension(WINDOW_WIDTH, WINDOW_HEIGHT));
         frame.setTitle("autosqueal");
         frame.setResizable(false);
+        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
+        frame.addWindowListener(new WindowAdapter() {
+            @Override
+            public void windowClosing(WindowEvent e) {
+                LOGGER.info("Shutting down...");
+                stopAutomation();
+                Main.unregisterJNativeHook();
+            }
+        });
 
         toggleButton = new Button();
         toggleButton.setBounds(new Rectangle((WINDOW_WIDTH / 2) - 60, WINDOW_HEIGHT - 60, 120, 30));
         toggleButton.addActionListener(e -> toggleRunning());
         frame.add(toggleButton);
-        toggleRunning();
 
         int bordersPx = 10;
         int rescaleRateo = ((WINDOW_WIDTH - (2 * bordersPx)) * 100) / RunnerUtil.SCREEN_WIDTH;
@@ -50,9 +69,11 @@ public class MainWindow
         timestampLabel.setBounds(new Rectangle(bordersPx, bordersPx + rescaleHeight + bordersPx, 100, 30));
         frame.add(timestampLabel);
 
-
         frame.setLayout(null);
         frame.setVisible(true);
+
+        startAutomation();
+        updateToggleLabel();
     }
 
     private Image getScreenCapture(int rescaleWidth, int rescaleHeight) {
@@ -100,38 +121,61 @@ public class MainWindow
         return bimage;
     }
 
+    /**
+     * Toggles the automation on or off. Safe to call from any thread: the actual
+     * work is always marshalled to the EDT, since it touches Swing components.
+     */
     public void toggleRunning() {
-
-        String label;
-
-        if (cursorMover == null) {
-            timerRunner = new Timer();
-            CursorMoveListener cursorMoveListener = new CursorMoveListener();
-            timerRunner.schedule(cursorMoveListener, 0L, 1000L);
-
-            cursorMover = new CursorMover();
-            timerRunner.schedule(cursorMover, 1000L, RunnerUtil.SECONDS_BETWEEN_MOVES * 1000L);
-
-            label = "Stop [" +
-                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_CONTROL) +
-                    "][" +
-                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_ALT) +
-                    "]";
-        }
-        else {
-            timerRunner.cancel();
-
-            cursorMover = null;
-
-            label = "Start [" +
-                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_CONTROL) +
-                    "][" +
-                    NativeKeyEvent.getKeyText(NativeKeyEvent.VC_ALT) +
-                    "]";
-        }
-
-        toggleButton.setLabel(label);
-
+        SwingUtilities.invokeLater(() -> {
+            if (cursorMover == null) {
+                startAutomation();
+            } else {
+                stopAutomation();
+            }
+            updateToggleLabel();
+        });
     }
 
-}
+    private void startAutomation() {
+        LOGGER.info("Starting automation");
+
+        scheduler = Executors.newScheduledThreadPool(3, runnable -> {
+            Thread thread = new Thread(runnable, "autosqueal-scheduler");
+            thread.setDaemon(true);
+            return thread;
+        });
+        cursorMover = new CursorMover(scheduler);
+
+        // note: away-detection (CursorMoveListener) is intentionally not scheduled here.
+        // as it is, it cannot tell the user's movements apart from the movements the app
+        // itself makes, so it would always think the user is present. it will be reworked
+        // and re-enabled in a future iteration.
+        scheduler.scheduleWithFixedDelay(cursorMover,
+                AUTOMATION_START_DELAY_SECONDS,
+                RunnerUtil.SECONDS_BETWEEN_MOVES,
+                TimeUnit.SECONDS);
+    }
+
+    private void stopAutomation() {
+        LOGGER.info("Stopping automation");
+
+        if (scheduler != null) {
+            scheduler.shutdownNow();
+            scheduler = null;
+        }
+        cursorMover = null;
+    }
+
+    private void updateToggleLabel() {
+        if (toggleButton == null) {
+            // the window is not initialized yet
+            return;
+        }
+
+        String hotkey = "[" + NativeKeyEvent.getKeyText(NativeKeyEvent.VC_CONTROL) + "]"
+                + "[" + NativeKeyEvent.getKeyText(NativeKeyEvent.VC_ALT) + "]";
+
+        toggleButton.setLabel((cursorMover == null ? "Start " : "Stop ") + hotkey);
+    }
+
+}