【不要合并！！！】feat: enable xembed support for xwayland #971
Conversation
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia 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 guide (collapsed on small PRs)Reviewer's GuideEnable the tray daemon’s Xembed/Xwayland-related X connection initialization path to be present in both X11 and Wayland sessions while effectively disabling the tray selection manager logic for now via a feature flag variable, and add region markers around the X connection setup block. Flow diagram for updated X connection initialization in Daemon.Startflowchart TD
A[Daemon.Start] --> B[Create sessionBus]
B --> C[Create sigLoop and start]
C --> D[Set enableTraySelectionManager = false]
D --> E{XDG_SESSION_TYPE != wayland?}
E -->|no| G[Skip XConn initialization]
E -->|yes| F{enableTraySelectionManager == true?}
F -->|no| G
F -->|yes| H[Call x.NewConn and assign to XConn]
H --> I[Initialize tray selection manager on X]
G --> J{DDE_DISABLE_STATUS_NOTIFIER_WATCHER != 1?}
I --> J
J -->|yes| K[Create StatusNotifierWatcher]
J -->|no| L[Skip StatusNotifierWatcher]
K --> M[Return from Daemon.Start]
L --> M
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 there - I've reviewed your changes and found some issues that need to be addressed.
- By unconditionally calling
x.NewConn()you now hard-fail the daemon on sessions without an X server/Xwayland; consider keeping an environment-based guard or gracefully degrading if the X connection cannot be established so Wayland-only sessions still work. - The
// #region/// #endregionmarkers are non-idiomatic in Go and inconsistent with typical comment style in this file; consider removing them or replacing with a simple explanatory comment block if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By unconditionally calling `x.NewConn()` you now hard-fail the daemon on sessions without an X server/Xwayland; consider keeping an environment-based guard or gracefully degrading if the X connection cannot be established so Wayland-only sessions still work.
- The `// #region` / `// #endregion` markers are non-idiomatic in Go and inconsistent with typical comment style in this file; consider removing them or replacing with a simple explanatory comment block if needed.
## Individual Comments
### Comment 1
<location> `trayicon1/daemon.go:51-53` </location>
<code_context>
- if err != nil {
- return err
- }
+ // Enable this on both x11 and wayland(for xwayland support)
+ // #region init x conn
+ XConn, err = x.NewConn()
+ if err != nil {
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally creating the X connection may cause Start() to fail on pure Wayland setups without Xwayland.
This used to be guarded by `XDG_SESSION_TYPE != "wayland"`, so the daemon could start on Wayland even without X. With `x.NewConn()` now unconditional, a failure (e.g., Wayland without Xwayland or DISPLAY) causes `Start()` to error and the daemon to fail to start. If X is meant to be optional, consider treating connection failures as non-fatal (e.g., log and continue without the tray manager, or only require a successful X connection when you know X should be available) to preserve the previous behavior while still enabling Xwayland when present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
TAG Bot New tag: 6.1.66 |
Contributor
|
TAG Bot New tag: 6.1.67 |
BLumia
force-pushed
the
xembed-treeland
branch
from
7aa7637 to
9c46abe
Compare
BLumia
changed the title
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The new
enableTraySelectionManagerflag is hardcoded tofalse, which means the X connection (and thus tray selection manager) is never initialized on any session type; if this is meant to be configurable, wire it to a real config/env option or remove the flag and keep the previous behavior adjusted for Wayland/Xwayland as intended. - The comment
// Enable this on both x11 and wayland(for xwayland support)does not match the current logic (guarded byenableTraySelectionManagerand still checkingXDG_SESSION_TYPE); please align the condition with the intended behavior or update the comment to avoid confusion. - The
// #region/// #endregionmarkers are not idiomatic in Go and may not be recognized by all editors; consider using a regular comment block or extracting the X initialization into a small helper function to make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `enableTraySelectionManager` flag is hardcoded to `false`, which means the X connection (and thus tray selection manager) is never initialized on any session type; if this is meant to be configurable, wire it to a real config/env option or remove the flag and keep the previous behavior adjusted for Wayland/Xwayland as intended.
- The comment `// Enable this on both x11 and wayland(for xwayland support)` does not match the current logic (guarded by `enableTraySelectionManager` and still checking `XDG_SESSION_TYPE`); please align the condition with the intended behavior or update the comment to avoid confusion.
- The `// #region` / `// #endregion` markers are not idiomatic in Go and may not be recognized by all editors; consider using a regular comment block or extracting the X initialization into a small helper function to make the intent clearer.
## Individual Comments
### Comment 1
<location> `trayicon1/daemon.go:51-54` </location>
<code_context>
- if os.Getenv("XDG_SESSION_TYPE") != "wayland" {
- // init x conn
+ // Enable this on both x11 and wayland(for xwayland support)
+ // #region init x conn
+ var enableTraySelectionManager = false
+ if os.Getenv("XDG_SESSION_TYPE") != "wayland" && enableTraySelectionManager {
XConn, err = x.NewConn()
if err != nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** The new flag and condition effectively disable the X connection everywhere and conflict with the comment about enabling on both X11 and Wayland.
Because `enableTraySelectionManager` is hardcoded to `false`, this block will never run on any session type. The condition also still excludes `XDG_SESSION_TYPE == "wayland"`, which conflicts with the comment about supporting both X11 and Wayland (via Xwayland).
If the goal is to gate this behind a flag while allowing Xwayland, consider either:
- Removing the `XDG_SESSION_TYPE != "wayland"` check and relying only on the flag, or
- Making `enableTraySelectionManager` configurable (env/config) and updating the condition so Xwayland sessions are actually included.
As it stands, the tray selection manager is effectively disabled everywhere.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+54
| // Enable this on both x11 and wayland(for xwayland support) | ||
| // #region init x conn | ||
| var enableTraySelectionManager = false | ||
| if os.Getenv("XDG_SESSION_TYPE") != "wayland" && enableTraySelectionManager { |
There was a problem hiding this comment.
issue (bug_risk): The new flag and condition effectively disable the X connection everywhere and conflict with the comment about enabling on both X11 and Wayland.
Because enableTraySelectionManager is hardcoded to false, this block will never run on any session type. The condition also still excludes XDG_SESSION_TYPE == "wayland", which conflicts with the comment about supporting both X11 and Wayland (via Xwayland).
If the goal is to gate this behind a flag while allowing Xwayland, consider either:
- Removing the
XDG_SESSION_TYPE != "wayland"check and relying only on the flag, or - Making
enableTraySelectionManagerconfigurable (env/config) and updating the condition so Xwayland sessions are actually included.
As it stands, the tray selection manager is effectively disabled everywhere.
BLumia
force-pushed
the
xembed-treeland
branch
from
9c46abe to
d539068
Compare
Contributor
|
TAG Bot New tag: 6.1.68 |
Contributor
|
TAG Bot New tag: 6.1.69 |
BLumia
force-pushed
the
xembed-treeland
branch
from
d539068 to
fb9ff7d
Compare
Contributor
|
TAG Bot New tag: 6.1.70 |
Contributor
|
TAG Bot New tag: 6.1.71 |
不再由 dde-daemon 的 trayicon1 模块注册 xembed selectionmanager Log:
BLumia
force-pushed
the
xembed-treeland
branch
from
fb9ff7d to
3c09ec8
Compare
deepin pr auto review这段代码修改的目的是在 Wayland 会话下支持 XWayland，从而扩展托盘图标的兼容性。以下是对该代码修改的详细审查意见： 1. 语法与逻辑审查
2. 代码质量与规范审查
3. 性能审查
4. 安全性审查
改进建议代码示例假设你的目的是无条件启用 X11 连接初始化（为了同时支持 X11 和 XWayland），建议修改如下： func (d *Daemon) Start() error {
d.sigLoop = dbusutil.NewSignalLoop(sessionBus, 10)
d.sigLoop.Start()
// Init X connection for X11 or XWayland support
// 尝试建立 X 连接，如果失败则返回错误（或者根据需求决定是否忽略错误）
XConn, err = x.NewConn()
if err != nil {
// 如果是在纯 Wayland 环境且不需要 XWayland 支持，这里可能需要只记录日志而不返回错误
// 但根据原代码逻辑，连接失败是致命错误
logger.Warningf("Failed to connect to X server: %v", err)
// return err // 根据实际需求决定是否直接退出
} else {
// 原有的初始化逻辑...
// ...
}
if os.Getenv("DDE_DISABLE_STATUS_NOTIFIER_WATCHER") != "1" {
d.snw = newStatusNotifierWatcher(service, d.sigLoop)
// ...
}
// ...
}或者，如果你确实需要通过一个开关来控制是否初始化 X 连接，建议使用环境变量： // 使用环境变量控制是否启用 X11 托盘管理器
enableX11Tray := os.Getenv("DDE_ENABLE_X11_TRAY") != "0"
if enableX11Tray {
XConn, err = x.NewConn()
// ...
}总结这段代码目前的修改存在致命的逻辑错误（变量为 |
Contributor
|
TAG Bot New tag: 6.1.72 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
为 wayland 会话也初始化 Xembed 托盘支持.
Summary by Sourcery
Enhancements: