feat(linux): Wayland(kwin)下的截图和控制#1357
Open
lava114514 wants to merge 10 commits into
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 个问题，并给出了一些高层次的反馈：
KWinControlUnitMgr::get_features对外声明了MaaControllerFeature_UseKeyboardDownAndUpInsteadOfClick，但key_down/key_up目前尚未实现并且总是返回false，因此要么实现键盘 uinput 处理，要么移除该特性标志，以避免调用方误以为键盘事件可用。- 在
PipeWireScreencap中，有若干未使用或只部分接线的成员（core_events_、stream_mutex_/stream_cv_/stream_params_received_，pw_on_core_error从未被挂接），这增加了复杂度，也可能让未来的维护者困惑；建议要么完整地接好 core 事件处理，要么移除这些未使用的状态和回调。 - D-Bus 辅助方法（
dbus_create_session、dbus_select_sources、dbus_start_stream）各自单独调用dbus_bus_get并打开新的连接；建议复用一个单一的DBusConnection，或清晰地文档化/集中管理连接，以避免重复的初始化和潜在的资源抖动。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `KWinControlUnitMgr::get_features` 对外声明了 `MaaControllerFeature_UseKeyboardDownAndUpInsteadOfClick`，但 `key_down`/`key_up` 目前尚未实现并且总是返回 `false`，因此要么实现键盘 uinput 处理，要么移除该特性标志，以避免调用方误以为键盘事件可用。
- 在 `PipeWireScreencap` 中，有若干未使用或只部分接线的成员（`core_events_`、`stream_mutex_`/`stream_cv_`/`stream_params_received_`，`pw_on_core_error` 从未被挂接），这增加了复杂度，也可能让未来的维护者困惑；建议要么完整地接好 core 事件处理，要么移除这些未使用的状态和回调。
- D-Bus 辅助方法（`dbus_create_session`、`dbus_select_sources`、`dbus_start_stream`）各自单独调用 `dbus_bus_get` 并打开新的连接；建议复用一个单一的 `DBusConnection`，或清晰地文档化/集中管理连接，以避免重复的初始化和潜在的资源抖动。
## Individual Comments
### Comment 1
<location path="source/MaaKWinControlUnit/Input/UInputController.cpp" line_range="71-73" />
<code_context>
+ LogInfo << "UInput device closed";
+}
+
+bool UInputController::connected() const
+{
+ return connected_;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `connected()` 在未加锁的情况下读取共享状态，这可能与 `open()`/`close()` 产生竞争。
`connected_` 在 `mutex_` 保护下被写入，但在这里读取时没有加锁。如果 `connected()` 可以与 `open()`/`close()` 并发运行，这就是一个数据竞争。可以将 `connected_` 改为 `std::atomic<bool>`，或者在 `connected()` 中获取 `mutex_`，以保持同步方式一致。
</issue_to_address>
### Comment 2
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.h" line_range="112-125" />
<code_context>
+ struct spa_hook stream_hook_;
+
+ /* Heap-allocated event vtables (must outlive the listener) */
+ struct pw_core_events* core_events_ = nullptr;
+ struct pw_stream_events* stream_events_ = nullptr;
+
+ /* Negotiated frame dimensions (set by param_changed callback) */
+ int frame_width_ = 0;
+ int frame_height_ = 0;
+
+ /* Synchronisation: stream parameters received */
+ std::mutex stream_mutex_;
+ std::condition_variable stream_cv_;
+ bool stream_params_received_ = false;
+
+ /* Latest frame data (protected by frame_mutex_) */
</code_context>
<issue_to_address>
**suggestion:** 若干成员（`core_events_`、`stream_mutex_`/`stream_cv_`、`stream_params_received_`）从未使用。
这些成员被声明但从未使用，会增加噪音，并暗示有未完成的行为。请考虑移除它们，或者在本次变更中真正接入（例如用于 core 错误监听或同步参数协商），以免在类中留下无用的状态。
```suggestion
/* Heap-allocated event vtables (must outlive the listener) */
struct pw_stream_events* stream_events_ = nullptr;
/* Negotiated frame dimensions (set by param_changed callback) */
int frame_width_ = 0;
int frame_height_ = 0;
/* Latest frame data (protected by frame_mutex_) */
```
</issue_to_address>
### Comment 3
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.cpp" line_range="1052-152" />
<code_context>
+ need_unmap = true;
+ }
+ }
+ else if (data.type == SPA_DATA_DmaBuf) {
+ LogError << "DMABUF not supported (need EGL import)";
+ return false;
+ }
+ else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 对 DMABUF buffer 直接失败可能会在 compositor 切换 buffer 类型时导致捕获中断。
在 `process_frame` 中，当前对基于 DMABUF 的 buffer 是记录日志后返回 `false`。在那些只提供 DMABUF（现在或未来的 KWin/PipeWire 版本） 的系统上，一旦 buffer 类型切换，这会让屏幕捕获永久失败。如果 EGL import 还未实现，建议清楚地暴露这一限制（比如通过 `get_info()`/特性标志），或者增加回退方案（例如丢弃该帧但保留上一帧图像，这样 `screencap()` 仍能成功）。
建议实现方式：
```cpp
else if (data.type == SPA_DATA_DmaBuf) {
if (!m_loggedDmabufUnsupported) {
LogWarning << "DMABUF buffers not yet supported; dropping frame (need EGL import)";
m_loggedDmabufUnsupported = true;
}
// Mark this frame as DMABUF-backed so we can drop it gracefully later,
// keeping the previously captured image available to callers.
is_dmabuf_frame = true;
continue;
}
```
```cpp
if (!mapped_ptr) {
// If this was a DMABUF-backed frame, drop it but keep the previous image
// so screencap() can still succeed instead of hard-failing.
if (is_dmabuf_frame) {
return true;
}
return false;
}
```
1. 确保 `process_frame`（或其所在的函数）在 buffer 循环前定义并初始化一个本地标志：
- 在函数顶部附近添加：
```cpp
bool is_dmabuf_frame = false;
```
2. 在截屏类中添加一个成员，用于只记录一次 DMABUF 限制日志：
- 在类声明（头文件）中添加：
```cpp
bool m_loggedDmabufUnsupported = false;
```
如果该代码库不使用默认成员初始化，则在构造函数中初始化。
3. 通过现有的能力/自省 API 暴露缺失的 DMABUF 支持：
- 在报告截屏能力的 `get_info()` 或等价函数中，增加一个类似 `supportsDmabuf = false` 的标志（或遵循现有约定的名称），以便调用方可以检测并据此处理。
4. 如果调用方假定成功的 `process_frame()` 总是会更新底层图像，你可能需要调整这部分逻辑，使得返回 `true` 且 `is_dmabuf_frame == true` 表示“没有新帧；保留上一帧图像”，或者引入显式的状态枚举来替代简单的 `bool`。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- KWinControlUnitMgr::get_features advertises MaaControllerFeature_UseKeyboardDownAndUpInsteadOfClick, but key_down/key_up are currently unimplemented and always return false, so either implement keyboard uinput handling or drop that feature flag to avoid callers assuming working key events.
- In PipeWireScreencap there are several unused or partially wired members (core_events_, stream_mutex_/stream_cv_/stream_params_received_, pw_on_core_error is never hooked), which adds complexity and may confuse future maintainers; consider either fully wiring core event handling or removing the unused state and callbacks.
- The D-Bus helper methods (dbus_create_session, dbus_select_sources, dbus_start_stream) each call dbus_bus_get separately and open fresh connections; consider reusing a single DBusConnection or clearly documenting/centralising connection management to avoid repeated setup and potential resource churn.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- KWinControlUnitMgr::get_features advertises MaaControllerFeature_UseKeyboardDownAndUpInsteadOfClick, but key_down/key_up are currently unimplemented and always return false, so either implement keyboard uinput handling or drop that feature flag to avoid callers assuming working key events.
- In PipeWireScreencap there are several unused or partially wired members (core_events_, stream_mutex_/stream_cv_/stream_params_received_, pw_on_core_error is never hooked), which adds complexity and may confuse future maintainers; consider either fully wiring core event handling or removing the unused state and callbacks.
- The D-Bus helper methods (dbus_create_session, dbus_select_sources, dbus_start_stream) each call dbus_bus_get separately and open fresh connections; consider reusing a single DBusConnection or clearly documenting/centralising connection management to avoid repeated setup and potential resource churn.
## Individual Comments
### Comment 1
<location path="source/MaaKWinControlUnit/Input/UInputController.cpp" line_range="71-73" />
<code_context>
+ LogInfo << "UInput device closed";
+}
+
+bool UInputController::connected() const
+{
+ return connected_;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** connected() reads shared state without locking, which can race with open()/close().
`connected_` is written under `mutex_`, but read here without locking. If `connected()` can run concurrently with `open()`/`close()`, this is a data race. Either make `connected_` an `std::atomic<bool>` or take `mutex_` in `connected()` to keep synchronization consistent.
</issue_to_address>
### Comment 2
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.h" line_range="112-125" />
<code_context>
+ struct spa_hook stream_hook_;
+
+ /* Heap-allocated event vtables (must outlive the listener) */
+ struct pw_core_events* core_events_ = nullptr;
+ struct pw_stream_events* stream_events_ = nullptr;
+
+ /* Negotiated frame dimensions (set by param_changed callback) */
+ int frame_width_ = 0;
+ int frame_height_ = 0;
+
+ /* Synchronisation: stream parameters received */
+ std::mutex stream_mutex_;
+ std::condition_variable stream_cv_;
+ bool stream_params_received_ = false;
+
+ /* Latest frame data (protected by frame_mutex_) */
</code_context>
<issue_to_address>
**suggestion:** Several members (core_events_, stream_mutex_/stream_cv_, stream_params_received_) are never used.
These members are declared but never used, which adds noise and implies unfinished behavior. Please either remove them or wire them into the current change (e.g., for core error listening or synchronous param negotiation) so we don’t leave dead state in the class.
```suggestion
/* Heap-allocated event vtables (must outlive the listener) */
struct pw_stream_events* stream_events_ = nullptr;
/* Negotiated frame dimensions (set by param_changed callback) */
int frame_width_ = 0;
int frame_height_ = 0;
/* Latest frame data (protected by frame_mutex_) */
```
</issue_to_address>
### Comment 3
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.cpp" line_range="1052-152" />
<code_context>
+ need_unmap = true;
+ }
+ }
+ else if (data.type == SPA_DATA_DmaBuf) {
+ LogError << "DMABUF not supported (need EGL import)";
+ return false;
+ }
+ else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Hard-failing on DMABUF buffers may break capture on compositors that switch buffer types.
In `process_frame`, DMABUF-backed buffers currently log and return `false`. On systems that only offer DMABUF (now or in future KWin/PipeWire versions), this would make screencap fail permanently once the buffer type switches. If EGL import isn’t implemented yet, consider clearly exposing this limitation (e.g., via `get_info()`/feature flag) or adding a fallback (e.g., drop the frame but keep the previous image so `screencap()` still succeeds).
Suggested implementation:
```cpp
else if (data.type == SPA_DATA_DmaBuf) {
if (!m_loggedDmabufUnsupported) {
LogWarning << "DMABUF buffers not yet supported; dropping frame (need EGL import)";
m_loggedDmabufUnsupported = true;
}
// Mark this frame as DMABUF-backed so we can drop it gracefully later,
// keeping the previously captured image available to callers.
is_dmabuf_frame = true;
continue;
}
```
```cpp
if (!mapped_ptr) {
// If this was a DMABUF-backed frame, drop it but keep the previous image
// so screencap() can still succeed instead of hard-failing.
if (is_dmabuf_frame) {
return true;
}
return false;
}
```
1. Ensure `process_frame` (or the containing function) defines and initializes a local flag before the buffer loop:
- Add near the top of the function:
```cpp
bool is_dmabuf_frame = false;
```
2. Add a member to the screencap class to log the DMABUF limitation only once:
- In the class declaration (header), add:
```cpp
bool m_loggedDmabufUnsupported = false;
```
or, if default member initializers are not used in this codebase, initialize it in the constructor.
3. Expose the missing-DMABUF support via the existing capability/introspection API:
- In the `get_info()` or equivalent function that reports screencap capabilities, add a flag such as `supportsDmabuf = false` (or similar, matching existing conventions), and ensure callers can detect this and handle it appropriately.
4. If the caller assumes that a successful `process_frame()` always updates the backing image, you may need to adjust that logic so that a `true` return combined with `is_dmabuf_frame == true` means "no new frame; keep previous image", or alternatively introduce an explicit status enum instead of a bare `bool`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体反馈：
- 在
UInputController::maa_to_linux_keycode中，ASCII 数字映射把'0'和'1'..'9'一样处理，导致'0'被映射到错误的 Linux 键码（应该像测试用例中那样是KEY_0）；请添加一个显式的'0'分支以保持行为一致。 MaaController.h里MaaKWinControllerCreate的注释仍然写着截图功能尚未实现并且需要 PipeWire，但现在你已经提供了基于 PipeWire 的实现，建议更新这段注释，让它反映当前的实际行为。- PipeWire/DBus 依赖的 ARM64 交叉编译配置在
build.yml和test.yml中完全重复了同一段内容；可以考虑把它抽取到一个共享脚本或可复用的 action 中，以减少维护成本，并把逻辑集中到一个地方。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- In `UInputController::maa_to_linux_keycode`, ASCII digit mapping treats `'0'` the same as `'1'..'9'`, causing `'0'` to map to the wrong Linux keycode (should be `KEY_0` like in the test reference); add an explicit `'0'` case to keep behavior consistent.
- The comment for `MaaKWinControllerCreate` in `MaaController.h` still states that screencap is not implemented and requires PipeWire, but you now provide a PipeWire-based implementation, so it would be good to update that note to reflect the current behavior.
- The ARM64 cross-compilation setup for PipeWire/DBus dependencies is duplicated verbatim in both `build.yml` and `test.yml`; consider extracting this into a shared script or reusable action to reduce maintenance overhead and keep the logic in one place.
## Individual Comments
### Comment 1
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.cpp" line_range="1036-1045" />
<code_context>
+ bool need_unmap = false;
+ size_t map_size = 0;
+
+ if (!mapped_ptr) {
+ if (data.type == SPA_DATA_MemFd && data.fd >= 0) {
+ map_size = data.maxsize > 0 ? data.maxsize : data.chunk->size;
+ if (map_size > 0) {
+ mapped_ptr = mmap(nullptr, map_size, PROT_READ, MAP_PRIVATE, data.fd, 0);
+ if (mapped_ptr == MAP_FAILED) {
+ LogError << "mmap of PipeWire buffer failed: " << strerror(errno);
+ return false;
+ }
+ need_unmap = true;
+ }
+ }
+ else if (data.type == SPA_DATA_DmaBuf) {
+ // DMABuf cannot be directly mapped via mmap; silently drop the frame
+ // and return true to indicate "no error, but skip this frame".
+ LogWarn << "DMABUF buffer type received, dropping frame (EGL import not supported)";
</code_context>
<issue_to_address>
**issue (bug_risk):** DMABUF 分支在没有生成图像的情况下报告成功，会向调用方发出误导性的成功信号。
这里你记录了一条 warning 日志并 `return true`，但从未写入 `out_image`。由于对 `pw_on_stream_process` 来说 `true` 表示“帧已就绪”，它会存储一个空的 `Mat` 并唤醒条件变量，从而让 `screencap()` 在没有可用帧的情况下退出。可以考虑在这里返回 `false` 以忽略该帧，或者仅在确保 `out_image` 非空时才返回 `true`。
</issue_to_address>
### Comment 2
<location path="source/MaaKWinControlUnit/Input/UInputController.cpp" line_range="335-344" />
<code_context>
+ return { screen_width_, screen_height_ };
+}
+
+int UInputController::maa_to_linux_keycode(int key_code)
+{
+ // ASCII letter mapping
+ if (key_code >= 'A' && key_code <= 'Z') {
+ return KEY_A + (key_code - 'A');
+ }
+ if (key_code >= 'a' && key_code <= 'z') {
+ return KEY_A + (key_code - 'a');
+ }
+ // ASCII digit mapping
+ if (key_code >= '0' && key_code <= '9') {
+ return KEY_1 + (key_code - '1');
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** 字符 '0' 的数字键映射不正确，会产生无效的 Linux 键码。
这种算术映射只对 '1'–'9' 有效；对于 '0'，结果是 `KEY_1 - 1`，这是无效值，会导致 '0' 的按键事件错误或被忽略。请对 '0' 做特殊处理映射到 `KEY_0`，或使用一个显式的查找表来映射 '0'–'9'。
</issue_to_address>
### Comment 3
<location path="source/MaaKWinControlUnit/API/KWinControlUnitAPI.cpp" line_range="21-22" />
<code_context>
+
+#else
+
+ if (!device_node) {
+ LogError << "device_node is null";
+ return nullptr;
</code_context>
<issue_to_address>
**nitpick:** 日志消息声称 device_node 可能是空字符串，但实际上只检查了指针是否为 null。
条件只检查了空指针，但日志信息暗示也处理了空字符串情况。可以考虑增加一个显式的空字符串检查（例如 `device_node[0] == '\0'`），或者修改日志信息，只提到指针为 null 的情况。
</issue_to_address>
### Comment 4
<location path="source/MaaKWinControlUnit/test_kwin_keyboard.cpp" line_range="29-38" />
<code_context>
+// ─── Keycode mapping reference (mirrors UInputController::maa_to_linux_keycode) ───
</code_context>
<issue_to_address>
**issue (testing):** 当前的映射测试只验证了一个重复实现的参考版本，而不是实际线上使用的 maa_to_linux_keycode 逻辑。
由于 Phase 1 中使用的 `reference_keycode` 是手动“镜像” `UInputController::maa_to_linux_keycode` 的实现，即便真实实现退化，只要两边以同样错误的方式被修改，测试仍然可能通过。为了增强这些测试，可以考虑：
- 添加一个仅用于测试的小钩子或辅助函数（例如在测试头文件中使用 `friend` 或包装函数）来直接调用 `maa_to_linux_keycode`，并在测试中使用它；或者
- 将 `maa_to_linux_keycode` 抽取到一个共享模块，让库代码和测试都使用同一份实现。
这样可以确保 Phase 1 实际执行的是线上使用的映射逻辑，而不是一个重复的参考实现。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
UInputController::maa_to_linux_keycode, ASCII digit mapping treats'0'the same as'1'..'9', causing'0'to map to the wrong Linux keycode (should beKEY_0like in the test reference); add an explicit'0'case to keep behavior consistent. - The comment for
MaaKWinControllerCreateinMaaController.hstill states that screencap is not implemented and requires PipeWire, but you now provide a PipeWire-based implementation, so it would be good to update that note to reflect the current behavior. - The ARM64 cross-compilation setup for PipeWire/DBus dependencies is duplicated verbatim in both
build.ymlandtest.yml; consider extracting this into a shared script or reusable action to reduce maintenance overhead and keep the logic in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UInputController::maa_to_linux_keycode`, ASCII digit mapping treats `'0'` the same as `'1'..'9'`, causing `'0'` to map to the wrong Linux keycode (should be `KEY_0` like in the test reference); add an explicit `'0'` case to keep behavior consistent.
- The comment for `MaaKWinControllerCreate` in `MaaController.h` still states that screencap is not implemented and requires PipeWire, but you now provide a PipeWire-based implementation, so it would be good to update that note to reflect the current behavior.
- The ARM64 cross-compilation setup for PipeWire/DBus dependencies is duplicated verbatim in both `build.yml` and `test.yml`; consider extracting this into a shared script or reusable action to reduce maintenance overhead and keep the logic in one place.
## Individual Comments
### Comment 1
<location path="source/MaaKWinControlUnit/Screencap/PipeWireScreencap.cpp" line_range="1036-1045" />
<code_context>
+ bool need_unmap = false;
+ size_t map_size = 0;
+
+ if (!mapped_ptr) {
+ if (data.type == SPA_DATA_MemFd && data.fd >= 0) {
+ map_size = data.maxsize > 0 ? data.maxsize : data.chunk->size;
+ if (map_size > 0) {
+ mapped_ptr = mmap(nullptr, map_size, PROT_READ, MAP_PRIVATE, data.fd, 0);
+ if (mapped_ptr == MAP_FAILED) {
+ LogError << "mmap of PipeWire buffer failed: " << strerror(errno);
+ return false;
+ }
+ need_unmap = true;
+ }
+ }
+ else if (data.type == SPA_DATA_DmaBuf) {
+ // DMABuf cannot be directly mapped via mmap; silently drop the frame
+ // and return true to indicate "no error, but skip this frame".
+ LogWarn << "DMABUF buffer type received, dropping frame (EGL import not supported)";
</code_context>
<issue_to_address>
**issue (bug_risk):** DMABUF branch reports success without producing a frame, which can signal a false-positive to callers.
Here you log a warning and `return true`, but never write `out_image`. Since `true` means "frame ready" to `pw_on_stream_process`, it will store an empty `Mat` and signal the condition variable, letting `screencap()` exit without a usable frame. Consider returning `false` here so the frame is ignored, or only returning `true` after ensuring `out_image` is non-empty.
</issue_to_address>
### Comment 2
<location path="source/MaaKWinControlUnit/Input/UInputController.cpp" line_range="335-344" />
<code_context>
+ return { screen_width_, screen_height_ };
+}
+
+int UInputController::maa_to_linux_keycode(int key_code)
+{
+ // ASCII letter mapping
+ if (key_code >= 'A' && key_code <= 'Z') {
+ return KEY_A + (key_code - 'A');
+ }
+ if (key_code >= 'a' && key_code <= 'z') {
+ return KEY_A + (key_code - 'a');
+ }
+ // ASCII digit mapping
+ if (key_code >= '0' && key_code <= '9') {
+ return KEY_1 + (key_code - '1');
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Digit key mapping for '0' is incorrect and produces an invalid Linux keycode.
This arithmetic mapping only works for '1'–'9'; for '0' it yields `KEY_1 - 1`, which is invalid and will cause incorrect or ignored key events for '0'. Please either special-case '0' to `KEY_0` or replace this arithmetic with an explicit lookup table for '0'–'9'.
</issue_to_address>
### Comment 3
<location path="source/MaaKWinControlUnit/API/KWinControlUnitAPI.cpp" line_range="21-22" />
<code_context>
+
+#else
+
+ if (!device_node) {
+ LogError << "device_node is null";
+ return nullptr;
</code_context>
<issue_to_address>
**nitpick:** Log message claims device_node can be empty even though only null is checked.
The condition only checks for a null pointer, but the message suggests empty strings are also handled. Either add an explicit empty-string check (e.g. `device_node[0] == '\0'`) or update the log message to mention only the null case.
</issue_to_address>
### Comment 4
<location path="source/MaaKWinControlUnit/test_kwin_keyboard.cpp" line_range="29-38" />
<code_context>
+// ─── Keycode mapping reference (mirrors UInputController::maa_to_linux_keycode) ───
</code_context>
<issue_to_address>
**issue (testing):** The mapping tests currently only validate a duplicated reference implementation, not the actual production maa_to_linux_keycode logic.
Because Phase 1 uses a `reference_keycode` that manually mirrors `UInputController::maa_to_linux_keycode`, the tests can still pass even if the real implementation regresses (as long as both are changed in the same wrong way). To strengthen these tests, either:
- Add a small test-only hook or helper (e.g., a `friend` or wrapper in a test header) that calls `maa_to_linux_keycode` directly, and use that in the tests; or
- Extract `maa_to_linux_keycode` into a shared module used by both the library and tests so they execute the same code.
This ensures Phase 1 exercises the real mapping logic rather than a duplicated reference implementation.
</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
+29
to
+38
| // ─── Keycode mapping reference (mirrors UInputController::maa_to_linux_keycode) ─── | ||
|
|
||
| static int reference_keycode(int c) | ||
| { | ||
| // ASCII letter mapping | ||
| if (c >= 'A' && c <= 'Z') { | ||
| return KEY_A + (c - 'A'); | ||
| } | ||
| if (c >= 'a' && c <= 'z') { | ||
| return KEY_A + (c - 'a'); |
Contributor
There was a problem hiding this comment.
issue (testing): 当前的映射测试只验证了一个重复实现的参考版本，而不是实际线上使用的 maa_to_linux_keycode 逻辑。
由于 Phase 1 中使用的 reference_keycode 是手动“镜像” UInputController::maa_to_linux_keycode 的实现，即便真实实现退化，只要两边以同样错误的方式被修改，测试仍然可能通过。为了增强这些测试，可以考虑：
- 添加一个仅用于测试的小钩子或辅助函数（例如在测试头文件中使用
friend或包装函数）来直接调用maa_to_linux_keycode，并在测试中使用它；或者 - 将
maa_to_linux_keycode抽取到一个共享模块，让库代码和测试都使用同一份实现。
这样可以确保 Phase 1 实际执行的是线上使用的映射逻辑，而不是一个重复的参考实现。
Original comment in English
issue (testing): The mapping tests currently only validate a duplicated reference implementation, not the actual production maa_to_linux_keycode logic.
Because Phase 1 uses a reference_keycode that manually mirrors UInputController::maa_to_linux_keycode, the tests can still pass even if the real implementation regresses (as long as both are changed in the same wrong way). To strengthen these tests, either:
- Add a small test-only hook or helper (e.g., a
friendor wrapper in a test header) that callsmaa_to_linux_keycodedirectly, and use that in the tests; or - Extract
maa_to_linux_keycodeinto a shared module used by both the library and tests so they execute the same code.
This ensures Phase 1 exercises the real mapping logic rather than a duplicated reference implementation.
|
这个方案组合感觉有点怪，pipewire 录屏截图是比较强大，但是输入用 uinput 又感觉有点暴力了? 如果操作的时候窗口还是必须在前台，是不是直接用偷懒一点的截图方法就行了, 比如 XDG Screenshot 之类的 |
Author
XDG Screenshot好像要每次截图授权 |
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.
添加对Wayland(KWin)的支持
具体实现：
控制器：使用/dev/uinput 创建虚拟鼠标设备进行输入
屏幕捕获 (PipeWireScreencap): 基于 xdg-desktop-portal 的 CreateSession -> SelectSources -> Start -> OpenPipeWireRemote 协议
测试环境 ：
操作系统： Arch Linux
桌面环境: KDE Plasma (Wayland)： 6.6.5
KDE 程序框架版本： 6.26.0
Qt 版本： 6.11.1
内核版本： 7.0.11-1-cachyos-bore (64 位)
图形平台： Wayland
pipewire 1.6.6-1 +xdg-desktop-portal-kde 6.6.5-1
测试结果：
测试截图和简单点击和滑动事件通过
要求：
1.运行前需确保用户对 /dev/uinput 具有读写权限（建议添加 udev 规则：KERNEL=="uinput", MODE="0660", GROUP="input"）
2.调用截图功能时会弹出 KDE 原生授权窗口，需要手动授权
由 Sourcery 总结
添加一个基于 KWin 的 Linux 控制器，支持 PipeWire 屏幕捕获和 uinput 输入，将其集成到现有框架和构建系统中，并扩展 CI 以提供 PipeWire/D-Bus 依赖（包括 ARM64 交叉编译支持）。
新功能：
MaaKWinControllerCreateAPI，用于通过指定的 uinput 设备节点和屏幕尺寸来构造 KWin 控制器。增强：
WITH_KWIN_CONTROLLERCMake 标志使 KWin 控制器的构建变为可选，并将其库接入整体构建和依赖图中。构建：
MaaKWinControlUnit子项目、其共享库以及测试可执行文件，并由WITH_KWIN_CONTROLLER选项进行控制。CI：
Original summary in English
Summary by Sourcery
Add a new KWin-based Linux controller with PipeWire screencapture and uinput input support, integrate it into the framework and build system, and extend CI to provide PipeWire/D-Bus dependencies including ARM64 cross-compilation.
New Features:
Enhancements:
Build:
CI:
新功能：
MaaKWinControllerCreateAPI，用于根据设备节点和屏幕尺寸创建 KWin 控制器。增强：
LibraryHolder和MaaFramework，在启用时动态加载 KWin 控制单元库。构建：
MaaKWinControlUnit库和测试接入 CMake 构建系统，并由WITH_KWIN_CONTROLLER选项进行控制。Original summary in English
由 Sourcery 总结
添加一个基于 KWin 的 Linux 控制器，支持 PipeWire 屏幕捕获和 uinput 输入，将其集成到现有框架和构建系统中，并扩展 CI 以提供 PipeWire/D-Bus 依赖（包括 ARM64 交叉编译支持）。
新功能：
MaaKWinControllerCreateAPI，用于通过指定的 uinput 设备节点和屏幕尺寸来构造 KWin 控制器。增强：
WITH_KWIN_CONTROLLERCMake 标志使 KWin 控制器的构建变为可选，并将其库接入整体构建和依赖图中。构建：
MaaKWinControlUnit子项目、其共享库以及测试可执行文件，并由WITH_KWIN_CONTROLLER选项进行控制。CI：
Original summary in English
Summary by Sourcery
Add a new KWin-based Linux controller with PipeWire screencapture and uinput input support, integrate it into the framework and build system, and extend CI to provide PipeWire/D-Bus dependencies including ARM64 cross-compilation.
New Features:
Enhancements:
Build:
CI: