fix: fix system proxy status change error #485
Merged
+65
−20
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 GuideThis PR wires system proxy GSettings changes into the network service and UI controller, ensuring that when system proxy hosts or mode change via GSettings, the D-Bus service emits a proxyChanged signal and the dock button state and proxy status stay in sync. It also factors out system proxy existence computation and links in gsettings-qt6 for the src target. Sequence diagram for system proxy host change updating dock statussequenceDiagram
actor User
participant SettingsApp
participant GSettingsDaemon
participant QGSettings_NetworkProxy as QGSettings_NetworkProxy
participant NetworkProxy
participant DBus
participant ProxyController
participant NetManagerThreadPrivate
participant DockProxyItem
User->>SettingsApp: Change system proxy host
SettingsApp->>GSettingsDaemon: Write org.gnome.system.proxy.* host
GSettingsDaemon-->>QGSettings_NetworkProxy: Notify changed(host)
QGSettings_NetworkProxy-->>NetworkProxy: changed(key=host)
NetworkProxy->>NetworkProxy: onConfigChanged(key)
NetworkProxy->>NetworkProxy: sender() as QGSettings
NetworkProxy-->>DBus: emit proxyChanged(type, host)
DBus-->>ProxyController: proxyChanged(type, value)
ProxyController->>ProxyController: onProxyChanged(type, value)
ProxyController->>ProxyController: queryProxyDataByType(type)
ProxyController->>ProxyController: update m_sysProxyConfig
ProxyController->>ProxyController: checkSystemProxyExist()
ProxyController-->>NetManagerThreadPrivate: systemProxyExistChanged(flag)
NetManagerThreadPrivate->>DockProxyItem: onSystemProxyExistChanged(flag)
DockProxyItem->>DockProxyItem: updateenabled(flag)
DockProxyItem->>DockProxyItem: refresh dock button state
Sequence diagram for system proxy mode change via GSettingssequenceDiagram
actor User
participant SettingsApp
participant GSettingsDaemon
participant QGSettings_ProxyController as QGSettings_ProxyController
participant ProxyController
participant NetManagerThreadPrivate
participant DockProxyItem
User->>SettingsApp: Change system proxy mode
SettingsApp->>GSettingsDaemon: Write org.gnome.system.proxy mode
GSettingsDaemon-->>QGSettings_ProxyController: Notify changed(mode)
QGSettings_ProxyController-->>ProxyController: changed(key=mode)
ProxyController->>ProxyController: onSystemProxyChanged(key)
ProxyController->>ProxyController: onProxyMethodChanged(mode)
ProxyController-->>NetManagerThreadPrivate: proxyMethodChanged(mode)
NetManagerThreadPrivate->>DockProxyItem: onSystemProxyMethodChanged(mode)
DockProxyItem->>DockProxyItem: updatemethod(mode)
DockProxyItem->>DockProxyItem: updateenabled(mode==Auto or mode==Manual)
Note over ProxyController,ProxyController: ProxyController also calls onSystemProxyChanged("autoconfigUrl") on startup to sync auto proxy URL and systemProxyExist flag
Class diagram for updated ProxyController and NetworkProxyclassDiagram
class ProxyController {
- NetworkInter* m_networkInter
- ProxyMethod m_proxyMethod
- QString m_autoProxyURL
- QList~SysProxyConfig~ m_sysProxyConfig
- AppProxyConfig m_appProxyConfig
- bool m_appProxyExist
- bool m_systemProxyExist
- QGSettings* m_proxySettings
+ ProxyController(QObject* parent)
+ void setProxyMethod(ProxyMethod pm)
+ void queryAutoProxy()
+ void checkSystemProxyExist()
+ AppProxyType appProxyType(QString v)
+ QString appProxyType(AppProxyType v)
+ bool systemProxyExist() const
+ QString autoProxy() const
+ AppProxyConfig appProxyConfig() const
+ Q_SIGNAL void proxyMethodChanged(QString method)
+ Q_SIGNAL void systemProxyExistChanged(bool exist)
+ Q_SIGNAL void autoProxyChanged(QString url)
+ Q_SIGNAL void proxyChanged()
+ Q_SLOT void onIPChanged(QString value)
+ Q_SLOT void onPasswordChanged(QString value)
+ Q_SLOT void onUserChanged(QString value)
+ Q_SLOT void onPortChanged(uint value)
+ Q_SLOT void onProxyMethodChanged(QString method)
+ Q_SLOT void onSystemProxyChanged(QString key)
+ Q_SLOT void onProxyChanged(QString type, QString value)
}
class NetworkProxy {
- QDBusConnection m_dbusConnection
- NetworkStateHandler* m_networkStateHandler
- QGSettings* m_proxySettings
- QGSettings* m_proxyChildSettingsHttp
- QGSettings* m_proxyChildSettingsHttps
- QGSettings* m_proxyChildSettingsFtp
- QGSettings* m_proxyChildSettingsSocks
+ NetworkProxy(QDBusConnection& dbusConnection, NetworkStateHandler* handler, QObject* parent)
+ void SetProxyAuthentication(QString proxyType, QString user, QString password)
+ QGSettings* getProxyChildSettings(QString proxyType)
+ Q_SIGNAL void ProxyMethodChanged(QString proxyMode)
+ Q_SIGNAL void proxyChanged(QString type, QString value)
+ Q_SLOT void onConfigChanged(QString key)
}
class QGSettings {
+ QVariant get(QString key)
+ void set(QString key, QVariant value)
+ void setProperty(QString name, QVariant value)
+ Q_SIGNAL void changed(QString key)
}
class NetworkInter
class NetworkStateHandler
class SysProxyConfig {
QString url
}
class AppProxyConfig
ProxyController --> QGSettings : uses
ProxyController --> NetworkInter : uses
ProxyController --> SysProxyConfig : manages
ProxyController --> AppProxyConfig : manages
NetworkProxy --> QGSettings : owns
NetworkProxy --> NetworkStateHandler : uses
NetworkProxy --> QDBusConnection : uses
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 the DBus introspection XML for
proxyChanged, both<arg>elements are namedmethod; consider renaming them to distinct, meaningful names (e.g.typeandvalue) to match the signal signature and avoid confusion or tooling issues. - In
ProxyController,m_proxySettingsis created withnew QGSettings(...)without a parent; consider passingthisas the QObject parent to avoid leaking it and to match the ownership pattern used elsewhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the DBus introspection XML for `proxyChanged`, both `<arg>` elements are named `method`; consider renaming them to distinct, meaningful names (e.g. `type` and `value`) to match the signal signature and avoid confusion or tooling issues.
- In `ProxyController`, `m_proxySettings` is created with `new QGSettings(...)` without a parent; consider passing `this` as the QObject parent to avoid leaking it and to match the ownership pattern used elsewhere.
## Individual Comments
### Comment 1
<location> `src/proxycontroller.cpp:22` </location>
<code_context>
, m_networkInter(new NetworkInter(networkService, networkPath, QDBusConnection::sessionBus(), this))
, m_proxyMethod(ProxyMethod::Init)
, m_systemProxyExist(false)
+ , m_proxySettings(new QGSettings(QByteArray("org.gnome.system.proxy"), QByteArray()))
{
Q_ASSERT(m_networkInter);
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider giving m_proxySettings a QObject parent to avoid lifecycle and leak issues
Since `ProxyController` is a `QObject` and `QGSettings` also derives from `QObject`, you can construct it with `this` as the parent: `new QGSettings(..., this)`. That ensures it’s destroyed with the controller and avoids leaks or lifetime mismatches.
</issue_to_address>
### Comment 2
<location> `network-service-plugin/src/session/networkproxy.h:67-65` </location>
<code_context>
" <signal name='ProxyMethodChanged'>\n"
" <arg name='method' type='s'></arg>\n"
" </signal>\n"
+ " <signal name='proxyChanged'>\n"
+ " <arg name='method' type='s'></arg>\n"
+ " <arg name='method' type='s'></arg>\n"
+ " </signal>\n"
</code_context>
<issue_to_address>
**nitpick:** Use distinct, descriptive argument names in the D-Bus introspection for proxyChanged
Both `proxyChanged` signal args are named `method` in the introspection XML. Even though D-Bus only uses order and types, duplicated/misleading names hurt readability and maintenance. Please rename them to distinct, descriptive names (e.g., `type` and `value`) in the XML while keeping the signature unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/proxycontroller.cpp
Outdated
| , m_networkInter(new NetworkInter(networkService, networkPath, QDBusConnection::sessionBus(), this)) | ||
| , m_proxyMethod(ProxyMethod::Init) | ||
| , m_systemProxyExist(false) | ||
| , m_proxySettings(new QGSettings(QByteArray("org.gnome.system.proxy"), QByteArray())) |
There was a problem hiding this comment.
issue (bug_risk): Consider giving m_proxySettings a QObject parent to avoid lifecycle and leak issues
Since ProxyController is a QObject and QGSettings also derives from QObject, you can construct it with this as the parent: new QGSettings(..., this). That ensures it’s destroyed with the controller and avoids leaks or lifetime mismatches.
| @@ -64,6 +64,10 @@ class NetworkProxy : public QObject, protected QDBusContext | |||
| " <signal name='ProxyMethodChanged'>\n" | |||
| " <arg name='method' type='s'></arg>\n" | |||
There was a problem hiding this comment.
nitpick: Use distinct, descriptive argument names in the D-Bus introspection for proxyChanged
Both proxyChanged signal args are named method in the introspection XML. Even though D-Bus only uses order and types, duplicated/misleading names hurt readability and maintenance. Please rename them to distinct, descriptive names (e.g., type and value) in the XML while keeping the signature unchanged.
when the system proxy hosts changed,must change the button status of dock Log: fix proxy status Influence: dock proxy status PMS: BUG-313855
deepin pr auto review这份代码diff主要涉及网络代理配置的初始化、信号连接以及系统代理存在性检查的逻辑优化。以下是对语法逻辑、代码质量、代码性能和代码安全方面的详细审查和改进意见： 1. 语法逻辑审查问题点 1： onSystemProxyExistChanged(networkController->proxyController()->systemProxyExist());紧接着： connect(networkController->proxyController(), &ProxyController::systemProxyExistChanged, this, &NetManagerThreadPrivate::onSystemProxyExistChanged);分析：代码先手动调用了一次槽函数，然后才连接信号。这是正确的做法，确保初始化时状态同步。逻辑无误。 问题点 2： " <signal name='proxyChanged'>\n"
" <arg name='method' type='s'></arg>\n"
" <arg name='method' type='s'></arg>\n"分析：两个参数都命名为 问题点 3：端口范围检查 if (portInt < 0 || portInt >= 65535) {分析：将 2. 代码质量审查改进点 1：魔法字符串 const QString ProxyTypeAuto = "auto";
...
m_proxySettings->setProperty("type", ProxyTypeAuto);
...
host = childSettings->get("autoconfigUrl").toString();
...
if (key == "host" || key == "autoconfigUrl") {分析：虽然引入了常量 const QString GKeyProxyAutoConfigUrl = "autoconfigUrl";
const QString GKeyProxyHost = "host";改进点 2：代码重复 void ProxyController::checkSystemProxyExist()
{
bool isConfExist = false;
for (auto conf : m_sysProxyConfig) {
if (!conf.url.isEmpty()) {
isConfExist = true;
break;
}
}
bool systemProxyExist = isConfExist || !m_autoProxyURL.isEmpty();
if (m_systemProxyExist != systemProxyExist) {
m_systemProxyExist = systemProxyExist;
Q_EMIT systemProxyExistChanged(m_systemProxyExist);
}
}这段代码是从 改进点 3：空实现槽函数 void NetworkProxy::onConfigChanged(const QString &key) { }新代码实现了具体逻辑： void NetworkProxy::onConfigChanged(const QString &key)
{
QGSettings *settings = qobject_cast<QGSettings *>(sender());
if (!settings)
return;
if (key == "host" || key == "autoconfigUrl") {
emit proxyChanged(settings->property("type").toString(), settings->get(key).toString());
}
}分析：这是一个很好的改进，使得配置变更能够被正确处理。 3. 代码性能审查问题点 1：频繁的字符串比较 if (key == "host" || key == "autoconfigUrl") {分析：每次配置变更都会进行字符串比较。虽然性能影响不大，但如果配置项很多，可以考虑使用哈希表或枚举来优化。 问题点 2：不必要的信号连接 connect(this, &ProxyController::proxyChanged, this, [ this ] { checkSystemProxyExist(); });分析：这个连接会导致每次 4. 代码安全审查问题点 1：类型转换 int portInt = port.toInt();分析：如果 bool ok;
int portInt = port.toInt(&ok);
if (!ok || portInt < 0 || portInt > 65535) {
// 错误处理
}问题点 2：空指针检查 QGSettings *childSettings = getProxyChildSettings(proxyType);
if (!childSettings) {
// 错误处理
}
QString host;
int port = 0;
if (proxyType == ProxyTypeAuto) {
host = childSettings->get("autoconfigUrl").toString();
} else {
host = childSettings->get(GKeyProxyHost).toString();
port = childSettings->get(GKeyProxyPort).toInt();
}分析：虽然有空指针检查，但 问题点 3：D-Bus 连接 QDBusConnection::sessionBus().connect(networkService, networkPath, networkInterface, "proxyChanged", this, SLOT(onProxyChanged(const QString &, const QString &)));分析：D-Bus 信号连接没有检查返回值。如果连接失败，会导致功能失效。 if (!QDBusConnection::sessionBus().connect(...)) {
qCWarning(DSM()) << "Failed to connect D-Bus signal proxyChanged";
}总结建议
这些改进将提高代码的健壮性、可维护性和安全性。 |
caixr23
approved these changes
Feb 3, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, 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 |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
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.
when the system proxy hosts changed,must change the button status of dock
Log: fix proxy status
Influence: dock proxy status
PMS: BUG-313855
Summary by Sourcery
Handle system proxy configuration changes consistently across the network service and UI, ensuring proxy status reflects GSettings updates in real time.
Bug Fixes:
Enhancements:
Build: