fix: skip in-use font when disabling via D-Bus StandardFont query#197
Open
JWWTSL wants to merge 1 commit into
Open
fix: skip in-use font when disabling via D-Bus StandardFont query#197JWWTSL wants to merge 1 commit into
JWWTSL wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JWWTSL 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 |
Reviewer's GuideAdds D-Bus querying of the DDE Appearance1 StandardFont to detect the system font set via dde-control-center, and updates the disable-logic so currently in-use fonts (application font or DDE standard font) are skipped during batch disable while improving the warning messaging logic. Sequence diagram for batch font disable with D-Bus StandardFont checksequenceDiagram
actor User
participant DFontPreviewListView
participant DDE_Appearance1
participant DMessageManager
User ->> DFontPreviewListView: onEnableBtnClicked(itemIndexes, setValue)
DFontPreviewListView ->> DDE_Appearance1: QDBusInterface.call(Get, StandardFont)
DDE_Appearance1 -->> DFontPreviewListView: QDBusReply(StandardFont)
loop for each font in itemIndexes
alt setValue is false
DFontPreviewListView ->> DFontPreviewListView: check in-use
alt [font is in use (app font or StandardFont)]
DFontPreviewListView ->> DFontPreviewListView: increment skippedInUse
else [font not in use]
alt [font is system font]
DFontPreviewListView ->> DFontPreviewListView: set needShowTips
else [normal font]
DFontPreviewListView ->> DFontPreviewListView: disableFont(filePath)
end
end
else setValue is true
DFontPreviewListView ->> DFontPreviewListView: enableFont(filePath)
end
end
alt needShowTips or count > 0
DFontPreviewListView ->> DMessageManager: sendMessage(parentWidget, icon, message)
end
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:
- The D-Bus query for
StandardFontis duplicated in bothonEnableBtnClickedandselectedFonts; consider extracting it into a small helper (or caching the result) to avoid repetition and keep the logic in sync. - The synchronous D-Bus call on the session bus is done directly in UI-related code; if this call can be slow or fail, consider adding basic error/timeout handling or moving it off the hot path to avoid potential UI stalls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The D-Bus query for `StandardFont` is duplicated in both `onEnableBtnClicked` and `selectedFonts`; consider extracting it into a small helper (or caching the result) to avoid repetition and keep the logic in sync.
- The synchronous D-Bus call on the session bus is done directly in UI-related code; if this call can be slow or fail, consider adding basic error/timeout handling or moving it off the hot path to avoid potential UI stalls.
## Individual Comments
### Comment 1
<location path="deepin-font-manager/interfaces/dfontpreviewlistview.cpp" line_range="1966-1975" />
<code_context>
QMutexLocker locker(&m_mutex);
QString fontName;
+ // 通过 D-Bus 查询 DDE 外观服务设置的当前系统字体
+ QString ddeStandardFont;
+ {
+ QDBusInterface iface(QStringLiteral("org.deepin.dde.Appearance1"),
+ QStringLiteral("/org/deepin/dde/Appearance1"),
+ QStringLiteral("org.freedesktop.DBus.Properties"),
+ QDBusConnection::sessionBus());
+ QDBusReply<QVariant> reply = iface.call(QStringLiteral("Get"),
+ QStringLiteral("org.deepin.dde.Appearance1"),
+ QStringLiteral("StandardFont"));
+ if (reply.isValid())
+ ddeStandardFont = reply.value().toString();
+ }
</code_context>
<issue_to_address>
**issue (performance):** Avoid performing the synchronous D-Bus call while holding m_mutex to prevent UI stalls and lock contention.
In `onEnableBtnClicked`, the D-Bus call runs while `m_mutex` is held via `QMutexLocker`, so a slow or blocked IPC call will stall any code needing this mutex and hurt UI responsiveness. Please move the D-Bus query outside the locked section, or shrink the critical section so only the truly shared state access is protected by `m_mutex`.
</issue_to_address>
### Comment 2
<location path="deepin-font-manager/interfaces/dfontpreviewlistview.cpp" line_range="2659-2667" />
<code_context>
+ (itemData.fontInfo.familyName == qApp->font().family()) ||
+ (DFontInfoManager::instance()->getFontFamilyStyle(itemData.fontInfo.filePath)
+ .contains(qApp->font().family())) ||
+ (!ddeStandardFont.isEmpty() &&
+ (itemData.fontInfo.familyName == ddeStandardFont ||
+ DFontInfoManager::instance()->getFontFamilyStyle(itemData.fontInfo.filePath)
+ .contains(ddeStandardFont)))) {
// qDebug() << "Calculate current font";
</code_context>
<issue_to_address>
**suggestion (performance):** Cache the result of getFontFamilyStyle in selectedFonts to avoid redundant work.
`DFontInfoManager::instance()->getFontFamilyStyle(itemData.fontInfo.filePath)` is evaluated twice in this condition and may be relatively expensive. Please store the result in a local `QStringList` (as in `onEnableBtnClicked`) and reuse it for both the app font and `ddeStandardFont` checks.
```suggestion
} else {
const QStringList selectedFonts = DFontInfoManager::instance()->getFontFamilyStyle(itemData.fontInfo.filePath);
if ((itemData.fontData == m_curFontData) ||
(itemData.fontInfo.filePath == m_dataThread->getFontData(m_curFontData).fontInfo.filePath) ||
(itemData.fontInfo.familyName == qApp->font().family()) ||
selectedFonts.contains(qApp->font().family()) ||
(!ddeStandardFont.isEmpty() &&
(itemData.fontInfo.familyName == ddeStandardFont ||
selectedFonts.contains(ddeStandardFont)))) {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JWWTSL
force-pushed
the
bug-347233
branch
3 times, most recently
from
12448c1 to
eebd341
Compare
lzwind
reviewed
May 14, 2026
| // 通过 D-Bus 查询 DDE 外观服务设置的当前系统字体 | ||
| QString ddeStandardFont; | ||
| { | ||
| QDBusInterface iface(QStringLiteral("org.deepin.dde.Appearance1"), |
log: Query org.deepin.dde.Appearance1 StandardFont property via D-Bus to detect the system font set through dde-control-center. Skip currently in-use user fonts during multi-select disable and show the font name in the warning message. pms: bug-347233
deepin pr auto review你好！我是CodeGeeX，很高兴为你审查这段代码。 这段代码的主要目的是在禁用字体时，增加对“正在使用中的字体”的保护逻辑，防止用户禁用当前系统界面正在使用的字体（如应用字体、DDE外观标准字体等），并优化了相关的提示信息。同时，修改了底层的 FontConfig 查询逻辑，使其按文件路径精确匹配。 整体来看，代码的业务逻辑是清晰的，但在代码性能、语法健壮性、代码安全和可维护性方面，我有以下改进建议： 1. 代码性能问题：D-Bus 同步调用阻塞主线程 改进意见：
2. 语法逻辑问题一： 改进意见：
问题二：大括号嵌套错位（潜在 Bug） + } else {
+ const QStringList fontFamilies = ...
+ if (...) {
// 旧代码：qDebug() << "Calculate current font";
// 旧代码：选中字体与当前使用的字体为同一字体文件
@@ -2632,6 +2704,7 @@ void DFontPreviewListView::selectedFonts(const DFontPreviewItemData &curData,
*allIndexList << index;
}
}
+ }
}这里你将原本的 问题三： if (curCnt > 0) {
if (skippedInUse == 0) {
skippedInUse = curCnt;
}
needShowTips = true;
}
改进意见：
3. 代码安全问题一：C 风格字符串转换缺乏长度保护 FcPatternAddString(pat, FC_FILE, reinterpret_cast<const FcChar8 *>(filePah.toUtf8().data()));
改进意见：
问题二：拼写错误 4. 综合改进后的代码示例针对以上问题，我为你提供了修改后的核心代码片段： 1. 提取并优化 D-Bus 调用 (dfontpreviewlistview.cpp) // 提取为私有成员函数，并考虑未来改为异步+缓存机制
QString DFontPreviewListView::getDDEStandardFont() const
{
QDBusInterface iface(QStringLiteral("org.deepin.dde.Appearance1"),
QStringLiteral("/org/deepin/dde/Appearance1"),
QStringLiteral("org.freedesktop.DBus.Properties"),
QDBusConnection::sessionBus());
// 建议后续优化为 asyncCall
QDBusReply<QVariant> reply = iface.call(QStringLiteral("Get"),
QStringLiteral("org.deepin.dde.Appearance1"),
QStringLiteral("StandardFont"));
if (reply.isValid()) {
return reply.value().toString();
}
return QString();
}2. 修复 FontConfig 安全与精确匹配问题 (dfontinfomanager.cpp) QStringList DFontInfoManager::getFontFamilyStyle(const QString &filePath)
{
const FcChar8 *format = reinterpret_cast<const FcChar8 *>("%{=fclist}");
FcObjectSet *os = FcObjectSetBuild(FC_FAMILY, FC_STYLE, FC_FILE, nullptr);
FcPattern *pat = FcPatternCreate();
// 修复：保存 QByteArray 避免悬空指针，修正拼写 filePath
const QByteArray utf8Path = filePath.toUtf8();
FcPatternAddString(pat, FC_FILE, reinterpret_cast<const FcChar8 *>(utf8Path.constData()));
// 增加：确保 FC_FILE 是精确匹配，而不是模糊匹配
FcPatternAddBool(pat, FC_FILE, FcTrue);
FcFontSet *fs = FcFontList(nullptr, pat, os);
// ... 后续逻辑
}3. 统一判断逻辑并修复括号嵌套 (dfontpreviewlistview.cpp) // 在 DFontPreviewListView 中添加统一的判断方法
bool DFontPreviewListView::isFontInUse(const FontData &fontData, const QString &appFontFamily, const QString &ddeStandardFont) const
{
QStringList fontFamilies = DFontInfoManager::instance()->getFontFamilyStyle(fontData.fontInfo.filePath);
bool isInUse = (fontData == m_curFontData) ||
(fontData.fontInfo.filePath == m_dataThread->getFontData(m_curFontData).fontInfo.filePath) ||
(fontData.fontInfo.familyName == appFontFamily) ||
fontFamilies.contains(appFontFamily) ||
(!ddeStandardFont.isEmpty() && fontData.fontInfo.familyName == ddeStandardFont) ||
(!ddeStandardFont.isEmpty() && fontFamilies.contains(ddeStandardFont));
return isInUse;
}
// 在 selectedFonts 和 onEnableBtnClicked 中调用：
// 例如 selectedFonts 中：
QString ddeStandardFont = getDDEStandardFont();
QString appFontFamily = qApp->font().family();
// ...
} else {
if (isFontInUse(itemData, appFontFamily, ddeStandardFont)) {
// 处理正在使用的逻辑
}
} // 确保这里的括号与外层 for 循环正确对齐希望这些审查意见对你有所帮助！如果有任何疑问，欢迎随时提问。 |
|
你好 ， 邮件已收到谢谢
|
Contributor
|
TAG Bot New tag: 6.5.38 |
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
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.
log: Query org.deepin.dde.Appearance1 StandardFont property via D-Bus to detect the system font set through dde-control-center. Skip currently in-use user fonts during multi-select disable and show the font name in the warning message.
pms: bug-347233
Summary by Sourcery
Prevent disabling fonts that are currently in use and align user warnings with the detected active system font.
Bug Fixes:
Enhancements: