fix: resolve DBUS deadlock in application tray#430
Open
18202781743 wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: resolve DBUS deadlock in application tray#43018202781743 wants to merge 1 commit intolinuxdeepin:masterfrom
18202781743 wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743 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 GuideSwitches StatusNotifierItem DBus interface generation from Qt’s synchronous qt_add_dbus_interfaces to DTK’s asynchronous dtk_add_dbus_interfaces to avoid DBus deadlocks in the application tray, adds the required DTK Tools dependency and include path, and cleans up declarations/namespacing for the generated types. Sequence diagram for asynchronous StatusNotifierItem DBus property accesssequenceDiagram
participant DockApplicationTrayPlugin
participant SniTrayProtocolHandler
participant StatusNotifierItemAsyncProxy
participant DBusBus
participant StatusNotifierItemService
DockApplicationTrayPlugin->>SniTrayProtocolHandler: requestStatusNotifierProperties
SniTrayProtocolHandler->>StatusNotifierItemAsyncProxy: getPropertiesAsync
StatusNotifierItemAsyncProxy->>DBusBus: org.kde.StatusNotifierItem.GetAll
DBusBus->>StatusNotifierItemService: GetAll
StatusNotifierItemService-->>DBusBus: properties
DBusBus-->>StatusNotifierItemAsyncProxy: properties
StatusNotifierItemAsyncProxy-->>SniTrayProtocolHandler: propertiesCallback
SniTrayProtocolHandler-->>DockApplicationTrayPlugin: updateTrayIconState
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 1 issue, and left some high level feedback:
- Avoid
using namespace org::kde;insniprotocolhandler.h, as this is a header and will leak the namespace into all includers; prefer qualifying the specific types or introducing a using-alias for the required classes only. - In the CMakeLists, consider using
${CMAKE_CURRENT_SOURCE_DIR}/api/instead of${CMAKE_SOURCE_DIR}/plugins/application-tray/api/for the include directory so the target remains more self-contained and easier to reuse or move.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid `using namespace org::kde;` in `sniprotocolhandler.h`, as this is a header and will leak the namespace into all includers; prefer qualifying the specific types or introducing a using-alias for the required classes only.
- In the CMakeLists, consider using `${CMAKE_CURRENT_SOURCE_DIR}/api/` instead of `${CMAKE_SOURCE_DIR}/plugins/application-tray/api/` for the include directory so the target remains more self-contained and easier to reuse or move.
## Individual Comments
### Comment 1
<location> `plugins/application-tray/sniprotocolhandler.h:20-21` </location>
<code_context>
class DBusMenuImporter;
-class StatusNotifierItem;
namespace tray {
class DDEindicatorProtocolHandler;
</code_context>
<issue_to_address>
**issue:** Avoid `using namespace` in a public header to prevent polluting includers’ namespaces.
Putting `using namespace org::kde;` in a header injects all of `org::kde` into every includer’s global namespace, risking symbol collisions and confusing name lookup. Prefer fully qualifying `org::kde` types here or introducing a narrow alias (e.g. `namespace kde = org::kde;`) and using that explicitly, and keep any broad `using` directives in the .cpp file instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1. Changed from Qt's DBus interface generation to DTK's asynchronous DBus interface generation for StatusNotifierItem 2. Added DTK Tools dependency to CMakeLists.txt for dtk_add_dbus_interfaces 3. Modified DBus interface generation to use dtk_add_dbus_interfaces for org.kde.StatusNotifierItem.xml 4. Updated include directories to include the local api directory 5. Removed forward declaration of StatusNotifierItem class since it's now generated by DTK 6. Added namespace usage for org::kde in sniprotocolhandler.h The issue was that Qt's synchronous DBus property calls could cause deadlocks in the application tray. By switching to DTK's asynchronous DBus interface generation, property access becomes non-blocking, preventing the deadlock situation. This change specifically targets the StatusNotifierItem interface which was causing the deadlock. Log: Fixed application tray DBus deadlock issue Influence: 1. Test application tray functionality with various system tray applications 2. Verify tray icons load correctly without hanging 3. Test DBus communication with StatusNotifierItem services 4. Monitor system responsiveness when multiple tray applications are running 5. Test tray menu interactions and property updates 6. Verify no regression in tray icon display or functionality fix: 解决应用托盘DBus死锁问题 1. 将StatusNotifierItem的DBus接口生成从Qt改为DTK的异步DBus接口生成 2. 在CMakeLists.txt中添加DTK Tools依赖以使用dtk_add_dbus_interfaces 3. 修改DBus接口生成，对org.kde.StatusNotifierItem.xml使用 dtk_add_dbus_interfaces 4. 更新包含目录以包含本地api目录 5. 移除StatusNotifierItem类的前向声明，因为现在由DTK生成 6. 在sniprotocolhandler.h中添加org::kde命名空间使用 问题在于Qt的同步DBus属性调用可能导致应用托盘死锁。通过切换到DTK的异步 DBus接口生成，属性访问变为非阻塞操作，防止了死锁情况。此更改专门针对导致 死锁的StatusNotifierItem接口。 Log: 修复应用托盘DBus死锁问题 Influence: 1. 测试应用托盘功能与各种系统托盘应用程序 2. 验证托盘图标正确加载且无卡顿 3. 测试与StatusNotifierItem服务的DBus通信 4. 监控运行多个托盘应用程序时的系统响应性 5. 测试托盘菜单交互和属性更新 6. 验证托盘图标显示和功能无回归问题
deepin pr auto review这份代码变更主要涉及 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议这次变更主要是为了适配 DTK 的构建系统和优化 DBus 通信性能。
|
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
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.
DBus interface generation for StatusNotifierItem
dtk_add_dbus_interfaces
org.kde.StatusNotifierItem.xml
now generated by DTK
The issue was that Qt's synchronous DBus property calls could cause
deadlocks in the application tray. By switching to DTK's asynchronous
DBus interface generation, property access becomes non-blocking,
preventing the deadlock situation. This change specifically targets the
StatusNotifierItem interface which was causing the deadlock.
Log: Fixed application tray DBus deadlock issue
Influence:
applications
running
fix: 解决应用托盘DBus死锁问题
dtk_add_dbus_interfaces
问题在于Qt的同步DBus属性调用可能导致应用托盘死锁。通过切换到DTK的异步
DBus接口生成，属性访问变为非阻塞操作，防止了死锁情况。此更改专门针对导致
死锁的StatusNotifierItem接口。
Log: 修复应用托盘DBus死锁问题
Influence:
Summary by Sourcery
Switch StatusNotifierItem DBus interface generation to DTK’s asynchronous interfaces to resolve application tray DBus deadlocks.
Bug Fixes:
Enhancements:
Build: