Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
给 AI 代理（Agents）的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Controller/ControllerAgent.cpp" line_range="870-871" />
<code_context>
cv::Point point = preproc_touch_point(param.point);
- if (!control_unit_->touch_move(0, point.x, point.y, 0)) {
- LogWarn << "Failed to move to scroll position" << VAR(point);
+ auto move_to_scroll_position = [&]() {
+ if (!control_unit_->touch_move(0, point.x, point.y, 0)) {
+ LogWarn << "Failed to move to scroll position" << VAR(point);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** 请考虑在 `touch_move` 调用失败时向上层传播该失败，而不是只记录日志。
之前，当 `touch_move` 失败时，`handle_scroll` 会返回 `false`，但现在的新 lambda 只记录一条警告日志，并且仍然尝试执行 `scroll`，这改变了可观测行为，并可能掩盖定位错误。如果你希望保留原有语义，建议在 `touch_move` 失败时返回 `false`（或通过其他方式发出失败信号），而不仅仅是记录日志。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的审查。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Controller/ControllerAgent.cpp" line_range="870-871" />
<code_context>
cv::Point point = preproc_touch_point(param.point);
- if (!control_unit_->touch_move(0, point.x, point.y, 0)) {
- LogWarn << "Failed to move to scroll position" << VAR(point);
+ auto move_to_scroll_position = [&]() {
+ if (!control_unit_->touch_move(0, point.x, point.y, 0)) {
+ LogWarn << "Failed to move to scroll position" << VAR(point);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider propagating failures from `touch_move` instead of only logging.
Previously, `handle_scroll` returned `false` when `touch_move` failed, but the new lambda only logs a warning and still attempts `scroll`, changing observable behavior and potentially hiding positioning errors. If you want to preserve the original semantics, consider returning `false` (or otherwise signaling the failure) when `touch_move` fails instead of just logging it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+870
to
+871
| auto move_to_scroll_position = [&]() { | ||
| if (!control_unit_->touch_move(0, point.x, point.y, 0)) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): 请考虑在 touch_move 调用失败时向上层传播该失败，而不是只记录日志。
之前，当 touch_move 失败时，handle_scroll 会返回 false，但现在的新 lambda 只记录一条警告日志，并且仍然尝试执行 scroll，这改变了可观测行为，并可能掩盖定位错误。如果你希望保留原有语义，建议在 touch_move 失败时返回 false（或通过其他方式发出失败信号），而不仅仅是记录日志。
Original comment in English
issue (bug_risk): Consider propagating failures from touch_move instead of only logging.
Previously, handle_scroll returned false when touch_move failed, but the new lambda only logs a warning and still attempts scroll, changing observable behavior and potentially hiding positioning errors. If you want to preserve the original semantics, consider returning false (or otherwise signaling the failure) when touch_move fails instead of just logging it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR further subdivides ControlUnit interfaces to make controller capabilities (notably scroll and relative_move) explicit by type, and updates bindings/tests/docs accordingly.
Changes:
- Refactors
ControlUnitAPIinto more specific interfaces (Win32ControlUnitAPI,CustomControlUnitAPI,GamepadControlUnitAPI) and routesscrollvia supported types only. - Splits Win32 input
relative_moveinto a separateRelativeMoveInputinterface and gates runtime support viadynamic_pointer_cast. - Updates Python/NodeJS bindings, C API docs, pipeline docs, and tests to reflect new scroll support behavior (dbg/adb/etc. fail).
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/python/binding_test.py | Updates Python binding tests to assert dbg scroll fails. |
| test/agent/agent_child_test.py | Updates agent child test to assert scroll fails. |
| source/include/LibraryHolder/ControlUnit.h | Tightens library-holder return types for custom/gamepad control units. |
| source/include/ControlUnit/GamepadControlUnitAPI.h | Updates Gamepad control-unit handle type in exported API. |
| source/include/ControlUnit/ControlUnitAPI.h | Introduces specialized ControlUnit API subclasses and handle typedefs. |
| source/binding/Python/maa/controller.py | Updates scroll documentation to reflect supported controller types. |
| source/binding/NodeJS/src/apis/controller.d.ts | Updates scroll documentation to reflect supported controller types. |
| source/MaaWlRootsControlUnit/Manager/WlRootsControlUnitMgr.h | Removes scroll override from WlRoots control unit. |
| source/MaaWlRootsControlUnit/Manager/WlRootsControlUnitMgr.cpp | Removes WlRoots scroll implementation. |
| source/MaaWlRootsControlUnit/Client/WaylandClient.h | Removes WaylandClient scroll API. |
| source/MaaWlRootsControlUnit/Client/WaylandClient.cpp | Removes WaylandClient scroll implementation/constants. |
| source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp | Gates relative_move behind RelativeMoveInput runtime support. |
| source/MaaWin32ControlUnit/Input/SeizeInput.h | Makes SeizeInput implement RelativeMoveInput. |
| source/MaaWin32ControlUnit/Input/MessageInput.h | Removes unsupported relative_move declaration. |
| source/MaaWin32ControlUnit/Input/MessageInput.cpp | Removes unsupported relative_move implementation. |
| source/MaaWin32ControlUnit/Input/LegacyEventInput.h | Removes unsupported relative_move declaration. |
| source/MaaWin32ControlUnit/Input/LegacyEventInput.cpp | Removes unsupported relative_move implementation. |
| source/MaaWin32ControlUnit/Base/UnitBase.h | Introduces RelativeMoveInput interface. |
| source/MaaPlayCoverControlUnit/Manager/PlayCoverControlUnitMgr.h | Removes scroll override from PlayCover control unit. |
| source/MaaPlayCoverControlUnit/Manager/PlayCoverControlUnitMgr.cpp | Removes PlayCover scroll implementation. |
| source/MaaGamepadControlUnit/Manager/GamepadControlUnitMgr.h | Switches gamepad manager to GamepadControlUnitAPI and drops scroll/relative_move overrides. |
| source/MaaGamepadControlUnit/Manager/GamepadControlUnitMgr.cpp | Removes gamepad scroll/relative_move implementations. |
| source/MaaGamepadControlUnit/API/GamepadControlUnitAPI.cpp | Updates exported create/destroy signatures to new handle type. |
| source/MaaFramework/Controller/ControllerAgent.cpp | Routes scroll only for Win32/custom control units; errors otherwise. |
| source/MaaDbgControlUnit/ReplayRecording/ReplayRecording.h | Removes scroll override from replay recording. |
| source/MaaDbgControlUnit/ReplayRecording/ReplayRecording.cpp | Removes replay recording scroll logic. |
| source/MaaDbgControlUnit/CarouselImage/CarouselImage.h | Removes scroll override from carousel image dbg controller. |
| source/MaaDbgControlUnit/CarouselImage/CarouselImage.cpp | Removes carousel image scroll implementation. |
| source/MaaCustomControlUnit/Manager/CustomControlUnitMgr.h | Switches custom manager to CustomControlUnitAPI. |
| source/MaaAdbControlUnit/Manager/InputAgent.h | Removes scroll override from ADB input agent. |
| source/MaaAdbControlUnit/Manager/InputAgent.cpp | Removes ADB input agent scroll implementation. |
| source/MaaAdbControlUnit/Manager/AdbControlUnitMgr.h | Removes scroll override from ADB control unit manager. |
| source/MaaAdbControlUnit/Manager/AdbControlUnitMgr.cpp | Removes ADB control unit manager scroll implementation. |
| source/MaaAdbControlUnit/Input/MtouchHelper.h | Removes scroll override from MtouchHelper. |
| source/MaaAdbControlUnit/Input/MtouchHelper.cpp | Removes MtouchHelper scroll implementation. |
| source/MaaAdbControlUnit/Input/AdbShellInput.h | Removes scroll override from AdbShellInput. |
| source/MaaAdbControlUnit/Input/AdbShellInput.cpp | Removes AdbShellInput scroll implementation. |
| source/MaaAdbControlUnit/EmulatorExtras/MuMuPlayerExtras.h | Removes scroll override from MuMu extras. |
| source/MaaAdbControlUnit/EmulatorExtras/MuMuPlayerExtras.cpp | Removes MuMu extras scroll implementation. |
| source/MaaAdbControlUnit/Base/UnitBase.h | Removes scroll from ADB InputBase interface. |
| source/LibraryHolder/ControlUnit/ControlUnit.cpp | Updates holder implementations to return specialized API shared_ptrs. |
| include/MaaFramework/Instance/MaaController.h | Updates scroll API note to reflect supported controller types. |
| docs/en_us/3.1-PipelineProtocol.md | Updates scroll support note in pipeline protocol docs. |
| docs/en_us/2.2-IntegratedInterfaceOverview.md | Updates scroll support note in integrated interface overview. |
| * @return The control id of the scroll action. | ||
| * | ||
| * @note Not all controllers support scroll. If not supported, the action will fail. | ||
| * @note Scroll is supported by Win32 controllers and custom controllers that implement scroll. |
There was a problem hiding this comment.
This docstring notes which controller types support scroll, but it no longer states what happens when scroll is posted to an unsupported controller. Consider adding an explicit note (similar to MaaControllerPostRelativeMove docs) that the action will fail on unsupported controllers so API consumers know to check job status.
Suggested change
| * @note Scroll is supported by Win32 controllers and custom controllers that implement scroll. | |
| * @note Scroll is supported by Win32 controllers and custom controllers that implement scroll. | |
| * @note If the controller does not support scroll, the action will fail and the job status will indicate failure. |
| > [!NOTE] | ||
| > | ||
| > - Adb Controller and PlayCover Controller do not support scroll operations. Only Win32 Controller is supported. | ||
| > - Win32 controllers and custom controllers that implement `scroll` support scroll operations. |
There was a problem hiding this comment.
The NOTE explains which controllers support Scroll, but it omits that posting this action to other controllers will fail (the previous wording made this explicit). Consider adding a bullet clarifying that unsupported controller types will return a failed action/job, to set correct expectations for pipeline users.
Suggested change
| > - Win32 controllers and custom controllers that implement `scroll` support scroll operations. | |
| > - Win32 controllers and custom controllers that implement `scroll` support scroll operations. | |
| > - Posting a `Scroll` action to controllers that do not implement `scroll` will result in a failed action/job. |
Comment on lines
462
to
467
| Asynchronously scroll. This is an asynchronous operation that immediately returns an operation id. You can query the status via `MaaControllerStatus` and `MaaControllerWait`. | ||
|
|
||
| > [!NOTE] | ||
| > | ||
| > - Only Win32 Controller supports scroll operations. Adb Controller does not support scrolling. | ||
| > - Win32 controllers and custom controllers that implement `scroll` support scroll operations. | ||
| > - 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. |
There was a problem hiding this comment.
Similar to the relative-move section above, this scroll section should explicitly mention that operations posted to unsupported controllers will fail (so callers know they must check status). Right now it only lists supported controller types.
|
|
||
| Note: | ||
| 不是所有控制器都支持滚动操作 / Not all controllers support scroll operation | ||
| Win32 控制器和实现了 scroll 的自定义控制器支持滚动操作 / Win32 controllers and custom controllers with scroll support this operation |
There was a problem hiding this comment.
The English sentence in this Note is grammatically incorrect ("custom controllers with scroll support this operation"). Consider rephrasing to match the other bindings/docs, e.g. "Win32 controllers and custom controllers that implement scroll support this operation."
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.
Summary by Sourcery
通过将滚动支持显式化并与具体控制器关联，以及将相对移动能力从基础输入接口中分离出来，优化控制单元接口设计。
新功能：
CustomControlUnitAPI和GamepadControlUnitAPI接口，用于更清晰地描述不同控制器的能力。RelativeMoveInput接口。Bug 修复：
增强：
ControllerAgent中的滚动处理限制为 Win32 控制单元以及显式实现滚动功能的自定义控制单元，否则记录错误日志。GamepadControlUnitAPI。文档：
测试：
杂项：
scroll和relative_move方法，简化控制单元 API 表面。Original summary in English
Summary by Sourcery
Refine control unit interfaces by making scroll support explicit and controller-specific, and by separating relative-move capabilities from the base input interfaces.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: