refactor: move XSettings operations to dedicated thread #714

zzxyb · 2026-01-28T06:47:14Z

Previously SettingManager was moved to a separate QThread in Helper, but QThread lifecycle management was error-prone. This commit moves thread creation and management into SettingManager itself, using QMetaObject::invokeMethod for all XSettings/XResource operations to ensure thread safety.

Log:
Tasks:

Influence: XSettings operations now run on a dedicated thread managed by SettingManager, avoiding potential crashes from improper QThread destruction.

sourcery-ai

Sorry @zzxyb, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

deepin-ci-robot · 2026-01-28T06:47:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zzxyb

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Previously SettingManager was moved to a separate QThread in Helper, but QThread lifecycle management was error-prone. This commit moves thread creation and management into SettingManager itself, using QMetaObject::invokeMethod for all XSettings/XResource operations to ensure thread safety. Log: Tasks: Influence: XSettings operations now run on a dedicated thread managed by SettingManager, avoiding potential crashes from improper QThread destruction.

zccrs

话说有必要提供xsettings内容的读取吗？如果有，写入的值可以用个变量存储下，读取的时候从变量里给，而不是从xsettings里给，我们不关注xsettings里的值被外部设置的情况。

zccrs · 2026-01-28T07:05:14Z

src/xsettings/settingmanager.cpp

 {
+    if (m_async) {
+        m_thread = new QThread;
+        this->moveToThread(m_thread);


如果指定了parent，这个move操作应该会失败或者会收到一个警告，parent和child必须是在同个线程。

zccrs · 2026-01-28T07:05:59Z

src/xsettings/settingmanager.cpp

+void SettingManager::setGTKThemeAsync(const QString &themeName)
+{
+    QMetaObject::invokeMethod(this,
+                              "onSetGTKTheme",


为什么不用 &SettingManager::onSetGTKTheme 的方法，尽量不要用字符串，避免写错了编译期无法发现问题

zccrs · 2026-01-28T07:06:28Z

src/xsettings/settingmanager.cpp

+        "onGetGTKTheme",
+        Qt::BlockingQueuedConnection,
+        Q_RETURN_ARG(QString, result)
+        );


zccrs · 2026-01-28T07:06:33Z

src/xsettings/settingmanager.cpp

+    QString result;
+    QMetaObject::invokeMethod(
+        const_cast<SettingManager *>(this),
+        "onGetGTKTheme",


其它同样的问题就不一一评论了

zccrs · 2026-01-28T07:07:29Z

src/xsettings/settingmanager.cpp

+    QMetaObject::invokeMethod(
+        const_cast<SettingManager *>(this),
+        "onGetGTKTheme",
+        Qt::BlockingQueuedConnection,


这个函数调用方是谁？block了有没有可能导致主线程卡死？

其它同样的问题就不一一评论了

Copilot

Pull request overview

Refactors XSettings/XResource handling so SettingManager owns and manages its own dedicated QThread, aiming to make X11 setting operations thread-safe via QMetaObject::invokeMethod.
在 SettingManager 内部创建并管理专用 QThread，并尝试通过 QMetaObject::invokeMethod 让 XSettings/XResource 的 X11 设置操作具备线程安全性。

Changes:

SettingManager now optionally runs asynchronously on a dedicated thread and routes operations through queued/blocked invokeMethod calls.
SettingManager 现在可选地在专用线程中异步运行，并通过队列/阻塞的 invokeMethod 路由操作。
Removed external QThread ownership from Session/Helper and moved lifecycle responsibility into SettingManager.
从 Session/Helper 移除外部 QThread 管理，线程生命周期由 SettingManager 自行负责。
Added private async wrappers + slots to separate public API calls from thread-executed X11 operations.
增加私有异步包装与槽函数，将公开 API 与在线程中执行的 X11 操作分离。

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
`src/xsettings/settingmanager.h`	Adds async/thread fields and declares async helpers + slots to support threaded execution. / 增加异步/线程成员与相关接口声明以支持线程化执行。
`src/xsettings/settingmanager.cpp`	Implements internal `QThread` management and `invokeMethod` dispatch for XSettings/XResource operations. / 实现内部线程管理与基于 `invokeMethod` 的调用分发。
`src/seat/helper.h`	Removes `settingManagerThread` from `Session`. / 从 `Session` 移除 `settingManagerThread` 成员。
`src/seat/helper.cpp`	Drops external thread creation/teardown and calls `SettingManager` directly. / 删除外部线程创建/销毁逻辑，改为直接调用 `SettingManager`。

Comments suppressed due to low confidence (1)

src/seat/helper.cpp:193

EN: Session::~Session() deletes settingManager directly. After this PR, SettingManager may have thread affinity to its internal worker thread, so deleting it here risks cross-thread destruction. Prefer requesting shutdown and using deleteLater() from the manager thread (or make SettingManager self-own via finished -> deleteLater), then just clear the pointer here.
ZH: Session::~Session() 直接 delete settingManager。此 PR 后 SettingManager 可能隶属于其内部工作线程，在这里删除会有跨线程销毁风险。建议改为请求其关闭并在管理线程内 deleteLater()（或让 SettingManager 通过 finished -> deleteLater 自管理销毁），这里仅清空指针。

    if (settingManager) {
        delete settingManager;
        settingManager = nullptr;
    }

Copilot · 2026-01-28T07:18:02Z

src/xsettings/settingmanager.cpp

-    m_resource->setPropertyValue(XResource::toByteArray(XResource::Gtk_ThemeName), themeName);
-    m_settings->setPropertyValue(XSettings::toByteArray(XSettings::Gtk_ThemeName), themeName);
+    if (async()) {
+        setGTKThemeAsync(themeName);


EN: setGTKTheme() currently queues onSetGTKTheme() when async is enabled, but then still calls onSetGTKTheme() directly in the caller thread. That defeats the thread-safety goal and can also apply the change twice. Make the async path return (like the other setters) so X11 writes only happen on the dedicated thread.
ZH: 当启用 async 时，setGTKTheme() 已经把 onSetGTKTheme() 排队到线程里，但随后又在调用线程直接调用一次 onSetGTKTheme()，这会破坏线程安全目标并可能重复写入。请让 async 分支像其它 setter 一样 return，确保 X11 写入只在专用线程执行。

Suggested change

setGTKThemeAsync(themeName);

setGTKThemeAsync(themeName);

return;

Copilot · 2026-01-28T07:18:03Z

src/xsettings/settingmanager.cpp

-    m_resource->setPropertyValue(XResource::toByteArray(XResource::Gtk_FontName), name);
-    m_settings->setPropertyValue(XSettings::toByteArray(XSettings::Gtk_FontName), name);
+    if (async()) {
+        setFontAsync(name);


EN: Same issue as setGTKTheme(): setFont() queues the async call but then still performs onSetFont() synchronously on the caller thread. Add a return after setFontAsync(name) to keep XSettings/XResource operations confined to the manager thread.
ZH: 与 setGTKTheme() 相同：setFont() 在 async 模式下已排队调用，但仍在调用线程同步执行 onSetFont()。请在 setFontAsync(name) 后 return，保证 XSettings/XResource 操作只在管理线程内执行。

Suggested change

setFontAsync(name);

setFontAsync(name);

return;

Copilot · 2026-01-28T07:18:03Z

src/xsettings/settingmanager.cpp

+    QMetaObject::invokeMethod(this,
+                              "onSetGlobalScale",
+                              Qt::QueuedConnection,
+                              Q_ARG(int, scale));


EN: setGlobalScaleAsync() passes scale as Q_ARG(int, scale), which truncates the qreal and may also fail metatype matching for the qreal slot. Use Q_ARG(qreal, scale) (or the typed invokeMethod overload) so fractional DPR values are preserved.
ZH: setGlobalScaleAsync() 使用 Q_ARG(int, scale) 传参，会把 qreal 截断成整数，并且可能导致与 qreal 槽函数的元类型不匹配。请改为 Q_ARG(qreal, scale)（或使用带类型的 invokeMethod 重载），以保留小数 DPR 值。

Suggested change

Q_ARG(int, scale));

Q_ARG(qreal, scale));

Copilot · 2026-01-28T07:18:03Z

src/xsettings/settingmanager.cpp

+        const_cast<SettingManager *>(this),
+        "onGlobalScale",
+        Qt::BlockingQueuedConnection,
+        Q_RETURN_ARG(qreal, result)
+        );


EN: globalScaleAsync() invokes a non-existent method name ("onGlobalScale"). The declared getter slot is onGetGlobalScale(), so this call will fail at runtime and always return the default value. Update the invoked method name and consider using the function-pointer QMetaObject::invokeMethod overload to get compile-time checking.
ZH: globalScaleAsync() 调用了不存在的方法名（"onGlobalScale"）。实际声明的 getter 槽是 onGetGlobalScale()，因此这里运行时会失败并一直返回默认值。请修正调用的方法名，并考虑使用函数指针形式的 QMetaObject::invokeMethod 以获得编译期校验。

Copilot · 2026-01-28T07:18:03Z

src/xsettings/settingmanager.cpp

+    if (m_thread) {
+        m_thread->quit();
+        m_thread->wait();
+        delete m_thread;
+    }


EN: SettingManager is moved to m_thread, but its destructor can be triggered from another thread (e.g. Session::~Session() does delete settingManager). Destroying a QObject (and its children) from the wrong thread can cause Qt warnings/crashes. Consider ensuring destruction happens in the manager thread (e.g. expose a shutdown() that stops the thread via BlockingQueuedConnection and then deleteLater(), or connect m_thread->finished to this->deleteLater() and have callers only request shutdown).
ZH: SettingManager 被移动到 m_thread，但析构可能在其它线程触发（例如 Session::~Session() 直接 delete settingManager）。在错误线程销毁 QObject（及其子对象）可能产生 Qt 警告甚至崩溃。建议保证对象在管理线程中销毁（例如提供 shutdown()：通过 BlockingQueuedConnection 停止线程后再 deleteLater()；或把 m_thread->finished 连接到 this->deleteLater()，并让调用方只请求关闭）。

Copilot · 2026-01-28T07:18:04Z

src/xsettings/settingmanager.cpp

+    if (m_thread) {
+        m_thread->quit();
+        m_thread->wait();
+        delete m_thread;
+    }


EN: m_thread->wait() without a timeout can hang compositor shutdown indefinitely if the worker thread can't exit (e.g. stuck in an XCB call). Previously there was a bounded wait. Consider using a timeout (e.g. wait(QDeadlineTimer(...))) and logging/escalation on failure.
ZH: m_thread->wait() 没有超时，如果工作线程无法退出（例如卡在 XCB 调用中）会导致合成器关闭无限期卡住。此前这里是有等待上限的。建议增加超时（如 wait(QDeadlineTimer(...))），并在失败时记录日志/采取升级处理。

Copilot · 2026-01-28T07:18:04Z

src/xsettings/settingmanager.h

+    QString onGetCursorTheme() const;
+
+    void onSetCursorSize(qreal value);
+    qreal onGetcursorSize() const;


EN: Slot name onGetcursorSize() has inconsistent casing compared to the other getters (onGetGTKTheme, onGetFont, ...). Consider renaming to onGetCursorSize() and updating the invokeMethod call to match for consistency and readability.
ZH: 槽函数名 onGetcursorSize() 的大小写与其它 getter（onGetGTKTheme、onGetFont 等）不一致。建议重命名为 onGetCursorSize() 并同步更新 invokeMethod 调用，提升一致性与可读性。

Suggested change

qreal onGetcursorSize() const;

qreal onGetCursorSize() const;

sourcery-ai bot reviewed Jan 28, 2026

View reviewed changes

zzxyb requested a review from zccrs January 28, 2026 06:47

zzxyb force-pushed the xsettings branch from 1fcde30 to 23b7cd2 Compare January 28, 2026 07:09

zccrs reviewed Jan 28, 2026

View reviewed changes

zccrs requested a review from Copilot January 28, 2026 07:10

Copilot started reviewing on behalf of zccrs January 28, 2026 07:11 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

	setGTKThemeAsync(themeName);
	setGTKThemeAsync(themeName);
	return;

	qreal onGetcursorSize() const;
	qreal onGetCursorSize() const;

refactor: move XSettings operations to dedicated thread #714

Are you sure you want to change the base?

refactor: move XSettings operations to dedicated thread #714

Conversation

zzxyb commented Jan 28, 2026

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

deepin-ci-robot commented Jan 28, 2026

Uh oh!

zccrs left a comment

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

zccrs Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants