-
Notifications
You must be signed in to change notification settings - Fork 0
Claude/fix active target mode 011 c uy l7 ny9 pfo gabvg1q lj w #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Claude/fix active target mode 011 c uy l7 ny9 pfo gabvg1q lj w #12
Conversation
Changes: - Added ClickTestWindow.swift with visual click targets for testing - Integrated test window into Advanced tab Developer Tools section - Fixed mouse monitoring lifecycle to prevent conflicts during automation - Disable mouse monitor when automation starts in active target mode - Re-enable mouse monitor when automation stops or on emergency stop - Added cursor cleanup to app termination handler - Improved active target mode stability and user experience The test window provides 5 colored targets with click counters, making it easy to validate auto-clicker functionality in both regular and active target modes.
Changes: - Fixed click registration issue by replacing absolute positioning with VStack/HStack layout - All targets now properly register clicks (not just bottom-right) - Increased window size to 1000x800 (ideal 1200x900) for better visibility - Increased target size from 120px to 150px - Enhanced font sizes and visual styling - Removed unused calculatePosition method The layout now uses proper SwiftUI stack-based layout instead of absolute positioning, preventing targets from overlapping in the z-order and ensuring all tap gestures work correctly.
Critical fixes: - Fixed window size to 900x750 (fits on all screens) - Removed NavigationView/toolbar approach that caused overflow - Added custom header with always-visible Close and Reset buttons - Added Escape key shortcut to close window (Cmd+.) - Made content scrollable to prevent overflow issues - Adjusted target sizes (130px) and spacing to fit properly This fixes the issue where users couldn't close the window and had to force quit the application.
Changed the click test window from a modal sheet to an independent WindowGroup that can be moved around the screen independently from the main ClickIt window. Changes: - Added new WindowGroup with id "click-test-window" in ClickItApp.swift - Updated AdvancedTab to use openWindow environment action instead of sheet - Modified DeveloperTools component to open separate window - Updated button description to clarify it opens in a separate window This allows users to position the click test window anywhere on screen to properly test the auto-clicker functionality without being constrained to the main application window.
Fixed an issue where active target mode would only perform a single click after being enabled. The automation configuration was hardcoded to use fixed position clicking (useDynamicMouseTracking: false) instead of respecting the active target mode setting. Changes: - Updated createAutomationConfiguration() to set useDynamicMouseTracking based on clickSettings.isActiveTargetMode - When active target mode is enabled, automation now follows cursor position - When active target mode is disabled, automation uses fixed position This fixes the reported issue where autoclicking would only do 1 click after enabling active target mode and making the first click. Fixes: ClickItViewModel.swift:234
Root cause analysis revealed that automation was stopping after a single click due to strict timing constraints combined with stopOnError=true: 1. Click timing constraints are very strict (15ms total: 10ms delay + 5ms deviation) 2. First clicks often exceed this on busy systems 3. stopOnError defaulted to true, stopping automation on first timing failure 4. Visual feedback showed regardless, masking the actual failure Solution: - Disable stopOnError specifically for active target mode - Active target mode is designed for continuous clicking - Users can manually stop by clicking again - Occasional timing failures shouldn't halt the entire automation - Normal automation modes still respect the stopOnError setting This complements the previous fix (a821c39) which enabled dynamic mouse tracking. Both changes are needed for active target mode to work correctly: - a821c39: Made clicks follow cursor position (useDynamicMouseTracking) - This commit: Prevents stopping on timing errors (stopOnError) Fixes: ClickItViewModel.swift:231
Fixed an issue where clicks in active target mode would alternate between two different screen positions due to coordinate system mismatch. Problem: - NSEvent.mouseLocation returns AppKit coordinates (origin bottom-left, Y↑) - ClickEngine expects CoreGraphics coordinates (origin top-left, Y↓) - Mouse position was being used directly without conversion - This caused clicks to alternate between intended position and mirrored position Solution: - Convert AppKit coordinates to CoreGraphics before clicking - Track separate coordinates for clicking vs visual feedback - clickLocation: CoreGraphics coordinates for ClickEngine - visualFeedbackLocation: AppKit coordinates for overlay - Use existing convertAppKitToCoreGraphicsMultiMonitor() method Changes: - Line 368-373: Convert mouse position from AppKit to CoreGraphics - Line 375-377: Also convert fixed positions for visual feedback - Line 390-392: Use AppKit coordinates for visual feedback overlay This ensures clicks happen exactly where the cursor is positioned, not at a vertically mirrored location. Fixes: ClickCoordinator.swift:368
Added intuitive mouse-based controls for active target mode with redundant stop options for maximum flexibility and ease of use. Controls: - LEFT-CLICK: Start autoclicking at cursor position - RIGHT-CLICK: Stop autoclicking immediately - ESC KEY: Stop autoclicking (emergency stop - already existed) Changes in HotkeyManager.swift: - Added onRightMouseClick callback for stop action - Updated registerMouseMonitor to monitor both .leftMouseDown and .rightMouseDown - Enhanced handleMouseClick to differentiate between left/right clicks - Updated logging to show LEFT=START, RIGHT=STOP Changes in ClickItViewModel.swift: - Split handleActiveTargetClick into: - handleActiveTargetLeftClick: Only starts automation - handleActiveTargetRightClick: Only stops automation - Updated setupMouseClickHandler to register both left and right handlers - Updated removeMouseClickHandler to clean up both handlers - Improved logging to clearly indicate which action is being performed Benefits: - No more confusing toggle behavior - Clear separation: left=start, right=stop - Two ways to stop (right-click or ESC) for maximum flexibility - Mouse-only users can use right-click - Keyboard users can use ESC - More intuitive and less prone to accidental toggles This complements previous fixes: - a821c39: Dynamic mouse tracking - c548735: Disabled stopOnError - 9ce9775: Coordinate system conversion
Changed active target mode controls to use a simpler, more intuitive scheme: - Right-click to toggle (start/stop) automation - All emergency keys (ESC, DELETE, F1, etc.) still work to stop This simplifies the control scheme from the previous hybrid approach and eliminates confusion between left and right mouse buttons. Controls: - RIGHT-CLICK: Toggle automation on/off at cursor position - ESC/DELETE/F1/etc: Stop automation (emergency stops) Changes in HotkeyManager.swift: - Removed onLeftMouseClick callback (no longer needed) - Changed onRightMouseClick to handle toggle behavior - Updated registerMouseMonitor to only monitor .rightMouseDown - Simplified handleMouseClick to only process right-clicks - Updated logging to reflect toggle behavior Changes in ClickItViewModel.swift: - Removed handleActiveTargetLeftClick (no longer needed) - Updated handleActiveTargetRightClick to toggle (start/stop) - Simplified setupMouseClickHandler to only register right-click - Simplified removeMouseClickHandler to only remove right-click - Updated logging to reflect toggle behavior Benefits: - Single mouse button for all mouse control (right-click) - Clear toggle behavior: click once to start, click again to stop - Emergency keys still available for instant stop - Simpler mental model: one button, one action (toggle) - Fewer lines of code, easier to maintain Supersedes: fe68ce8 (hybrid left/right approach)
Fixed two critical issues preventing active target mode from working: Issue 1: Incorrect UI Instruction - QuickStartTab.swift line 311 said "Left-click" but implementation uses "Right-click" - Users were clicking the wrong button! - Fixed: Changed text to "Right-click to start/stop clicking at cursor position" Issue 2: Double Validation Barrier - handleActiveTargetRightClick() allowed active target mode through first check - But startAutomation() had second guard that didn't account for active target mode - This blocked automation from starting even with valid right-click - Fixed: Added active target mode bypass in startAutomation() Changes: QuickStartTab.swift: - Line 311: Changed "Left-click" → "Right-click" in help text ClickItViewModel.swift: - Lines 164-173: Updated startAutomation() guard logic - In active target mode: only requires targetPoint + interval + !isRunning - In normal mode: uses full canStartAutomation validation - Added detailed debug logging to help troubleshoot prerequisites User Workflow Now: 1. Enable "Active Target Mode" toggle (cursor becomes crosshair) 2. Right-click anywhere to start (no need for "Start Automation" button!) 3. Automation clicks continuously at cursor position with configured interval 4. Right-click again to stop, OR use ESC/DELETE/F1 emergency keys Prerequisites for Active Target Mode: - Interval must be > 0 (✅ default is 10 seconds) - Automation must not already be running - That's it! No need to pre-set target or validate other settings Fixes: QuickStartTab.swift:311, ClickItViewModel.swift:164
Fixed critical bug where right-click would not restart automation after stopping via ESC key or natural completion (maxClicks/maxDuration reached). Root Cause: When automation stops externally (not via ViewModel.stopAutomation()), the $isActive observer (line 486) updates UI state but forgets to re-register mouse monitoring. This left right-click detection disabled after the first stop, making restart impossible. The Bug Flow: 1. Right-click to start → executeAutomation() unregisters mouse monitoring 2. Press ESC key → ClickCoordinator.emergencyStopAutomation() called 3. isActive becomes false → observer triggers 4. Observer updates isRunning/isPaused/appStatus ✅ 5. Observer calls cancelTimer() ✅ 6. Observer FORGETS to re-register mouse monitoring ❌ 7. Next right-click does nothing (no monitors listening!) Comparison: - stopAutomation() (line 294): ✅ Re-registers monitoring - emergencyStopAutomation() (line 604): ✅ Re-registers monitoring - $isActive observer (line 490): ❌ Forgot to re-register (NOW FIXED) The Fix: Added mouse monitoring re-registration to the $isActive observer at lines 498-502, matching the logic in stopAutomation() and emergencyStopAutomation(). Now works for ALL stop scenarios: ✅ Manual right-click stop → re-registers monitoring ✅ ESC/DELETE/F1 emergency stop → re-registers monitoring ✅ Natural completion (maxClicks) → re-registers monitoring ✅ Timer expiration → re-registers monitoring User can now: 1. Enable active target mode 2. Right-click to start 3. Stop via ANY method (ESC, right-click, completion) 4. Right-click again to restart ← NOW WORKS! Fixes: ClickItViewModel.swift:498-502
Summary of ChangesHello @jsonify, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature called 'Active Target Mode', which allows the auto-clicker to dynamically target the current mouse cursor position. This mode is toggled via a right-click, providing a more interactive and flexible way to use the application. Additionally, a new 'Click Test Window' has been added to aid in the visual verification and debugging of auto-clicking actions. These changes enhance both the user experience and the developer's ability to test the core functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new "active target mode" feature, where clicks follow the cursor. The changes are comprehensive, including a new CursorManager for cursor visuals, updates to HotkeyManager for right-click detection, and significant logic adjustments in ClickItViewModel and ClickCoordinator to support this new mode. The implementation also includes a new test window and ensures settings are persisted with backward compatibility. My feedback focuses on improving maintainability and robustness by reducing code duplication and refining some implementation details.
| func unregisterMouseMonitor() { | ||
| if let monitor = globalMouseMonitor { | ||
| NSEvent.removeMonitor(monitor) | ||
| globalMouseMonitor = nil | ||
| } | ||
|
|
||
| if let monitor = localMouseMonitor { | ||
| NSEvent.removeMonitor(monitor) | ||
| localMouseMonitor = nil | ||
| } | ||
|
|
||
| print("HotkeyManager: Successfully unregistered mouse click monitoring") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of unregisterMouseMonitor() is very similar to the existing unregisterGlobalHotkey() method. To reduce code duplication and improve maintainability, you could extract this common logic into a private helper function.
For example:
private func removeMonitors(global: inout Any?, local: inout Any?) {
if let monitor = global {
NSEvent.removeMonitor(monitor)
global = nil
}
if let monitor = local {
NSEvent.removeMonitor(monitor)
local = nil
}
}
func unregisterMouseMonitor() {
removeMonitors(global: &globalMouseMonitor, local: &localMouseMonitor)
print("HotkeyManager: Successfully unregistered mouse click monitoring")
}You could then refactor unregisterGlobalHotkey() to use this helper as well.
| self.cursorUpdateTimer = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { [weak self] _ in | ||
| guard let self = self, self.isTargetCursorActive else { return } | ||
| NSCursor.crosshair.set() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timer closure should invalidate the timer itself when it's no longer needed. This prevents the timer from continuing to fire unnecessarily if isTargetCursorActive becomes false, making the implementation more robust and efficient.
self.cursorUpdateTimer = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { [weak self] timer in
guard let self = self, self.isTargetCursorActive else {
timer.invalidate()
return
}
NSCursor.crosshair.set()
}| /// Restores the normal system cursor | ||
| func restoreNormalCursor() { | ||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self = self else { return } | ||
|
|
||
| if self.isTargetCursorActive { | ||
| self.isTargetCursorActive = false | ||
|
|
||
| // Stop the cursor update timer | ||
| self.cursorUpdateTimer?.invalidate() | ||
| self.cursorUpdateTimer = nil | ||
|
|
||
| // Restore arrow cursor | ||
| NSCursor.arrow.set() | ||
|
|
||
| self.logger.info("Normal cursor restored") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Force restore cursor (useful for cleanup on app termination) | ||
| func forceRestoreNormalCursor() { | ||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self = self else { return } | ||
|
|
||
| self.isTargetCursorActive = false | ||
| self.cursorUpdateTimer?.invalidate() | ||
| self.cursorUpdateTimer = nil | ||
| NSCursor.arrow.set() | ||
|
|
||
| self.logger.info("Cursor forcefully restored") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions restoreNormalCursor and forceRestoreNormalCursor contain a significant amount of duplicated code. You can refactor them to reduce this duplication, which will improve maintainability. forceRestoreNormalCursor can be implemented as a call to a more generic restoreNormalCursor function.
/// Restores the normal system cursor
func restoreNormalCursor(force: Bool = false) {
DispatchQueue.main.async { [weak self] in
guard let self = self else { return }
if force || self.isTargetCursorActive {
self.isTargetCursorActive = false
// Stop the cursor update timer
self.cursorUpdateTimer?.invalidate()
self.cursorUpdateTimer = nil
// Restore arrow cursor
NSCursor.arrow.set()
self.logger.info(force ? "Cursor forcefully restored" : "Normal cursor restored")
}
}
}
/// Force restore cursor (useful for cleanup on app termination)
func forceRestoreNormalCursor() {
restoreNormalCursor(force: true)
}
}| // Re-enable mouse monitoring if active target mode is still enabled | ||
| if clickSettings.isActiveTargetMode { | ||
| HotkeyManager.shared.registerMouseMonitor() | ||
| print("ClickItViewModel: Re-enabled mouse monitoring after automation stopped") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for re-enabling the mouse monitor is duplicated in stopAutomation(), the clickCoordinator.$isActive sink (line 498), and emergencyStopAutomation() (line 615). To improve maintainability and reduce code duplication, you should extract this into a private helper method.
For example:
private func reEnableMouseMonitorIfNeeded() {
if clickSettings.isActiveTargetMode {
HotkeyManager.shared.registerMouseMonitor()
print("ClickItViewModel: Re-enabled mouse monitoring after automation stopped.")
}
}You can then call reEnableMouseMonitorIfNeeded() in all three places.
| targetPoint = currentMousePosition | ||
| clickSettings.clickLocation = currentMousePosition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code consistency and reduce duplication, you can use the existing setTargetPoint(_:) helper method, which correctly updates both targetPoint and clickSettings.clickLocation.
| targetPoint = currentMousePosition | |
| clickSettings.clickLocation = currentMousePosition | |
| self.setTargetPoint(currentMousePosition) |
No description provided.