Add error-returning mouse click helpers (ClickE) #750
Conversation
|
|
WalkthroughThis pull request introduces error-aware variants for mouse actions across Mac, X11, and Windows platforms, replacing fire-and-forget semantics with explicit error returns. Additionally, a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
这是一封自动回复邮件。已经收到您的来信，我会尽快回复。
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
robotgo.go (1)
724-724: Consider movingdeferafter argument validation.The
defer MilliSleep(MouseSleep)at line 724 will execute even when returning early due to invalid arguments (lines 711, 719). If no C call is made, sleeping is unnecessary overhead.🔎 Suggested fix
Move the defer after argument validation, or remove it and add explicit
MilliSleepbeforereturn nil:if len(args) > 1 { dbl, ok := args[1].(bool) if !ok { return errors.New("second argument must be a bool indicating double click") } double = dbl } - defer MilliSleep(MouseSleep) - if !double { if code := C.toggleMouseErr(true, button); code != 0 { return formatClickError(int(code), button, "down", false) } // match clickMouse timing C.microsleep(C.double(5.0)) if code := C.toggleMouseErr(false, button); code != 0 { return formatClickError(int(code), button, "up", false) } } else { if code := C.doubleClickErr(button); code != 0 { return formatClickError(int(code), button, "double", true) } } + MilliSleep(MouseSleep) return nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mouse/mouse_c.hrobotgo.go
🧰 Additional context used
🧬 Code graph analysis (2)
robotgo.go (1)
keycode.go (4)
WheelDown(28-28)WheelUp(29-29)WheelLeft(30-30)WheelRight(31-31)
mouse/mouse_c.h (2)
base/xdisplay_c.h (1)
XGetMainDisplay(18-48)robotgo_win.go (1)
SendInput(39-41)
🔇 Additional comments (6)
robotgo.go (3)
55-58: LGTM!The new imports (
fmtfor error formatting andsyscallfor Windows error code translation) are correctly added to support the new error-returning functionality.
510-526: LGTM!The function correctly maps button constants to readable names with a sensible fallback for unknown values. This pairs well with the error formatting logic.
746-783: LGTM with minor observation.The platform-specific error formatting is well-structured. Note that
0: "kCGErrorSuccess"(line 757) is unreachable since this function is only called whencode != 0, but it's harmless documentation of the full enum.mouse/mouse_c.h (3)
198-200: LGTM!The wrapper correctly maintains backward compatibility by delegating to the new error-returning function.
202-215: LGTM!The error propagation logic is correct - errors from either the down or up toggle are properly surfaced.
256-259: LGTM!The wrapper correctly maintains backward compatibility.
| #elif defined(USE_X11) | ||
| Display *display = XGetMainDisplay(); | ||
| XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime); | ||
| Status status = XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime); | ||
| XSync(display, false); | ||
|
|
||
| return status ? 0 : 1; |
There was a problem hiding this comment.
Missing NULL check for X11 display.
XGetMainDisplay() can return NULL if it fails to open a display (as shown in base/xdisplay_c.h). Calling XTestFakeButtonEvent with a NULL display will cause undefined behavior or crash.
🔎 Suggested fix
#elif defined(USE_X11)
Display *display = XGetMainDisplay();
+ if (display == NULL) {
+ return 1;
+ }
Status status = XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime);
XSync(display, false);
return status ? 0 : 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #elif defined(USE_X11) | |
| Display *display = XGetMainDisplay(); | |
| XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime); | |
| Status status = XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime); | |
| XSync(display, false); | |
| return status ? 0 : 1; | |
| #elif defined(USE_X11) | |
| Display *display = XGetMainDisplay(); | |
| if (display == NULL) { | |
| return 1; | |
| } | |
| Status status = XTestFakeButtonEvent(display, button, down ? True : False, CurrentTime); | |
| XSync(display, false); | |
| return status ? 0 : 1; |
🤖 Prompt for AI Agents
In mouse/mouse_c.h around lines 176-181, XGetMainDisplay() may return NULL and
the code calls XTestFakeButtonEvent/XSync unconditionally; add a NULL check
immediately after obtaining display and handle the failure (e.g., log the error
if logging is available and return the same non-zero error value used elsewhere)
so you do not call XTestFakeButtonEvent or XSync with a NULL display.
| CGEventSourceRef source = CGEventSourceCreate(kCGEventSourceStateHIDSystemState); | ||
| CGEventRef event = CGEventCreateMouseEvent(source, mouseTypeDown, currentPos, kCGMouseButtonLeft); |
There was a problem hiding this comment.
Hardcoded kCGMouseButtonLeft inconsistent with toggleMouseErr.
Line 225 uses kCGMouseButtonLeft as the button parameter, but toggleMouseErr (line 164) correctly uses (CGMouseButton)button. This inconsistency may cause issues when double-clicking with center or other mouse buttons.
🔎 Suggested fix
- CGEventRef event = CGEventCreateMouseEvent(source, mouseTypeDown, currentPos, kCGMouseButtonLeft);
+ CGEventRef event = CGEventCreateMouseEvent(source, mouseTypeDown, currentPos, (CGMouseButton)button);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CGEventSourceRef source = CGEventSourceCreate(kCGEventSourceStateHIDSystemState); | |
| CGEventRef event = CGEventCreateMouseEvent(source, mouseTypeDown, currentPos, kCGMouseButtonLeft); | |
| CGEventSourceRef source = CGEventSourceCreate(kCGEventSourceStateHIDSystemState); | |
| CGEventRef event = CGEventCreateMouseEvent(source, mouseTypeDown, currentPos, (CGMouseButton)button); |
🤖 Prompt for AI Agents
In mouse/mouse_c.h around lines 224 to 225, the CGEventCreateMouseEvent call
hardcodes kCGMouseButtonLeft for the button argument while toggleMouseErr used
(CGMouseButton)button; change the hardcoded kCGMouseButtonLeft to use
(CGMouseButton)button so the created mouse event respects the provided button
value (and apply the same change for any corresponding mouse-up event nearby),
ensuring the 'button' variable is in scope and consistent with the rest of the
function.
4 participants
Description
This PR adds error-returning variants of the mouse click functions, allowing users to detect and handle mouse operation failures.
Motivation
We use robotgo in production and occasionally experience silent click failures. Currently, we rely on retry mechanisms as a workaround, but without direct error feedback from the API, we cannot reliably determine whether a click operation succeeded.
Changes
C layer (
mouse/mouse_c.h):toggleMouseErr(bool down, MMMouseButton button)- returns error code instead of voidclickMouseErr(MMMouseButton button)- returns error code for single clickdoubleClickErr(MMMouseButton button)- returns error code for double clicktoggleMouse,clickMouse,doubleClick) preserved as wrappers for backward compatibilityGo layer (
robotgo.go):ClickE(args ...interface{}) error- click with error handlingMouseButtonString(btn C.MMMouseButton) string- converts button code to readable nameformatClickError()- formats platform-specific error messagesPlatform-specific error handling
kCGErrorCannotCompleteSendInputreturn value andGetLastError()XTestFakeButtonEventreturn statusWhy this is needed
The existing
Click()function does not provide any feedback when a mouse operation fails. In production environments, system configurations are complex and failures are difficult to reproduce. To demonstrate that these system APIs can indeed fail, we have gathered the following documented cases:macOS (
CGEventCreateMouseEvent):NULLwhen accessibility permissions are not grantedCGEventTapCreatefails on macOS Mojave+ without proper permissions (GitHub Issue)Windows (
SendInput):GetLastError()indicates the cause (MSDN Archive)DESKTOP_JOURNALPLAYBACKaccess (Microsoft Blog)Linux (
XTestFakeButtonEvent):BadValueerror for invalid button numbers (Arch Linux Forums)With
ClickE(), users can now detect these failures and implement proper error handling or retry logic in their automation scripts.Testing
Test code:
Backward compatibility
This PR maintains full backward compatibility. All existing functions continue to work as before. The new
ClickE()function is an additional option for users who need error handling.Signed-off-by: PekingSpades [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.