feat: add item move support and secret agent improvements #465
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 GuideIntroduces move semantics for network items to keep the Qt item model in sync when wireless connections move between sections, adds a flag to optionally fall back to remove/add behavior, refactors the NetworkSecretAgent to wait briefly for panel clients before spawning a dialog, and registers the Network1 D-Bus service with appropriate bus policies. Sequence diagram for wireless secrets handling with client wait and timeoutsequenceDiagram
actor user as User
participant nm as NetworkManager
participant agent as NetworkSecretAgent
participant panel as PanelClient
participant dialog as AuthDialog
user->>nm: RequestWirelessConnection
nm->>agent: GetSecrets(connection type 802_11_wireless)
agent->>agent: askPasswords
alt noPanelClients
agent->>agent: set request.status = WaitClient
agent->>agent: m_waitClientTimer.start(5000)
alt panelConnectsBeforeTimeout
panel-->>agent: newConnection
agent->>agent: m_waitClientTimer.stop()
agent->>agent: newConnectionHandler
agent->>agent: waitClientTimeOut
agent->>agent: runAuthDialog(request)
agent-->>panel: write requestSecrets
else timeoutWithoutPanel
agent-->>agent: waitClientTimeOut (QTimer timeout)
agent->>agent: runAuthDialog(request)
agent->>agent: set request.status = WaitDialog
agent->>dialog: start NMSecretDialogBin
dialog-->>agent: secretsResult or timeout
end
else hasPanelClientsInitially
agent->>agent: runAuthDialog(request)
agent-->>panel: write requestSecrets
end
agent-->>nm: ReturnSecrets
nm-->>user: ConnectionEstablished
Class diagram for item move support and secret agent changesclassDiagram
class NetItem {
+void childAdded(NetItem child)
+void childAboutToBeRemoved(NetItem parent, int pos)
+void childRemoved(NetItem child)
+void childAboutToBeMoved(NetItem parent, int pos, NetItem newParent, int newPos)
+void childMoved(NetItem child)
+void childrenChanged()
}
class NetItemPrivate {
+bool addChild(NetItemPrivate child, int index)
+void removeChild(NetItemPrivate child)
+void moveChild(NetItemPrivate child, NetItemPrivate newParent)
+void updatename(QString name)
+void updateid(QString id)
-NetItem m_item
-QVector~NetItem~ m_children
-NetItem m_parent
}
class NetModel {
+NetModel(QObject parent)
+QModelIndex index(NetItem item)
+QModelIndex parent(QModelIndex index)
+int rowCount(QModelIndex parent)
+int columnCount(QModelIndex parent)
+QVariant data(QModelIndex index, int role)
+void connectObject(NetItem obj)
+void disconnectObject(NetItem obj)
#void updateObject()
#void aboutToAddObject(NetItem parent, int pos)
#void addObject(NetItem child)
#void aboutToRemoveObject(NetItem parent, int pos)
#void removeObject(NetItem child)
#void aboutToBeMoveObject(NetItem parent, int pos, NetItem newParent, int newPos)
#void moveObject(NetItem child)
-NetItem m_treeRoot
}
class NetType {
<<enum>> NetManagerFlag
NetManagerFlag Net_Device
NetManagerFlag Net_VPN
NetManagerFlag Net_SysProxy
NetManagerFlag Net_Airplane
NetManagerFlag Net_AirplaneTips
NetManagerFlag Net_VPNTips
NetManagerFlag Net_UseSecretAgent
NetManagerFlag Net_CheckPortal
NetManagerFlag Net_autoUpdateHiddenConfig
NetManagerFlag Net_noUseMoveItem
NetManagerFlag Net_DockFlags
NetManagerFlag Net_LockFlags
NetManagerFlag Net_GreeterFlags
NetManagerFlag Net_DccFlags
}
class NetManagerThreadPrivate {
+static QVariantMap CheckParamValid(QVariantMap param)
+static bool CheckPasswordValid(QString key, QString password)
+bool NetCheckAvailable() const
+bool AirplaneModeEnabled() const
+NetType.NetManagerFlags flags() const
+void setEnabled(bool enabled)
+void setAutoScanInterval(int ms)
-NetType.NetManagerFlags m_flags
-bool m_netCheckAvailable
-bool m_airplaneModeEnabled
-QMap~NetworkDetails*, QString~ m_detailsItemsMap
}
class NetWirelessDeviceItemPrivate {
+void updatehasConnection(bool hasConnection)
+NetItemPrivate getParentPrivate()
+QString id()
}
class NetManagerPrivate {
+void onDataChanged(int dataType, QString id, QVariant value)
-NetManagerThreadPrivate m_managerThread
}
class SecretsRequest {
<<enum>> Type
Type GetSecrets
Type SaveSecrets
Type DeleteSecrets
<<enum>> Status
Status Begin
Status WaitClient
Status WaitDialog
Status End
+SecretsRequest(Type type)
+Type type
+Status status
+QVariantMap connection
+QString callId
+QByteArray inputCache
+QProcess process
}
class NetworkSecretAgent {
+NetworkSecretAgent(QObject parent)
+void askPasswords(SecretsRequest request, QStringList hints)
+void newConnectionHandler()
+void disconnectedHandler()
+void authDialogFinished(int exitCode, QProcess.ExitStatus status)
+void authDialogReadOutput()
+void authDialogReadError()
+void authDialogError(QProcess.ProcessError error)
+void authDialogStarted()
+void authDialogReadAllOutput(bool isEnd)
+void doSecretsResult(QString callId, QByteArray data, bool isEnd)
+void runAuthDialog(SecretsRequest request)
+void waitClientTimeOut()
-QString nextId()
-int m_callNextId
-QList~SecretsRequest~ m_calls
-QLocalServer m_server
-QList~QLocalSocket*~ m_clients
-QByteArray m_lastData
-SecretService m_secretService
-QTimer m_waitClientTimer
}
NetItemPrivate --> NetItem : owns m_item
NetItem --> NetItemPrivate : uses private
NetModel --> NetItem : observes signals
NetManagerPrivate --> NetManagerThreadPrivate : uses m_managerThread
NetManagerThreadPrivate --> NetType : uses NetManagerFlag
NetworkSecretAgent --> SecretsRequest : manages
NetworkSecretAgent --> QLocalServer : uses
NetworkSecretAgent --> QLocalSocket : manages clients
NetWirelessDeviceItemPrivate --> NetItemPrivate : inherits
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 2 issues, and left some high level feedback:
- In NetworkSecretAgent::waitClientTimeOut(), the iterator is never advanced inside the loop when request.status != SecretsRequest::WaitClient, which will lead to an infinite loop; make sure to increment the iterator (or use a range-for) so the function can safely scan all requests.
- NetworkSecretAgent::waitClientTimeOut() currently breaks after handling the first SecretsRequest in WaitClient state, leaving any additional waiting requests unprocessed with no timer running; consider iterating over and handling all WaitClient entries or otherwise ensuring they will be processed later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In NetworkSecretAgent::waitClientTimeOut(), the iterator is never advanced inside the loop when request.status != SecretsRequest::WaitClient, which will lead to an infinite loop; make sure to increment the iterator (or use a range-for) so the function can safely scan all requests.
- NetworkSecretAgent::waitClientTimeOut() currently breaks after handling the first SecretsRequest in WaitClient state, leaving any additional waiting requests unprocessed with no timer running; consider iterating over and handling all WaitClient entries or otherwise ensuring they will be processed later.
## Individual Comments
### Comment 1
<location> `network-service-plugin/src/session/networksecretagent.cpp:559-561` </location>
<code_context>
+ }
+}
+
+void NetworkSecretAgent::waitClientTimeOut()
+{
+ for (auto it = m_calls.begin(); it != m_calls.end();) {
+ SecretsRequest &request = *it;
+ if (request.status == SecretsRequest::WaitClient) {
</code_context>
<issue_to_address>
**issue (bug_risk):** waitClientTimeOut iterates m_calls without advancing the iterator, causing an infinite loop
The loop never increments `it`, so any non-`WaitClient` entry will cause an infinite loop and hang the thread. Even when a `WaitClient` is found, earlier non-matching entries would still loop forever. This should either increment the iterator in the `for` header:
```cpp
for (auto it = m_calls.begin(); it != m_calls.end(); ++it) {
SecretsRequest &request = *it;
if (request.status == SecretsRequest::WaitClient) {
runAuthDialog(request);
break;
}
}
```
or use a range-based `for` if you don’t need the iterator itself.
</issue_to_address>
### Comment 2
<location> `network-service-plugin/src/session/networksecretagent.cpp:210-213` </location>
<code_context>
- for (auto &&client : m_clients) {
- client->write("\nrequestSecrets:" + reqJSON.toUtf8() + "\n");
- }
+ if (connType == "802-11-wireless" && m_clients.isEmpty()) {
+ // 启动定时器，等待
+ request.status = SecretsRequest::WaitClient;
+ m_waitClientTimer->start();
} else {
- // run auth dialog
</code_context>
<issue_to_address>
**suggestion (bug_risk):** A single shared wait timer for all wireless requests can lead to surprising behavior when multiple requests are pending
Using a single `m_waitClientTimer` for all wireless `SecretsRequest`s means a new request can reset the timer while a previous one is still in `WaitClient`. Since `waitClientTimeOut` advances only one request at a time, earlier requests can be delayed or starved. Consider either tracking wait state per `SecretsRequest` (own timer or timestamp) or enforcing that only one wireless request may be in `WaitClient` at a time and rejecting/queuing additional ones accordingly.
Suggested implementation:
```cpp
request.inputCache = reqJSON.toUtf8();
// For wireless connections, only allow one request to be in WaitClient state at a time.
// If m_waitClientTimer is already active, we skip the wait and immediately fall back
// to the normal handling path to avoid starving earlier pending requests.
if (connType == "802-11-wireless" && m_clients.isEmpty() && !m_waitClientTimer->isActive()) {
// 启动定时器，等待
request.status = SecretsRequest::WaitClient;
m_waitClientTimer->start();
} else {
```
If you prefer true queuing (i.e. multiple wireless requests all going through a client-wait phase in order), you will need to:
1. Track pending `SecretsRequest`s in a queue structure instead of relying solely on a single timer.
2. Adjust the `waitClientTimeOut` handler (or equivalent) to pop the next queued wireless request and move it out of `WaitClient` state, starting the timer again if more remain.
3. Optionally, add a helper such as `bool NetworkSecretAgent::hasWaitingWirelessRequest() const` and base the condition on that instead of `m_waitClientTimer->isActive()`.
The current change implements the "only one wireless request may be in WaitClient at a time" policy with minimal impact: additional wireless requests will follow the existing non-wait path while a wait is already in progress.
</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
+210
to
+213
| if (connType == "802-11-wireless" && m_clients.isEmpty()) { | ||
| // 启动定时器，等待 | ||
| request.status = SecretsRequest::WaitClient; | ||
| m_waitClientTimer->start(); |
There was a problem hiding this comment.
suggestion (bug_risk): A single shared wait timer for all wireless requests can lead to surprising behavior when multiple requests are pending
Using a single m_waitClientTimer for all wireless SecretsRequests means a new request can reset the timer while a previous one is still in WaitClient. Since waitClientTimeOut advances only one request at a time, earlier requests can be delayed or starved. Consider either tracking wait state per SecretsRequest (own timer or timestamp) or enforcing that only one wireless request may be in WaitClient at a time and rejecting/queuing additional ones accordingly.
Suggested implementation:
request.inputCache = reqJSON.toUtf8();
// For wireless connections, only allow one request to be in WaitClient state at a time.
// If m_waitClientTimer is already active, we skip the wait and immediately fall back
// to the normal handling path to avoid starving earlier pending requests.
if (connType == "802-11-wireless" && m_clients.isEmpty() && !m_waitClientTimer->isActive()) {
// 启动定时器，等待
request.status = SecretsRequest::WaitClient;
m_waitClientTimer->start();
} else {
If you prefer true queuing (i.e. multiple wireless requests all going through a client-wait phase in order), you will need to:
- Track pending
SecretsRequests in a queue structure instead of relying solely on a single timer. - Adjust the
waitClientTimeOuthandler (or equivalent) to pop the next queued wireless request and move it out ofWaitClientstate, starting the timer again if more remain. - Optionally, add a helper such as
bool NetworkSecretAgent::hasWaitingWirelessRequest() constand base the condition on that instead ofm_waitClientTimer->isActive().
The current change implements the "only one wireless request may be in WaitClient at a time" policy with minimal impact: additional wireless requests will follow the existing non-wait path while a wait is already in progress.
caixr23
force-pushed
the
master
branch
4 times, most recently
from
33161af to
7e79f48
Compare
Contributor
|
TAG Bot New tag: 2.0.78 |
caixr23
force-pushed
the
master
branch
2 times, most recently
from
05752fc to
e9977cb
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances network item management by adding move operations for wireless network items and improving the secret agent's reliability. The changes enable wireless network items to be moved between "Mine" and "Other" sections without remove/add operations, and add a client waiting mechanism to the NetworkSecretAgent for better password dialog handling.
Key Changes
- Added child move functionality with proper model synchronization through new signals (
childAboutToBeMoved,childMoved) and a move flag to filter out intermediate add/remove signals - Implemented a 5-second client wait mechanism in NetworkSecretAgent before falling back to auth dialogs for wireless password requests
- Updated D-Bus service configuration from
org.deepin.service.SessionNetwork/SystemNetworktoorg.deepin.dde.Network1
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| network-service-plugin/src/system/org.deepin.dde.Network1.conf | Updated D-Bus service name from org.deepin.service.SessionNetwork to org.deepin.dde.Network1 |
| network-service-plugin/src/session/org.deepin.dde.Network1.conf | Added new session D-Bus configuration file for org.deepin.dde.Network1 service |
| network-service-plugin/src/session/networksecretagent.h | Added WaitClient status enum value and timer member for client waiting mechanism |
| network-service-plugin/src/session/networksecretagent.cpp | Implemented client waiting logic with timer and refactored auth dialog launching |
| network-service-plugin/CMakeLists.txt | Updated D-Bus config file installation paths to match new service names |
| net-view/window/private/netmodel.h | Added move-related slots and moving flag to support item move operations |
| net-view/window/private/netmodel.cpp | Implemented move signal handlers with flag-based filtering of intermediate signals |
| net-view/operation/private/netmanagerthreadprivate.h | Code style improvements (const correctness and formatting) |
| net-view/operation/private/netitemprivate.h | Added moveChild method declaration and changed removeChild return type to bool |
| net-view/operation/private/netitemprivate.cpp | Implemented moveChild functionality and refactored getChildIndex/removeChild |
| net-view/operation/netmanager.cpp | Replaced remove/add pattern with moveChild calls for wireless connection changes |
| net-view/operation/netitem.h | Added childAboutToBeMoved and childMoved signals |
| debian/deepin-service-plugin-network.install | Changed to wildcard pattern for D-Bus configuration file installation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| auto it = std::find(m_children.begin(), m_children.end(), child); | ||
| if (it == m_children.end()) { | ||
| return false; |
There was a problem hiding this comment.
The function returns false (a boolean) when the child is not found, but the return type is int. This should return -1 instead to match the return type and the subsequent ternary expression.
Suggested change
| return false; | |
| return -1; |
Comment on lines
+158
to
+157
| Q_EMIT m_item->childAboutToBeMoved(m_item, it - m_children.begin(), newParent->item(), newParent->getChildrenNumber()); | ||
| removeChild(child); | ||
| newParent->addChild(child); |
There was a problem hiding this comment.
The moveChild function calls removeChild(child) and newParent->addChild(child) which will emit their own add/remove signals. This means that in addition to the move signals (childAboutToBeMoved and childMoved), the old parent will also emit childAboutToBeRemoved and childRemoved, while the new parent will emit childAboutToBeAdded and childAdded. According to the comment in netitem.h line 66, these add/remove signals should be filtered out during moves, but this design is error-prone and could lead to incorrect model updates if not handled carefully in all consumers.
Suggested change
| Q_EMIT m_item->childAboutToBeMoved(m_item, it - m_children.begin(), newParent->item(), newParent->getChildrenNumber()); | |
| removeChild(child); | |
| newParent->addChild(child); | |
| const int oldIndex = static_cast<int>(it - m_children.begin()); | |
| const int newIndex = newParent->getChildrenNumber(); | |
| Q_EMIT m_item->childAboutToBeMoved(m_item, oldIndex, newParent->item(), newIndex); | |
| // Perform the move without emitting add/remove signals. | |
| m_children.erase(it); | |
| child->m_parent = newParent->item(); | |
| newParent->m_children.push_back(child->item()); | |
| // Notify both parents that their children collections changed. | |
| Q_EMIT m_item->childrenChanged(); | |
| Q_EMIT newParent->item()->childrenChanged(); |
| if (connType == "802-11-wireless" && m_clients.isEmpty()) { | ||
| // 启动定时器，等待 | ||
| request.status = SecretsRequest::WaitClient; | ||
| m_waitClientTimer->start(); |
There was a problem hiding this comment.
The single timer m_waitClientTimer is shared for all wireless secret requests. If multiple wireless secret requests arrive while waiting for clients, calling start() on line 213 for a subsequent request will restart the timer, potentially causing the first request to wait longer than 5 seconds or the wrong request to be processed when waitClientTimeOut() is called. Consider using per-request timers or tracking multiple waiting requests properly.
Suggested change
| m_waitClientTimer->start(); | |
| if (!m_waitClientTimer->isActive()) { | |
| m_waitClientTimer->start(); | |
| } |
deepin pr auto review我来对这个git diff进行详细的代码审查：
// netitemprivate.cpp 中的改进
int NetItemPrivate::getChildIndex(const NetItem *child) const
{
auto it = std::find(m_children.begin(), m_children.end(), child);
return it == m_children.end() ? -1 : it - m_children.begin();
}优点：
// netitemprivate.cpp 中的 moveChild 函数
bool NetItemPrivate::moveChild(NetItemPrivate *child, NetItemPrivate *newParent)
{
if (!child || !newParent || child->m_parent == newParent->item()) {
return false;
}
// ...
}优点：
// netmodel.cpp 中的移动操作处理
void NetModel::aboutToBeMoveObject(const NetItem *parent, int pos, const NetItem *newParent, int newPos)
{
m_moving = true;
QModelIndex i = index(parent);
QModelIndex newI = index(newParent);
beginMoveRows(i, pos, pos, newI, newPos);
}优点：
// networksecretagent.cpp 中的改进
void NetworkSecretAgent::waitClientTimeOut()
{
auto it = std::find_if(m_calls.begin(), m_calls.end(), [](const SecretsRequest &req) {
return req.status == SecretsRequest::WaitClient;
});
// ...
}优点：
bool NetItemPrivate::moveChild(NetItemPrivate *child, NetItemPrivate *newParent)
{
if (!child || !newParent || child->m_parent == newParent->item()) {
qWarning() << "Invalid move operation";
return false;
}
// ...
}
void NetworkSecretAgent::runAuthDialog(SecretsRequest &request)
{
if (request.status != SecretsRequest::WaitClient) {
qWarning() << "Invalid request status";
return;
}
// ...
}
# CMakeLists.txt
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/src/system/org.deepin.dde.Network1.conf
DESTINATION share/dbus-1/system.d/
RENAME org.deepin.dde.Network1.conf.${PROJECT_VERSION})
bool NetItemPrivate::moveChild(NetItemPrivate *child, NetItemPrivate *newParent)
{
if (!checkPermission()) {
qWarning() << "Permission denied";
return false;
}
// ...
}
void NetworkSecretAgent::askPasswords(SecretsRequest &request, const QStringList &fields)
{
// 加密敏感数据
QString encryptedReq = encryptRequest(reqJSON);
// ...
}
这些改进主要围绕：
建议在后续版本中：
这些改动整体上是积极的，提高了代码质量和可维护性。 |
mhduiy
approved these changes
Dec 30, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy 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 child move functionality with proper signal emissions for model synchronization 2. Implemented Net_noUseMoveItem flag to control move behavior in different contexts 3. Enhanced wireless connection handling to use move operations instead of remove/add for better performance 4. Improved NetworkSecretAgent with client connection waiting mechanism and timeout handling 5. Added D-Bus configuration files for proper service registration Log: Enhanced network item management with move operations and improved secret agent reliability Influence: 1. Test wireless network item movement between "Mine" and "Other" sections 2. Verify network list updates correctly when connections change 3. Test secret agent password dialog behavior with and without client connections 4. Validate D-Bus service registration and accessibility 5. Check network model synchronization during item operations feat: 添加项移动支持和密钥代理改进 1. 添加子项移动功能，包含正确的信号发射以实现模型同步 2. 实现 Net_noUseMoveItem 标志以在不同上下文中控制移动行为 3. 增强无线连接处理，使用移动操作替代删除/添加以提高性能 4. 改进 NetworkSecretAgent，添加客户端连接等待机制和超时处理 5. 添加 D-Bus 配置文件以支持正确的服务注册 Log: 增强网络项管理，支持移动操作并改进密钥代理可靠性 Influence: 1. 测试无线网络项在"我的"和"其他"部分之间的移动 2. 验证连接变更时网络列表的正确更新 3. 测试有客户端连接和无客户端连接时的密钥代理密码对话框行为 4. 验证 D-Bus 服务注册和可访问性 5. 检查项操作期间网络模型的同步情况 PMS: BUG-307099 BUG-336865
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.
synchronization
different contexts
of remove/add for better performance
and timeout handling
Log: Enhanced network item management with move operations and improved
secret agent reliability
Influence:
sections
connections
feat: 添加项移动支持和密钥代理改进
Log: 增强网络项管理，支持移动操作并改进密钥代理可靠性
Influence:
PMS: BUG-307099 BUG-336865
Summary by Sourcery
Introduce item move support in the network view model and improve the network secret agent’s handling of wireless password requests and service registration.
New Features:
Enhancements:
Deployment: