【不要合并！！！】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 |
不再由 dde-daemon 的 trayicon1 模块注册 xembed selectionmanager Log:
BLumia
force-pushed
the
xembed-treeland
branch
from
d539068 to
fb9ff7d
Compare
deepin pr auto review我来对这个代码变更进行详细审查：
改进建议： // #region init x conn
// 通过环境变量控制是否启用托盘选择管理器
enableTraySelectionManager := os.Getenv("DDE_ENABLE_TRAY_SELECTION_MANAGER") == "1"
if (os.Getenv("XDG_SESSION_TYPE") != "wayland" || os.Getenv("XDG_SESSION_TYPE") == "wayland") && enableTraySelectionManager {
XConn, err = x.NewConn()
if err != nil {
logger.Warning("Failed to initialize X connection:", err)
return err
}
// ... 其余代码保持不变
}
// #endregion主要改进点：
这样的改进会让代码更加灵活和可维护，同时保持了原有的功能和安全性。 |
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: