Skip to content

feat: add destructor methods and reorganize protocol#49

Merged
wineee merged 1 commit intolinuxdeepin:masterfrom
zzxyb:fix
Apr 28, 2026
Merged

feat: add destructor methods and reorganize protocol#49
wineee merged 1 commit intolinuxdeepin:masterfrom
zzxyb:fix

Conversation

@zzxyb
Copy link
Copy Markdown
Contributor

@zzxyb zzxyb commented Mar 31, 2026

No description provided.

@zzxyb zzxyb marked this pull request as draft March 31, 2026 09:10
Comment thread xml/treeland-output-manager-v1.xml Outdated
@zzxyb zzxyb force-pushed the fix branch 2 times, most recently from 6130332 to 78435e3 Compare March 31, 2026 13:13
@zzxyb zzxyb changed the title Fix fix protocols error Apr 20, 2026
@zzxyb zzxyb marked this pull request as ready for review April 20, 2026 07:24
@wineee wineee requested a review from Copilot April 21, 2026 08:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reorganizes multiple Wayland protocol XML definitions to address protocol/XML validation issues, primarily by moving/adding destroy destructors and reordering enums/requests/events.

Changes:

  • Adds and/or relocates destroy destructor requests across many interfaces.
  • Reorders protocol elements (requests/events/enums) and annotates some args with enum=... (e.g., error codes, directions).
  • Performs structural cleanup in several protocol files (moving blocks to different positions).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
xml/treeland-window-management-v1.xml Moves destroy request earlier in the interface.
xml/treeland-wallpaper-color-v1.xml Moves destroy request to the top of the manager interface.
xml/treeland-virtual-output-manager-v1.xml Adds manager destroy, reorders event placement, adds enum reference for error code.
xml/treeland-shortcut-manager-v1.xml Adds manager destroy and reorders elements in the context interface.
xml/treeland-screensaver-v1.xml Reorders requests and introduces destroy plus an error enum.
xml/treeland-personalization-manager-v1.xml Adds manager/context destroy requests and reorders multiple interfaces’ request blocks and enums.
xml/treeland-output-manager-v1.xml Moves error enum and destroy for the color-control interface earlier.
xml/treeland-foreign-toplevel-manager-v1.xml Adds manager/handle/context destructors and significantly reorders requests/enums/events.
xml/treeland-ddm-v1.xml Adds a destroy request and adjusts spacing/structure.
xml/treeland-dde-shell-v1.xml Adds manager and overlap-checker destructors and reorders events/enum blocks.
xml/treeland-capture-unstable-v1.xml Reorders requests/enums and relocates destroy; moves copy request before events.
xml/treeland-app-id-resolver-v1.xml Reorders resolver requests/events and adjusts blank lines.
Comments suppressed due to low confidence (5)

xml/treeland-dde-shell-v1.xml:26

  • Placing destroy first in treeland_dde_shell_manager_v1 changes request opcode numbering for existing requests like get_window_overlap_checker / get_shell_surface.

To avoid breaking clients, append destroy after existing requests (and bump the interface version / add since), or introduce a new major protocol version instead of inserting a new request before the existing ones.

        <request name="destroy" type="destructor">
            <description summary="destroy the dde shell manager">
                Destroy the treeland_dde_shell_manager_v1 object.
            </description>
        </request>

        <request name="get_window_overlap_checker">
            <arg name="id" type="new_id" interface="treeland_window_overlap_checker" />
        </request>

xml/treeland-wallpaper-color-v1.xml:20

  • destroy is now the first request in treeland_wallpaper_color_manager_v1, which changes the opcode numbers for watch/unwatch. In Wayland, reordering requests in the XML is a wire-protocol breaking change.

If the goal is just to satisfy protocol XML constraints/style, keep the existing request order (e.g., put destroy after watch/unwatch and before events). If destroy is a new request, it should be appended at the end and gated behind a version bump (since) / new major protocol version.

    <interface name="treeland_wallpaper_color_manager_v1" version="1">
        <request name="destroy" type="destructor">
            <description summary="destroy the context object">
                The client no longer cares about wallpaper_color.
            </description>
        </request>

        <request name="watch">
            <description summary="watch wallpaper color">
                Monitor the wallpaper color of a given screen.
            </description>
            <arg name="output" type="string" summary="system output name" />
        </request>

xml/treeland-shortcut-manager-v1.xml:30

  • destroy was added ahead of register_shortcut_context, which changes the request opcode numbering for treeland_shortcut_manager_v1. This is a protocol-breaking change for any existing v1 clients.

If a destroy request is needed, it should be appended after existing requests and protected by a version bump (since) or introduced in a new major protocol (there is already a treeland_shortcut_manager_v2.xml for evolution).

        <request name="destroy" type="destructor">
            <description summary="destroy treeland_shortcut_manager_v1 object" />
        </request>

        <request name="register_shortcut_context">
            <description summary="register shortcut key">
                The format of the shortcut key is 'Modify+Key', such as 'Ctrl+Alt+T'.
                If the format is wrong, the synthesizer will give a "format error". If the shortcut
                key is already registered,
                the compositor will give a "register error" and issue a destruction to the context.
            </description>
            <arg name="key" type="string" />
            <arg name="id" type="new_id" interface="treeland_shortcut_context_v1" />
        </request>

xml/treeland-window-management-v1.xml:33

  • Moving the existing destroy request before set_desktop changes the request opcode numbering for treeland_window_management_v1 (Wayland request opcodes are assigned by XML order). That is a wire-protocol breaking change for any existing clients.

To fix the XML ordering issue without breaking compatibility, keep the original request order by placing destroy after set_desktop (but still before the first <event>), or bump the protocol/iface version and append new requests only at the end with proper versioning (since).

        <request name="destroy" type="destructor">
            <description summary="destroy the window manager object" />
        </request>

        <request name="set_desktop">
            <description summary="show/hide the desktop">
                Tell the compositor to show/hide the desktop.
            </description>
            <arg name="state" type="uint" summary="requested state" />
        </request>

xml/treeland-output-manager-v1.xml:59

  • In treeland_output_color_control_v1, moving destroy ahead of set_color_temperature changes request opcode numbering. Wayland request order is part of the wire protocol, so this is a breaking change.

If you need requests before events for validation, keep the original request order (e.g., set_color_temperature, set_brightness, commit, destroy) and move the whole request block above the events, or bump the interface version and append new requests only at the end with since.

        <request name="destroy" type="destructor">
            <description summary="Destroy the color control interface." />
        </request>

        <request name="set_color_temperature">
            <description summary="Set color temperature for output">
                Color temperature settings are applied only after a commit request is made.
                Setting a value outside the range [1000, 20000] is a protocol error.
            </description>
            <arg name="temperature" type="uint" summary="color temperature in Kelvin"/>
        </request>

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread xml/treeland-personalization-manager-v1.xml Outdated
Comment thread xml/treeland-foreign-toplevel-manager-v1.xml Outdated
Comment thread xml/treeland-ddm-v1.xml Outdated
Comment thread xml/treeland-app-id-resolver-v1.xml Outdated
Comment thread xml/treeland-foreign-toplevel-manager-v1.xml Outdated
Comment thread xml/treeland-virtual-output-manager-v1.xml Outdated
Comment thread xml/treeland-screensaver-v1.xml Outdated
Comment thread xml/treeland-personalization-manager-v1.xml Outdated
Comment thread xml/treeland-personalization-manager-v1.xml Outdated
Comment thread xml/treeland-dde-shell-v1.xml Outdated
@wineee
Copy link
Copy Markdown
Member

wineee commented Apr 22, 2026

break 应该改名字

@zzxyb zzxyb marked this pull request as draft April 24, 2026 01:44
Log:

Influence:
1. Test destruction and verify proper cleanup
@zzxyb zzxyb changed the title fix protocols error feat: add destructor methods and reorganize protocol Apr 28, 2026
@zzxyb zzxyb marked this pull request as ready for review April 28, 2026 06:53
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码 diff 显示了对多个 Wayland 协议 XML 文件的修改,主要是为全局管理器接口添加了 destroy 请求,并相应地将接口版本从 1 升级到了 2

以下是对这些修改的审查意见,涵盖语法逻辑、代码质量、代码性能和代码安全方面:

1. 语法逻辑

  • 接口版本控制:

    • 现状: 修改正确地将所有相关接口(treeland_dde_shell_manager_v1, treeland_ddm_v1, treeland_personalization_manager_v1, treeland_screensaver_v1, treeland_shortcut_manager_v1, treeland_virtual_output_manager_v1)的版本号从 1 增加到了 2
    • 意见: 这是正确的做法。在 Wayland 协议中,添加新的请求(尤其是像 destroy 这样改变对象生命周期行为的请求)必须增加版本号,以确保客户端和服务器能够协商兼容性。
  • Destroy 请求的位置:

    • 现状: 在所有文件中,destroy 请求都被添加在了接口定义的末尾。
    • 意见: 建议改进。根据 Wayland 协议的惯例和最佳实践(参考 wl_displaywl_registry 等核心协议),destroy 请求通常应该作为接口的第一个请求(或者在接口描述之后紧接着)。
    • 原因: 虽然协议允许请求顺序任意,但将 destroy 放在前面可以明确表明该对象可以被销毁,并且作为一种"析构"操作,它在逻辑上优先于业务操作。放在末尾虽然不影响功能,但可读性稍差。
  • Destructor 类型:

    • 现状: 所有新增的 destroy 请求都正确标记了 type="destructor"
    • 意见: 正确。这告诉 Wayland 库在该请求发送后,对象相关的资源应当被立即释放,且不应再发送该对象的其他事件。

2. 代码质量

  • 描述文本:

    • 现状: 描述文本大多比较简短,例如 destroy treeland_screensaver_v1 object
    • 意见: 建议改进。描述文本可以更加规范和详细。
    • 示例: 可以参考 treeland_dde_shell_manager_v1 中的描述:
      <description summary="destroy the dde shell manager">
          Destroy the treeland_dde_shell_manager_v1 object.
          Objects created through this interface remain valid.
      </description>
      建议为其他接口也添加类似的说明,特别是明确指出销毁管理器对象是否会影响通过该管理器创建的子对象(例如 treeland_virtual_output_v1)。通常情况下,销毁管理器不应自动销毁子对象,但这需要在文档中明确,以避免客户端产生误解。
  • 缩进一致性:

    • 现状: 在 treeland-personalization-manager-v1.xml 中,destroy 请求的缩进似乎多了一个空格(<request name="destroy"...> 前面有 10 个空格,而上面的 <request name="get_appearance_context"...> 只有 9 个空格)。
    • 意见: 建议修复。请保持 XML 文件缩进的一致性,通常使用 4 个空格或 1 个 Tab。

3. 代码性能

  • 影响: 这些修改仅涉及协议定义的 XML 文件,不涉及具体的实现代码。因此,对运行时性能没有直接影响。
  • 注意: 在服务端(Compositor)实现这些 destroy 请求时,应确保操作是 O(1) 或低开销的,仅涉及资源句柄的释放和指针置空,不应执行繁重的阻塞操作。

4. 代码安全

  • 资源释放:
    • 意见: 必须确保服务端在处理 destroy 请求时,正确清理所有与该全局管理器对象相关的内部状态,但不要释放通过该管理器派生出来的对象(除非协议明确要求这样做,这通常是不好的设计)。如果客户端持有子对象的引用,管理器被销毁不应导致子对象悬空或崩溃。
  • 空指针检查:
    • 意见: 在实现代码中,处理 destroy 请求时必须检查对象是否已经被销毁(防止双重释放),尽管 type="destructor" 通常会由 Wayland 库辅助处理一部分逻辑,但服务端仍需健壮地处理异常情况。
  • Since 版本:
    • 现状: 所有 destroy 请求都正确标记了 since="2"
    • 意见: 正确。这确保了只支持版本 1 的旧客户端不会尝试发送这个请求,旧客户端通常依赖连接断开或 wl_display 的销毁来清理资源。

总结建议

  1. 调整顺序: 建议将 destroy 请求移动到接口定义的最前面(在 <description> 之后)。
  2. 完善文档: 统一并丰富 destroy 请求的 <description> 内容,明确说明销毁管理器对子对象的影响。
  3. 修复缩进: 修正 treeland-personalization-manager-v1.xml 中的缩进问题。

修改后的代码片段示例(以 treeland-screensaver-v1.xml 为例):

<interface name="treeland_screensaver_v1" version="2">
    <description summary="Simple idle inhibit protocol">
        This object implements a simple idle inhibit protocol used
        to implement org.freedesktop.ScreenSaver D-Bus interface.
        <!-- ... -->
    </description>

    <!-- 建议将 destroy 移到这里 -->
    <request name="destroy" type="destructor" since="2">
        <description summary="destroy the screensaver object">
            Destroy the treeland_screensaver_v1 object.
            Objects created through this interface (if any) remain valid.
        </description>
    </request>

    <request name="inhibit">
        <!-- ... -->
    </request>
    
    <!-- ... -->
</interface>

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wineee, zzxyb

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wineee wineee merged commit a4dfae7 into linuxdeepin:master Apr 28, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants