feat(kwin):python绑定#1375
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些整体层面的反馈:
- 在
binding_test.py的__main__代码块中,你注释掉了所有现有的测试调用,只运行test_kwin_controller_create;这实际上在直接运行该文件时禁用了其余的绑定测试,因此要么恢复这些调用,要么把 KWin 相关的测试移动到现有的测试序列中。 - 在
KWinController.__init__中,在不检查符号是否存在的情况下直接调用Library.framework().MaaKWinControllerCreate,会在符号缺失的环境中于导入时抛出AttributeError;建议使用hasattr或 try/except 做保护,并改为抛出一个更清晰、可控的RuntimeError,而不是依赖测试去处理这个AttributeError。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `binding_test.py`'s `__main__` block you've commented out all existing test calls and only run `test_kwin_controller_create`; this effectively disables the rest of the binding tests when the file is run directly, so either restore those calls or move the KWin-specific test into the existing sequence.
- In `KWinController.__init__`, calling `Library.framework().MaaKWinControllerCreate` without checking for its presence will raise an `AttributeError` at import time in environments where the symbol is missing; consider guarding this with `hasattr` or a try/except and raising a clearer, controlled `RuntimeError` instead of relying on tests to handle the `AttributeError`.
## Individual Comments
### Comment 1
<location path="source/binding/Python/maa/controller.py" line_range="1095" />
<code_context>
+ self._set_kwin_api_properties()
+
+ self._handle = Library.framework().MaaKWinControllerCreate(
+ device_node.encode(),
+ screen_width,
+ screen_height,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider normalizing `device_node` more defensively before encoding.
This assumes `device_node` is a `str` and uses the default encoding. If a `pathlib.Path` or `bytes` is passed, this can fail or double-encode. Using `os.fspath(device_node)` with `os.fsencode(...)`, or at least validating the type, would make this safer for typical path-like inputs and non-ASCII paths.
Suggested implementation:
```python
super().__init__()
self._set_kwin_api_properties()
normalized_device_node = os.fsencode(os.fspath(device_node))
self._handle = Library.framework().MaaKWinControllerCreate(
normalized_device_node,
screen_width,
screen_height,
MaaBool(use_win32_vk_code),
)
```
1. Ensure `os` is imported at the top of `source/binding/Python/maa/controller.py`, e.g. `import os`.
2. If this module already defines a helper for path normalization/encoding, prefer using that helper instead of introducing the `normalized_device_node` variable directly, to stay consistent with the rest of the codebase.
</issue_to_address>请帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据这些反馈改进对你的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
binding_test.py's__main__block you've commented out all existing test calls and only runtest_kwin_controller_create; this effectively disables the rest of the binding tests when the file is run directly, so either restore those calls or move the KWin-specific test into the existing sequence. - In
KWinController.__init__, callingLibrary.framework().MaaKWinControllerCreatewithout checking for its presence will raise anAttributeErrorat import time in environments where the symbol is missing; consider guarding this withhasattror a try/except and raising a clearer, controlledRuntimeErrorinstead of relying on tests to handle theAttributeError.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `binding_test.py`'s `__main__` block you've commented out all existing test calls and only run `test_kwin_controller_create`; this effectively disables the rest of the binding tests when the file is run directly, so either restore those calls or move the KWin-specific test into the existing sequence.
- In `KWinController.__init__`, calling `Library.framework().MaaKWinControllerCreate` without checking for its presence will raise an `AttributeError` at import time in environments where the symbol is missing; consider guarding this with `hasattr` or a try/except and raising a clearer, controlled `RuntimeError` instead of relying on tests to handle the `AttributeError`.
## Individual Comments
### Comment 1
<location path="source/binding/Python/maa/controller.py" line_range="1095" />
<code_context>
+ self._set_kwin_api_properties()
+
+ self._handle = Library.framework().MaaKWinControllerCreate(
+ device_node.encode(),
+ screen_width,
+ screen_height,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider normalizing `device_node` more defensively before encoding.
This assumes `device_node` is a `str` and uses the default encoding. If a `pathlib.Path` or `bytes` is passed, this can fail or double-encode. Using `os.fspath(device_node)` with `os.fsencode(...)`, or at least validating the type, would make this safer for typical path-like inputs and non-ASCII paths.
Suggested implementation:
```python
super().__init__()
self._set_kwin_api_properties()
normalized_device_node = os.fsencode(os.fspath(device_node))
self._handle = Library.framework().MaaKWinControllerCreate(
normalized_device_node,
screen_width,
screen_height,
MaaBool(use_win32_vk_code),
)
```
1. Ensure `os` is imported at the top of `source/binding/Python/maa/controller.py`, e.g. `import os`.
2. If this module already defines a helper for path normalization/encoding, prefer using that helper instead of introducing the `normalized_device_node` variable directly, to stay consistent with the rest of the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
为KWinController添加python绑定
Summary by Sourcery
为 Linux 上的 KWin Wayland 控制器添加一个 Python 绑定以及基础测试覆盖。
新功能:
KWinController类,通过 PipeWire/xdg-desktop-portal 和/dev/uinput控制 KWin。测试:
KWinController、验证其基础属性,并在控制器不可用时优雅地跳过测试。Original summary in English
Summary by Sourcery
Add a Python binding and basic test coverage for the KWin Wayland controller on Linux.
New Features:
Tests:
新特性:
KWinController类,通过 PipeWire/xdg-desktop-portal 和/dev/uinput驱动 KWin。测试:
KWinController并验证其基本属性和空操作行为;在不可用时能优雅地跳过。Original summary in English
Summary by Sourcery
为 Linux 上的 KWin Wayland 控制器添加一个 Python 绑定以及基础测试覆盖。
新功能:
KWinController类,通过 PipeWire/xdg-desktop-portal 和/dev/uinput控制 KWin。测试:
KWinController、验证其基础属性,并在控制器不可用时优雅地跳过测试。Original summary in English
Summary by Sourcery
Add a Python binding and basic test coverage for the KWin Wayland controller on Linux.
New Features:
Tests: