Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体上的反馈：
- 在
PostToPidInput::scroll中，[nsEvent0 CGEvent]返回的是一个已保留 (retained) 的CGEventRef，但从未被CFRelease，这会导致内存泄漏；如果你继续使用nsEvent0这种做法，务必在CGEventPostToPid之后释放event0。 - 使用 KVC 将
nsEvent0的type修改为NSEventTypeScrollWheel（[nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]）依赖于私有实现细节，在不同 macOS 版本之间可能不够稳定；建议考虑完全通过CGEventCreateScrollWheelEvent来构造所需的滚动事件。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PostToPidInput::scroll`, `[nsEvent0 CGEvent]` returns a retained `CGEventRef` that is never `CFRelease`d, causing a leak; if you keep the `nsEvent0` approach, make sure to release `event0` after `CGEventPostToPid`.
- The use of KVC to mutate `nsEvent0`'s `type` to `NSEventTypeScrollWheel` (`[nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]`) relies on private implementation details and may be fragile across macOS versions; consider constructing the needed scroll events purely via `CGEventCreateScrollWheelEvent` instead.
## Individual Comments
### Comment 1
<location path="source/MaaMacOSControlUnit/Input/PostToPidInput.mm" line_range="171-180" />
<code_context>
+ return false;
+ }
+ [nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"];
+ CGEventRef event0 = [nsEvent0 CGEvent];
+ if (!event0) {
+ LogError << "Failed to create scroll event0";
+ return false;
+ }
+
+ CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx);
+ if (!event1) {
+ LogError << "Failed to create scroll event1";
+ return false;
+ }
+
+ CGEventPostToPid(pid_, event0);
+ CGEventPostToPid(pid_, event1);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** CGEventRef from NSEvent is never released, causing a leak (and on error path too).
Since `[nsEvent0 CGEvent]` returns a retained Core Foundation object, it must be `CFRelease`d on both success and failure paths. For example:
```objc
CGEventRef event0 = [nsEvent0 CGEvent];
if (!event0) {
LogError << "Failed to create scroll event0";
return false;
}
CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx);
if (!event1) {
LogError << "Failed to create scroll event1";
CFRelease(event0);
return false;
}
CGEventPostToPid(pid_, event0);
CGEventPostToPid(pid_, event1);
CFRelease(event0);
CFRelease(event1);
```
This ensures proper ownership and no leaks on any path.
</issue_to_address>
### Comment 2
<location path="docs/en_us/2.2-IntegratedInterfaceOverview.md" line_range="474" />
<code_context>
> [!NOTE]
>
-> - Win32 controllers and custom controllers that implement `scroll` support scroll operations.
+> - Win32, MacOS, and custom controllers that implement `scroll` support scroll operations.
> - Posting scroll to a controller that does not implement `scroll` will cause the action to fail.
> - The `dx`/`dy` values are sent directly as scroll increments. The Windows standard wheel increment is 120 (WHEEL_DELTA) per notch. Using multiples of 120 is recommended for best compatibility.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using the standard platform spelling "macOS" instead of "MacOS".
Apple’s official platform name is "macOS"; please update this reference for accurate, consistent naming.
```suggestion
> - Win32, macOS, and custom controllers that implement `scroll` support scroll operations.
```
</issue_to_address>
### Comment 3
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1103" />
<code_context>
> [!NOTE]
>
-> - Win32 controllers and custom controllers that implement `scroll` support scroll operations.
+> - Win32, MacOS, and custom controllers that implement `scroll` support scroll operations.
> - Posting scroll to a controller that does not implement `scroll` will cause the action to fail.
> - The `dx`/`dy` values are sent directly as scroll increments. The Windows standard wheel increment is 120 (WHEEL_DELTA) per notch. Using multiples of 120 is recommended for best compatibility.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "MacOS" to the standard "macOS".
Apple’s official branding uses the lowercase “macOS” form.
```suggestion
> - Win32, macOS, and custom controllers that implement `scroll` support scroll operations.
```
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
PostToPidInput::scroll,[nsEvent0 CGEvent]returns a retainedCGEventRefthat is neverCFReleased, causing a leak; if you keep thensEvent0approach, make sure to releaseevent0afterCGEventPostToPid. - The use of KVC to mutate
nsEvent0'stypetoNSEventTypeScrollWheel([nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]) relies on private implementation details and may be fragile across macOS versions; consider constructing the needed scroll events purely viaCGEventCreateScrollWheelEventinstead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PostToPidInput::scroll`, `[nsEvent0 CGEvent]` returns a retained `CGEventRef` that is never `CFRelease`d, causing a leak; if you keep the `nsEvent0` approach, make sure to release `event0` after `CGEventPostToPid`.
- The use of KVC to mutate `nsEvent0`'s `type` to `NSEventTypeScrollWheel` (`[nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]`) relies on private implementation details and may be fragile across macOS versions; consider constructing the needed scroll events purely via `CGEventCreateScrollWheelEvent` instead.
## Individual Comments
### Comment 1
<location path="source/MaaMacOSControlUnit/Input/PostToPidInput.mm" line_range="171-180" />
<code_context>
+ return false;
+ }
+ [nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"];
+ CGEventRef event0 = [nsEvent0 CGEvent];
+ if (!event0) {
+ LogError << "Failed to create scroll event0";
+ return false;
+ }
+
+ CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx);
+ if (!event1) {
+ LogError << "Failed to create scroll event1";
+ return false;
+ }
+
+ CGEventPostToPid(pid_, event0);
+ CGEventPostToPid(pid_, event1);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** CGEventRef from NSEvent is never released, causing a leak (and on error path too).
Since `[nsEvent0 CGEvent]` returns a retained Core Foundation object, it must be `CFRelease`d on both success and failure paths. For example:
```objc
CGEventRef event0 = [nsEvent0 CGEvent];
if (!event0) {
LogError << "Failed to create scroll event0";
return false;
}
CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx);
if (!event1) {
LogError << "Failed to create scroll event1";
CFRelease(event0);
return false;
}
CGEventPostToPid(pid_, event0);
CGEventPostToPid(pid_, event1);
CFRelease(event0);
CFRelease(event1);
```
This ensures proper ownership and no leaks on any path.
</issue_to_address>
### Comment 2
<location path="docs/en_us/2.2-IntegratedInterfaceOverview.md" line_range="474" />
<code_context>
> [!NOTE]
>
-> - Win32 controllers and custom controllers that implement `scroll` support scroll operations.
+> - Win32, MacOS, and custom controllers that implement `scroll` support scroll operations.
> - Posting scroll to a controller that does not implement `scroll` will cause the action to fail.
> - The `dx`/`dy` values are sent directly as scroll increments. The Windows standard wheel increment is 120 (WHEEL_DELTA) per notch. Using multiples of 120 is recommended for best compatibility.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using the standard platform spelling "macOS" instead of "MacOS".
Apple’s official platform name is "macOS"; please update this reference for accurate, consistent naming.
```suggestion
> - Win32, macOS, and custom controllers that implement `scroll` support scroll operations.
```
</issue_to_address>
### Comment 3
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1103" />
<code_context>
> [!NOTE]
>
-> - Win32 controllers and custom controllers that implement `scroll` support scroll operations.
+> - Win32, MacOS, and custom controllers that implement `scroll` support scroll operations.
> - Posting scroll to a controller that does not implement `scroll` will cause the action to fail.
> - The `dx`/`dy` values are sent directly as scroll increments. The Windows standard wheel increment is 120 (WHEEL_DELTA) per notch. Using multiples of 120 is recommended for best compatibility.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "MacOS" to the standard "macOS".
Apple’s official branding uses the lowercase “macOS” form.
```suggestion
> - Win32, macOS, and custom controllers that implement `scroll` support scroll operations.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
PostToPid 版的 scroll 比较魔幻 直接 CGEventCreateScrollWheelEvent 构造的event无法被目标接受（就像鼠标事件一样，可能是缺少目标的windowID） 但是在发送这个event之前发一个被魔改的 NSEvent 就可以实现滚动 |
Contributor
Author
[nsEvent0 CGEvent] 是一个 getter，不是 Create 或者 Copy，返回的对象并不拥有，生命周期由nsEvent0负责，不能调 CFRelease。 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds macOS scroll support to the control units and wires the framework’s controller agent to route Scroll actions to macOS controllers, alongside updating the macOS test GUI/pipeline and documenting scroll support.
Changes:
- Implement scroll event injection for macOS controllers (PID-targeted and global-event modes) and route
ScrollinControllerAgentto macOS control units. - Update macOS test GUI and pipeline to visually exercise scroll behavior (scrollable canvas + new scroll steps).
- Update English docs to state macOS controllers support scroll operations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
source/MaaMacOSControlUnit/Input/PostToPidInput.mm |
Adds PID-targeted scroll injection via Quartz/AppKit event creation. |
source/MaaMacOSControlUnit/Input/GlobalEventInput.mm |
Adds global scroll injection using current cursor position and improves event creation error handling. |
source/MaaFramework/Controller/ControllerAgent.cpp |
Routes Scroll actions to macOS control units (in addition to Win32/custom). |
test/macos_test/MacOSTestGUI.mm |
Reworks the macOS test window to include a scrollable canvas and logs scroll events. |
test/macos_test/MacOSTestGUI.h |
Declares the new scroll callback hook used by the test GUI. |
test/macos_test/pipeline.json |
Adds scroll test nodes (down/up/right) to the macOS test pipeline. |
test/macos_test/GenerateBee.py |
Adjusts the bee template background color to match the updated test UI. |
docs/en_us/3.1-PipelineProtocol.md |
Updates scroll note to include macOS controllers. |
docs/en_us/2.2-IntegratedInterfaceOverview.md |
Updates controller API scroll note to include macOS controllers. |
Comment on lines
+176
to
+178
| CGEventRef pos_event = CGEventCreate(nullptr); | ||
| CGPoint location = CGEventGetLocation(pos_event); | ||
| CFRelease(pos_event); |
There was a problem hiding this comment.
scroll() uses CGEventCreate(nullptr) without a null check, then immediately calls CGEventGetLocation(pos_event) and CFRelease(pos_event). If CGEventCreate returns null, this will crash. Add a null check (and only release when non-null), or use a safer way to query the current cursor location.
Suggested change
| CGEventRef pos_event = CGEventCreate(nullptr); | |
| CGPoint location = CGEventGetLocation(pos_event); | |
| CFRelease(pos_event); | |
| CGPoint location = CGPointZero; | |
| CGEventRef pos_event = CGEventCreate(nullptr); | |
| if (pos_event) { | |
| location = CGEventGetLocation(pos_event); | |
| CFRelease(pos_event); | |
| } else { | |
| LogWarn << "Warning: Failed to create position event, using default location (0, 0)"; | |
| } |
Comment on lines
+180
to
+188
| // wheel1: 垂直滚动（正值向上，负值向下），wheel2: 水平滚动（正值向左，负值向右） | ||
| CGEventRef event = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx); | ||
| if (!event) { | ||
| LogError << "Failed to create scroll wheel event"; | ||
| return false; | ||
| } | ||
|
|
||
| CGEventSetLocation(event, location); | ||
| CGEventPost(kCGHIDEventTap, event); |
There was a problem hiding this comment.
The horizontal scroll direction comment/code path appears to conflict with the documented Scroll semantics (docs say dx > 0 scrolls right, dx < 0 scrolls left). Here wheel2 is described as dx > 0 scroll left, dx < 0 scroll right, and dx is passed through directly. Please align macOS behavior with the protocol (likely negate dx when constructing the CGEvent, and update the comment accordingly).
Comment on lines
+156
to
+187
| NSPoint location = NSMakePoint(latest_touch_x_, window_h_ - latest_touch_y_); | ||
| NSEvent* nsEvent0 = [NSEvent mouseEventWithType:NSEventTypeMouseMoved | ||
| location:location | ||
| modifierFlags:0 | ||
| timestamp:[NSDate timeIntervalSinceReferenceDate] | ||
| windowNumber:window_id_ | ||
| context:nil | ||
| eventNumber:0 | ||
| clickCount:1 | ||
| pressure:0]; | ||
| if (!nsEvent0) { | ||
| LogError << "Failed to create scroll nsEvent0"; | ||
| return false; | ||
| } | ||
| [nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]; | ||
| CGEventRef event0 = [nsEvent0 CGEvent]; | ||
| if (!event0) { | ||
| LogError << "Failed to create scroll event0"; | ||
| return false; | ||
| } | ||
|
|
||
| CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx); | ||
| if (!event1) { | ||
| LogError << "Failed to create scroll event1"; | ||
| return false; | ||
| } | ||
|
|
||
| CGEventPostToPid(pid_, event0); | ||
| CGEventPostToPid(pid_, event1); | ||
|
|
||
| CFRelease(event1); | ||
|
|
There was a problem hiding this comment.
[nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"] relies on KVC to mutate an NSEvent's (read-only) type. This is not a public/safe API and can raise an exception at runtime on some macOS versions. Prefer constructing a proper scroll CGEvent (or NSEvent if supported) without KVC hacks.
Suggested change
| NSPoint location = NSMakePoint(latest_touch_x_, window_h_ - latest_touch_y_); | |
| NSEvent* nsEvent0 = [NSEvent mouseEventWithType:NSEventTypeMouseMoved | |
| location:location | |
| modifierFlags:0 | |
| timestamp:[NSDate timeIntervalSinceReferenceDate] | |
| windowNumber:window_id_ | |
| context:nil | |
| eventNumber:0 | |
| clickCount:1 | |
| pressure:0]; | |
| if (!nsEvent0) { | |
| LogError << "Failed to create scroll nsEvent0"; | |
| return false; | |
| } | |
| [nsEvent0 setValue:@(NSEventTypeScrollWheel) forKey:@"type"]; | |
| CGEventRef event0 = [nsEvent0 CGEvent]; | |
| if (!event0) { | |
| LogError << "Failed to create scroll event0"; | |
| return false; | |
| } | |
| CGEventRef event1 = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx); | |
| if (!event1) { | |
| LogError << "Failed to create scroll event1"; | |
| return false; | |
| } | |
| CGEventPostToPid(pid_, event0); | |
| CGEventPostToPid(pid_, event1); | |
| CFRelease(event1); | |
| // 使用 CGEvent 原生滚动事件，避免通过 KVC 修改 NSEvent 的只读 type。 | |
| CGPoint location = CGPointMake(latest_touch_x_, latest_touch_y_); | |
| CGEventRef event = CGEventCreateScrollWheelEvent(nullptr, kCGScrollEventUnitPixel, 2, dy, dx); | |
| if (!event) { | |
| LogError << "Failed to create scroll event"; | |
| return false; | |
| } | |
| CGEventSetLocation(event, location); | |
| CGEventPostToPid(pid_, event); | |
| CFRelease(event); |
| LogError << "Failed to create scroll event1"; | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
The scroll CGEventRef event1 = CGEventCreateScrollWheelEvent(...) is posted without setting its location. Posting a separate event0 (built from a mouse event) won’t reliably associate the scroll delta with the intended point, especially since event0’s type is being modified. Set the scroll event’s location explicitly (and consider posting a normal mouse-move event first if needed), or derive the correct screen location and apply it to event1 via CGEventSetLocation.
Suggested change
| // Ensure the scroll event has the correct location, matching the mouse event. | |
| CGEventSetLocation(event1, CGEventGetLocation(event0)); |
Comment on lines
+48
to
+49
| // 将滚动事件传递给父视图（NSScrollView），实现实际滚动 | ||
| [super scrollWheel:event]; |
There was a problem hiding this comment.
EventHandlingView’s scrollWheel: override is unlikely to run when the cursor is over the scrollable area, because scroll events will be dispatched to the NSScrollView/document view (CanvasView) under the cursor. Also, calling [super scrollWheel:event] on NSView doesn’t forward the event to the parent NSScrollView. If you want both logging and actual scrolling, handle scrollWheel: in the document view (or install an event monitor) and forward via the responder chain (e.g., [[self nextResponder] scrollWheel:event]).
Suggested change
| // 将滚动事件传递给父视图（NSScrollView），实现实际滚动 | |
| [super scrollWheel:event]; | |
| // 将滚动事件通过响应链传递给父视图（例如 NSScrollView），实现实际滚动 | |
| [[self nextResponder] scrollWheel:event]; |
Comment on lines
160
to
+172
| // 创建自定义容器视图（支持事件处理） | ||
| EventHandlingView* containerView = [[EventHandlingView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400) gui:this]; | ||
|
|
||
| // 创建文本标签（上面） | ||
| NSTextField* textField = [[NSTextField alloc] initWithFrame:NSMakeRect(0, 320, 400, 60)]; | ||
| [textField setStringValue:@"这是一个测试窗口，用于 MaaFW 识别。"]; | ||
| [textField setEditable:NO]; | ||
| [textField setBordered:NO]; | ||
| [textField setBackgroundColor:[NSColor clearColor]]; | ||
| [textField setAlignment:NSTextAlignmentCenter]; | ||
| [textField setFont:[NSFont systemFontOfSize:14]]; | ||
| [containerView addSubview:textField]; | ||
|
|
||
| // 创建 emoji 标签（下面居中） | ||
| NSTextField* emojiField = [[NSTextField alloc] initWithFrame:NSMakeRect(150, 150, 100, 100)]; | ||
| [emojiField setStringValue:@"🐝"]; | ||
| [emojiField setEditable:NO]; | ||
| [emojiField setBordered:NO]; | ||
| [emojiField setBackgroundColor:[NSColor clearColor]]; | ||
| [emojiField setAlignment:NSTextAlignmentCenter]; | ||
| [emojiField setFont:[NSFont systemFontOfSize:100]]; | ||
| [containerView addSubview:emojiField]; | ||
| EventHandlingView* containerView = [[EventHandlingView alloc] initWithFrame:NSMakeRect(0, 0, 720, 720) gui:this]; | ||
|
|
||
| // 可滚动子画布 | ||
| NSScrollView* scrollView = [[NSScrollView alloc] initWithFrame:NSMakeRect(0, 40, 720, 680)]; | ||
| [scrollView setHasVerticalScroller:YES]; | ||
| [scrollView setHasHorizontalScroller:NO]; | ||
| [scrollView setBorderType:NSLineBorder]; | ||
|
|
||
| // 子画布（翻转坐标，原点在左上角） | ||
| CanvasView* canvasView = [[CanvasView alloc] initWithFrame:NSMakeRect(0, 0, 1440, 1360)]; | ||
| [canvasView setWantsLayer:YES]; | ||
| [canvasView.layer setBackgroundColor:[[NSColor colorWithWhite:0.95 alpha:1.0] CGColor]]; |
There was a problem hiding this comment.
After introducing the NSScrollView + CanvasView, the existing mouse event handlers in EventHandlingView (mouseDown/Dragged/Up) will no longer be triggered for interactions inside the scrollable canvas, because those events will target the document view instead. If these callbacks are still needed for the test harness, move the mouse handlers to CanvasView (or make the document view an EventHandlingView) so clicks/drags inside the canvas are still reported.
| "focus": "Scroll Right Test!", | ||
| "action": "Scroll", | ||
| "target": "bee_recognition", | ||
| "dx": -360, |
There was a problem hiding this comment.
This node is labeled "Scroll Right Test!" but sets dx to a negative value. The pipeline protocol docs define dx > 0 as scroll right and dx < 0 as scroll left, so this test appears to exercise the opposite direction (or the implementation uses inverted sign). Please make the test direction/sign consistent with the documented semantics (and with the macOS implementation).
Suggested change
| "dx": -360, | |
| "dx": 360, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
本PR增加了 MacOS scroll 支持
由 Sourcery 提供的摘要
在各控制单元中添加对 macOS 滚动的支持，并更新 macOS 测试 UI 以验证滚动行为。
新功能：
PostToPidInput为以 PID 为目标的 macOS 控制器启用滚轮输入。ControllerAgent中添加 macOS 控制器滚动处理路径，使 macOS 单元可以处理滚动操作。增强内容：
CGEvent时的日志记录和错误处理。文档：
测试：
Original summary in English
Summary by Sourcery
Add macOS scroll support across the control units and update macOS test UI to exercise scroll behavior.
New Features:
Enhancements:
Documentation:
Tests: