Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee 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 GuideThe PR introduces the dde-shell unstable v2 server protocol and registers it beside v1, integrates its metadata and placement hints into shell surfaces, fixes explicit (0,0) placement and immediate runtime re-placement, and adds CI/build adjustments plus protocol lifecycle and behavior tests. Sequence diagram for v2 shell surface creation and placement hintssequenceDiagram
participant Client
participant Manager as DDEShellManagerInterfaceV2
participant Surface as DDEShellSurfaceV2
participant Handler as ShellHandler
participant Wrapper as SurfaceWrapper
participant Output
Client->>Manager: get_shell_surface(id, wl_surface)
Manager->>Manager: getByWlrSurface(wl_surface)
Manager-->>Client: already_shell_surface error [duplicate]
Manager->>Surface: create
Manager-->>Handler: surfaceCreated(Surface)
Handler->>Wrapper: setSurfaceRole(Overlay)
Client->>Surface: set_position_hint(output, x, y)
Surface->>Surface: resolve global position
Surface-->>Handler: positionHintChanged(globalPos)
Handler->>Wrapper: setClientRequstPos(globalPos)
Wrapper-->>Output: clientRequstPosChanged()
Output->>Output: placeClientRequstPos(surface, globalPos)
Client->>Surface: set_cursor_placement_hint(x_offset, y_offset)
Surface-->>Handler: cursorPlacementHintChanged(offset)
Handler->>Wrapper: setCursorPlacement(true)
Wrapper-->>Output: cursorPlacementChanged()
Output->>Output: placeUnderCursor(surface)
State diagram for mutually exclusive v2 placement modesstateDiagram-v2
[*] --> Automatic
Automatic --> FixedPosition: set_position_hint
Automatic --> CursorPlacement: set_cursor_placement_hint
FixedPosition --> FixedPosition: set_position_hint
FixedPosition --> CursorPlacement: set_cursor_placement_hint
CursorPlacement --> CursorPlacement: set_cursor_placement_hint
CursorPlacement --> FixedPosition: set_position_hint
FixedPosition --> Automatic: set_auto_placement
CursorPlacement --> Automatic: set_auto_placement
FixedPosition: global position including (0,0)
CursorPlacement: cursor offsets x and y
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/surface/surfacewrapper.cpp" line_range="2878-2888" />
<code_context>
+void SurfaceWrapper::setAutoPlaceYOffset(int offset)
{
- if (m_autoPlaceYOffset == offset)
+ const bool wantCursor = offset != 0;
+ // The offset also derives the placement mode and the fixed-request state,
+ // so an unchanged value may still need to apply them (e.g. v1
+ // set_auto_placement(0) releasing a fixed-position request).
+ if (m_autoPlaceYOffset == offset && m_cursorPlacement == wantCursor
+ && (wantCursor || (!m_hasClientRequstPos && positionAutomatic())))
return;
m_autoPlaceYOffset = offset;
- setPositionAutomatic(offset == 0);
+ // Entering cursor placement supersedes a stale fixed-position request;
+ // leaving it (offset 0) returns the surface to automatic placement.
+ setCursorPlacement(wantCursor);
+ if (wantCursor) {
</code_context>
<issue_to_address>
**issue (broader_impact):** Nonzero v1 `set_auto_placement` y-offsets are interpreted as cursor-placement requests, so v1 surfaces with an ordinary edge-clamping offset are placed under the pointer instead of using the v1 automatic layout.
**Triggers:** When a v1 client calls `set_auto_placement` with a nonzero y offset.
**Suggested fix:** Keep v1 y-offset handling separate from the v2 cursor-placement mode, or set `cursorPlacement` only from v2 requests.
</issue_to_address>
### Comment 2
<location path="src/modules/dde-shell/CMakeLists.txt" line_range="3-7" />
<code_context>
find_package(TreelandProtocols REQUIRED)
+# NOTE: the v2 XML (treeland-dde-shell-unstable-v2.xml) only exists in
+# treeland-protocols master / 0.7.0 sources, which are not tagged or
+# released yet. The debian build-dependency stays at >= 0.6.0 and the
+# build environment must provide the 0.7.0 XML (e.g. the CI workflow
+# builds it from the protocols master source).
+
+# --- Deprecated v1 protocol (kept for transition until dde-shell migrates) ---
</code_context>
<issue_to_address>
**issue (bug_risk):** The module now unconditionally generates code from `treeland-dde-shell-unstable-v2.xml`, but the build dependency remains documented and configured at >= 0.6.0, where that XML is absent; builds using the released 0.6.0 protocol package fail during CMake configuration.
**Triggers:** When the build environment installs treeland-protocols 0.6.x instead of building protocols master.
**Suggested fix:** Raise the package/build dependency to >= 0.7.0 and ensure the v2 XML is provided by that dependency.
</issue_to_address>
### Comment 3
<location path="tests/protocols/treeland-dde-shell-v2/treeland-dde-shell-v2.c" line_range="210-227" />
<code_context>
+// v2 protocol requires the compositor to destroy the object automatically.
+static int surface_destroy_releases_shell_surface(struct test_ctx *ctx)
+{
+ struct wl_surface *surface = wl_compositor_create_surface(ctx->compositor);
+ if (!surface)
+ return 0;
+ struct treeland_dde_shell_surface_v2 *shell =
+ treeland_dde_shell_manager_v2_get_shell_surface(ctx->manager, surface);
+ if (!shell || wl_display_roundtrip(ctx->display) < 0)
+ return 0;
+
+ wl_surface_destroy(surface);
+ if (wl_display_roundtrip(ctx->display) < 0)
+ return 0;
+
+ // The server-side test bridge resets its tracked object on destruction,
+ // so the state query must come back empty afterwards.
+ struct dde_shell_surface_v2_state state;
+ if (!invoke_on_server_thread(dde_shell_v2_query_surface_state, &state))
+ return 0;
+ return !state.position_set && !state.cursor_set && !state.role_overlay
+ && state.skip_flags == 0;
+}
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The lifecycle test creates a second shell-surface proxy and never destroys the client-side `shell` proxy after destroying its wl_surface, leaking that proxy until connection teardown and masking whether the protocol correctly cleans up both sides of the resource relationship.
**Triggers:** When the surface-destruction test runs repeatedly in the same client connection.
**Suggested fix:** Destroy the local shell proxy after the roundtrip, or explicitly assert that its client-side destruction callback ran.
</issue_to_address>
### Comment 4
<location path="src/modules/dde-shell/ddeshellmanagerinterfacev2.cpp" line_range="346" />
<code_context>
+ return d->acceptKeyboardFocus;
+}
+
+DDEShellSurfaceV2 *DDEShellSurfaceV2::get(wl_resource *native)
+{
+ WSurface *surface = WSurface::fromHandle(wlr_surface_from_resource(native));
+ if (surface) {
+ return DDEShellSurfaceV2::get(surface);
+ }
+
+ return nullptr;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `DDEShellSurfaceV2::get(wl_resource *native)` calls `wlr_surface_from_resource(native)` without checking that `native` is non-null or that it is a wl_surface resource, so callers passing another resource type can trigger the wlroots assertion instead of returning null.
**Triggers:** When a caller asks `DDEShellSurfaceV2::get` to inspect a null or non-wl_surface resource.
**Suggested fix:** Validate `native` and its resource class before calling `wlr_surface_from_resource`.
```suggestion
if (!native || strcmp(wl_resource_get_class(native), wl_surface_interface.name) != 0)
return nullptr;
WSurface *surface = WSurface::fromHandle(wlr_surface_from_resource(native));
```
</issue_to_address>
wineee
force-pushed
the
dde-shell-v2
branch
from
September 28, 2026 09:17
fcbcdf9 to
1bbb9d2
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical lifecycle, dependency, duplicate-surface, and privilege issues remain, along with placement and test coverage gaps.
Review effort: Lite
Findings: 4
Open (7)
What changed in this PR
Adds dde-shell unstable v2 support alongside v1, with updated placement behavior and protocol tests.
Changes:
- Add v2 manager/surface integration and lifecycle handling.
- Update fixed and cursor placement behavior.
- Add protocol fixtures, tests, documentation, and build registration.
| File | Description |
|---|---|
tests/protocols/treeland-dde-shell-v2/treeland-dde-shell-v2.h |
Test state and fixture declarations |
tests/protocols/treeland-dde-shell-v2/treeland-dde-shell-v2.c |
v2 client protocol cases |
tests/protocols/treeland-dde-shell-v2/setup.cpp |
Server-side test fixture |
tests/protocols/treeland-dde-shell-v2/README.md |
v2 coverage documentation |
tests/protocols/treeland-dde-shell-v2/CMakeLists.txt |
v2 test target |
tests/protocols/INDEX.md |
Protocol test index |
tests/protocols/CMakeLists.txt |
Test directory registration |
src/surface/surfacewrapper.h |
Placement properties and state |
src/surface/surfacewrapper.cpp |
Placement state transitions |
src/seat/helper.h |
v2 manager ownership |
src/seat/helper.cpp |
v2 global registration |
src/output/output.h |
Placement API updates |
src/output/output.cpp |
Cursor and fixed-position placement |
src/modules/dde-shell/ddeshellmanagerinterfacev2.h |
v2 public interfaces |
src/modules/dde-shell/ddeshellmanagerinterfacev2.cpp |
v2 protocol implementation |
src/modules/dde-shell/CMakeLists.txt |
Protocol generation and sources |
src/core/shellhandler.h |
v2 handler declaration |
src/core/shellhandler.cpp |
v2 integration and cleanup |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wineee
force-pushed
the
dde-shell-v2
branch
2 times, most recently
from
September 29, 2026 02:14
972486e to
d757c33
Compare
Implement treeland_dde_shell_manager_v2 and treeland_dde_shell_surface_v2 (treeland-protocols 0.7.0) and register the v2 global alongside the deprecated v1 one for the migration period. v2 brings a 0-based overlay default role, a set_skip_flags bitfield, mutually exclusive position/cursor placement hints resolved to global coordinates on request, an already_shell_surface error on duplicate creation, destroy as the first request, and auto-destroy of the surface resource when the wl_surface is destroyed. Make (0,0) a valid fixed position: track position-request presence with an explicit hasClientRequstPos flag instead of the QPoint::isNull() sentinel, and re-apply placement immediately when the placement mode switches at runtime. Raise the treeland-protocols build dependency to >= 0.7.0. v1 behavior changes: set_surface_position/set_auto_placement now take effect immediately at runtime instead of on the next arrange pass; set_surface_position(0,0) pins the surface to the anchor origin instead of falling back to auto placement. Log: v1 set_surface_position applies immediately; (0,0) pins to origin Influence: 1. Run the v2 protocol suite (test_treeland_dde_shell_v2, 14 cases) and the v1 regression suite (28 cases) 2. With a v2 client, verify position hints land at the resolved global position, including (0,0) on multi-screen setups 3. Verify runtime switches between position hint and cursor placement re-place the surface immediately 4. Regression-test v1 windows: set_surface_position(0,0) now pins to the origin instead of reverting to auto layout feat(dde-shell): 适配 dde-shell-unstable-v2 协议 实现 treeland_dde_shell_manager_v2 与 treeland_dde_shell_surface_v2 （treeland-protocols 0.7.0），迁移期将 v2 全局对象与已废弃的 v1 并行 注册。v2 采用 0 基 overlay 默认 role、set_skip_flags 位域、互斥的 position/cursor 定位 hint（请求时解析为全局坐标）、重复创建报 already_shell_surface 错误、destroy 位于首请求位，且 wl_surface 销毁 时自动销毁 surface 资源。 使 (0,0) 成为合法固定位置：以显式 hasClientRequstPos 标志替代 QPoint::isNull() 哨兵，运行时切换定位模式时立即重新摆放。 v1 行为变化：set_surface_position/set_auto_placement 运行时立即生效 （此前延迟到下次 arrange）；set_surface_position(0,0) 固定到锚点原点 而非退回自动布局。
wineee
force-pushed
the
dde-shell-v2
branch
from
September 30, 2026 02:19
d757c33 to
841a2e3
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.
Implement treeland_dde_shell_manager_v2 and
treeland_dde_shell_surface_v2 (treeland-protocols 0.7.0) and register the v2 global alongside the deprecated v1 one for the migration period. v2 brings a 0-based overlay default role, a set_skip_flags bitfield, mutually exclusive position/cursor placement hints resolved to global coordinates on request, an already_shell_surface error on duplicate creation, destroy as the first request, and auto-destroy of the surface resource when the wl_surface is destroyed.
Make (0,0) a valid fixed position: track position-request presence with an explicit hasClientRequstPos flag instead of the QPoint::isNull() sentinel, and re-apply placement immediately when the placement mode switches at runtime.
Raise the treeland-protocols build dependency to >= 0.7.0.
v1 behavior changes: set_surface_position/set_auto_placement now take effect immediately at runtime instead of on the next arrange pass; set_surface_position(0,0) pins the surface to the anchor origin instead of falling back to auto placement.
Log: v1 set_surface_position applies immediately; (0,0) pins to origin
Influence:
feat(dde-shell): 适配 dde-shell-unstable-v2 协议
实现 treeland_dde_shell_manager_v2 与 treeland_dde_shell_surface_v2 （treeland-protocols 0.7.0），迁移期将 v2 全局对象与已废弃的 v1 并行 注册。v2 采用 0 基 overlay 默认 role、set_skip_flags 位域、互斥的 position/cursor 定位 hint（请求时解析为全局坐标）、重复创建报
already_shell_surface 错误、destroy 位于首请求位，且 wl_surface 销毁 时自动销毁 surface 资源。
使 (0,0) 成为合法固定位置：以显式 hasClientRequstPos 标志替代
QPoint::isNull() 哨兵，运行时切换定位模式时立即重新摆放。
v1 行为变化：set_surface_position/set_auto_placement 运行时立即生效 （此前延迟到下次 arrange）；set_surface_position(0,0) 固定到锚点原点 而非退回自动布局。
close: #1419
Summary by Sourcery
Adapt DDE shell handling to the unstable v2 protocol while preserving v1 compatibility and correcting placement state management.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: