feat: able to control if we should register tray selectionmanager via DConfig#971
feat: able to control if we should register tray selectionmanager via DConfig#971BLumia wants to merge 1 commit into
Conversation
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.
|
TAG Bot New tag: 6.1.66 |
|
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.
BLumia
force-pushed
the
xembed-treeland
branch
from
9c46abe to
d539068
Compare
|
TAG Bot New tag: 6.1.68 |
|
TAG Bot New tag: 6.1.69 |
BLumia
force-pushed
the
xembed-treeland
branch
from
d539068 to
fb9ff7d
Compare
|
TAG Bot New tag: 6.1.70 |
|
TAG Bot New tag: 6.1.71 |
BLumia
force-pushed
the
xembed-treeland
branch
from
fb9ff7d to
3c09ec8
Compare
|
TAG Bot New tag: 6.1.72 |
|
TAG Bot New tag: 6.1.73 |
|
TAG Bot New tag: 6.1.74 |
|
TAG Bot New tag: 6.1.75 |
|
TAG Bot New tag: 6.1.76 |
|
TAG Bot New tag: 6.1.77 |
|
TAG Bot New tag: 6.1.78 |
|
TAG Bot New tag: 6.1.79 |
|
TAG Bot New tag: 6.1.80 |
|
TAG Bot New tag: 6.1.81 |
|
TAG Bot New tag: 6.1.82 |
|
TAG Bot New tag: 6.1.83 |
|
TAG Bot New tag: 6.1.84 |
|
TAG Bot New tag: 6.1.85 |
|
TAG Bot New tag: 6.1.86 |
|
TAG Bot New tag: 6.1.87 |
|
TAG Bot New tag: 6.1.88 |
BLumia
changed the title
BLumia
force-pushed
the
xembed-treeland
branch
2 times, most recently
from
c20dfb4 to
0f5492c
Compare
…anager 允许通过 DConfig 控制 dde-daemon 的 trayicon1 模块是否注册自身为 xembed selectionmanager，默认为仅在 x11 下注册（现状），允许任何时候都注册、 仅x11/仅wayland注册、任何时候都不注册。 Log:
deepin pr auto review你好！我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改的主要目的是通过引入DConfig配置，让托盘 整体来看，代码逻辑清晰，配置项设计合理。但在代码逻辑的严谨性、性能、安全性以及代码规范方面，有一些改进建议供你参考： 1. 语法与逻辑
2. 代码性能
3. 代码安全
4. 代码质量与规范
改进后的代码示例结合以上建议，以下是优化后的代码参考： daemon.go 优化部分： type Daemon struct {
*loader.ModuleBase
sigLoop *dbusutil.SignalLoop
snw *statusNotifierWatcher
// 将 DConfig 作为成员变量，避免资源泄漏和重复创建
trayIconDConfig *dconfig.DConfig
}
const moduleName = "trayicon"
const (
dconfigAppID = "org.deepin.dde.daemon"
dconfigTrayIconID = "org.deepin.dde.daemon.trayicon"
dconfigKeyTraySelectionScope = "traySelectionManagerScope"
scopeNever = "never"
scopeX11Only = "x11-only"
scopeWaylandOnly = "wayland-only"
scopeAlways = "always"
)
func NewDaemon(logger *log.Logger) *Daemon {
daemon := new(Daemon)
daemon.ModuleBase = loader.NewModuleBase(moduleName, daemon, logger)
// 初始化 DConfig
var err error
daemon.trayIconDConfig, err = dconfig.NewDConfig(dconfigAppID, dconfigTrayIconID, "")
if err != nil {
logger.Warning("failed to create trayicon dconfig:", err)
}
return daemon
}
func shouldRegisterTraySelectionManager(scope string) bool {
sessionType := os.Getenv("XDG_SESSION_TYPE")
isWayland := sessionType == "wayland"
switch scope {
case scopeNever:
return false
case scopeX11Only:
return !isWayland
case scopeWaylandOnly:
return isWayland
case scopeAlways:
return true
default:
// 统一降级策略：遇到非法配置，打印警告并按 X11-Only 处理（与原逻辑保持一致）
logger.Warningf("unknown tray selection scope: %q, fallback to x11-only", scope)
return !isWayland
}
}
func (d *Daemon) getTraySelectionManagerScope() string {
if d.trayIconDConfig == nil {
// 降级为 X11-Only
return scopeX11Only
}
scope, err := d.trayIconDConfig.GetValueString(dconfigKeyTraySelectionScope)
if err != nil {
logger.Warning("failed to get tray selection manager scope:", err)
// 降级为 X11-Only
return scopeX11Only
}
return scope
}
func (d *Daemon) Start() error {
var err error
service := loader.GetService()
sessionBus, err := service.SessionBus()
if err != nil {
return err
}
d.sigLoop = dbusutil.NewSignalLoop(sessionBus, 10)
d.sigLoop.Start()
scope := d.getTraySelectionManagerScope()
if shouldRegisterTraySelectionManager(scope) {
XConn, err = x.NewConn()
if err != nil {
return err
}
// ... 原有逻辑 ...
}
// ...
}JSON 配置优化建议： {
"magic": "dsg.config.meta",
"version": "1.0",
"contents": {
"traySelectionManagerScope": {
"value": "x11-only",
"serial": 0,
"flags": [],
"name": "traySelectionManagerScope",
"name[zh_CN]": "托盘 Selection Manager 注册范围",
"description": "Control when to register tray selection manager: never, x11-only, wayland-only, always",
"description[zh_CN]": "控制何时注册托盘 Selection Manager：never（不注册）、x11-only（仅 X11）、wayland-only（仅 Wayland）、always（始终注册）",
"permissions": "readwrite",
"visibility": "public"
}
}
}(注：JSON中的默认值 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, fly602 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 |
3 participants
为 wayland 会话也初始化 Xembed 托盘支持.
Summary by Sourcery
Enhancements: