feat: add dock library and refactor dock plugin management#1560
feat: add dock library and refactor dock plugin management#1560wjyrich merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideIntroduces a new shared dock library (libdde-shell-dock) with a DAppletDock base class and central DockItemInfo type, refactors dock plugin discovery/visibility logic to use metadata and the new interface instead of hardcoded IDs, updates existing plugins (notably multitaskview) and build/packaging to consume the new library, and cleans up Debian install paths for headers and libraries. Sequence diagram for dock plugin visibility update via DAppletDocksequenceDiagram
actor UserSettings
participant SettingsUI
participant DockSettings
participant DockDBusProxy
participant DockPanel
participant DAppletDockPlugin as DAppletDock_plugin
participant TrayApplet
UserSettings->>SettingsUI: toggle plugin visibility
SettingsUI->>DockSettings: setPluginsVisible(pluginsVisible)
DockSettings-->>DockSettings: update internal map
DockSettings->>DockDBusProxy: pluginsVisibleChanged(pluginsVisible)
DockDBusProxy->>DockPanel: applets()
DockPanel-->>DockDBusProxy: list of applets
loop for each applet with dockVersion 1.0
DockDBusProxy->>DAppletDock_plugin: dockItemInfo()
DAppletDock_plugin-->>DockDBusProxy: DockItemInfo(itemKey)
alt pluginsVisible contains itemKey
DockDBusProxy->>DAppletDock_plugin: setVisible(visible)
else
DockDBusProxy->>DockSettings: pluginsVisible()
DockSettings-->>DockDBusProxy: current map
DockDBusProxy->>DockSettings: setPluginsVisible(updatedMap)
end
end
alt tray item affected
DockDBusProxy->>TrayApplet: setItemOnDock(settingKey, itemKey, visible)
DockDBusProxy-->>UserSettings: pluginVisibleChanged(itemKey, visible)
end
Class diagram for new dock library and updated dock pluginsclassDiagram
class DApplet {
}
class DockItemInfo {
+QString itemKey
}
class DAppletDock {
<<abstract>>
+DAppletDock(QObject *parent)
+~DAppletDock()
+DockItemInfo dockItemInfo()
+void setVisible(bool visible)
}
class MultiTaskView {
+MultiTaskView(QObject *parent)
+bool init()
+QString iconName
+void setIconName(QString iconName)
}
class DockPanel {
+QList~QObject*~ applets()
}
class DockDBusProxy {
-DockPanel* m_parent
-DAppletProxy* m_oldDockApplet
-DAppletProxy* m_trayApplet
+DockItemInfos plugins()
+QRect geometry()
+void setItemOnDock(QString settingKey, QString itemKey, bool visible)
-void updateDockPluginsVisible(QVariantMap pluginsVisible)
}
class DockSettings {
+static DockSettings* instance()
+QVariantMap pluginsVisible()
+void setPluginsVisible(QVariantMap pluginsVisible)
}
class DAppletProxy {
}
DAppletDock --|> DApplet
MultiTaskView --|> DAppletDock
DockDBusProxy --> DockPanel : parent()
DockPanel "1" o-- "*" DApplet : applets()
DockDBusProxy --> DockSettings : read_write_pluginsVisible
DockDBusProxy --> DAppletProxy : use_tray_applet
DockDBusProxy --> DAppletDock : cast_from_applets
DAppletDock --> DockItemInfo : returns
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 3 issues, and left some high level feedback:
- The repeated inline checks for
pluginMetaData().value("dockVersion").toString() == "1.0"would be easier to maintain and less error-prone if encapsulated in a small helper (e.g.isDockApplet()/dockApplets()and a sharedconstexprkey/version string) rather than hardcoded strings scattered acrossDockDBusProxy. - In both
updateDockPluginsVisibleandsetItemOnDock,DockSettings::instance()->pluginsVisible()is read and written inside per-applet branches; consider fetching and updating this map once per call (and only writing back once) to avoid redundant lookups and potential inconsistencies when multiple dock items are processed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated inline checks for `pluginMetaData().value("dockVersion").toString() == "1.0"` would be easier to maintain and less error-prone if encapsulated in a small helper (e.g. `isDockApplet()` / `dockApplets()` and a shared `constexpr` key/version string) rather than hardcoded strings scattered across `DockDBusProxy`.
- In both `updateDockPluginsVisible` and `setItemOnDock`, `DockSettings::instance()->pluginsVisible()` is read and written inside per-applet branches; consider fetching and updating this map once per call (and only writing back once) to avoid redundant lookups and potential inconsistencies when multiple dock items are processed.
## Individual Comments
### Comment 1
<location path="panels/dock/dockdbusproxy.cpp" line_range="76-85" />
<code_context>
+void DockDBusProxy::updateDockPluginsVisible(const QVariantMap &pluginsVisible)
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid repeated reads/writes to DockSettings when normalizing pluginsVisible defaults.
In `updateDockPluginsVisible`, each missing key triggers a fresh `DockSettings::instance()->pluginsVisible()` read and a `setPluginsVisible()` write, possibly multiple times per call. This can cause unnecessary I/O or signal emissions. Instead, copy the current `QVariantMap` once, update it for all missing keys inside the loop, and then call `setPluginsVisible()` a single time if any changes were made.
Suggested implementation:
```cpp
void DockDBusProxy::updateDockPluginsVisible(const QVariantMap &pluginsVisible)
{
// Cache the current pluginsVisible settings once and track whether we modify them.
QVariantMap settingPluginsVisible = DockSettings::instance()->pluginsVisible();
bool pluginsVisibleChanged = false;
for (auto *applet : parent()->applets()) {
```
```cpp
if (pluginsVisible.contains(itemKey)) {
dockApplet->setVisible(pluginsVisible[itemKey].toBool());
} else {
// We already have a cached settingPluginsVisible; mark that we may update it.
pluginsVisibleChanged = true;
```
To fully implement the optimization and avoid repeated writes:
1. Replace any *subsequent* `DockSettings::instance()->pluginsVisible()` reads inside `updateDockPluginsVisible` with reads from the cached `settingPluginsVisible` introduced at the top of the function.
2. Ensure all code that mutates the plugins-visible map updates `settingPluginsVisible` instead of a local variable (e.g. `settingPluginsVisible.insert(itemKey, ...)`).
3. Move any existing `DockSettings::instance()->setPluginsVisible(...)` calls out of the loop and replace them with a **single** call at the end of the function, guarded by the boolean:
```cpp
if (pluginsVisibleChanged) {
DockSettings::instance()->setPluginsVisible(settingPluginsVisible);
}
```
4. Preserve the original default-visibility behavior for missing keys (whatever logic currently decides the default) when you migrate the mutations into `settingPluginsVisible`.
</issue_to_address>
### Comment 2
<location path="panels/dock/dockdbusproxy.cpp" line_range="79" />
<code_context>
+void DockDBusProxy::updateDockPluginsVisible(const QVariantMap &pluginsVisible)
+{
+ for (auto *applet : parent()->applets()) {
+ if (applet->pluginMetaData().value("dockVersion").toString() == "1.0") {
+ if (auto dockApplet = qobject_cast<DS_NAMESPACE::DAppletDock *>(applet)) {
+ DockItemInfo itemInfo = dockApplet->dockItemInfo();
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding hard-coded dockVersion string comparisons.
This `pluginMetaData().value("dockVersion").toString() == "1.0"` check is repeated in several places. Hard-coding the version string makes future version changes or multi-version support harder and fragile. Prefer either a dedicated metadata flag (e.g. `isDockApplet`) or a shared version constant defined in one header and reused here.
</issue_to_address>
### Comment 3
<location path="panels/dock/dockdbusproxy.cpp" line_range="80" />
<code_context>
+{
+ for (auto *applet : parent()->applets()) {
+ if (applet->pluginMetaData().value("dockVersion").toString() == "1.0") {
+ if (auto dockApplet = qobject_cast<DS_NAMESPACE::DAppletDock *>(applet)) {
+ DockItemInfo itemInfo = dockApplet->dockItemInfo();
+ QString itemKey = itemInfo.itemKey;
</code_context>
<issue_to_address>
**question (bug_risk):** Check threading assumptions when calling setVisible directly instead of via QMetaObject::invokeMethod.
Previously this was done via `QMetaObject::invokeMethod(..., Qt::QueuedConnection, ...)`, guaranteeing execution on the receiver’s thread. Now `dockApplet->setVisible()` is called directly. If `pluginsVisibleChanged` (or callers of `updateDockPluginsVisible` / `setItemOnDock`) can run off the GUI thread while `DAppletDock` expects GUI-thread access, this risks race conditions or thread violations. If these signals are always emitted on the GUI thread, this is fine; otherwise, consider restoring queued delivery or clearly documenting the threading guarantees.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
cc86ac8 to
1b664e7
Compare
9560252 to
9f3bbf4
Compare
02d2270 to
c966f80
Compare
| { | ||
| "Plugin": { | ||
| "Version": "1.0", | ||
| "dockVersion": "1.0", |
There was a problem hiding this comment.
改成 "dock": {
"version": "1.0"
}
这样吧,方便之后扩展,
| PRIVATE | ||
| Qt${QT_VERSION_MAJOR}::QuickTemplates2Private | ||
| Qt${QT_VERSION_MAJOR}::QuickPrivate | ||
| Qt${QT_VERSION_MAJOR}::GuiPrivate |
| property bool shouldVisible: Applet.visible | ||
| dockOrder: 15 | ||
| // 1:4 the distance between app : dock height; get width/height≈0.8 | ||
| implicitWidth: useColumnLayout ? Panel.rootObject.dockSize : Panel.rootObject.dockItemMaxSize * 0.8 |
There was a problem hiding this comment.
这些是不是都可以放在AppletDockItem里呀,
| explicit DAppletDock(QObject *parent = nullptr); | ||
| virtual ~DAppletDock() override; | ||
|
|
||
| virtual DockItemInfo dockItemInfo(); |
There was a problem hiding this comment.
还需要这个DockItemInfo么,它是不是可以拆开了呀,分成不同的接口出去,
| void visibleChanged(); | ||
|
|
||
| private: | ||
| bool m_visible = true; |
There was a problem hiding this comment.
这种用d指针的方式吧,m_visible放在private里,保持之后的兼容性,
c966f80 to
5749b7a
Compare
| @@ -0,0 +1,2 @@ | |||
| usr/include/dde-shell/dock/*.h | |||
| usr/lib/*/libdde-shell-dock.so* | |||
There was a problem hiding this comment.
这个libdde-shell-dock.so,不安装-dev包就不用安装了么?
|
|
||
| target_compile_definitions(dde-shell-dock PRIVATE DS_LIB) | ||
|
|
||
| install(TARGETS dde-shell-dock EXPORT DDEShellTargets DESTINATION "${LIB_INSTALL_DIR}" PUBLIC_HEADER DESTINATION "${INCLUDE_INSTALL_DIR}/dock") No newline at end of file |
a9016eb to
c90840e
Compare
c90840e to
84a022c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mhduiy, 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 |
1. Added new libdde-shell-dock library to provide dock item interface 2. Created DAppletDock abstract base class for dock plugins with dockItemInfo() and setVisible() methods 3. Refactored dock plugin management to use new library instead of hardcoded plugin IDs 4. Updated build system to include new library and development packages 5. Modified multitaskview plugin to inherit from DAppletDock instead of DApplet 6. Added dockVersion metadata field to identify dock-compatible plugins 7. Fixed installation paths for header files and libraries in Debian packaging Log: Improved dock plugin architecture with standardized interface Influence: 1. Test dock plugin visibility management through settings 2. Verify multitaskview plugin still works correctly 3. Check that tray functionality remains unchanged 4. Test building and installation of new libdde-shell-dock packages 5. Verify backward compatibility with existing dock plugins 6. Test dock item information retrieval through D-Bus interface feat: 新增Dock库并重构Dock插件管理 1. 新增libdde-shell-dock库,提供Dock项目接口 2. 创建DAppletDock抽象基类,包含dockItemInfo()和setVisible()方法 3. 重构Dock插件管理,使用新库替代硬编码的插件ID 4. 更新构建系统以包含新库和开发包 5. 修改multitaskview插件继承自DAppletDock而非DApplet 6. 新增dockVersion元数据字段以识别Dock兼容插件 7. 修复Debian打包中头文件和库的安装路径 Log: 改进Dock插件架构,提供标准化接口 Influence: 1. 测试通过设置管理Dock插件可见性 2. 验证multitaskview插件仍正常工作 3. 检查托盘功能是否保持不变 4. 测试新建libdde-shell-dock包的构建和安装 5. 验证与现有Dock插件的向后兼容性 6. 测试通过D-Bus接口获取Dock项目信息 PMS: TASK-388449
84a022c to
0b8e0f2
Compare
deepin pr auto review这份代码 diff 展示了对 DDE Shell Dock 组件的一次重要重构,主要目的是将 dock 相关的通用功能(如 以下是对语法逻辑、代码质量、代码性能和代码安全的详细审查及改进意见: 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进意见
总结这次重构是一个高质量的改动,成功地实现了代码的模块化和解耦,提高了可维护性和扩展性。新的插件系统通过元数据版本控制,更加灵活和安全。代码风格保持一致,版权信息更新及时。 主要的改进点集中在性能微优化(缓存插件列表)和接口设计的严谨性(纯虚函数)上,但这些都不是严重问题,属于锦上添花。整体逻辑清晰,符合现代 C++ 和 QML 的最佳实践。 |
Log: Improved dock plugin architecture with standardized interface
Influence:
feat: 新增Dock库并重构Dock插件管理
Log: 改进Dock插件架构,提供标准化接口
Influence:
PMS: TASK-388449
Summary by Sourcery
Introduce a shared dock library and unify dock plugin handling around a standard dock applet interface.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment: