feat(dock): integrate optional event logging support#1569
Open
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Open
feat(dock): integrate optional event logging support#1569Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideAdds optional dde-api-based event logging to dock-related components (dock DBus proxy, dock settings, multitask view, and task manager settings), guarded by a new CMake-detected HAVE_DDE_API_EVENTLOGGER macro so that logging is enabled only when the eventlogger.hpp header is available. Sequence diagram for tray plugin visibility change logging via DockDBusProxysequenceDiagram
actor User
participant DockUI
participant DockDBusProxy
participant DockSettings
participant EventLogger as DDE_EventLogger.EventLogger
User->>DockUI: Toggle tray plugin visibility
DockUI->>DockDBusProxy: setItemOnDock(settingKey, itemKey, visible)
alt trayApplet not ready
DockDBusProxy->>DockSettings: setPluginsVisible(updatedPluginsVisible)
DockDBusProxy->>DockDBusProxy: logTrayPluginListChange(itemKey, visible)
else trayApplet ready
DockDBusProxy->>DockDBusProxy: pluginVisibleChanged(itemKey, visible)
DockDBusProxy->>DockDBusProxy: logTrayPluginListChange(itemKey, visible)
end
DockDBusProxy->>DockDBusProxy: plugins() %% collect DockItemInfos
DockDBusProxy->>DockDBusProxy: visiblePluginCount(itemInfos)
alt visible plugin count changed
DockDBusProxy->>DockDBusProxy: visiblePluginList(itemInfos)
DockDBusProxy->>EventLogger: writeEventLog(EventLoggerData(EVENT_LOGGER_TRAY_PLUGIN_LIST, taskmanager_config, payload))
DockDBusProxy->>DockDBusProxy: update m_visiblePluginCount
else unchanged
DockDBusProxy-->>DockDBusProxy: return without logging
end
Updated class diagram for dock event logging integrationclassDiagram
class DockDBusProxy {
+DockDBusProxy(DockPanel* parent)
+DockItemInfos plugins()
+void setItemOnDock(QString settingKey, QString itemKey, bool visible)
+void updateDockPluginsVisible(QVariantMap pluginsVisible)
#DS_NAMESPACE::DAppletProxy* m_oldDockApplet
#DS_NAMESPACE::DAppletProxy* m_trayApplet
#QTimer m_trayPluginListLogTimer
#int m_visiblePluginCount
+void logTrayPluginList()
+void logTrayPluginListChange(QString changedItemKey, bool visible)
}
class DockSettings {
+DockSettings(QObject* parent)
+void init()
+Position position()
+void setPosition(Position position)
+ItemAlignment itemAlignment()
+void setItemAlignment(ItemAlignment alignment)
-void addWriteJob(QString job)
-Position m_dockPosition
-ItemAlignment m_alignment
-QScopedPointer~DConfig~ m_dockConfig
}
class MultiTaskView {
+MultiTaskView(QObject* parent)
+bool init()
+void openWorkspace()
-QString m_iconName
-QObject* m_multitaskview
-bool m_kWinEffect
}
class TaskManagerSettings {
+TaskManagerSettings(QObject* parent)
+bool isAllowedForceQuit()
+bool mergeAppModel()
+void setMergeAppModel(bool enable)
+QStringList dockedElements()
+void addDockedElement(QString element)
+void removeDockedElement(QString element)
+void logMergeAppModel(bool mergeAppModelOn)
-void migrateFromDockedItems()
-void saveDockedElements()
-QScopedPointer~DConfig~ m_taskManagerDconfig
-bool m_windowSplit
-QStringList m_dockedElements
}
class EventLogger {
+static EventLogger& instance()
+void init(QString appId, bool sync)
+void writeEventLog(EventLoggerData data)
}
class EventLoggerData {
+EventLoggerData(qint64 eventId, QString category, QJsonObject payload)
+qint64 eventId
+QString category
+QJsonObject payload
}
class DockPanel {
+void ReloadPlugins()
}
class DConfig {
+QVariant value(QString key)
+void setValue(QString key, QVariant value)
+void valueChanged(QString key)
}
DockDBusProxy --> DockPanel : parent
DockDBusProxy --> EventLogger : writeEventLog
DockSettings --> EventLogger : writeEventLog
MultiTaskView --> EventLogger : writeEventLog
TaskManagerSettings --> EventLogger : writeEventLog
EventLoggerData --> EventLogger : used_by
DockSettings --> DConfig : m_dockConfig
TaskManagerSettings --> DConfig : m_taskManagerDconfig
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 4 issues, and left some high level feedback:
- EventLogger::instance().init("org.deepin.dde.shell", false) is called in multiple classes (DockDBusProxy, DockSettings, MultiTaskView, TaskManagerSettings); consider centralizing this initialization or adding a shared helper to avoid repeated init calls and make future changes to the logger setup easier to manage.
- DockDBusProxy::logTrayPluginList and logTrayPluginListChange both recompute visible counts, plugin lists and then write essentially the same event; factoring this into a shared helper (e.g., logTrayPluginList(const DockItemInfos &)) would reduce duplication and the risk of inconsistent behavior between the two paths.
- Several new logging calls use qDebug()/qCInfo for event logger messages that may fire on user interactions (e.g., tray plugin changes, dock position/mode changes, merge_app_model toggles); consider routing these through appropriate logging categories and levels or reducing their verbosity to avoid noisy logs in normal operation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- EventLogger::instance().init("org.deepin.dde.shell", false) is called in multiple classes (DockDBusProxy, DockSettings, MultiTaskView, TaskManagerSettings); consider centralizing this initialization or adding a shared helper to avoid repeated init calls and make future changes to the logger setup easier to manage.
- DockDBusProxy::logTrayPluginList and logTrayPluginListChange both recompute visible counts, plugin lists and then write essentially the same event; factoring this into a shared helper (e.g., logTrayPluginList(const DockItemInfos &)) would reduce duplication and the risk of inconsistent behavior between the two paths.
- Several new logging calls use qDebug()/qCInfo for event logger messages that may fire on user interactions (e.g., tray plugin changes, dock position/mode changes, merge_app_model toggles); consider routing these through appropriate logging categories and levels or reducing their verbosity to avoid noisy logs in normal operation.
## Individual Comments
### Comment 1
<location path="panels/dock/multitaskview/multitaskview.cpp" line_range="31-36" />
<code_context>
+constexpr qint64 EVENT_LOGGER_KWIN_MULTITASK_VIEW = 1000300000;
+constexpr int EventLaunchTypeDockIcon = 2;
+
+QString kwinVersion()
+{
+ static const QString version = [] {
+ QProcess process;
+ process.start(QStringLiteral("dpkg-query"), { QStringLiteral("-W"), QStringLiteral("-f=${Version}"), QStringLiteral("kwin-x11") });
+ if (!process.waitForFinished(1000) || process.exitStatus() != QProcess::NormalExit || process.exitCode() != 0) {
+ return QString();
+ }
</code_context>
<issue_to_address>
**issue (performance):** Blocking dpkg-query call on the UI path can freeze the dock for up to 1s on first use.
This runs in the GUI thread on the first call (via `openWorkspace()` → `logMultiTaskViewEvent()`), so the synchronous `dpkg-query` + `waitForFinished(1000)` can stall the interface. Please move this work off the UI thread (e.g., background task or lazy, non-blocking init with caching), and/or reduce the timeout and handle failures without blocking.
</issue_to_address>
### Comment 2
<location path="panels/dock/docksettings.cpp" line_range="270" />
<code_context>
+void DockSettings::setPosition(const Position& position)
</code_context>
<issue_to_address>
**question (bug_risk):** Dock config events after startup only include one of shell_pos or shell_dock_mode, which may surprise downstream consumers.
On startup, `dock_config` includes both `shell_pos` and `shell_dock_mode`, but subsequent updates (`setPosition` / `setItemAlignment`) emit only the changed field. If consumers treat each event as a full snapshot, this will produce partial records. Either always emit both fields (using current in-memory state) on each update, or clearly define these events as incremental and document/encode them as such.
</issue_to_address>
### Comment 3
<location path="panels/dock/dockdbusproxy.cpp" line_range="14" />
<code_context>
#include <QObject>
+#include <QJsonObject>
+
+#ifdef HAVE_DDE_API_EVENTLOGGER
+#include <dde-api/eventlogger.hpp>
+#endif
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new event-logging code by extracting shared aggregation and logging helpers and simplifying the timer wiring to avoid duplication and clarify control flow.
You can reduce duplication and simplify flow without changing behavior by extracting shared logic and slightly restructuring the timer wiring.
### 1. Deduplicate `logTrayPluginList` / `logTrayPluginListChange`
Both functions compute `itemInfos`, then do the same “count → compare → update → list → log” sequence. Extract that into a single helper and reuse it:
```cpp
#ifdef HAVE_DDE_API_EVENTLOGGER
bool DockDBusProxy::updateAndLogTrayPluginList(const DockItemInfos &itemInfos)
{
const int count = visiblePluginCount(itemInfos);
if (count == m_visiblePluginCount)
return false;
m_visiblePluginCount = count;
const QString trayPluginList = visiblePluginList(itemInfos);
DDE_EventLogger::EventLogger::instance().writeEventLog(
DDE_EventLogger::EventLoggerData(EVENT_LOGGER_TRAY_PLUGIN_LIST,
QStringLiteral("taskmanager_config"),
{ { QStringLiteral("tray_plugin_list"), trayPluginList } }));
qDebug() << "EventLogger: tray_plugin_list:" << trayPluginList;
return true;
}
void DockDBusProxy::logTrayPluginList()
{
updateAndLogTrayPluginList(plugins());
}
void DockDBusProxy::logTrayPluginListChange(const QString &changedItemKey, bool visible)
{
DockItemInfos itemInfos = plugins();
for (DockItemInfo &itemInfo : itemInfos) {
if (itemInfo.itemKey == changedItemKey) {
itemInfo.visible = visible;
break;
}
}
updateAndLogTrayPluginList(itemInfos);
}
#endif
```
This keeps the event format and debug output in one place and makes future changes less error‑prone.
### 2. Unify aggregation over `DockItemInfos`
`visiblePluginList` and `visiblePluginCount` are very similar traversals. You can keep both public helpers but internally share a single aggregation, avoiding drift if the filter logic changes:
```cpp
#ifdef HAVE_DDE_API_EVENTLOGGER
struct VisiblePluginStats {
int count = 0;
QString list;
};
VisiblePluginStats visiblePluginStats(const DockItemInfos &itemInfos)
{
QStringList pluginNames;
int count = 0;
for (const DockItemInfo &itemInfo : itemInfos) {
if (itemInfo.visible) {
++count;
if (!itemInfo.name.isEmpty())
pluginNames.append(itemInfo.name);
}
}
pluginNames.removeDuplicates();
return { count, pluginNames.join(QStringLiteral(",")) };
}
QString visiblePluginList(const DockItemInfos &itemInfos)
{
return visiblePluginStats(itemInfos).list;
}
int visiblePluginCount(const DockItemInfos &itemInfos)
{
return visiblePluginStats(itemInfos).count;
}
#endif
```
This keeps all filter logic in one spot while preserving your existing API.
### 3. Simplify timer + signal wiring
Right now the `QTimer` timeout connects `pluginsChanged` to `logTrayPluginList` inside the timeout lambda. You can make the lifecycle more obvious by always wiring `pluginsChanged` to restart the one‑shot timer, and let the timer be the only place that calls `logTrayPluginList()`:
```cpp
#ifdef HAVE_DDE_API_EVENTLOGGER
DockDBusProxy::DockDBusProxy(DockPanel* parent)
: QObject(parent)
, m_oldDockApplet(nullptr)
, m_trayApplet(nullptr)
{
// ...
DDE_EventLogger::EventLogger::instance().init("org.deepin.dde.shell", false);
m_trayPluginListLogTimer.setSingleShot(true);
m_trayPluginListLogTimer.setInterval(1000);
connect(&m_trayPluginListLogTimer, &QTimer::timeout,
this, &DockDBusProxy::logTrayPluginList);
// ...
QTimer *timer = new QTimer;
timer->setInterval(1000);
connect(timer, &QTimer::timeout, this, [=] {
if (getOtherApplet()) {
timer->stop();
timer->deleteLater();
connect(m_trayApplet, SIGNAL(pluginsChanged()),
this, SIGNAL(pluginsChanged()));
connect(this, &DockDBusProxy::pluginsChanged, this, [this] {
m_trayPluginListLogTimer.start();
}, Qt::UniqueConnection);
m_trayPluginListLogTimer.start(); // initial delayed log
}
});
timer->start();
}
#endif
```
This keeps the “debounce and delay logging by 1s” behavior but makes the control flow easier to follow: `pluginsChanged` → restart timer → timer timeout → `logTrayPluginList()`.
</issue_to_address>
### Comment 4
<location path="panels/dock/docksettings.cpp" line_range="12" />
<code_context>
#include <QObject>
+#include <QJsonObject>
+
+#ifdef HAVE_DDE_API_EVENTLOGGER
+#include <dde-api/eventlogger.hpp>
+#endif
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the event-logging logic into small helper methods to centralize the schema and avoid repeating conditional blocks across the class.
You can reduce duplication and centralize the event‑logging schema by introducing small helpers and reusing them instead of inlining `#ifdef` blocks in multiple places.
### 1. Centralize dock config logging
Add a private helper in `DockSettings`:
```cpp
// docksettings.h
class DockSettings : public QObject {
Q_OBJECT
// ...
private:
void logDockConfig(std::optional<Position> pos,
std::optional<ItemAlignment> align,
const QString &reason) const;
};
```
```cpp
// docksettings.cpp
void DockSettings::logDockConfig(std::optional<Position> pos,
std::optional<ItemAlignment> align,
const QString &reason) const
{
#ifdef HAVE_DDE_API_EVENTLOGGER
QVariantMap payload;
if (pos)
payload.insert("shell_pos", position2String(*pos));
if (align)
payload.insert("shell_dock_mode", itemAlignment2String(*align));
DDE_EventLogger::EventLogger::instance().writeEventLog(
DDE_EventLogger::EventLoggerData(EVENT_LOGGER_DOCK_CONFIG, "dock_config", payload));
qCInfo(dockSettingsLog) << "EventLogger: dock config" << reason << payload;
#else
Q_UNUSED(pos);
Q_UNUSED(align);
qCInfo(dockSettingsLog) << "EventLogger not available:" << reason;
#endif
}
```
Then simplify the call sites:
```cpp
void DockSettings::init()
{
if (m_dockConfig && m_dockConfig->isValid()) {
// ... load config as before ...
logDockConfig(m_dockPosition, m_alignment, QStringLiteral("on startup"));
// connect(m_dockConfig, ...) unchanged
}
}
void DockSettings::setPosition(const Position& position)
{
if (position == m_dockPosition) return;
m_dockPosition = position;
Q_EMIT positionChanged(m_dockPosition);
addWriteJob(positionJob);
logDockConfig(position, std::nullopt, QStringLiteral("position changed"));
}
void DockSettings::setItemAlignment(const ItemAlignment& alignment)
{
if (alignment == m_alignment) return;
m_alignment = alignment;
Q_EMIT itemAlignmentChanged(m_alignment);
addWriteJob(itemAlignmentJob);
logDockConfig(std::nullopt, alignment, QStringLiteral("itemAlignment changed"));
}
```
This keeps the event ID, event name, and key names in one place and removes repeated `#ifdef` clutter from the setters.
### 2. Isolate EventLogger initialization
Similarly, hide the constructor `#ifdef` behind a small helper:
```cpp
// docksettings.h
private:
void initEventLogger();
```
```cpp
// docksettings.cpp
void DockSettings::initEventLogger()
{
#ifdef HAVE_DDE_API_EVENTLOGGER
DDE_EventLogger::EventLogger::instance().init("org.deepin.dde.shell", false);
qCInfo(dockSettingsLog) << "EventLogger initialized";
#else
qCInfo(dockSettingsLog) << "EventLogger not available (dde-api eventlogger.hpp not found)";
#endif
}
DockSettings::DockSettings(QObject *parent)
: QObject(parent)
, m_writeTimer(new QTimer(this))
// ...
{
m_writeTimer->setSingleShot(true);
m_writeTimer->setInterval(1000);
initEventLogger();
init();
}
```
This preserves all existing functionality while making the primary behavior of `DockSettings` (state, signals, persistence) easier to read and keeping logging logic maintainable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ivy233
force-pushed
the
feat/event-logger-integration
branch
2 times, most recently
from
ef4e814 to
729c624
Compare
Ivy233
requested review from
18202781743,
BLumia,
caixr23,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
Ivy233
force-pushed
the
feat/event-logger-integration
branch
from
729c624 to
3a58b83
Compare
18202781743
reviewed
Apr 28, 2026
18202781743
reviewed
Apr 28, 2026
Ivy233
force-pushed
the
feat/event-logger-integration
branch
2 times, most recently
from
db6b171 to
06e2df5
Compare
|
TAG Bot New tag: 2.0.39 |
Ivy233
force-pushed
the
feat/event-logger-integration
branch
from
06e2df5 to
9eb0deb
Compare
Ivy233
force-pushed
the
feat/event-logger-integration
branch
from
9eb0deb to
aeb7034
Compare
18202781743
reviewed
Apr 29, 2026
Ivy233
force-pushed
the
feat/event-logger-integration
branch
from
aeb7034 to
f1bfb5b
Compare
deepin pr auto review代码审查报告：DDE Dock 事件日志集成总体评价这段代码实现了在 DDE Dock 中集成事件日志功能，通过引入 详细审查意见1. 语法和逻辑1.1 CMakeLists.txt 中的依赖处理问题：在主 CMakeLists.txt 中使用 建议： find_package(DDEAPI 6.0.39 QUIET)这样可以确保使用足够新的 DDEAPI 版本，与 debian/control 中的依赖版本保持一致。 1.2 dockdbusproxy.cpp 中的日志记录时机问题：在 建议：
1.3 logCurrentVisiblePluginList 函数逻辑问题：该函数在处理 建议： void DockDBusProxy::logCurrentVisiblePluginList(const QString &changedItemKey, bool changedVisible)
{
QStringList visibleItemKeys;
for (const auto &info : plugins()) {
if (info.itemKey == changedItemKey) {
if (changedVisible)
visibleItemKeys.append(info.itemKey);
} else if (info.visible) {
visibleItemKeys.append(info.itemKey);
}
}
// 简化为：
for (const auto &info : plugins()) {
bool isVisible = (info.itemKey == changedItemKey) ? changedVisible : info.visible;
if (isVisible) {
visibleItemKeys.append(info.itemKey);
}
}
// 其余代码不变
}2. 代码质量2.1 常量定义问题：事件ID常量（如 建议： #pragma once
namespace EventLoggerIds {
constexpr qint64 TRAY_PLUGIN_LIST = 1000610007;
constexpr qint64 DOCK_CONFIG = 1000610006;
constexpr qint64 KWIN_MULTITASK_VIEW = 1000300000;
constexpr qint64 MERGE_APP_MODEL = 1000610011;
}然后在各源文件中使用 2.2 日志记录函数命名问题：日志记录函数命名不一致，如 建议：
2.3 代码重复问题：在多个文件中重复了相同的条件编译和日志记录代码。 建议： // eventlogger_helper.h
#pragma once
#ifdef HAVE_DDE_API_EVENTLOGGER
#include <dde-api/eventlogger.hpp>
namespace EventLoggerHelper {
inline void logEvent(qint64 eventId, const QString &eventName, const QJsonObject &payload) {
DDE_EventLogger::EventLogger::instance().writeEventLog(
DDE_EventLogger::EventLoggerData(eventId, eventName, payload));
}
}
#else
namespace EventLoggerHelper {
inline void logEvent(qint64, const QString &, const QJsonObject &) {
// 空实现
}
}
#endif3. 代码性能3.1 多次调用 plugins() 函数问题：在 建议： void DockDBusProxy::logInitialPluginState()
{
const auto pluginInfos = plugins(); // 只调用一次
QStringList visibleItemKeys;
for (const auto &info : pluginInfos) {
if (info.visible) {
visibleItemKeys.append(info.itemKey);
}
}
// 其余代码不变
}3.2 QProcess 使用问题：在 建议：
4. 代码安全4.1 潜在的空指针解引用问题：在 建议： void DockDBusProxy::logInitialPluginState()
{
const auto pluginInfos = plugins();
if (pluginInfos.isEmpty()) {
qCWarning(dockLog) << "No plugins available for logging";
return;
}
// 其余代码不变
}4.2 字符串拼接问题：在日志记录中使用 建议： QStringList visibleItemKeys;
for (const auto &info : pluginInfos) {
if (info.visible) {
visibleItemKeys.append(info.itemKey);
if (visibleItemKeys.size() >= 100) { // 限制最大数量
qCWarning(dockLog) << "Too many plugins to log, truncating";
break;
}
}
}4.3 事件ID硬编码问题：事件ID直接硬编码在代码中，缺乏验证机制。 建议： namespace EventLoggerIds {
// 定义有效的事件ID范围
constexpr qint64 MIN_EVENT_ID = 1000000000;
constexpr qint64 MAX_EVENT_ID = 9999999999;
inline bool isValidEventId(qint64 id) {
return id >= MIN_EVENT_ID && id <= MAX_EVENT_ID;
}
}
// 使用时验证
if (EventLoggerIds::isValidEventId(EVENT_LOGGER_TRAY_PLUGIN_LIST)) {
// 记录日志
}5. 其他建议
总结这段代码实现了在 DDE Dock 中集成事件日志功能，整体设计合理，但存在一些可以改进的地方。主要改进方向包括：
通过这些改进，可以使代码更加健壮、可维护和高效。 |
18202781743
approved these changes
Apr 29, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, Ivy233 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 |
集成事件日志记录并使用 cmake find_package 管理 - Migrate from find_path to find_package(DDEAPI) - Add debounce timer for initial plugin state logging - Remove manual init in DockPanel::load() (now auto-initialized) - Link DDEAPI::EventLogger to dockpanel, multitaskview, taskmanager PMS: TASK-388657
Ivy233
force-pushed
the
feat/event-logger-integration
branch
from
f1bfb5b to
9bf7407
Compare
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
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.
Add conditional event logging support for dock-related modules and wire the required build detection and package dependency. The implementation keeps the logging integration optional when the event logger headers are unavailable.
feat(dock): 集成可选事件日志支持
为 dock 相关模块添加条件事件日志支持，并接入所需的构建检测与包依赖。该实现会在缺少事件日志头文件时保持日志集成可选。
PMS: TASK-388657
Summary by Sourcery
Integrate optional event logging across dock components while keeping functionality unchanged when the dde-api event logger is unavailable.
New Features:
Enhancements:
Build: