feat: register tray selectionmanager on wayland #406
+736
−4
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 GuideImplements a Wayland-compatible XEmbed system tray selection manager by introducing FdoSelectionManager and TrayManager1 DBus service, wiring them into TrayPlugin when not on X11, and updating build configuration and dependencies accordingly. Sequence diagram for XEmbed tray icon docking via FdoSelectionManager and TrayManager1sequenceDiagram
autonumber
participant TrayPlugin
participant FdoSelectionManager
participant KSelectionOwner
participant X11Server
participant TrayManager1
participant DBusSession as DBusSessionBus
participant ClientTrayApp as XEmbedClient
TrayPlugin->>TrayPlugin: constructor
TrayPlugin->>TrayPlugin: registerDBusMetaTypes
TrayPlugin->>FdoSelectionManager: new FdoSelectionManager(this) when not X11 and XAvaliable
activate FdoSelectionManager
FdoSelectionManager->>KSelectionOwner: new KSelectionOwner(_NET_SYSTEM_TRAY,...)
FdoSelectionManager->>FdoSelectionManager: QTimer singleShot init
FdoSelectionManager->>X11Server: xcb_damage_query_version
FdoSelectionManager->>QGuiApplication: installNativeEventFilter
FdoSelectionManager->>KSelectionOwner: claim(false)
KSelectionOwner-->>FdoSelectionManager: claimedOwnership()
FdoSelectionManager->>FdoSelectionManager: onClaimedOwnership
FdoSelectionManager->>FdoSelectionManager: initTrayManager
FdoSelectionManager->>TrayManager1: new TrayManager1(this)
FdoSelectionManager->>DBusSession: registerObject /org/deepin/dde/TrayManager1
FdoSelectionManager->>DBusSession: registerService org.deepin.dde.TrayManager1
FdoSelectionManager->>X11Server: setSystemTrayVisual property
XEmbedClient->>X11Server: send _NET_SYSTEM_TRAY_OPCODE DOCK(winId)
X11Server-->>FdoSelectionManager: XCB_CLIENT_MESSAGE via nativeEventFilter
FdoSelectionManager->>FdoSelectionManager: dock(winId)
FdoSelectionManager->>FdoSelectionManager: addDamageWatch(winId)
FdoSelectionManager->>TrayManager1: registerIcon(winId)
TrayManager1-->>DBusSession: signal Added(winId)
DBusSession-->>Clients: TrayManager1 Added(winId)
Class diagram for new Wayland tray selection manager integrationclassDiagram
direction LR
class TrayPlugin {
+TrayPlugin(QObject *parent)
+~TrayPlugin()
-QHash~QString, QWidget*~ m_widget
-QHash~QString, QWidget*~ m_tooltip
-PluginProxyInterface* m_proyInter
-FdoSelectionManager* m_selectionManager
}
class FdoSelectionManager {
+FdoSelectionManager(QObject *parent)
+~FdoSelectionManager()
+bool nativeEventFilter(QByteArray eventType, void* message, qintptr* result)
-void init()
-bool addDamageWatch(xcb_window_t client)
-void dock(xcb_window_t embed_win)
-void undock(xcb_window_t client)
-void setSystemTrayVisual()
-void initTrayManager()
-void onClaimedOwnership()
-void onFailedToClaimOwnership()
-void onLostOwnership()
-TrayManager1* m_trayManager
-uint8_t m_damageEventBase
-QHash~xcb_window_t, u_int32_t~ m_damageWatches
-KSelectionOwner* m_selectionOwner
}
class TrayManager1 {
+TrayManager1(QObject *parent)
+~TrayManager1()
+void registerIcon(xcb_window_t win)
+void unregisterIcon(xcb_window_t win)
+void notifyIconChanged(xcb_window_t win)
+TrayList trayIcons() const
+bool haveIcon(xcb_window_t win) const
+bool Manage()
+QString GetName(uint32_t win)
+void EnableNotification(uint32_t win, bool enabled)
+signal void Added(uint32_t id)
+signal void Removed(uint32_t id)
+signal void Changed(uint32_t id)
+signal void Inited()
-TrayManager1Adaptor* m_adaptor
-QHash~xcb_window_t, bool~ m_icons
}
class TrayManager1Adaptor {
<<qt_dbus_adaptor>>
}
class KSelectionOwner {
}
class Util {
+static Util* instance()
+xcb_connection_t* getX11Connection()
+xcb_window_t getRootWindow()
+xcb_atom_t getAtomFromDisplay(const char* name)
+xcb_atom_t getAtomByName(const char* name)
}
class UniqueCPointer~T~ {
-CDeleter m_deleter
}
class CDeleter {
+void operator()(T* ptr)
}
TrayPlugin --> FdoSelectionManager : owns
FdoSelectionManager --> TrayManager1 : creates and owns
FdoSelectionManager --> KSelectionOwner : owns
FdoSelectionManager --> Util : uses
FdoSelectionManager --> UniqueCPointer : uses
TrayManager1 --> TrayManager1Adaptor : owns
UniqueCPointer --|> std_unique_ptr
CDeleter --> UniqueCPointer : deleter type
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 - I've found 5 issues, and left some high level feedback:
- In FdoSelectionManager::undock(), the early return condition uses
if (m_trayManager->haveIcon(winId)) { return; }, which looks inverted compared to dock(); this prevents unregistering existing icons and likely should beif (!m_trayManager->haveIcon(winId)) { return; }. - In fdoselectionmanager.h the hash is declared as
QHash<xcb_window_t, u_int32_t> m_damageWatches;– consider using the standarduint32_ttype for consistency with the rest of the xcb-related code and platform headers. - FdoSelectionManager::init and ownership handlers call
qApp->exit(-1)on failures; since this code runs inside a plugin, it may be safer to signal the error back to the host instead of terminating the entire application.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In FdoSelectionManager::undock(), the early return condition uses `if (m_trayManager->haveIcon(winId)) { return; }`, which looks inverted compared to dock(); this prevents unregistering existing icons and likely should be `if (!m_trayManager->haveIcon(winId)) { return; }`.
- In fdoselectionmanager.h the hash is declared as `QHash<xcb_window_t, u_int32_t> m_damageWatches;` – consider using the standard `uint32_t` type for consistency with the rest of the xcb-related code and platform headers.
- FdoSelectionManager::init and ownership handlers call `qApp->exit(-1)` on failures; since this code runs inside a plugin, it may be safer to signal the error back to the host instead of terminating the entire application.
## Individual Comments
### Comment 1
<location> `plugins/application-tray/fdoselectionmanager.cpp:44-47` </location>
<code_context>
+ QTimer::singleShot(0, this, &FdoSelectionManager::init);
+}
+
+FdoSelectionManager::~FdoSelectionManager()
+{
+ qCDebug(SELECTIONMGR) << "closing";
+ m_selectionOwner->release();
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Native event filter is never removed in the destructor.
Since the constructor installs this as a native event filter via `qApp->installNativeEventFilter(this)`, the destructor should also call `qApp->removeNativeEventFilter(this)` before releasing state to avoid dangling event filter callbacks into a destroyed object.
</issue_to_address>
### Comment 2
<location> `plugins/application-tray/fdoselectionmanager.cpp:154-163` </location>
<code_context>
+ return false;
+}
+
+void FdoSelectionManager::dock(xcb_window_t winId)
+{
+ Q_CHECK_PTR(m_trayManager);
+ qCDebug(SELECTIONMGR) << "trying to dock window " << winId;
+
+ if (m_trayManager->haveIcon(winId)) {
+ return;
+ }
+
+ if (addDamageWatch(winId)) {
+ // Register with TrayManager1 if available
+ m_trayManager->registerIcon(winId);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential race/use-before-init of m_trayManager in dock/undock paths.
`dock`/`undock` dereference `m_trayManager` unconditionally, but it’s only created in `initTrayManager()` (from `onClaimedOwnership()`). A tray client message can arrive before `claimedOwnership`, so `dock()` may run with `m_trayManager == nullptr`, tripping `Q_CHECK_PTR` in debug or causing UB in release. Please either ensure `m_trayManager` is constructed earlier (e.g. in `init()`) or skip/defer handling until it is initialized.
</issue_to_address>
### Comment 3
<location> `plugins/application-tray/fdoselectionmanager.cpp:169-178` </location>
<code_context>
+ }
+}
+
+void FdoSelectionManager::undock(xcb_window_t winId)
+{
+ Q_CHECK_PTR(m_trayManager);
+ qCDebug(SELECTIONMGR) << "trying to undock window " << winId;
+
+ if (m_trayManager->haveIcon(winId)) {
+ return;
+ }
+
+ // Unregister from TrayManager1 if available
+ m_trayManager->unregisterIcon(winId);
+
+ // m_proxies[winId]->deleteLater();
+ // m_proxies.remove(winId);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Undock logic returns early when an icon exists, preventing proper unregister.
```cpp
if (m_trayManager->haveIcon(winId)) {
return;
}
// ...
m_trayManager->unregisterIcon(winId);
```
This condition looks reversed: when the icon exists we return and never call `unregisterIcon`, so the icon is never removed. I think this should be:
```cpp
if (!m_trayManager->haveIcon(winId)) {
return;
}
m_trayManager->unregisterIcon(winId);
```
</issue_to_address>
### Comment 4
<location> `plugins/application-tray/fdoselectionmanager.h:49` </location>
<code_context>
+
+ uint8_t m_damageEventBase;
+
+ QHash<xcb_window_t, u_int32_t> m_damageWatches;
+ // QHash<xcb_window_t, TrayManagerProxy *> m_proxies;
+ KSelectionOwner *m_selectionOwner;
</code_context>
<issue_to_address>
**suggestion:** Use of non‑standard `u_int32_t` type for damage IDs.
`m_damageWatches` is declared as `QHash<xcb_window_t, u_int32_t>`. `u_int32_t` is a non‑standard extension and may not exist on all platforms/compilers. Please switch to a portable type such as `uint32_t` from `<cstdint>` or, if it better reflects the intent, an appropriate XCB damage type like `xcb_damage_direct_t`/`xcb_damage_damage_t`.
Suggested implementation:
```c
QHash<xcb_window_t, uint32_t> m_damageWatches;
```
To ensure `uint32_t` is available in this header on all platforms, add `#include <cstdint>` (or `<stdint.h>`) in the include section of `fdoselectionmanager.h` if it is not already indirectly included. Place it alongside the other standard-library includes.
</issue_to_address>
### Comment 5
<location> `plugins/application-tray/CMakeLists.txt:11-12` </location>
<code_context>
find_package(PkgConfig REQUIRED)
find_package(Qt${QT_VERSION_MAJOR} ${REQUIRED_QT_VERSION} REQUIRED Core Gui Widgets DBus)
find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED Core Gui Widget)
+find_package(KF6WindowSystem 6.6 REQUIRED) # for x11 tray section owner
-pkg_check_modules(X11 REQUIRED IMPORTED_TARGET x11 xcb xcb-image xcb-composite xcb-xfixes xcb-util xcb-shape xtst xcb-xtest xcb-res xcb-ewmh)
</code_context>
<issue_to_address>
**suggestion (typo):** Typo and capitalization in inline comment: use "X11 tray selection owner".
In `# for x11 tray section owner`, "section" should be "selection" (per the X11 "selection owner" concept), and "x11" should be capitalized to "X11": `# for X11 tray selection owner`.
```suggestion
find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED Core Gui Widget)
find_package(KF6WindowSystem 6.6 REQUIRED) # for X11 tray selection owner
```
</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
+44
to
+47
| FdoSelectionManager::~FdoSelectionManager() | ||
| { | ||
| qCDebug(SELECTIONMGR) << "closing"; | ||
| m_selectionOwner->release(); |
There was a problem hiding this comment.
issue (bug_risk): Native event filter is never removed in the destructor.
Since the constructor installs this as a native event filter via qApp->installNativeEventFilter(this), the destructor should also call qApp->removeNativeEventFilter(this) before releasing state to avoid dangling event filter callbacks into a destroyed object.
Comment on lines
+154
to
+167
| void FdoSelectionManager::dock(xcb_window_t winId) | ||
| { | ||
| Q_CHECK_PTR(m_trayManager); | ||
| qCDebug(SELECTIONMGR) << "trying to dock window " << winId; | ||
|
|
||
| if (m_trayManager->haveIcon(winId)) { | ||
| return; | ||
| } | ||
|
|
||
| if (addDamageWatch(winId)) { |
There was a problem hiding this comment.
issue (bug_risk): Potential race/use-before-init of m_trayManager in dock/undock paths.
dock/undock dereference m_trayManager unconditionally, but it’s only created in initTrayManager() (from onClaimedOwnership()). A tray client message can arrive before claimedOwnership, so dock() may run with m_trayManager == nullptr, tripping Q_CHECK_PTR in debug or causing UB in release. Please either ensure m_trayManager is constructed earlier (e.g. in init()) or skip/defer handling until it is initialized.
Comment on lines
+169
to
+182
| void FdoSelectionManager::undock(xcb_window_t winId) | ||
| { | ||
| Q_CHECK_PTR(m_trayManager); | ||
| qCDebug(SELECTIONMGR) << "trying to undock window " << winId; | ||
|
|
||
| if (m_trayManager->haveIcon(winId)) { | ||
| return; | ||
| } | ||
|
|
||
| // Unregister from TrayManager1 if available |
There was a problem hiding this comment.
issue (bug_risk): Undock logic returns early when an icon exists, preventing proper unregister.
if (m_trayManager->haveIcon(winId)) {
return;
}
// ...
m_trayManager->unregisterIcon(winId);This condition looks reversed: when the icon exists we return and never call unregisterIcon, so the icon is never removed. I think this should be:
if (!m_trayManager->haveIcon(winId)) {
return;
}
m_trayManager->unregisterIcon(winId);|
|
||
| uint8_t m_damageEventBase; | ||
|
|
||
| QHash<xcb_window_t, u_int32_t> m_damageWatches; |
There was a problem hiding this comment.
suggestion: Use of non‑standard u_int32_t type for damage IDs.
m_damageWatches is declared as QHash<xcb_window_t, u_int32_t>. u_int32_t is a non‑standard extension and may not exist on all platforms/compilers. Please switch to a portable type such as uint32_t from <cstdint> or, if it better reflects the intent, an appropriate XCB damage type like xcb_damage_direct_t/xcb_damage_damage_t.
Suggested implementation:
QHash<xcb_window_t, uint32_t> m_damageWatches;To ensure uint32_t is available in this header on all platforms, add #include <cstdint> (or <stdint.h>) in the include section of fdoselectionmanager.h if it is not already indirectly included. Place it alongside the other standard-library includes.
Comment on lines
11
to
12
| find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED Core Gui Widget) | ||
| find_package(KF6WindowSystem 6.6 REQUIRED) # for x11 tray section owner |
There was a problem hiding this comment.
suggestion (typo): Typo and capitalization in inline comment: use "X11 tray selection owner".
In # for x11 tray section owner, "section" should be "selection" (per the X11 "selection owner" concept), and "x11" should be capitalized to "X11": # for X11 tray selection owner.
Suggested change
| find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED Core Gui Widget) | |
| find_package(KF6WindowSystem 6.6 REQUIRED) # for x11 tray section owner | |
| find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED Core Gui Widget) | |
| find_package(KF6WindowSystem 6.6 REQUIRED) # for X11 tray selection owner |
tsic404
requested changes
Dec 25, 2025
| void FdoSelectionManager::onLostOwnership() | ||
| { | ||
| qCWarning(SELECTIONMGR) << "lost ownership of Systray Manager"; | ||
| qApp->exit(-1); |
Member
Author
There was a problem hiding this comment.
已修改为强制claim，保留了此信号的日志输出以防万一。退出行为已删除
| { | ||
| // Create and register the TrayManager1 DBus interface | ||
| if (!m_trayManager) { | ||
| m_trayManager = new TrayManager1(this); |
Contributor
There was a problem hiding this comment.
Manage() 调用对接到 claim(true) 上
| registerDBusToolTipMetaType(); | ||
| registerDBusImageListMetaType(); | ||
|
|
||
| if (!KWindowSystem::isPlatformX11() && UTIL->isXAvaliable()) { |
Contributor
There was a problem hiding this comment.
不能用 qpa，使用 XDG_SESSION_TYPE 判断
| QHash<QString, QWidget*> m_widget; | ||
| QHash<QString, QWidget*> m_tooltip; | ||
| PluginProxyInterface *m_proyInter; | ||
| FdoSelectionManager *m_selectionManager = nullptr; |
Contributor
There was a problem hiding this comment.
FdoSelectionManager 属于 Xembed 的实现，不应该在 TrayPlugin 这个抽象类上。
|
TAG Bot New tag: 2.0.20 |
BLumia
commented
Dec 25, 2025
| xcb_damage_query_version_unchecked(c, XCB_DAMAGE_MAJOR_VERSION, XCB_DAMAGE_MINOR_VERSION); | ||
| } else { | ||
| // no XDamage means | ||
| qCCritical(SELECTIONMGR) << "could not load damage extension."; |
Contributor
There was a problem hiding this comment.
走到这了，那就是 X server 缺少必要的扩展。业务逻辑依赖这些扩展，可以退出。
tsic404
reviewed
Dec 26, 2025
| { | ||
| qApp->installNativeEventFilter(this); | ||
|
|
||
| m_trayManager->Manage(); |
Contributor
There was a problem hiding this comment.
Dbus 的注册是下面执行的，那么此处的调用是不是应该延后？
| xcb_damage_query_version_unchecked(c, XCB_DAMAGE_MAJOR_VERSION, XCB_DAMAGE_MINOR_VERSION); | ||
| } else { | ||
| // no XDamage means | ||
| qCCritical(SELECTIONMGR) << "could not load damage extension."; |
Contributor
There was a problem hiding this comment.
走到这了，那就是 X server 缺少必要的扩展。业务逻辑依赖这些扩展，可以退出。
BLumia
force-pushed
the
xembed-tray
branch
2 times, most recently
from
1ea5c5e to
c259057
Compare
tsic404
reviewed
Dec 26, 2025
| return true; | ||
| } | ||
|
|
||
| QString TrayManager1::GetName(uint32_t win) |
Contributor
There was a problem hiding this comment.
这个是什么？下面的 connection 可以拿到吗？以及使用 KWindownInfo 可以起到预期效果吗？
支持在 wayland 环境下为 Xembed 支持注册 selectionowner 并暴露相关 托盘到 dbus 接口上。 Log:
deepin pr auto review我来对这个diff进行代码审查。这个diff主要涉及dde-tray-loader项目的更新，包括版本升级、许可证添加、依赖更新以及新的FDO系统托盘选择管理器的实现。
安全性：
性能：
代码质量：
安全性：
性能：
代码质量：
建议改进：
总体来说，这个更新实现了Freedesktop.org标准的系统托盘支持，提高了兼容性，代码质量良好，安全性考虑周到，性能优化合理。建议在合并前进行充分的测试，特别是在不同桌面环境下的兼容性测试。 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 支持注册 selectionowner 并暴露相关托盘到 dbus 接口上。
Summary by Sourcery
Add a Wayland-capable XEmbed tray selection manager and expose embedded tray icons over a new DBus TrayManager1 interface.
New Features:
Enhancements:
Build: