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 - 我发现了 4 个问题，并给出了一些整体性反馈：
- 在 NodeJS 绑定中，
ControllerImpl::post_set_background_managed_keys的定义省略了env参数名，但在函数体中仍然使用了env，这会导致无法编译；请通过为参数添加名称EnvType env，使 cpp 定义与头文件保持一致。 - 在
ControllerAgent::handle_managed_click_key/handle_managed_long_press_key中，最开始调用的background_key_up(keycode)的返回值被忽略了，因此在重新按下之前释放卡住按键失败的情况会被静默丢弃；建议将该返回值并入ret，以便暴露真实错误。
供 AI Agents 使用的提示
Please address the comments from this code review:
## Overall Comments
- In the NodeJS binding, `ControllerImpl::post_set_background_managed_keys`'s definition omits the `env` parameter name but still uses `env` in the body, which will not compile; align the cpp definition with the header by naming the `EnvType env` parameter.
- In `ControllerAgent::handle_managed_click_key`/`handle_managed_long_press_key`, the initial `background_key_up(keycode)` result is ignored, so a failure to release a stuck key before re-pressing is silently dropped; consider incorporating that return value into `ret` to surface real errors.
## Individual Comments
### Comment 1
<location path="source/Common/MaaController.cpp" line_range="255-264" />
<code_context>
return ctrl->post_key_up(keycode);
}
+MaaCtrlId MaaControllerPostSetBackgroundManagedKeys(MaaController* ctrl, const int32_t* keycodes, MaaSize count)
+{
+ LogFunc << VAR_VOIDP(ctrl) << reinterpret_cast<const void*>(keycodes) << VAR(count);
+
+ if (!ctrl) {
+ LogError << "handle is null";
+ return MaaInvalidId;
+ }
+ if (!keycodes) {
+ LogError << "keycodes is null";
+ return MaaInvalidId;
+ }
+ if (count == 0 || count == MaaNullSize) {
+ LogError << "count is invalid" << VAR(count);
+ return MaaInvalidId;
+ }
+
+ std::vector<int> keys;
+ keys.reserve(static_cast<size_t>(count));
+ for (MaaSize i = 0; i < count; ++i) {
+ keys.emplace_back(keycodes[i]);
+ }
+
+ return ctrl->post_set_background_managed_keys(keys);
+}
+
</code_context>
<issue_to_address>
**question (bug_risk):** Empty key lists are rejected at the C API layer, which may conflict with higher-level bindings that can pass an empty sequence.
This treats `count == 0` as an error and returns `MaaInvalidId`. The new NodeJS binding calls `post_set_background_managed_keys` with `nullptr` when `keycodes` is empty, which will hit this path. Please clarify the intended contract: if empty lists are invalid, the bindings should enforce or document that; if empty should mean “clear managed keys”, the C API and implementation need to be updated to support that behavior instead of treating it as an error.
</issue_to_address>
### Comment 2
<location path="test/agent/agent_child_test.py" line_range="295-297" />
<code_context>
# 测试按键操作
controller.post_key_down(65).wait()
controller.post_key_up(65).wait()
+ assert (
+ not controller.post_set_background_managed_keys([0x57, 0x41]).wait().succeeded
+ ), "background managed keys should not be supported by non-Win32 controller"
# 测试滚动操作
</code_context>
<issue_to_address>
**suggestion (testing):** Add an agent test that exercises the positive remote Win32 path for background managed keys
The new assertion covers the rejection case for non‑Win32 controllers, but there’s still no test for the successful remote Win32 path that your agent changes added.
If CI supports it, please add a complementary test that:
- Starts an agent with a Win32 controller backend
- Invokes `controller.post_set_background_managed_keys([...]).wait()` from the child
- Asserts the job succeeds (and optionally that subsequent key actions still work)
This will exercise the new reverse message type end‑to‑end and guard against regressions in the agent protocol wiring.
Suggested implementation:
```python
# 测试滚动操作
assert not controller.post_scroll(0, 120).wait().succeeded
def test_agent_win32_background_managed_keys(win32_agent, win32_controller_factory):
"""
验证在 Win32 控制器后端下，后台托管按键配置能够通过 agent 正常下发并成功执行。
"""
# 启动带有 Win32 控制器后端的 agent 子进程
with win32_agent() as agent:
controller = win32_controller_factory(agent)
# 配置后台托管按键（例如 W 和 A）
job = controller.post_set_background_managed_keys([0x57, 0x41]).wait()
assert job.succeeded, "Win32 controller should support background managed keys"
# 后续按键操作仍然应该可以正常工作
controller.post_key_down(0x57).wait()
controller.post_key_up(0x57).wait()
controller.post_key_down(0x41).wait()
controller.post_key_up(0x41).wait()
```
1. Adjust the fixture names `win32_agent` and `win32_controller_factory` to match the existing test fixtures/utilities used to start an agent with a Win32 controller backend and to construct a remote controller proxy for that agent.
2. Ensure `win32_controller_factory(agent)` returns the same type of `controller` object used elsewhere in this file (i.e., one that exposes `post_set_background_managed_keys`, `post_key_down`, and `post_key_up` and routes them over the agent protocol).
3. If the agent startup API differs (e.g., context manager vs helper function, or additional configuration flags required to select the Win32 backend), adapt the `with win32_agent() as agent:` block accordingly.
4. If tests are platform-conditional, you may need to decorate `test_agent_win32_background_managed_keys` with the same skip/xfail markers (e.g., `@pytest.mark.windows`) used by other Win32‑specific tests in this suite.
</issue_to_address>
### Comment 3
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1127" />
<code_context>
+> [!NOTE]
+>
+> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released first before performing the click.
+
### `LongPressKey`
</code_context>
<issue_to_address>
**suggestion (typo):** Rephrase "released first before" to avoid redundant wording.
Consider rephrasing to something like “it will be released before performing the click” or “it will first be released, then the click is performed.”
```suggestion
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the click.
```
</issue_to_address>
### Comment 4
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1143" />
<code_context>
+> [!NOTE]
+>
+> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released first before performing the long press.
+
### `KeyDown`
</code_context>
<issue_to_address>
**suggestion (typo):** Adjust "released first before" for smoother grammar.
The phrase “it will be released first before performing the long press” is redundant. Consider simplifying to “it will be released before performing the long press” or “it will first be released, then the long press is performed.”
Suggested implementation:
```
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the click.
```
```
> [!NOTE]
>
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the long press.
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈来改进后续的 review。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In the NodeJS binding,
ControllerImpl::post_set_background_managed_keys's definition omits theenvparameter name but still usesenvin the body, which will not compile; align the cpp definition with the header by naming theEnvType envparameter. - In
ControllerAgent::handle_managed_click_key/handle_managed_long_press_key, the initialbackground_key_up(keycode)result is ignored, so a failure to release a stuck key before re-pressing is silently dropped; consider incorporating that return value intoretto surface real errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the NodeJS binding, `ControllerImpl::post_set_background_managed_keys`'s definition omits the `env` parameter name but still uses `env` in the body, which will not compile; align the cpp definition with the header by naming the `EnvType env` parameter.
- In `ControllerAgent::handle_managed_click_key`/`handle_managed_long_press_key`, the initial `background_key_up(keycode)` result is ignored, so a failure to release a stuck key before re-pressing is silently dropped; consider incorporating that return value into `ret` to surface real errors.
## Individual Comments
### Comment 1
<location path="source/Common/MaaController.cpp" line_range="255-264" />
<code_context>
return ctrl->post_key_up(keycode);
}
+MaaCtrlId MaaControllerPostSetBackgroundManagedKeys(MaaController* ctrl, const int32_t* keycodes, MaaSize count)
+{
+ LogFunc << VAR_VOIDP(ctrl) << reinterpret_cast<const void*>(keycodes) << VAR(count);
+
+ if (!ctrl) {
+ LogError << "handle is null";
+ return MaaInvalidId;
+ }
+ if (!keycodes) {
+ LogError << "keycodes is null";
+ return MaaInvalidId;
+ }
+ if (count == 0 || count == MaaNullSize) {
+ LogError << "count is invalid" << VAR(count);
+ return MaaInvalidId;
+ }
+
+ std::vector<int> keys;
+ keys.reserve(static_cast<size_t>(count));
+ for (MaaSize i = 0; i < count; ++i) {
+ keys.emplace_back(keycodes[i]);
+ }
+
+ return ctrl->post_set_background_managed_keys(keys);
+}
+
</code_context>
<issue_to_address>
**question (bug_risk):** Empty key lists are rejected at the C API layer, which may conflict with higher-level bindings that can pass an empty sequence.
This treats `count == 0` as an error and returns `MaaInvalidId`. The new NodeJS binding calls `post_set_background_managed_keys` with `nullptr` when `keycodes` is empty, which will hit this path. Please clarify the intended contract: if empty lists are invalid, the bindings should enforce or document that; if empty should mean “clear managed keys”, the C API and implementation need to be updated to support that behavior instead of treating it as an error.
</issue_to_address>
### Comment 2
<location path="test/agent/agent_child_test.py" line_range="295-297" />
<code_context>
# 测试按键操作
controller.post_key_down(65).wait()
controller.post_key_up(65).wait()
+ assert (
+ not controller.post_set_background_managed_keys([0x57, 0x41]).wait().succeeded
+ ), "background managed keys should not be supported by non-Win32 controller"
# 测试滚动操作
</code_context>
<issue_to_address>
**suggestion (testing):** Add an agent test that exercises the positive remote Win32 path for background managed keys
The new assertion covers the rejection case for non‑Win32 controllers, but there’s still no test for the successful remote Win32 path that your agent changes added.
If CI supports it, please add a complementary test that:
- Starts an agent with a Win32 controller backend
- Invokes `controller.post_set_background_managed_keys([...]).wait()` from the child
- Asserts the job succeeds (and optionally that subsequent key actions still work)
This will exercise the new reverse message type end‑to‑end and guard against regressions in the agent protocol wiring.
Suggested implementation:
```python
# 测试滚动操作
assert not controller.post_scroll(0, 120).wait().succeeded
def test_agent_win32_background_managed_keys(win32_agent, win32_controller_factory):
"""
验证在 Win32 控制器后端下，后台托管按键配置能够通过 agent 正常下发并成功执行。
"""
# 启动带有 Win32 控制器后端的 agent 子进程
with win32_agent() as agent:
controller = win32_controller_factory(agent)
# 配置后台托管按键（例如 W 和 A）
job = controller.post_set_background_managed_keys([0x57, 0x41]).wait()
assert job.succeeded, "Win32 controller should support background managed keys"
# 后续按键操作仍然应该可以正常工作
controller.post_key_down(0x57).wait()
controller.post_key_up(0x57).wait()
controller.post_key_down(0x41).wait()
controller.post_key_up(0x41).wait()
```
1. Adjust the fixture names `win32_agent` and `win32_controller_factory` to match the existing test fixtures/utilities used to start an agent with a Win32 controller backend and to construct a remote controller proxy for that agent.
2. Ensure `win32_controller_factory(agent)` returns the same type of `controller` object used elsewhere in this file (i.e., one that exposes `post_set_background_managed_keys`, `post_key_down`, and `post_key_up` and routes them over the agent protocol).
3. If the agent startup API differs (e.g., context manager vs helper function, or additional configuration flags required to select the Win32 backend), adapt the `with win32_agent() as agent:` block accordingly.
4. If tests are platform-conditional, you may need to decorate `test_agent_win32_background_managed_keys` with the same skip/xfail markers (e.g., `@pytest.mark.windows`) used by other Win32‑specific tests in this suite.
</issue_to_address>
### Comment 3
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1127" />
<code_context>
+> [!NOTE]
+>
+> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released first before performing the click.
+
### `LongPressKey`
</code_context>
<issue_to_address>
**suggestion (typo):** Rephrase "released first before" to avoid redundant wording.
Consider rephrasing to something like “it will be released before performing the click” or “it will first be released, then the click is performed.”
```suggestion
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the click.
```
</issue_to_address>
### Comment 4
<location path="docs/en_us/3.1-PipelineProtocol.md" line_range="1143" />
<code_context>
+> [!NOTE]
+>
+> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released first before performing the long press.
+
### `KeyDown`
</code_context>
<issue_to_address>
**suggestion (typo):** Adjust "released first before" for smoother grammar.
The phrase “it will be released first before performing the long press” is redundant. Consider simplifying to “it will be released before performing the long press” or “it will first be released, then the long press is performed.”
Suggested implementation:
```
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the click.
```
```
> [!NOTE]
>
> If a Win32 controller has declared a managed key domain through `MaaControllerPostSetBackgroundManagedKeys`, matching keys automatically route through the background guardian path. If a matching key is already being held by the guardian, it will be released before performing the long press.
```
</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
+295
to
+297
| assert ( | ||
| not controller.post_set_background_managed_keys([0x57, 0x41]).wait().succeeded | ||
| ), "background managed keys should not be supported by non-Win32 controller" |
Contributor
There was a problem hiding this comment.
suggestion (testing): 添加一个 agent 测试，用于覆盖后台托管按键在远程 Win32 路径上的成功场景。
新的断言覆盖了非 Win32 控制器被拒绝的情况，但对于你在 agent 端新增的远程 Win32 正向路径，目前仍然没有测试。
如果 CI 环境支持，请补充一个测试，用于：
- 启动一个使用 Win32 控制器后端的 agent
- 从子进程中调用
controller.post_set_background_managed_keys([...]).wait() - 断言该任务成功（并可选地验证后续按键操作仍然可以正常工作）
这将使新的反向消息类型得到端到端验证，并防止 agent 协议接线方面的回归。
建议的实现方式：
# 测试滚动操作
assert not controller.post_scroll(0, 120).wait().succeeded
def test_agent_win32_background_managed_keys(win32_agent, win32_controller_factory):
"""
验证在 Win32 控制器后端下，后台托管按键配置能够通过 agent 正常下发并成功执行。
"""
# 启动带有 Win32 控制器后端的 agent 子进程
with win32_agent() as agent:
controller = win32_controller_factory(agent)
# 配置后台托管按键（例如 W 和 A）
job = controller.post_set_background_managed_keys([0x57, 0x41]).wait()
assert job.succeeded, "Win32 controller should support background managed keys"
# 后续按键操作仍然应该可以正常工作
controller.post_key_down(0x57).wait()
controller.post_key_up(0x57).wait()
controller.post_key_down(0x41).wait()
controller.post_key_up(0x41).wait()- 将夹具名称
win32_agent和win32_controller_factory调整为与你用于启动带 Win32 控制器后端的 agent 以及构造远程控制器代理的既有测试夹具/工具保持一致。 - 确保
win32_controller_factory(agent)返回的controller对象类型与本文件其他位置使用的相同（即，它需要暴露post_set_background_managed_keys、post_key_down和post_key_up，并且通过 agent 协议转发这些调用）。 - 如果 agent 启动 API 有差异（例如上下文管理器 vs 帮助函数，或者需要额外的配置参数来选择 Win32 后端），请相应调整
with win32_agent() as agent:这一块。 - 若测试需要按平台区分执行，你可能需要为
test_agent_win32_background_managed_keys添加与本测试集内其他 Win32 专用测试相同的 skip/xfail 标记（例如@pytest.mark.windows）。
Original comment in English
suggestion (testing): Add an agent test that exercises the positive remote Win32 path for background managed keys
The new assertion covers the rejection case for non‑Win32 controllers, but there’s still no test for the successful remote Win32 path that your agent changes added.
If CI supports it, please add a complementary test that:
- Starts an agent with a Win32 controller backend
- Invokes
controller.post_set_background_managed_keys([...]).wait()from the child - Asserts the job succeeds (and optionally that subsequent key actions still work)
This will exercise the new reverse message type end‑to‑end and guard against regressions in the agent protocol wiring.
Suggested implementation:
# 测试滚动操作
assert not controller.post_scroll(0, 120).wait().succeeded
def test_agent_win32_background_managed_keys(win32_agent, win32_controller_factory):
"""
验证在 Win32 控制器后端下，后台托管按键配置能够通过 agent 正常下发并成功执行。
"""
# 启动带有 Win32 控制器后端的 agent 子进程
with win32_agent() as agent:
controller = win32_controller_factory(agent)
# 配置后台托管按键（例如 W 和 A）
job = controller.post_set_background_managed_keys([0x57, 0x41]).wait()
assert job.succeeded, "Win32 controller should support background managed keys"
# 后续按键操作仍然应该可以正常工作
controller.post_key_down(0x57).wait()
controller.post_key_up(0x57).wait()
controller.post_key_down(0x41).wait()
controller.post_key_up(0x41).wait()- Adjust the fixture names
win32_agentandwin32_controller_factoryto match the existing test fixtures/utilities used to start an agent with a Win32 controller backend and to construct a remote controller proxy for that agent. - Ensure
win32_controller_factory(agent)returns the same type ofcontrollerobject used elsewhere in this file (i.e., one that exposespost_set_background_managed_keys,post_key_down, andpost_key_upand routes them over the agent protocol). - If the agent startup API differs (e.g., context manager vs helper function, or additional configuration flags required to select the Win32 backend), adapt the
with win32_agent() as agent:block accordingly. - If tests are platform-conditional, you may need to decorate
test_agent_win32_background_managed_keyswith the same skip/xfail markers (e.g.,@pytest.mark.windows) used by other Win32‑specific tests in this suite.
Contributor
|
一样的问题，专门为一个具体的controller加一个通用的接口实在是太奇怪了 |
Contributor
Author
确实，不过我们目前好像不能同时使用两个键盘控制器，而这个方案在除了”给明日方舟终末地在没有焦点但是收到WM_ACTIVATE消息后的状态下让其可以在后台移动“之外没有任何已知的用途，定语少一个都不行，它甚至没法正常的输入移动之外的键，所以声明为一个controller的话大部分任务都不能完成了 |
Member
|
ControlUnit 重构了下，有冲突了 #1235 |
ZeroAd-06
force-pushed
the
feat/win32-background-managed-keys
branch
from
261854a to
ba24fd3
Compare
- Move set_background_managed_keys_option into ControlUnitAPI virtual section with override - Rename kCamelCase constants to snake_case per local convention - Document that BackgroundManagedKeyInput deliberately does not derive from InputBase - Annotate the _locked suffix mutex convention
ZeroAd-06
force-pushed
the
feat/win32-background-managed-keys
branch
from
ba24fd3 to
b906551
Compare
Contributor
Author
|
调整了一下接口实现。不过仍然看起来有一点不正常，我猜可能是需求本身就有点奇怪（但如果要实现终末地的后台移动的话，大概只能这么做了）。总之，现在它应该可以被再次 review 了。 |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体性的反馈：
- 可以考虑支持空的
BackgroundManagedKeys列表，用来清空/禁用受管域（而不是把val_size == 0/ 归一化后为空的 key 当作错误），这样调用方可以在不重新创建 controller 的情况下动态关闭 guardian。 BackgroundManagedKeyInput中用于派生RegisterHotKeyID 的managed_hotkey_base常量（2000）有点“魔法数”的感觉，而且很容易与同一进程中的其他热键发生冲突；更安全的做法是要么清晰地文档化这一约束，要么封装 ID 的分配逻辑，确保不会和其他用途重叠。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- Consider supporting an empty `BackgroundManagedKeys` list as a way to clear/disable the managed domain (instead of treating `val_size == 0`/empty normalized keys as an error), so callers can dynamically turn the guardian off without recreating the controller.
- The `managed_hotkey_base` constant (2000) used to derive `RegisterHotKey` IDs in `BackgroundManagedKeyInput` is a bit magic and could easily collide with other hotkeys in the same process; it would be safer to either document the constraint clearly or encapsulate ID allocation so it cannot overlap with other uses.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="351-355" />
<code_context>
+ LogError << "background_keyboard_ is null";
+ return false;
+ }
+ return background_keyboard_->key_down(key) && background_keyboard_->key_up(key);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling partial failure when clicking a managed key to avoid leaving it stuck down.
Here, `click_key` returns `key_down && key_up`. If `key_down` succeeds but `key_up` fails, the method returns `false` but the key may remain logically pressed. Depending on `BackgroundManagedKeyInput`’s failure semantics, consider either always attempting `key_up` even after an error, or at least logging the case where `key_down` succeeded but `key_up` failed to make the inconsistent state visible to callers.
```suggestion
if (!background_keyboard_) {
LogError << "background_keyboard_ is null";
return false;
}
const bool key_down_ok = background_keyboard_->key_down(key);
const bool key_up_ok = background_keyboard_->key_up(key);
if (key_down_ok && !key_up_ok) {
LogError << "Managed key " << key << " key_down succeeded but key_up failed; key may remain logically pressed.";
}
return key_down_ok && key_up_ok;
```
</issue_to_address>
### Comment 2
<location path="source/MaaAgentServer/RemoteInstance/RemoteController.cpp" line_range="45-43" />
<code_context>
ret = controller->set_option(key, &v, sizeof(v));
break;
}
+ case MaaCtrlOption_BackgroundManagedKeys: {
+ std::vector<int32_t> keys;
+ for (const auto& item : req.value.as_array()) {
+ keys.push_back(static_cast<int32_t>(item.as_integer()));
+ }
+ ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
+ break;
+ }
default:
</code_context>
<issue_to_address>
**issue (bug_risk):** Add JSON type validation for BackgroundManagedKeys to avoid undefined behavior on malformed input.
This code assumes `req.value` is always an array. If a client sends a payload where `value` is not an array, `as_array()` / `as_integer()` may throw or trigger undefined behavior, depending on the JSON library. Please add a type check (e.g. `req.value.is_array()`) and reject the request with a clear log when the type is wrong, similar to how you handle size validation elsewhere.
</issue_to_address>
### Comment 3
<location path="source/MaaAgentClient/Client/AgentClient.cpp" line_range="2463-2461" />
<code_context>
ret = controller->set_option(key, &v, sizeof(v));
break;
}
+ case MaaCtrlOption_BackgroundManagedKeys: {
+ std::vector<int32_t> keys;
+ for (const auto& item : req.value.as_array()) {
+ keys.push_back(static_cast<int32_t>(item.as_integer()));
+ }
+ ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
+ break;
+ }
default:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid sending an empty key buffer if the request array is empty, or define the intended semantics explicitly.
In this branch, an empty `req.value.as_array()` produces `keys.size() == 0` and you still pass `keys.data()` with `val_size == 0` into `set_option`. `ControllerAgent::set_background_managed_keys_option` currently treats `val_size == 0` as an error, so an empty array can’t be used to “clear” background managed keys and instead silently fails. Please either handle `keys.empty()` explicitly (and define whether that should clear or be rejected), or document that this option requires a non-empty array so callers don’t assume a clear behavior that isn’t implemented.
Suggested implementation:
```cpp
case MaaCtrlOption_BackgroundManagedKeys: {
const auto& array = req.value.as_array();
if (array.empty()) {
// An empty array is treated as "clear all background managed keys".
// This relies on ControllerAgent::set_background_managed_keys_option
// accepting (val == nullptr, val_size == 0) as a valid "clear" operation.
ret = controller->set_option(key, nullptr, 0);
break;
}
std::vector<int32_t> keys;
keys.reserve(array.size());
for (const auto& item : array) {
keys.push_back(static_cast<int32_t>(item.as_integer()));
}
ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
break;
}
```
To fully implement the "empty array clears keys" semantics, you will also need to update `ControllerAgent::set_background_managed_keys_option` (or the corresponding handler of `MaaCtrlOption_BackgroundManagedKeys`) so that it:
1. Interprets `val_size == 0` (and preferably `val == nullptr`) as a request to clear all background managed keys.
2. Returns a success result code for this case instead of treating it as an error.
3. Optionally, document this behavior near the option definition so callers know that an empty array is the supported way to clear background managed keys.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈持续改进评审质量。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- Consider supporting an empty
BackgroundManagedKeyslist as a way to clear/disable the managed domain (instead of treatingval_size == 0/empty normalized keys as an error), so callers can dynamically turn the guardian off without recreating the controller. - The
managed_hotkey_baseconstant (2000) used to deriveRegisterHotKeyIDs inBackgroundManagedKeyInputis a bit magic and could easily collide with other hotkeys in the same process; it would be safer to either document the constraint clearly or encapsulate ID allocation so it cannot overlap with other uses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider supporting an empty `BackgroundManagedKeys` list as a way to clear/disable the managed domain (instead of treating `val_size == 0`/empty normalized keys as an error), so callers can dynamically turn the guardian off without recreating the controller.
- The `managed_hotkey_base` constant (2000) used to derive `RegisterHotKey` IDs in `BackgroundManagedKeyInput` is a bit magic and could easily collide with other hotkeys in the same process; it would be safer to either document the constraint clearly or encapsulate ID allocation so it cannot overlap with other uses.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="351-355" />
<code_context>
+ LogError << "background_keyboard_ is null";
+ return false;
+ }
+ return background_keyboard_->key_down(key) && background_keyboard_->key_up(key);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling partial failure when clicking a managed key to avoid leaving it stuck down.
Here, `click_key` returns `key_down && key_up`. If `key_down` succeeds but `key_up` fails, the method returns `false` but the key may remain logically pressed. Depending on `BackgroundManagedKeyInput`’s failure semantics, consider either always attempting `key_up` even after an error, or at least logging the case where `key_down` succeeded but `key_up` failed to make the inconsistent state visible to callers.
```suggestion
if (!background_keyboard_) {
LogError << "background_keyboard_ is null";
return false;
}
const bool key_down_ok = background_keyboard_->key_down(key);
const bool key_up_ok = background_keyboard_->key_up(key);
if (key_down_ok && !key_up_ok) {
LogError << "Managed key " << key << " key_down succeeded but key_up failed; key may remain logically pressed.";
}
return key_down_ok && key_up_ok;
```
</issue_to_address>
### Comment 2
<location path="source/MaaAgentServer/RemoteInstance/RemoteController.cpp" line_range="45-43" />
<code_context>
ret = controller->set_option(key, &v, sizeof(v));
break;
}
+ case MaaCtrlOption_BackgroundManagedKeys: {
+ std::vector<int32_t> keys;
+ for (const auto& item : req.value.as_array()) {
+ keys.push_back(static_cast<int32_t>(item.as_integer()));
+ }
+ ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
+ break;
+ }
default:
</code_context>
<issue_to_address>
**issue (bug_risk):** Add JSON type validation for BackgroundManagedKeys to avoid undefined behavior on malformed input.
This code assumes `req.value` is always an array. If a client sends a payload where `value` is not an array, `as_array()` / `as_integer()` may throw or trigger undefined behavior, depending on the JSON library. Please add a type check (e.g. `req.value.is_array()`) and reject the request with a clear log when the type is wrong, similar to how you handle size validation elsewhere.
</issue_to_address>
### Comment 3
<location path="source/MaaAgentClient/Client/AgentClient.cpp" line_range="2463-2461" />
<code_context>
ret = controller->set_option(key, &v, sizeof(v));
break;
}
+ case MaaCtrlOption_BackgroundManagedKeys: {
+ std::vector<int32_t> keys;
+ for (const auto& item : req.value.as_array()) {
+ keys.push_back(static_cast<int32_t>(item.as_integer()));
+ }
+ ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
+ break;
+ }
default:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid sending an empty key buffer if the request array is empty, or define the intended semantics explicitly.
In this branch, an empty `req.value.as_array()` produces `keys.size() == 0` and you still pass `keys.data()` with `val_size == 0` into `set_option`. `ControllerAgent::set_background_managed_keys_option` currently treats `val_size == 0` as an error, so an empty array can’t be used to “clear” background managed keys and instead silently fails. Please either handle `keys.empty()` explicitly (and define whether that should clear or be rejected), or document that this option requires a non-empty array so callers don’t assume a clear behavior that isn’t implemented.
Suggested implementation:
```cpp
case MaaCtrlOption_BackgroundManagedKeys: {
const auto& array = req.value.as_array();
if (array.empty()) {
// An empty array is treated as "clear all background managed keys".
// This relies on ControllerAgent::set_background_managed_keys_option
// accepting (val == nullptr, val_size == 0) as a valid "clear" operation.
ret = controller->set_option(key, nullptr, 0);
break;
}
std::vector<int32_t> keys;
keys.reserve(array.size());
for (const auto& item : array) {
keys.push_back(static_cast<int32_t>(item.as_integer()));
}
ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
break;
}
```
To fully implement the "empty array clears keys" semantics, you will also need to update `ControllerAgent::set_background_managed_keys_option` (or the corresponding handler of `MaaCtrlOption_BackgroundManagedKeys`) so that it:
1. Interprets `val_size == 0` (and preferably `val == nullptr`) as a request to clear all background managed keys.
2. Returns a success result code for this case instead of treating it as an error.
3. Optionally, document this behavior near the option definition so callers know that an empty array is the supported way to clear background managed keys.
</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
+351
to
+355
| if (!background_keyboard_) { | ||
| LogError << "background_keyboard_ is null"; | ||
| return false; | ||
| } | ||
| return background_keyboard_->key_down(key) && background_keyboard_->key_up(key); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): 建议在点击受管按键时处理部分失败的情况，避免按键卡在按下状态。
这里 click_key 返回的是 key_down && key_up。如果 key_down 成功但 key_up 失败，方法会返回 false，但该按键可能在逻辑上仍然保持按下。根据 BackgroundManagedKeyInput 的失败语义，建议即便发生错误也始终尝试执行 key_up，或者至少在 key_down 成功而 key_up 失败时打日志，让调用方能看到这种不一致状态。
Suggested change
| if (!background_keyboard_) { | |
| LogError << "background_keyboard_ is null"; | |
| return false; | |
| } | |
| return background_keyboard_->key_down(key) && background_keyboard_->key_up(key); | |
| if (!background_keyboard_) { | |
| LogError << "background_keyboard_ is null"; | |
| return false; | |
| } | |
| const bool key_down_ok = background_keyboard_->key_down(key); | |
| const bool key_up_ok = background_keyboard_->key_up(key); | |
| if (key_down_ok && !key_up_ok) { | |
| LogError << "Managed key " << key << " key_down succeeded but key_up failed; key may remain logically pressed."; | |
| } | |
| return key_down_ok && key_up_ok; |
Original comment in English
suggestion (bug_risk): Consider handling partial failure when clicking a managed key to avoid leaving it stuck down.
Here, click_key returns key_down && key_up. If key_down succeeds but key_up fails, the method returns false but the key may remain logically pressed. Depending on BackgroundManagedKeyInput’s failure semantics, consider either always attempting key_up even after an error, or at least logging the case where key_down succeeded but key_up failed to make the inconsistent state visible to callers.
Suggested change
| if (!background_keyboard_) { | |
| LogError << "background_keyboard_ is null"; | |
| return false; | |
| } | |
| return background_keyboard_->key_down(key) && background_keyboard_->key_up(key); | |
| if (!background_keyboard_) { | |
| LogError << "background_keyboard_ is null"; | |
| return false; | |
| } | |
| const bool key_down_ok = background_keyboard_->key_down(key); | |
| const bool key_up_ok = background_keyboard_->key_up(key); | |
| if (key_down_ok && !key_up_ok) { | |
| LogError << "Managed key " << key << " key_down succeeded but key_up failed; key may remain logically pressed."; | |
| } | |
| return key_down_ok && key_up_ok; |
| @@ -42,6 +42,21 @@ bool RemoteController::set_option(MaaCtrlOption key, MaaOptionValue value, MaaOp | |||
| jvalue = *reinterpret_cast<const bool*>(value); | |||
| break; | |||
Contributor
There was a problem hiding this comment.
issue (bug_risk): 建议为 BackgroundManagedKeys 增加 JSON 类型校验，以避免在输入格式错误时产生未定义行为。
当前代码假设 req.value 一定是数组。如果客户端发送的 payload 中 value 不是数组，as_array() / as_integer() 可能会抛异常或触发未定义行为，这取决于所用 JSON 库。请增加类型检查（例如 req.value.is_array()），并在类型错误时用清晰的日志拒绝该请求，类似你在其他地方对大小进行校验的方式。
Original comment in English
issue (bug_risk): Add JSON type validation for BackgroundManagedKeys to avoid undefined behavior on malformed input.
This code assumes req.value is always an array. If a client sends a payload where value is not an array, as_array() / as_integer() may throw or trigger undefined behavior, depending on the JSON library. Please add a type check (e.g. req.value.is_array()) and reject the request with a clear log when the type is wrong, similar to how you handle size validation elsewhere.
| @@ -2460,6 +2460,14 @@ bool AgentClient::handle_controller_set_option(const json::value& j) | |||
| ret = controller->set_option(key, &v, sizeof(v)); | |||
| break; | |||
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): 建议在请求数组为空时避免发送空的按键缓冲区，或者明确规定这种情况的语义。
在这个分支中，如果 req.value.as_array() 为空，会得到 keys.size() == 0，但你仍然把 keys.data() 和 val_size == 0 传入 set_option。目前 ControllerAgent::set_background_managed_keys_option 会把 val_size == 0 当作错误处理，因此空数组不能用于“清空”后台受管按键，而是会静默失败。请显式处理 keys.empty() 的情况（并定义这是执行清空还是直接拒绝），或者在文档中说明该选项要求非空数组，以免调用方误以为支持清空行为。
建议实现：
case MaaCtrlOption_BackgroundManagedKeys: {
const auto& array = req.value.as_array();
if (array.empty()) {
// An empty array is treated as "clear all background managed keys".
// This relies on ControllerAgent::set_background_managed_keys_option
// accepting (val == nullptr, val_size == 0) as a valid "clear" operation.
ret = controller->set_option(key, nullptr, 0);
break;
}
std::vector<int32_t> keys;
keys.reserve(array.size());
for (const auto& item : array) {
keys.push_back(static_cast<int32_t>(item.as_integer()));
}
ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
break;
}
要完全实现“空数组清空按键”的语义，还需要更新 ControllerAgent::set_background_managed_keys_option（或 MaaCtrlOption_BackgroundManagedKeys 的对应处理逻辑），使其：
- 将
val_size == 0（以及最好同时val == nullptr）解释为清空所有后台受管按键的请求。 - 对这种情况返回成功结果码，而不是当作错误处理。
- 可选：在该选项的定义附近文档化这一行为，让调用方知道空数组是清空后台受管按键的受支持方式。
Original comment in English
suggestion (bug_risk): Avoid sending an empty key buffer if the request array is empty, or define the intended semantics explicitly.
In this branch, an empty req.value.as_array() produces keys.size() == 0 and you still pass keys.data() with val_size == 0 into set_option. ControllerAgent::set_background_managed_keys_option currently treats val_size == 0 as an error, so an empty array can’t be used to “clear” background managed keys and instead silently fails. Please either handle keys.empty() explicitly (and define whether that should clear or be rejected), or document that this option requires a non-empty array so callers don’t assume a clear behavior that isn’t implemented.
Suggested implementation:
case MaaCtrlOption_BackgroundManagedKeys: {
const auto& array = req.value.as_array();
if (array.empty()) {
// An empty array is treated as "clear all background managed keys".
// This relies on ControllerAgent::set_background_managed_keys_option
// accepting (val == nullptr, val_size == 0) as a valid "clear" operation.
ret = controller->set_option(key, nullptr, 0);
break;
}
std::vector<int32_t> keys;
keys.reserve(array.size());
for (const auto& item : array) {
keys.push_back(static_cast<int32_t>(item.as_integer()));
}
ret = controller->set_option(key, keys.data(), sizeof(int32_t) * keys.size());
break;
}
To fully implement the "empty array clears keys" semantics, you will also need to update ControllerAgent::set_background_managed_keys_option (or the corresponding handler of MaaCtrlOption_BackgroundManagedKeys) so that it:
- Interprets
val_size == 0(and preferablyval == nullptr) as a request to clear all background managed keys. - Returns a success result code for this case instead of treating it as an error.
- Optionally, document this behavior near the option definition so callers know that an empty array is the supported way to clear background managed keys.
MistEO
reviewed
Apr 20, 2026
Member
There was a problem hiding this comment.
🤖 对照刚合入的 #1232（MouseLockFollow）重看了下 — MaaCtrlOption_* + ControllerAgent::set_*_option 分发 + dynamic_pointer_cast<Win32ControlUnitAPI> 路由 + 其他 controller return false，这个 pattern 跟 #1232 完全对齐，那 #1231 架构层面就没问题了，之前那条收回（
剩下几个点还是想 raise 下：
-
send_f13_nudge是游戏特化，不该进 maafw 本体BackgroundManagedKeyInput::send_f13_nudge直接写死在 guard loop 和按下路径里，注释和文档里都明说是给终末地的 ——docs/en_us/2.2-IntegratedInterfaceOverview.md里甚至写了 "validated only for Endfield background WASD"，中文版同款。这是红线吧，maafw 文档不能出现具体游戏名，游戏特化行为也不能写死在 win32 input 层。建议把 F13 nudge 抽到上层（pipeline 定义或使用方 task 里按需发KeyDown(F13)），BackgroundManagedKeyInput只负责"让受管键保持期望状态"这一件事；文档里的 Endfield / 终末地字样也一并去掉。 -
sourcery 4-19 那轮提的 3 个 bug_risk 还没处理
Win32ControlUnitMgr::click_key里background_keyboard_->key_down(k) && background_keyboard_->key_up(k)—— 短路求值会让key_down成功但key_up失败时键留在按下状态（函数返回 false 但键没抬）。分两步 eval 并保证key_up一定执行。RemoteController.cpp的MaaCtrlOption_BackgroundManagedKeys分支假设req.value一定是 array，直接as_array()/as_integer()，类型不对会抛异常。加个is_array()/is_integer()校验。AgentClient.cpp空 array 会以(keys.data(), 0)进入set_option，而ControllerAgent::set_background_managed_keys_option又把val_size == 0当 error 吃掉，链路上两处语义打架。要么 binding 层就拒掉空列表，要么统一解释成"clear managed domain"（后者更有用，能动态关 guardian 不用重建 controller）。
-
Python
controller.py夹带了无关 formatting 改动看 diff 有
MaaControllerGetInfo调用被合成单行、WlRootsController.__init__参数缩进从 8 空格改成 4、set_mouse_lock_follow函数体被 autoformat 重排、_set_adb_api_properties/CustomController/ControllerEventSink/_on_raw_notification前的空行被删了 — 大概率是本地 black/ruff 顺手改的，和本 feature 无关，拆到独立 PR 去。 -
PR description 的测试 checkbox 全空着
test/python/binding_test.py里的新用例主要是 "声明 managed keys → 成功/失败" 级别，没覆盖按键持续性校正、多键并发、PostInactive释放这些关键路径。实机终末地 WASD 验证那条也还没勾，正面路径心里没底。补一下。
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
3 participants
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.
概述
为 Win32 控制器添加后台托管按键守护（Background Managed Key Guardian）能力，支持在后台持续维持指定按键的按下/释放状态。这是"实现终末地后台移动"计划的一部分，解决后台长按 WASD 移动的需求。
动机
《明日方舟：终末地》等游戏使用 WASD 控制角色移动，需要按键在后台被持续按住。传统的
PostMessage方式无法可靠地维持按键状态，因为系统层面的热键拦截、焦点切换等因素会导致按键"丢失"。本 PR 引入专用守护线程，以 5ms 间隔轮询 + 差量校正的方式确保按键状态一致性。实现方案
核心组件：
BackgroundManagedKeyInputGetAsyncKeyState检测实际按键状态，与期望状态做差量校正RegisterHotKey→SendInput→UnregisterHotKey的三步法确保按键注入不被系统吃掉透明路由机制
ControllerAgent的handle_click_key/handle_key_down/handle_key_up/handle_long_press_key自动检测受管键，命中则走后台守护路径生命周期管理
MaaControllerPostInactive自动释放所有受管键post_stop()自动 post inactive action 释放持有的输入状态API 变更
新增 C API：
新增 Python / Node.js 绑定：
post_set_background_managed_keys(keys)行为变更：
MaaControllerPostInactive现在会额外释放所有后台受管键变更范围
BackgroundManagedKeyInput守护类（~500 行核心实现）测试计划
post_set_background_managed_keys此 PR 是"实现终末地后台移动"计划的按键控制部分，与 feat/win32-mouse-lock-follow（鼠标锁定跟随）配合使用。
🤖 Generated with Claude Code
Summary by Sourcery
为 Win32 后台托管密钥 guardian 提供支持，并通过 Python、Node.js、agent 客户端/服务端的控制器选项和绑定暴露该功能，同时更新相应的测试和文档。
新特性：
BackgroundManagedKeyInputguardian，用于在托管域中维护指定按键，并通过该 guardian 路由匹配的按键操作。MaaCtrlOption_BackgroundManagedKeys控制器选项，并将其贯通到ControllerAgent、Win32ControlUnitMgr和Win32ControlUnitAPI。增强：
文档：
BackgroundManagedKeys控制器选项以及 Win32 后台托管密钥 guardian 的行为。测试：
Original summary in English
Summary by Sourcery
Add Win32 background managed key guardian support and expose it through controller options and bindings for Python, Node.js, agent client/server, with tests and docs updated accordingly.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
为 Win32 后台托管密钥 guardian 提供支持，并通过 Python、Node.js、agent 客户端/服务端的控制器选项和绑定暴露该功能，同时更新相应的测试和文档。
新特性：
BackgroundManagedKeyInputguardian，用于在托管域中维护指定按键，并通过该 guardian 路由匹配的按键操作。MaaCtrlOption_BackgroundManagedKeys控制器选项，并将其贯通到ControllerAgent、Win32ControlUnitMgr和Win32ControlUnitAPI。增强：
文档：
BackgroundManagedKeys控制器选项以及 Win32 后台托管密钥 guardian 的行为。测试：
Original summary in English
Summary by Sourcery
Add Win32 background managed key guardian support and expose it through controller options and bindings for Python, Node.js, agent client/server, with tests and docs updated accordingly.
New Features:
Enhancements:
Documentation:
Tests: