fix: move connection selection before network enable #472
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 guide (collapsed on small PRs)Reviewer's GuideMoves connection selection earlier in NetworkThread::enableDevice to avoid races with networking enablement and fixes the autoconnect timestamp comparison so the most recent connection is selected correctly. Sequence diagram for updated enableDevice connection selection timingsequenceDiagram
actor System
participant NetworkThread
participant Device
participant Connection
participant NetworkManager
System->>NetworkThread: enableDevice(device)
activate NetworkThread
Note over NetworkThread,Device: 1) Select autoconnect connection before enabling networking
NetworkThread->>Device: availableConnections()
Device-->>NetworkThread: connPaths
loop for each connPath in connPaths
NetworkThread->>Connection: connPath.settings()
Connection-->>NetworkThread: settings
alt settings.autoconnect is true
NetworkThread->>Connection: settings.timestamp()
Connection-->>NetworkThread: ts
alt ts is newer than maxTs or no selection yet
NetworkThread-->>NetworkThread: update maxTs and connPath0
end
else settings.autoconnect is false
NetworkThread-->>NetworkThread: skip connection
end
end
NetworkThread->>NetworkManager: isNetworkingEnabled()
NetworkManager-->>NetworkThread: enabled
alt enabled is false
NetworkThread->>NetworkManager: setNetworkingEnabled(true)
end
alt connPath0 is empty
NetworkThread-->>System: "/"
else
NetworkThread-->>System: connPath0
end
deactivate NetworkThread
Flow diagram for autoconnect connection selection in enableDeviceflowchart TD
A[Start enableDevice] --> B[Get connPaths from device.availableConnections]
B --> C[Initialize connPath0 as empty]
C --> D[Initialize maxTs as default QDateTime]
D --> E{More connPaths?}
E -->|No| F{Is connPath0 empty?}
F -->|Yes| G[Return /]
F -->|No| H[Return connPath0]
E -->|Yes| I[Take next connPath]
I --> J[Load settings from connPath.settings]
J --> K{settings.autoconnect?}
K -->|No| E
K -->|Yes| L[ts = settings.timestamp]
L --> M{ts > maxTs or connPath0 is empty?}
M -->|No| E
M -->|Yes| N[Set maxTs = ts and connPath0 = connPath.path]
N --> E
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 left some high level feedback:
- When picking the most recent autoconnect connection, consider basing the initialization/guard purely on
maxTs.isValid()(or!maxTs.isNull()) instead ofconnPath0.isEmpty()so the selection logic is clearly tied to timestamp validity rather than the side effect of having chosen a path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When picking the most recent autoconnect connection, consider basing the initialization/guard purely on `maxTs.isValid()` (or `!maxTs.isNull()`) instead of `connPath0.isEmpty()` so the selection logic is clearly tied to timestamp validity rather than the side effect of having chosen a path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ut003640
reviewed
Jan 29, 2026
The connection selection logic was moved to the beginning of enableDevice function to ensure it runs before enabling networking. This prevents potential race conditions where network connections might change after enabling the device. The connection selection algorithm now correctly updates maxTs variable when finding the most recent autoconnect connection. Log: Fixed connection selection timing issue during device activation Influence: 1. Test device activation with multiple available connections 2. Verify autoconnect functionality works correctly 3. Check that the most recent connection is properly selected 4. Test network activation with disabled networking 5. Verify connection selection when no autoconnect connections exist fix: 将连接选择逻辑移到网络启用之前 连接选择逻辑被移到 enableDevice 函数的开头，确保在网络启用之前运行。这可 以防止在启用设备后网络连接可能发生变化的潜在竞争条件。连接选择算法现在在 找到最新的自动连接连接时正确更新 maxTs 变量。 Log: 修复设备激活期间连接选择时机问题 Influence: 1. 测试具有多个可用连接时的设备激活 2. 验证自动连接功能正常工作 3. 检查是否正确选择了最新连接 4. 测试网络禁用状态下的网络激活 5. 验证没有自动连接连接时的连接选择 PMS: BUG-336935
deepin pr auto review这段代码的主要改动是将"查找并选择最佳连接"的逻辑块从函数后半部分移动到了函数前半部分（在启用网络之前）。以下是对这段代码的详细审查和改进建议： 1. 语法逻辑审查优点：
问题与建议：
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
改进后的代码建议建议将查找逻辑提取为独立函数，并优化变量命名： // 建议在类内部添加一个私有辅助函数
QString NetworkThread::findPreferredConnection(const NetworkManager::Device::Ptr &device)
{
auto availableConnections = device->availableConnections();
qCDebug(DSM()) << "available connections:" << availableConnections;
QString preferredPath;
QDateTime maxTimestamp;
for (const auto &connection : availableConnections) {
// 防御性编程：检查指针有效性
if (!connection) {
continue;
}
auto settings = connection->settings();
if (!settings || !settings->autoconnect()) {
continue;
}
QDateTime ts = settings->timestamp();
// 更新最新的时间戳和对应的路径
if (ts > maxTimestamp) {
maxTimestamp = ts;
preferredPath = connection->path();
}
}
return preferredPath;
}
QString NetworkThread::enableDevice(NetworkManager::Device::Ptr device)
{
// 1. 先确定要连接的目标（在状态变更前获取快照）
QString preferredConnPath = findPreferredConnection(device);
// 2. 启用网络
bool enabled = NetworkManager::isNetworkingEnabled();
if (!enabled) {
NetworkManager::setNetworkingEnabled(true);
}
// ... 原有的设备启用逻辑 ...
if (!device->isActive()) {
// ...
}
// 3. 返回结果
return preferredConnPath.isEmpty() ? "/" : preferredConnPath;
}总结这次代码修改（移动逻辑块的位置）在逻辑上是正确的，有助于提高状态获取的稳定性。主要改进点在于代码结构和可读性。通过提取辅助函数和优化命名，可以使代码更易于维护和理解。同时，增加对指针的非空检查能提高代码的健壮性。 |
ut003640
approved these changes
Jan 29, 2026
mhduiy
approved these changes
Jan 29, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy, ut003640 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 |
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.
The connection selection logic was moved to the beginning of enableDevice function to ensure it runs before enabling networking. This prevents potential race conditions where network connections might change after enabling the device. The connection selection algorithm now correctly updates maxTs variable when finding the most recent autoconnect connection.
Log: Fixed connection selection timing issue during device activation
Influence:
fix: 将连接选择逻辑移到网络启用之前
连接选择逻辑被移到 enableDevice 函数的开头，确保在网络启用之前运行。这可
以防止在启用设备后网络连接可能发生变化的潜在竞争条件。连接选择算法现在在
找到最新的自动连接连接时正确更新 maxTs 变量。
Log: 修复设备激活期间连接选择时机问题
Influence:
PMS: BUG-336935
Summary by Sourcery
Adjust device activation flow to select the appropriate autoconnect connection before enabling networking and correct the timestamp-based selection logic.
Bug Fixes: