Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-wm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideMigrates relative X window positioning from the deprecated DDE shell interface to a dedicated treeland-xwindow-control-unstable-v1 global, including server registration, example-client updates, protocol generation, and integration/unit coverage. Sequence diagram for relative X window positioning via the new protocolsequenceDiagram
participant Client
participant XWindowControl as treeland_xwindow_control_v1
participant Helper
participant Callback as wl_callback
Client->>XWindowControl: set_xwindow_position_relative(callback, wid, anchor, dx, dy)
XWindowControl->>Helper: setXWindowPositionRelative(wid, wsurface, dx, dy)
XWindowControl->>Callback: wl_callback_send_done(ok)
XWindowControl->>Callback: wl_resource_destroy()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin-wm
marked this pull request as ready for review
September 15, 2026 13:53
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/modules/xwindow-control/xwindowcontrolinterfacev1.cpp" line_range="53-55" />
<code_context>
-{
- WSurface *wsurface = WSurface::fromHandle(wlr_surface_from_resource(anchor));
- uint32_t ok = (wsurface && Helper::instance()->setXWindowPositionRelative(wid, wsurface, dx, dy)) ? 0 : 1;
- wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback);
- wl_callback_send_done(cb, ok);
- wl_resource_destroy(cb);
-}
-
</code_context>
<issue_to_address>
**issue (bug_risk):** `wl_resource_create` can return null when the callback resource cannot be allocated, but the code immediately passes that null pointer to `wl_callback_send_done` and then destroys it, causing a compositor crash instead of reporting an allocation failure to the client.
**Triggers:** When a client sends the request while Wayland resource allocation fails.
**Suggested fix:** Check `cb` before sending or destroying it, and call `wl_client_post_no_memory(resource->client())` when allocation fails.
```suggestion
wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback);
if (!cb) {
wl_client_post_no_memory(resource->client());
return;
}
wl_callback_send_done(cb, ok);
wl_resource_destroy(cb);
```
</issue_to_address>
Comment on lines
+53
to
+55
| wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback); | ||
| wl_callback_send_done(cb, ok); | ||
| wl_resource_destroy(cb); |
There was a problem hiding this comment.
issue (bug_risk): wl_resource_create can return null when the callback resource cannot be allocated, but the code immediately passes that null pointer to wl_callback_send_done and then destroys it, causing a compositor crash instead of reporting an allocation failure to the client.
Triggers: When a client sends the request while Wayland resource allocation fails.
Suggested fix: Check cb before sending or destroying it, and call wl_client_post_no_memory(resource->client()) when allocation fails.
Suggested change
| wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback); | |
| wl_callback_send_done(cb, ok); | |
| wl_resource_destroy(cb); | |
| wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback); | |
| if (!cb) { | |
| wl_client_post_no_memory(resource->client()); | |
| return; | |
| } | |
| wl_callback_send_done(cb, ok); | |
| wl_resource_destroy(cb); |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved invalid-anchor and callback allocation handling, incomplete success-path coverage, and protocol audit documentation gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Migrates X Window relative positioning to the dedicated treeland-xwindow-control-unstable-v1 protocol.
Changes:
- Adds and registers the new server protocol module.
- Removes the deprecated DDE-shell request and updates the example client.
- Adds Qt and Wayland protocol tests.
File summaries
| File | Reviewed change |
|---|---|
tests/test_protocol_xwindow-control/main.cpp |
Qt interface test coverage. |
tests/test_protocol_xwindow-control/CMakeLists.txt |
Registers the Qt test target. |
tests/protocols/treeland-xwindow-control-unstable-v1/treeland-xwindow-control-unstable-v1.h |
Defines protocol test state. |
tests/protocols/treeland-xwindow-control-unstable-v1/treeland-xwindow-control-unstable-v1.c |
Adds Wayland client coverage; currently exercises failure paths only. |
tests/protocols/treeland-xwindow-control-unstable-v1/setup.cpp |
Connects the production interface for testing. |
tests/protocols/treeland-xwindow-control-unstable-v1/CMakeLists.txt |
Registers the protocol integration test. |
tests/protocols/CMakeLists.txt |
Adds the protocol test directory; audit documentation is still required. |
tests/CMakeLists.txt |
Registers the test directory. |
src/seat/helper.h |
Adds ownership for the new interface. |
src/seat/helper.cpp |
Registers the protocol global during initialization. |
src/modules/xwindow-control/xwindowcontrolinterfacev1.h |
Declares the protocol interface. |
src/modules/xwindow-control/xwindowcontrolinterfacev1.cpp |
Implements requests and callbacks; invalid anchors and callback allocation require handling. |
src/modules/xwindow-control/CMakeLists.txt |
Generates and builds protocol bindings. |
src/modules/dde-shell/ddeshellmanagerinterfacev1.cpp |
Removes the deprecated request implementation. |
src/modules/CMakeLists.txt |
Registers the new module. |
examples/test_set_xwindow_position/main.cpp |
Migrates the example client to the new protocol. |
examples/test_set_xwindow_position/CMakeLists.txt |
Generates the new client bindings. |
Review details
Suppressed comments (1)
tests/protocols/treeland-xwindow-control-unstable-v1/CMakeLists.txt:6
- The protocol-test guidance requires a README and an
INDEX.mdcoverage entry for each target, and every existing full protocol target follows that convention. Registering this target without either file leaves the audit table unaware of the new protocol and still reports the old DDE request as the only uncovered location. Please add the protocol contract/coverage documentation. 协议测试规范要求每个 target 都有 README 和INDEX.md覆盖记录，现有完整协议 target 也都遵循这一约定。当前只注册 target 而没有这两个文件，会让审计表遗漏新协议并继续把旧 DDE 请求列为唯一未覆盖位置。请补充协议契约和覆盖文档。
treeland_add_protocol_test(
NAME treeland_xwindow_control_v1
XML "${TREELAND_PROTOCOLS_DATA_DIR}/treeland-xwindow-control-unstable-v1.xml"
SETUP "${CMAKE_CURRENT_SOURCE_DIR}/setup.cpp"
CLIENT "${CMAKE_CURRENT_SOURCE_DIR}/treeland-xwindow-control-unstable-v1.c"
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| wl_fixed_t dx, | ||
| wl_fixed_t dy) | ||
| { | ||
| WSurface *wsurface = WSurface::fromHandle(wlr_surface_from_resource(anchor)); |
Comment on lines
+53
to
+55
| wl_resource *cb = wl_resource_create(resource->client(), &wl_callback_interface, 1, callback); | ||
| wl_callback_send_done(cb, ok); | ||
| wl_resource_destroy(cb); |
Comment on lines
+107
to
+110
| wl_fixed_t dx = wl_fixed_from_int(0); | ||
| wl_fixed_t dy = wl_fixed_from_int(0); | ||
| struct wl_callback *cb = | ||
| treeland_xwindow_control_v1_set_xwindow_position_relative(ctx->manager, 0, surface, dx, dy); |
| add_subdirectory(treeland-wallpaper-shell-unstable-v1) | ||
| add_subdirectory(treeland-wallpaper-desktop-v1) | ||
| add_subdirectory(treeland-show-desktop-v1) | ||
| add_subdirectory(treeland-xwindow-control-unstable-v1) |
…ol-unstable-v1 protocol - Add new xwindow-control module with XWindowControlInterfaceV1 implementing the treeland_xwindow_control_v1 global interface - Register XWindowControlInterfaceV1 in Helper::initShell() - Remove deprecated set_xwindow_position_relative override from DDEShellManagerInterfaceV1Private (now handled by the new module) - Update example to use the new treeland_xwindow_control_v1 client interface - Add protocol integration test and Qt unit test for the new interface
wineee
force-pushed
the
agent/developer/a38088999aa5
branch
from
September 16, 2026 02:14
14b6f82 to
f6c629c
Compare
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.
Adapt treeland to the new treeland-xwindow-control-unstable-v1 protocol from treeland-protocols 0.6.0.
Changes
New module: src/modules/xwindow-control/
Modified files
Closes #WM-481
Summary by Sourcery
Migrate relative X window positioning to the treeland-xwindow-control-unstable-v1 protocol.
New Features:
Enhancements:
Build:
Tests: