Skip to content

Conversation

@calsys456
Copy link
Contributor

@calsys456 calsys456 commented Jan 27, 2026

In previous commit d63a6b we dispatch wayland events purely inside the QSocketNotifier. This will cause server events that is send before the notifier created is not being dispatched, which is not what we want and will cause severe issue in the current interaction model of ddm & treeland, as treeland will send an event immediately after ddm is connected. Fix it by dispatch once before create notifier.

Summary by Sourcery

Bug Fixes:

  • Dispatch and flush pending Wayland display events once before creating the QSocketNotifier to avoid missing early server events on connection.

@deepin-ci-robot
Copy link

Hi @calsys456. Thanks for your PR.

I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sourcery-ai
Copy link

sourcery-ai bot commented Jan 27, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds an initial Wayland event dispatch and flush before creating the QSocketNotifier so that server events sent immediately after connecting (but before the notifier exists) are properly processed.

Sequence diagram for Wayland connection and initial dispatch before QSocketNotifier

sequenceDiagram
    participant DDM as DdmDaemon
    participant TL as TreelandConnector
    participant WD as wl_display
    participant WSV as WaylandServer
    participant QSN as QSocketNotifier

    DDM->>TL: connect(socketPath)
    TL->>WD: wl_display_connect
    WSV-->>WD: send initial events after connection
    TL->>WD: wl_display_roundtrip
    WD-->>WSV: complete roundtrip
    TL->>WD: wl_display_dispatch
    note over TL,WD: Dispatch initial server events
    TL->>WD: wl_display_flush
    note over TL,WD: Flush outgoing requests
    TL->>WD: wl_display_get_fd
    TL->>QSN: new QSocketNotifier(fd, Read)
    TL->>QSN: connect(activated, lambda)
    QSN-->>TL: activated on readable fd
    TL->>WD: wl_display_dispatch
    TL->>WD: wl_display_flush
Loading

Flow diagram for updated Wayland client dispatch loop in TreelandConnector

flowchart TD
    A[connect socketPath] --> B[wl_display_connect]
    B --> C[wl_display_roundtrip]
    C --> D[wl_display_dispatch]
    D --> E[wl_display_flush]
    E --> F[wl_display_get_fd]
    F --> G[create QSocketNotifier Read]
    G --> H[on QSocketNotifier activated]
    H --> I[wl_display_dispatch]
    I --> J[wl_display_flush]
    J --> H
Loading

File-Level Changes

Change Details Files
Process pending Wayland events once immediately after connecting, before installing the QSocketNotifier, to avoid missing early server events.
  • Invoke wl_display_dispatch right after wl_display_roundtrip completes to handle any queued events
  • Call wl_display_flush immediately after the initial dispatch to ensure outgoing requests are sent to the Wayland server
  • Leave the existing QSocketNotifier-based dispatch/flush loop unchanged, preserving the current event handling model
src/daemon/TreelandConnector.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • Consider handling the return values of the initial wl_display_dispatch and wl_display_flush calls (e.g., logging or early return on -1) to be consistent with the error handling you already do inside the notifier callback.
  • Since wl_display_roundtrip already performs a blocking dispatch/flush cycle, it might be worth clarifying (in code comments) why an additional immediate wl_display_dispatch/wl_display_flush is needed here, or whether a non-blocking call like wl_display_dispatch_pending would better match the intent to clear pre-notifier events.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider handling the return values of the initial `wl_display_dispatch` and `wl_display_flush` calls (e.g., logging or early return on `-1`) to be consistent with the error handling you already do inside the notifier callback.
- Since `wl_display_roundtrip` already performs a blocking dispatch/flush cycle, it might be worth clarifying (in code comments) why an additional immediate `wl_display_dispatch`/`wl_display_flush` is needed here, or whether a non-blocking call like `wl_display_dispatch_pending` would better match the intent to clear pre-notifier events.

## Individual Comments

### Comment 1
<location> `src/daemon/TreelandConnector.cpp:208-209` </location>
<code_context>

     wl_display_roundtrip(m_display);

+    wl_display_dispatch(m_display);
+    wl_display_flush(m_display);
     m_notifier = new QSocketNotifier(wl_display_get_fd(m_display), QSocketNotifier::Read);
     QObject::connect(m_notifier, &QSocketNotifier::activated, this, [this] {
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Consider whether this synchronous dispatch is appropriate here, as it may block unexpectedly during connect.

`wl_display_dispatch` after `wl_display_roundtrip` can block waiting for new events, which may stall `connect` if none are pending. If you only need to process already-queued events, consider `wl_display_dispatch_pending` or relying solely on the `QSocketNotifier` callback to avoid blocking during setup.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

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 aims to ensure that initial Wayland events sent by Treeland immediately after ddm connects are not missed before the QSocketNotifier event loop is established. It does this by attempting to dispatch and flush Wayland display events once right after connection and before creating the notifier.

本 PR 旨在确保 Treeland 在 ddm 建立连接后立刻发送的初始 Wayland 事件不会在创建 QSocketNotifier 之前被遗漏。为此,它尝试在连接完成后、创建 notifier 之前,对 Wayland display 进行一次事件分发和 flush。

Changes:

  • After wl_display_roundtrip(m_display), add a loop using wl_display_prepare_read() and wl_display_dispatch_pending() followed by wl_display_flush() to handle any pending Wayland events before constructing the QSocketNotifier.
  • Keep the existing QSocketNotifier-based dispatch loop unchanged, still calling wl_display_dispatch() and wl_display_flush() on activation.

变更点:

  • 在调用 wl_display_roundtrip(m_display) 之后,新增一段循环,使用 wl_display_prepare_read()wl_display_dispatch_pending(),然后调用 wl_display_flush(),试图在创建 QSocketNotifier 之前处理所有挂起的 Wayland 事件。
  • 保持原有基于 QSocketNotifier 的分发逻辑不变,在激活时继续调用 wl_display_dispatch()wl_display_flush()

Reviewed changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

File Description
src/daemon/TreelandConnector.cpp Adjusts the Wayland connection setup to dispatch/flush display events once before installing the QSocketNotifier read handler.
src/daemon/TreelandConnector.cpp 调整 Wayland 连接建立流程,在安装 QSocketNotifier 读事件处理前,先对 display 执行一次事件分发与 flush。

In previous commit d63a6b we dispatch wayland events purely inside the
QSocketNotifier. This will cause server events that is send before the
notifier created is not being dispatched, which is not what we want and
will cause severe issue in the current interaction model of ddm &
treeland, as treeland will send an event immediately after ddm is
connected. Fix it by dispatch once before create notifier.
@zccrs zccrs merged commit 6376608 into linuxdeepin:master Jan 27, 2026
8 checks passed
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: calsys456, zccrs

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants