fix: pluginVisibleChanged signal needs to be sent when dragging the p… #1230
Conversation
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
Reviewer's GuideIntroduce a dedicated helper to emit plugin visibility updates over D-Bus and invoke it across all workflows that change tray plugin visibility (drag-drop, show/hide toggles, section hides). Sequence diagram for plugin visibility change via drag-and-dropsequenceDiagram
participant TraySortOrderModel
participant QDBusConnection
participant Dock1Service
TraySortOrderModel->>TraySortOrderModel: dropToDockTray(draggedSurfaceId, ...)
TraySortOrderModel->>TraySortOrderModel: handlePluginVisibleChanged(draggedSurfaceId, true)
TraySortOrderModel->>QDBusConnection: send setItemOnDock(pluginId, true)
QDBusConnection->>Dock1Service: setItemOnDock(Dock_Quick_Plugins, pluginId, true)
Dock1Service-->>QDBusConnection: reply (success or error)
QDBusConnection-->>TraySortOrderModel: reply (success or error)
Sequence diagram for plugin visibility change via show/hide togglesequenceDiagram
participant TraySortOrderModel
participant QDBusConnection
participant Dock1Service
TraySortOrderModel->>TraySortOrderModel: setSurfaceVisible(surfaceId, visible)
TraySortOrderModel->>TraySortOrderModel: handlePluginVisibleChanged(surfaceId, visible)
TraySortOrderModel->>QDBusConnection: send setItemOnDock(pluginId, visible)
QDBusConnection->>Dock1Service: setItemOnDock(Dock_Quick_Plugins, pluginId, visible)
Dock1Service-->>QDBusConnection: reply (success or error)
QDBusConnection-->>TraySortOrderModel: reply (success or error)
Sequence diagram for plugin visibility change via section hidesequenceDiagram
participant TraySortOrderModel
participant QDBusConnection
participant Dock1Service
TraySortOrderModel->>TraySortOrderModel: findSection(surfaceId, ...)
TraySortOrderModel->>TraySortOrderModel: handlePluginVisibleChanged(surfaceId, false)
TraySortOrderModel->>QDBusConnection: send setItemOnDock(pluginId, false)
QDBusConnection->>Dock1Service: setItemOnDock(Dock_Quick_Plugins, pluginId, false)
Dock1Service-->>QDBusConnection: reply (success or error)
QDBusConnection-->>TraySortOrderModel: reply (success or error)
Class diagram for updated TraySortOrderModelclassDiagram
class TraySortOrderModel {
+registerSurfaceId(surfaceData: QVariantMap) : QString
+loadDataFromDConfig()
+saveDataToDConfig()
+handlePluginVisibleChanged(surfaceId: QString, visible: bool)
-m_hiddenIds: QList<QString>
...
}
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `panels/dock/tray/traysortordermodel.cpp:527` </location>
<code_context>
+void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible)
+{
+ QString pluginId = surfaceId.split("::").last();
+ if (pluginId.isEmpty()) {
+ qWarning() << "surfaceId format has error:" << surfaceId;
</code_context>
<issue_to_address>
Splitting surfaceId by '::' may not be robust for all input formats.
If '::' is missing, pluginId will equal surfaceId, which may be incorrect. Please validate the input format or clarify the expected format in documentation.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible)
{
QString pluginId = surfaceId.split("::").last();
if (pluginId.isEmpty()) {
qWarning() << "surfaceId format has error:" << surfaceId;
return;
}
=======
/**
* Handles plugin visibility changes.
* @param surfaceId Expected format: "prefix::pluginId"
* @param visible Whether the plugin is visible
*/
void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible)
{
if (!surfaceId.contains("::")) {
qWarning() << "surfaceId format error: missing '::' delimiter:" << surfaceId;
return;
}
QString pluginId = surfaceId.split("::").last();
if (pluginId.isEmpty()) {
qWarning() << "surfaceId format has error: pluginId is empty:" << surfaceId;
return;
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
525
to
531
| qWarning() << "surfaceId format has error:" << surfaceId; | ||
| return; | ||
| } | ||
|
|
||
| QDBusMessage msg = QDBusMessage::createMethodCall( | ||
| "org.deepin.dde.Dock1", | ||
| "/org/deepin/dde/Dock1", |
There was a problem hiding this comment.
suggestion: Splitting surfaceId by '::' may not be robust for all input formats.
If '::' is missing, pluginId will equal surfaceId, which may be incorrect. Please validate the input format or clarify the expected format in documentation.
Suggested change
| void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible) | |
| { | |
| QString pluginId = surfaceId.split("::").last(); | |
| if (pluginId.isEmpty()) { | |
| qWarning() << "surfaceId format has error:" << surfaceId; | |
| return; | |
| } | |
| /** | |
| * Handles plugin visibility changes. | |
| * @param surfaceId Expected format: "prefix::pluginId" | |
| * @param visible Whether the plugin is visible | |
| */ | |
| void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible) | |
| { | |
| if (!surfaceId.contains("::")) { | |
| qWarning() << "surfaceId format error: missing '::' delimiter:" << surfaceId; | |
| return; | |
| } | |
| QString pluginId = surfaceId.split("::").last(); | |
| if (pluginId.isEmpty()) { | |
| qWarning() << "surfaceId format has error: pluginId is empty:" << surfaceId; | |
| return; | |
| } |
yixinshark
force-pushed
the
fix-dragToDockSendSignal
branch
2 times, most recently
from
f91440c to
be87ecd
Compare
deepin pr auto review我对这段代码进行了审查，发现以下几个需要改进的地方：
改进建议：
class DockDBusService {
public:
static void setItemOnDock(const QString &pluginId, bool visible);
private:
static constexpr const char* SERVICE_NAME = "org.deepin.dde.Dock1";
static constexpr const char* OBJECT_PATH = "/org/deepin/dde/Dock1";
static constexpr const char* INTERFACE_NAME = "org.deepin.dde.Dock1";
static constexpr const char* METHOD_NAME = "setItemOnDock";
};
void TraySortOrderModel::handlePluginVisibleChanged(const QString &surfaceId, bool visible)
{
if (surfaceId.isEmpty()) {
qWarning() << "Empty surfaceId";
return;
}
QStringList parts = surfaceId.split("::");
if (parts.size() != 2 || parts.at(0).isEmpty() || parts.at(1).isEmpty()) {
qWarning() << "Invalid surfaceId format:" << surfaceId;
return;
}
// 使用异步DBus调用
QDBusMessage msg = QDBusMessage::createMethodCall(
DockDBusService::SERVICE_NAME,
DockDBusService::OBJECT_PATH,
DockDBusService::INTERFACE_NAME,
DockDBusService::METHOD_NAME
);
msg << DockQuickPlugins << parts.at(1) << visible;
QDBusPendingCall asyncCall = QDBusConnection::sessionBus().asyncCall(msg);
QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(asyncCall, this);
QObject::connect(watcher, &QDBusPendingCallWatcher::finished,
[this, surfaceId, visible](QDBusPendingCallWatcher *watcher) {
QDBusPendingReply<> reply = *watcher;
if (reply.isError()) {
qWarning() << "DBus call failed for" << surfaceId
<< "error:" << reply.error().message();
// 这里可以添加重试或回滚逻辑
} else {
qDebug() << "Successfully updated visibility for" << surfaceId;
}
watcher->deleteLater();
});
}
void TraySortOrderModel::updateVisualIndexes()
{
if (m_updateTimer) {
m_updateTimer->start(100); // 100ms的防抖延迟
return;
}
m_updateTimer = new QTimer(this);
m_updateTimer->setSingleShot(true);
connect(m_updateTimer, &QTimer::timeout, this, [this]() {
// 原有的updateVisualIndexes实现
m_updateTimer->deleteLater();
m_updateTimer = nullptr;
});
m_updateTimer->start(100);
}
class TraySortOrderModel : public QStandardItemModel
{
// ... 其他成员 ...
private:
QTimer *m_updateTimer = nullptr;
static constexpr const char* DockQuickPlugins = "Dock_Quick_Plugins";
};这些改进将提高代码的可维护性、性能和健壮性，同时降低耦合度。 |
…lugin docked to tray as title Log: as title Pms: BUG-299881
yixinshark
force-pushed
the
fix-dragToDockSendSignal
branch
from
be87ecd to
0e639ff
Compare
mhduiy
approved these changes
Aug 26, 2025
18202781743
approved these changes
Aug 26, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy, yixinshark 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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.
…lugin docked to tray
as title
Log: as title
Pms: BUG-299881
Summary by Sourcery
Trigger the pluginVisibleChanged signal over D-Bus whenever a tray plugin’s visibility changes (e.g., when dragging into or out of the tray)
Bug Fixes:
Enhancements: