feat: add card surface plugin interface for QML-based plugins - #474
Conversation
Reviewer's GuideAdds a QML-based card surface plugin interface (PluginsItemInterfaceV3) and a CardPluginItem loader path, wires it into the existing plugin manager/loader to create and manage Wayland-backed QQuickView card surfaces, and updates the brightness dock plugin to implement V3 with a QML card, while preserving backward compatibility for V2/legacy plugins. Sequence diagram for creating a QML card surface on itemAddedsequenceDiagram
participant PluginManager
participant BrightnessPlugin
participant WidgetPlugin
participant CardPluginItem
participant EmbedPlugin
PluginManager->>BrightnessPlugin: loadPlugin(pluginFilePath)
BrightnessPlugin->>PluginManager: instance implements PluginsItemInterfaceV3
PluginManager->>WidgetPlugin: new WidgetPlugin(pluginsItemInterface)
BrightnessPlugin->>WidgetPlugin: itemAdded(this, cardItemKey())
WidgetPlugin->>WidgetPlugin: createCardItemIfNeeded(itemInter, itemKey)
WidgetPlugin->>CardPluginItem: new CardPluginItem(cardInterface, itemKey, this)
CardPluginItem->>CardPluginItem: init()
CardPluginItem->>CardPluginItem: QQuickView setSource(cardQmlSource())
WidgetPlugin->>EmbedPlugin: Plugin::EmbedPlugin::get(CardPluginItem.window())
EmbedPlugin->>EmbedPlugin: setPluginType(Plugin::EmbedPlugin::Card)
EmbedPlugin->>WidgetPlugin: dockColorThemeChanged(uint32_t)
WidgetPlugin->>CardPluginItem: setDockColorTheme(int)
EmbedPlugin->>CardPluginItem: eventGeometry(QRect)
CardPluginItem->>CardPluginItem: resize(QSize)
WidgetPlugin->>CardPluginItem: show()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- CardPluginItem instances are constructed with WidgetPlugin as parent and also deleted via qDeleteAll(m_cardItems) in the destructor, which can lead to double deletion; either drop the parent relationship or remove qDeleteAll and rely on QObject ownership.
- In createCardItemIfNeeded(), when cardItem->init() or window() fails you still return true, which prevents the normal widget path from being created; consider returning false on failure so the plugin can gracefully fall back to the non-card implementation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- CardPluginItem instances are constructed with WidgetPlugin as parent and also deleted via qDeleteAll(m_cardItems) in the destructor, which can lead to double deletion; either drop the parent relationship or remove qDeleteAll and rely on QObject ownership.
- In createCardItemIfNeeded(), when cardItem->init() or window() fails you still return true, which prevents the normal widget path from being created; consider returning false on failure so the plugin can gracefully fall back to the non-card implementation.
## Individual Comments
### Comment 1
<location path="src/loader/widgetplugin.cpp" line_range="382" />
<code_context>
return Plugin::EmbedPlugin::get(widget->windowHandle());
}
+bool WidgetPlugin::createCardItemIfNeeded(PluginsItemInterface *itemInter, const QString &itemKey)
+{
+ auto cardInterface = dynamic_cast<PluginsItemInterfaceV3 *>(itemInter);
</code_context>
<issue_to_address>
**issue (complexity):** Consider separating card-item classification from lifecycle management and making the card helpers void and explicitly branched in itemAdded/itemRemoved to clarify control flow and responsibilities.
The added card path does increase complexity, mainly through the overloaded `createCardItemIfNeeded` and its hidden control‑flow contract. You can simplify the logic and make `itemAdded`/`itemRemoved` easier to reason about by:
1. **Separate classification from lifecycle**
Extract a small helper that decides “is this a card item?” and use it to branch explicitly in `itemAdded`/`itemRemoved`. Then make the lifecycle helper `void` so its return value no longer encodes behavior.
```cpp
// New helper
bool WidgetPlugin::isCardItem(PluginsItemInterface *itemInter, const QString &itemKey) const
{
auto cardInterface = dynamic_cast<PluginsItemInterfaceV3 *>(itemInter);
return cardInterface && cardInterface->cardItemKey() == itemKey;
}
// Adjusted itemAdded
void WidgetPlugin::itemAdded(PluginsItemInterface * const itemInter, const QString &itemKey)
{
qDebug() << "itemAdded:" << itemKey;
if (isCardItem(itemInter, itemKey)) {
ensureCardItem(itemInter, itemKey);
return;
}
auto flag = getPluginFlags();
if (flag & Dock::Type_Quick) {
// existing quick path...
}
// existing normal widget path...
}
```
2. **Make card lifecycle explicit and non‑boolean**
Rename `createCardItemIfNeeded` to something like `ensureCardItem` and change the signature to `void`. Keep the semantics identical: normal widget path should never run for card items.
```cpp
// Refactored from createCardItemIfNeeded
void WidgetPlugin::ensureCardItem(PluginsItemInterface *itemInter, const QString &itemKey)
{
auto cardInterface = static_cast<PluginsItemInterfaceV3 *>(itemInter);
if (auto existing = m_cardItems.value(itemKey)) {
existing->show();
return;
}
auto cardItem = new CardPluginItem(cardInterface, itemKey, this);
if (!cardItem->init() || !cardItem->window()) {
cardItem->deleteLater();
qWarning() << "create card plugin surface failed" << itemInter->pluginName() << itemKey;
return; // still block normal widget path
}
auto plugin = Plugin::EmbedPlugin::get(cardItem->window());
plugin->setPluginFlags(getPluginFlags());
plugin->setPluginId(itemInter->pluginName());
plugin->setDisplayName(itemInter->pluginDisplayName());
plugin->setItemKey(itemKey);
plugin->setPluginType(Plugin::EmbedPlugin::Card);
plugin->setPluginSizePolicy(itemInter->pluginSizePolicy());
connect(plugin, &Plugin::EmbedPlugin::dockColorThemeChanged,
this, &WidgetPlugin::onDockColorThemeChanged, Qt::UniqueConnection);
connect(plugin, &Plugin::EmbedPlugin::dockColorThemeChanged,
cardItem, [cardItem](uint32_t colorTheme) {
cardItem->setDockColorTheme(static_cast<int>(colorTheme));
});
connect(plugin, &Plugin::EmbedPlugin::eventGeometry,
cardItem, [cardItem](const QRect &geometry) {
cardItem->resize(geometry.size());
});
m_cardItems.insert(itemKey, cardItem);
cardItem->show();
}
```
3. **Mirror the explicit branching in `itemRemoved`**
Keep the current behavior, but make the “card vs normal widget” decision obvious:
```cpp
void WidgetPlugin::itemRemoved(PluginsItemInterface * const itemInter, const QString &itemKey)
{
Q_UNUSED(itemInter);
if (auto cardItem = m_cardItems.take(itemKey)) {
cardItem->hide();
cardItem->deleteLater();
return;
}
auto widget = m_pluginsItemInterface->itemWidget(itemKey);
if (widget && widget->window() && widget->window()->windowHandle()) {
widget->window()->windowHandle()->hide();
}
auto quickPanel = m_pluginsItemInterface->itemWidget(Dock::QUICK_ITEM_KEY);
if (quickPanel && quickPanel->window() && quickPanel->window()->windowHandle()) {
quickPanel->window()->windowHandle()->hide();
}
}
```
These changes keep all current functionality (including “card creation failure blocks normal widget path”) but make the control flow and responsibilities clearer: classification (`isCardItem`), lifecycle (`ensureCardItem`), and normal widget handling are separated and easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return Plugin::EmbedPlugin::get(widget->windowHandle()); | ||
| } | ||
|
|
||
| bool WidgetPlugin::createCardItemIfNeeded(PluginsItemInterface *itemInter, const QString &itemKey) |
There was a problem hiding this comment.
issue (complexity): Consider separating card-item classification from lifecycle management and making the card helpers void and explicitly branched in itemAdded/itemRemoved to clarify control flow and responsibilities.
The added card path does increase complexity, mainly through the overloaded createCardItemIfNeeded and its hidden control‑flow contract. You can simplify the logic and make itemAdded/itemRemoved easier to reason about by:
-
Separate classification from lifecycle
Extract a small helper that decides “is this a card item?” and use it to branch explicitly initemAdded/itemRemoved. Then make the lifecycle helpervoidso its return value no longer encodes behavior.// New helper bool WidgetPlugin::isCardItem(PluginsItemInterface *itemInter, const QString &itemKey) const { auto cardInterface = dynamic_cast<PluginsItemInterfaceV3 *>(itemInter); return cardInterface && cardInterface->cardItemKey() == itemKey; } // Adjusted itemAdded void WidgetPlugin::itemAdded(PluginsItemInterface * const itemInter, const QString &itemKey) { qDebug() << "itemAdded:" << itemKey; if (isCardItem(itemInter, itemKey)) { ensureCardItem(itemInter, itemKey); return; } auto flag = getPluginFlags(); if (flag & Dock::Type_Quick) { // existing quick path... } // existing normal widget path... }
-
Make card lifecycle explicit and non‑boolean
RenamecreateCardItemIfNeededto something likeensureCardItemand change the signature tovoid. Keep the semantics identical: normal widget path should never run for card items.// Refactored from createCardItemIfNeeded void WidgetPlugin::ensureCardItem(PluginsItemInterface *itemInter, const QString &itemKey) { auto cardInterface = static_cast<PluginsItemInterfaceV3 *>(itemInter); if (auto existing = m_cardItems.value(itemKey)) { existing->show(); return; } auto cardItem = new CardPluginItem(cardInterface, itemKey, this); if (!cardItem->init() || !cardItem->window()) { cardItem->deleteLater(); qWarning() << "create card plugin surface failed" << itemInter->pluginName() << itemKey; return; // still block normal widget path } auto plugin = Plugin::EmbedPlugin::get(cardItem->window()); plugin->setPluginFlags(getPluginFlags()); plugin->setPluginId(itemInter->pluginName()); plugin->setDisplayName(itemInter->pluginDisplayName()); plugin->setItemKey(itemKey); plugin->setPluginType(Plugin::EmbedPlugin::Card); plugin->setPluginSizePolicy(itemInter->pluginSizePolicy()); connect(plugin, &Plugin::EmbedPlugin::dockColorThemeChanged, this, &WidgetPlugin::onDockColorThemeChanged, Qt::UniqueConnection); connect(plugin, &Plugin::EmbedPlugin::dockColorThemeChanged, cardItem, [cardItem](uint32_t colorTheme) { cardItem->setDockColorTheme(static_cast<int>(colorTheme)); }); connect(plugin, &Plugin::EmbedPlugin::eventGeometry, cardItem, [cardItem](const QRect &geometry) { cardItem->resize(geometry.size()); }); m_cardItems.insert(itemKey, cardItem); cardItem->show(); }
-
Mirror the explicit branching in
itemRemoved
Keep the current behavior, but make the “card vs normal widget” decision obvious:void WidgetPlugin::itemRemoved(PluginsItemInterface * const itemInter, const QString &itemKey) { Q_UNUSED(itemInter); if (auto cardItem = m_cardItems.take(itemKey)) { cardItem->hide(); cardItem->deleteLater(); return; } auto widget = m_pluginsItemInterface->itemWidget(itemKey); if (widget && widget->window() && widget->window()->windowHandle()) { widget->window()->windowHandle()->hide(); } auto quickPanel = m_pluginsItemInterface->itemWidget(Dock::QUICK_ITEM_KEY); if (quickPanel && quickPanel->window() && quickPanel->window()->windowHandle()) { quickPanel->window()->windowHandle()->hide(); } }
These changes keep all current functionality (including “card creation failure blocks normal widget path”) but make the control flow and responsibilities clearer: classification (isCardItem), lifecycle (ensureCardItem), and normal widget handling are separated and easier to follow.
| void WidgetPlugin::itemAdded(PluginsItemInterface * const itemInter, const QString &itemKey) | ||
| { | ||
| qDebug() << "itemAdded:" << itemKey; | ||
| if (createCardItemIfNeeded(itemInter, itemKey)) { |
| * The loader creates a QQuickView for this URL and exposes it to the dock | ||
| * compositor as a Wayland surface. | ||
| */ | ||
| virtual QUrl cardQmlSource() const |
There was a problem hiding this comment.
这里返回一个qwindow是不是更好,让应用控制,这样也能支持qml和qwidget,
|
TAG Bot New tag: 2.0.36 |
74167e4 to
59457ba
Compare
| qDebug() << "itemAdded:" << itemKey; | ||
|
|
||
| auto flag = getPluginFlags(); | ||
| if ((flag & Dock::Attribute_HasCard) && createCardItemIfNeeded(itemInter, itemKey)) { |
There was a problem hiding this comment.
不需要为false就return吧,逻辑是不是还可以往下走,只处理card需要的逻辑,card跟tray和quick一样,只是其中的一个item类型,按照之前quick这样的处理方式处理card,
There was a problem hiding this comment.
m_proxyInter->itemAdded(this, MEDIA_KEY);
m_proxyInter->itemAdded(this, cardItemKey()); 在上面会调用两次 根据不同的 flag来走。
| private: | ||
| PluginsItemInterface* m_pluginsItemInterface; | ||
| QScopedPointer<PluginItem> m_pluginItem; | ||
| QHash<QString, CardPluginItem *> m_cardItems; |
There was a problem hiding this comment.
这个不需要是个QHash吧,它是不是应该只有一个,
|
TAG Bot New tag: 2.0.37 |
|
TAG Bot New tag: 2.0.38 |
69f7be0 to
44c850d
Compare
|
/test github-pr-review-ci |
|
/test github-pr-review-ci |
|
TAG Bot New tag: 2.0.39 |
1. Introduce PluginsItemInterfaceV3 interface with cardItemKey(), cardWindow(), cardOrder(), cardContextMenu(), cardTipsWidget() and invokedCardMenuItem() methods to enable plugins to expose card surfaces in the dock 2. Add Attribute_HasCard plugin flag and implement CardPluginItem in the loader that manages the card surface lifecycle, including show/ hide, resize, tooltip display with hover delay, context menu and XDG activation token support 3. Implement CardPluginItem event filter for mouse interaction including right-click context menu, enter/leave for tooltip timer, and proper surface binding with Plugin::EmbedPlugin 4. Add MSG_DOCK_FASHION_MODE message so taskbar informs plugins when fashion mode changes, and MediaPlugin enables card area only in fashion mode 5. Add card sorting message MSG_CARD_ORDER sent after plugin surface creation, ensuring media card positioning is adjustable through plugin configuration 6. Handle plugin map lifecycle in EmbedPlugin with proper removal that prevents stale plugin bindings when windows are reused 7. Fix EmbedPlugin visibleChanged connection binding to plugin instance rather than window to prevent unbinding when reused for same window Log: Added dock card surface plugin feature. Media plugin now displays album art and music controls in a new card area next to the tray when in fashion mode, and datetime plugin adjusts from two-line to single-line layout when dock is compact. Influence: 1. Test plugin loading for various MPRIS players (pause, previous, next buttons on card) 2. Verify media card tooltip displays song information correctly 3. Test context menu in card surfaces with XDG activation 4. Switch dock between fashion and efficient modes, verify cards show/ hide correctly 5. Verify card surfaces don't appear in efficient mode 6. Test compact dock sizes to verify datetime single-line layout transition 7. Test fashion mode message propagation to plugins 8. Verify card order configuration affects card positioning feat: 增加插件卡片区域支持 1. 引入 PluginsItemInterfaceV3 接口,新增 cardItemKey()、 cardWindow()、cardOrder()、cardContextMenu()、cardTipsWidget() 及 invokedCardMenuItem() 方法,使插件可以展示卡片区域 2. 增加 Attribute_HasCard 插件属性,装载器实现 CardPluginItem 管理卡 片的生命周期,包括显示/隐藏、尺寸调整、带悬停延迟的气泡提示和带 XDG activation token 的右键菜单 3. CardPluginItem 通过事件过滤器处理鼠标交互,右键弹出上下文菜单,进出触 发气泡定时器,并正确绑定 Plugin::EmbedPlugin 插件表面 4. 新增 MSG_DOCK_FASHION_MODE 消息,任务栏在模式变化时通知插件,媒体插件 只在时尚模式下启用卡片区 5. 新增 MSG_CARD_ORDER 排序消息,在插件表面创建后发送,卡片顺序可通过插 件配置调整 6. 修复 EmbedPlugin 映射生命周期,窗口复用时防止残留的插件绑定 7. 修复 EmbedPlugin 的 visibleChanged 信号连接绑定到插件实例而非窗口,避 免窗口复用时解除后续新建插件的绑定 Log: 新增卡片区插件特性。媒体插件在时尚模式下可于托盤区域旁新卡片区展示 唱片封面和音乐控制,同时日期时间插件在 dock 尺寸紧凑时从两行显示切换为 一行。 Influence: 1. 测试各种 MPRIS 播放器的媒体插件加载(卡片上暂停/上一首/下一首按键) 2. 验证媒体卡片的气泡提示是否显示歌曲信息 3. 测试卡片上的右键菜单配合 XDG activation 是否可以正常激活 4. 在时尚模式和高效模式间切换时验证卡片正确显示/隐藏 5. 确保高效模式下不会出现卡片表面 6. 测试紧凑 dock 尺寸切换时日期时间控件的单行布局过渡 7. 测试时尚模式消息能否正确传递给插件 8. 验证卡片顺序配置是否影响卡片位置 PMS: TASK-392671
deepin pr auto reviewAI 代码审查报告
总体评分
总体评价: 代码安全维度无安全漏洞,代码质量和性能维度通过。语法逻辑维度存在空指针解引用风险和内存泄漏风险等问题未通过。代码整体架构清晰、注释完善、安全意识良好(artSource() 方法主动防止 SSRF),但存在若干需修复的缺陷。 漏洞统计
维度1:语法逻辑(3/25 ✕)
问题列表
OCR 补充发现(已合并)
维度2:代码质量(22/25 ✓)
优点
问题
维度3:代码性能(19/20 ✓)
优点
问题
维度4:代码安全(30/30 ✓)
安全审查结果
安全亮点
改进建议代码示例修复1: cardpluginitem.cpp showContextMenu() 空指针检查bool CardPluginItem::showContextMenu(const QPoint &position)
{
// ... existing code ...
auto *plugin = Plugin::EmbedPlugin::get(m_window);
if (!plugin) {
return false;
}
auto *pluginPopup = Plugin::PluginPopup::get(m_menu->windowHandle());
// 添加空指针检查,与 tipsWidget() 保持一致
if (!pluginPopup) {
return false;
}
pluginPopup->setPluginId(m_pluginInterface->pluginName());
pluginPopup->setItemKey(m_itemKey);
// ... rest of method ...
}修复2: cardpluginitem.cpp m_defaultTipsLabel 内存泄漏CardPluginItem::~CardPluginItem()
{
if (m_menu) {
delete m_menu;
m_menu = nullptr;
}
if (m_tipsContainer) {
delete m_tipsContainer;
m_tipsContainer = nullptr;
}
// 显式删除默认 tips label,防止从布局移除后泄漏
if (m_defaultTipsLabel) {
delete m_defaultTipsLabel;
m_defaultTipsLabel = nullptr;
}
if (m_window) {
m_window->hide();
}
m_window = nullptr;
}修复3: widgetplugin.cpp eventGeometry 信号连接添加 UniqueConnectionconnect(plugin, &Plugin::EmbedPlugin::eventGeometry, m_cardItem, [this](const QRect &geometry) {
if (m_cardItem) {
m_cardItem->resize(geometry.size());
}
}, Qt::UniqueConnection); // 添加 UniqueConnection 防止重复连接审查结论本次 PR 为 dde-tray-loader 新增了卡片区域(card surface)插件接口(V3),支持 QML 插件在任务栏卡片区域展示内容。核心改动包括:
代码整体质量较高,架构设计清晰,安全意识良好。主要问题集中在 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, wjyrich 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 |
|
/forcemerge |
|
This pr force merged! (status: behind) |
Introduce PluginsItemInterfaceV3 extending V2 with virtual functions for card item key, QML source, icon source and preferred size. Implement CardPluginItem class and integrate into loader to support creating card QQuickView surfaces. Brightness plugin updated to V3 with a simple QML card showing icon, title and slider.
Log: Added card surface support for dock plugins with QML implementation
Influence:
feat: 添加基于 QML 的卡片表面插件接口
引入 PluginsItemInterfaceV3 扩展 V2,增加卡片项键、QML 源、图标源和首 选大小的虚函数。实现 CardPluginItem 类并集成到加载器中,支持创建卡片
QQuickView 表面。亮度插件更新至 V3,使用简单的 QML 卡片显示图标、标题和
滑块。
Log: 新增卡片表面支持,插件可使用 QML 实现
Influence:
Summary by Sourcery
Introduce QML card-surface support for dock plugins while adding a responsive media card and preserving compatibility with existing plugins.
New Features:
Bug Fixes:
Enhancements:
Build:
PMS: TASK-392671