-
Notifications
You must be signed in to change notification settings - Fork 16
fix: fix wayland client dispatch loop #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds 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 QSocketNotifiersequenceDiagram
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
Flow diagram for updated Wayland client dispatch loop in TreelandConnectorflowchart 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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this 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_dispatchandwl_display_flushcalls (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_roundtripalready performs a blocking dispatch/flush cycle, it might be worth clarifying (in code comments) why an additional immediatewl_display_dispatch/wl_display_flushis needed here, or whether a non-blocking call likewl_display_dispatch_pendingwould 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
40b7512 to
5a19955
Compare
There was a problem hiding this 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 usingwl_display_prepare_read()andwl_display_dispatch_pending()followed bywl_display_flush()to handle any pending Wayland events before constructing theQSocketNotifier. - Keep the existing
QSocketNotifier-based dispatch loop unchanged, still callingwl_display_dispatch()andwl_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.
5a19955 to
f7bc3be
Compare
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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: