fix: Avoid showing XEMBED and SNI plugins together#418
Merged
Conversation
The trayplugin-loader will only show SNI plugin when both plugins are both available. Bug: https://pms.uniontech.com/bug-view-270397.html Log: Avoid showing XEMBED and SNI plugins together
Reviewer's GuideEnsures that for a given application PID only one tray protocol (SNI or XEmbed) is active at a time, preferring SNI when both are present, by tracking PIDs across both protocols and synchronizing add/remove events between them. Sequence diagram for SNI taking precedence over XEmbed for same PIDsequenceDiagram
participant SniTrayProtocol
participant SniTrayProtocolHandler
participant QDBusInterface as QDBusSessionBus
participant TrayPlugin
participant XembedProtocol
participant AbstractTrayProtocol as RegisteredMap
SniTrayProtocol->>SniTrayProtocol: registedItemChanged()
SniTrayProtocol->>SniTrayProtocol: detect new SNI item
SniTrayProtocol->>SniTrayProtocolHandler: create SniTrayProtocolHandler(id)
SniTrayProtocol->>QDBusSessionBus: servicePid(service)
QDBusSessionBus-->>SniTrayProtocol: pid
SniTrayProtocol->>SniTrayProtocol: m_item2Pid[id] = pid
SniTrayProtocol->>RegisteredMap: read registeredMap[pid]
alt pid already registered as SNI
SniTrayProtocol->>RegisteredMap: registeredMap[pid] = SNI
else pid not SNI (UNKNOWN or XEMBED)
SniTrayProtocol-->>TrayPlugin: signal removeXEmbedItemByPid(pid)
TrayPlugin-->>XembedProtocol: onRemoveItemByPid(pid)
XembedProtocol->>XembedProtocol: find item by pid via m_item2Pid
XembedProtocol->>XembedProtocol: remove from m_item2Pid and m_registedItem
XembedProtocol->>RegisteredMap: registeredMap.remove(pid)
SniTrayProtocol->>RegisteredMap: registeredMap[pid] = SNI
end
SniTrayProtocol->>SniTrayProtocol: m_registedItem.insert(id, handler)
SniTrayProtocol-->>TrayPlugin: signal trayCreated(handler)
Sequence diagram for XEmbed registration honoring SNI precedencesequenceDiagram
participant XembedProtocol
participant XembedProtocolHandler
participant UTIL as UtilHelper
participant AbstractTrayProtocol as RegisteredMap
participant TrayPlugin
XembedProtocol->>XembedProtocol: onTrayIconsChanged()
XembedProtocol->>XembedProtocol: detect new XEmbed item
XembedProtocol->>XembedProtocolHandler: create XembedProtocolHandler(windowId)
XembedProtocol->>UtilHelper: getWindowPid(windowId)
UtilHelper-->>XembedProtocol: pid
XembedProtocol->>RegisteredMap: read registeredMap[pid]
alt pid registered as SNI
XembedProtocol->>XembedProtocol: skip XEmbed item
else pid not SNI (UNKNOWN or XEMBED)
XembedProtocol->>RegisteredMap: registeredMap[pid] = XEMBED
XembedProtocol->>XembedProtocol: m_item2Pid[windowId] = pid
XembedProtocol->>XembedProtocol: m_registedItem.insert(windowId, handler)
XembedProtocol-->>TrayPlugin: signal trayCreated(handler)
end
Class diagram for coordinated SNI/XEmbed tray protocolsclassDiagram
class AbstractTrayProtocol {
<<QObject>>
+signal trayCreated(handler)
-static registeredMap : QHash~uint,Protocol~
}
class Protocol {
<<enum>>
UNKNOWN = 0
XEMBED
SNI
}
class AbstractTrayProtocolHandler {
<<QObject>>
}
class SniTrayProtocol {
<<QObject>>
+signal removeXEmbedItemByPid(pid uint)
-m_trayManager : OrgKdeStatusNotifierWatcherInterface*
-m_registedItem : QHash~QString,QSharedPointer~SniTrayProtocolHandler~~
-m_item2Pid : QHash~QString,uint~
-slot registedItemChanged()
}
class SniTrayProtocolHandler {
<<QObject>>
+static serviceAndPath(id QString) : QPair~QString,QString~
}
class XembedProtocol {
<<QObject>>
+slot onRemoveItemByPid(pid uint)
-m_trayManager : TrayManager*
-m_selectionManager : FdoSelectionManager*
-m_registedItem : QHash~uint32_t,QSharedPointer~AbstractTrayProtocolHandler~~
-m_item2Pid : QHash~uint,uint~
-slot onTrayIconsChanged()
}
class XembedProtocolHandler {
<<QObject>>
+windowId() : uint32_t
}
class TrayPlugin {
+init(proxyInter PluginProxyInterface*) void
-slot onTrayhandlerCreatd(handler QPointer~AbstractTrayProtocolHandler~)
}
AbstractTrayProtocol <|-- SniTrayProtocol
AbstractTrayProtocol <|-- XembedProtocol
AbstractTrayProtocolHandler <|-- SniTrayProtocolHandler
AbstractTrayProtocolHandler <|-- XembedProtocolHandler
AbstractTrayProtocol --> Protocol
SniTrayProtocol --> SniTrayProtocolHandler
XembedProtocol --> XembedProtocolHandler
TrayPlugin --> SniTrayProtocol
TrayPlugin --> XembedProtocol
TrayPlugin ..> SniTrayProtocol : connect trayCreated
TrayPlugin ..> XembedProtocol : connect trayCreated
TrayPlugin ..> XembedProtocol : connect removeXEmbedItemByPid
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这份代码 diff 主要实现了在系统托盘插件中，通过 PID（进程 ID）来协调 SNI (StatusNotifierItem) 和 XEmbed 两种托盘协议的互斥逻辑。即：如果一个进程同时提供了 SNI 和 XEmbed 托盘图标，优先使用 SNI 协议，并移除 XEmbed 协议下的图标。 以下是对代码的语法逻辑、代码质量、代码性能和代码安全的详细审查及改进意见： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进代码示例针对上述问题，这里提供一个关于基类改进和线程安全的建议代码片段： abstracttrayprotocol.h 修改建议： #include <QMutex>
#include <QReadWriteLock>
// ... namespace tray ...
enum class Protocol { // 使用 enum class
Unknown = 0,
XEmbed,
SNI
};
class AbstractTrayProtocol : public QObject
{
Q_OBJECT
public:
// ...
protected:
// 使用读写锁，允许多读单写
static QReadWriteLock m_mapLock;
inline static QHash<uint, Protocol> registeredMap;
// 辅助函数：安全地检查和更新协议类型
static bool tryRegisterProtocol(uint pid, Protocol type) {
QWriteLocker locker(&m_mapLock);
if (registeredMap.value(pid, Protocol::Unknown) == Protocol::Unknown ||
registeredMap.value(pid, Protocol::Unknown) == type) {
registeredMap[pid] = type;
return true;
}
return false;
}
static void unregisterProtocol(uint pid) {
QWriteLocker locker(&m_mapLock);
registeredMap.remove(pid);
}
static Protocol getProtocol(uint pid) {
QReadLocker locker(&m_mapLock);
return registeredMap.value(pid, Protocol::Unknown);
}
};sniprotocolhandler.cpp 逻辑改进建议： // 在注册项时
uint pid = ...;
if (pid > 0) { // 检查有效性
m_item2Pid[currentRegistedItem] = pid;
// 检查是否已有其他协议注册
Protocol currentProto = AbstractTrayProtocol::getProtocol(pid);
if (currentProto != Protocol::Unknown && currentProto != Protocol::SNI) {
// 只有当确实存在冲突时才发送信号
emit removeXEmbedItemByPid(pid);
}
// 更新当前协议
AbstractTrayProtocol::tryRegisterProtocol(pid, Protocol::SNI);
}
// 在移除项时
if (auto value = m_registedItem.value(alreadyRegistedItem, nullptr)) {
uint pid = m_item2Pid.take(alreadyRegistedItem); // take 自动移除
if (pid > 0) {
// 注意：这里不应直接 remove，因为同一进程可能有多个图标
// 实际项目中应引入引用计数逻辑
AbstractTrayProtocol::unregisterProtocol(pid);
}
m_registedItem.remove(alreadyRegistedItem);
}xembedprotocolhandler.h 性能改进建议： // 增加 pid 到 key 的反向映射，优化查找
QHash<uint, uint> m_item2Pid; // key(windowId) -> pid
QHash<uint, uint> m_pid2Item; // pid -> key(windowId)xembedprotocolhandler.cpp 查找逻辑优化： void XembedProtocol::onRemoveItemByPid(uint pid)
{
// 使用 O(1) 反向查找
if (m_pid2Item.contains(pid)) {
uint key = m_pid2Item.take(pid);
m_item2Pid.remove(key);
m_registedItem.remove(key);
}
}这些改进将显著提高代码的健壮性、安全性和执行效率。 |
![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:
- In several places you use
QHash::operator[]for lookups (e.g.m_item2Pid[alreadyRegistedItem],m_item2Pid[id],registeredMap[pid]), which will insert default entries on missing keys; consider usingvalue()or checkingcontains()first to avoid unintended insertions and incorrectregisteredMap/m_item2Pidstate. - In
XembedProtocol::onRemoveItemByPid,m_registedItem.keys()is recomputed multiple times insidestd::find_if, and you indexm_item2Pidwithout checking the key exists; caching the keys in a local variable and guardingm_item2Pidaccess withcontains()would make this safer and more efficient.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several places you use `QHash::operator[]` for lookups (e.g. `m_item2Pid[alreadyRegistedItem]`, `m_item2Pid[id]`, `registeredMap[pid]`), which will insert default entries on missing keys; consider using `value()` or checking `contains()` first to avoid unintended insertions and incorrect `registeredMap`/`m_item2Pid` state.
- In `XembedProtocol::onRemoveItemByPid`, `m_registedItem.keys()` is recomputed multiple times inside `std::find_if`, and you index `m_item2Pid` without checking the key exists; caching the keys in a local variable and guarding `m_item2Pid` access with `contains()` would make this safer and more efficient.
## Individual Comments
### Comment 1
<location> `plugins/application-tray/sniprotocolhandler.cpp:94-96` </location>
<code_context>
for (auto alreadyRegistedItem : m_registedItem.keys()) {
if (!currentRegistedItems.contains(alreadyRegistedItem)) {
if (auto value = m_registedItem.value(alreadyRegistedItem, nullptr)) {
+ uint pid = m_item2Pid[alreadyRegistedItem];
+ m_item2Pid.remove(alreadyRegistedItem);
+ registeredMap.remove(pid);
m_registedItem.remove(alreadyRegistedItem);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using operator[] for lookups in m_item2Pid to prevent unintended insertions and desynchronization
Using `m_item2Pid[alreadyRegistedItem]` here will insert a default entry if the key is missing. If `m_item2Pid` and `m_registedItem` are ever out of sync, you’ll silently create an entry with pid 0 and then call `registeredMap.remove(0)`. Prefer `m_item2Pid.value(alreadyRegistedItem, 0)` with a `contains` check, or guard the block with `if (m_item2Pid.contains(alreadyRegistedItem))` to avoid unintended insertions.
</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
+94
to
+96
| uint pid = m_item2Pid[alreadyRegistedItem]; | ||
| m_item2Pid.remove(alreadyRegistedItem); | ||
| registeredMap.remove(pid); |
There was a problem hiding this comment.
issue (bug_risk): Avoid using operator[] for lookups in m_item2Pid to prevent unintended insertions and desynchronization
Using m_item2Pid[alreadyRegistedItem] here will insert a default entry if the key is missing. If m_item2Pid and m_registedItem are ever out of sync, you’ll silently create an entry with pid 0 and then call registeredMap.remove(0). Prefer m_item2Pid.value(alreadyRegistedItem, 0) with a contains check, or guard the block with if (m_item2Pid.contains(alreadyRegistedItem)) to avoid unintended insertions.
tsic404
approved these changes
Jan 27, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, tsic404 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 |
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
4 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.
The trayplugin-loader will only show SNI plugin when both plugins are both available.
Bug: https://pms.uniontech.com/bug-view-270397.html
Log: Avoid showing XEMBED and SNI plugins together
此提交 cherry-pick 自 #197
Summary by Sourcery
Prevent simultaneous display of XEMBED and SNI tray icons for the same application by coordinating registration between the two protocols.
Bug Fixes:
Enhancements: