refactor(escape): Use the escape/unescape function from dtkcore to ensure consistency#352
Merged
Merged
Conversation
Reviewer's GuideRefactors the escaping/unescaping logic for D-Bus object paths and systemd-style application IDs to operate on raw UTF-8 bytes instead of truncated QChar cells, and adds comprehensive unit tests to verify correct UTF-8 handling, round-tripping, and spec compliance. 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
getObjectPathFromAppId,appIdis converted usingtoLocal8Bit()before passing toescapeToObjectPath, whereas the rest of the escaping/unescaping logic consistently uses UTF-8; consider switching this totoUtf8()to avoid locale-dependent behavior and to match the new UTF-8–based semantics. - The logging messages added for non-ASCII warnings build a long string from multiple literals without consistently including trailing spaces, resulting in messages like
for now,butandfuture.Please; it would be clearer to either use a single literal or ensure each fragment ends with the appropriate spacing. - The new overload set for
escapeToObjectPath(QStringView vs QByteArrayView) may be ambiguous for string literals like"graphical-session.target"; it would be safer to consistently pass eitherQStringView(e.g.u"..."_s) or explicitly constructedQByteArrayViewto make the intended overload selection unambiguous.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getObjectPathFromAppId`, `appId` is converted using `toLocal8Bit()` before passing to `escapeToObjectPath`, whereas the rest of the escaping/unescaping logic consistently uses UTF-8; consider switching this to `toUtf8()` to avoid locale-dependent behavior and to match the new UTF-8–based semantics.
- The logging messages added for non-ASCII warnings build a long string from multiple literals without consistently including trailing spaces, resulting in messages like `for now,but` and `future.Please`; it would be clearer to either use a single literal or ensure each fragment ends with the appropriate spacing.
- The new overload set for `escapeToObjectPath` (QStringView vs QByteArrayView) may be ambiguous for string literals like `"graphical-session.target"`; it would be safer to consistently pass either `QStringView` (e.g. `u"..."_s`) or explicitly constructed `QByteArrayView` to make the intended overload selection unambiguous.
## Individual Comments
### Comment 1
<location path="src/global.h" line_range="493-499" />
<code_context>
}
+ if (hasNonAscii) {
+ qCWarning(DDEAMUtils) << "String contains non-ASCII characters, which "
+ "does not conform to the DBus specification. A "
+ "compatible path is generated using UTF-8 byte escaping for now,"
+ "but this compatibility may be removed in the future."
+ "Please use ASCII-only names.";
+ }
</code_context>
<issue_to_address>
**nitpick (typo):** Warning message string concatenation misses spaces between some words.
Adjacent string literals are joined exactly as written, so these fragments produce `"for now,but"` and `"future.Please"` without spaces. Please add trailing/leading spaces to the relevant fragments so the final warning message reads correctly.
```suggestion
if (hasNonAscii) {
qCWarning(DDEAMUtils) << "String contains non-ASCII characters, which "
"does not conform to the DBus specification. A "
"compatible path is generated using UTF-8 byte escaping for now, "
"but this compatibility may be removed in the future. "
"Please use ASCII-only names.";
}
```
</issue_to_address>
### Comment 2
<location path="src/global.h" line_range="906-907" />
<code_context>
const auto &basePath = fromStaticRaw(DDEApplicationManager1ObjectPath);
if (!appId.isEmpty()) {
- return basePath % '/' % escapeToObjectPath(appId);
+ auto appIdBytes = appId.toLocal8Bit();
+ return basePath % '/' % escapeToObjectPath(appIdBytes);
}
</code_context>
<issue_to_address>
**question (bug_risk):** Question the use of toLocal8Bit here instead of a UTF-8 conversion for object path escaping.
`escapeToObjectPath(QStringView)` uses UTF‑8 (`str.toUtf8()`), but `getObjectPathFromAppId` now passes `appId.toLocal8Bit()` to the `QByteArrayView` overload. This makes the object path encoding locale‑dependent and inconsistent with the UTF‑8‑based escaping logic, and could produce different results on non‑UTF‑8 locales. If the app ID is meant to be UTF‑8, consider using `appId.toUtf8()` or the `QStringView` overload instead.
</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
+493
to
+499
| if (hasNonAscii) { | ||
| qCWarning(DDEAMUtils) << "String contains non-ASCII characters, which " | ||
| "does not conform to the DBus specification. A " | ||
| "compatible path is generated using UTF-8 byte escaping for now," | ||
| "but this compatibility may be removed in the future." | ||
| "Please use ASCII-only names."; | ||
| } |
There was a problem hiding this comment.
nitpick (typo): Warning message string concatenation misses spaces between some words.
Adjacent string literals are joined exactly as written, so these fragments produce "for now,but" and "future.Please" without spaces. Please add trailing/leading spaces to the relevant fragments so the final warning message reads correctly.
Suggested change
| if (hasNonAscii) { | |
| qCWarning(DDEAMUtils) << "String contains non-ASCII characters, which " | |
| "does not conform to the DBus specification. A " | |
| "compatible path is generated using UTF-8 byte escaping for now," | |
| "but this compatibility may be removed in the future." | |
| "Please use ASCII-only names."; | |
| } | |
| if (hasNonAscii) { | |
| qCWarning(DDEAMUtils) << "String contains non-ASCII characters, which " | |
| "does not conform to the DBus specification. A " | |
| "compatible path is generated using UTF-8 byte escaping for now, " | |
| "but this compatibility may be removed in the future. " | |
| "Please use ASCII-only names."; | |
| } |
Comment on lines
+906
to
+907
| auto appIdBytes = appId.toLocal8Bit(); | ||
| return basePath % '/' % escapeToObjectPath(appIdBytes); |
There was a problem hiding this comment.
question (bug_risk): Question the use of toLocal8Bit here instead of a UTF-8 conversion for object path escaping.
escapeToObjectPath(QStringView) uses UTF‑8 (str.toUtf8()), but getObjectPathFromAppId now passes appId.toLocal8Bit() to the QByteArrayView overload. This makes the object path encoding locale‑dependent and inconsistent with the UTF‑8‑based escaping logic, and could produce different results on non‑UTF‑8 locales. If the app ID is meant to be UTF‑8, consider using appId.toUtf8() or the QStringView overload instead.
BLumia
previously approved these changes
Apr 27, 2026
ComixHe
changed the title
BLumia
approved these changes
May 8, 2026
BLumia
requested changes
May 8, 2026
| namespace Detail { | ||
|
|
||
| inline bool isBaseAlnum(QChar ch) noexcept | ||
| inline bool isAlnum(char ch) noexcept |
|
TAG Bot New tag: 1.2.52 |
BLumia
previously approved these changes
May 9, 2026
…sure consistency Bug: PMS-357251 Signed-off-by: ComixHe <heyuming@deepin.org>
BLumia
approved these changes
May 9, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, ComixHe 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 |
deepin pr auto reviewGit Diff 代码审查报告总体评估这次修改主要将应用程序ID和对象路径的转义/反转义功能从本地实现迁移到Dtk6::Core库(DUtil)，并添加了全面的单元测试。整体改动方向正确，符合代码复用原则，但存在一些需要改进的地方。 详细审查1. 语法与逻辑1.1 头文件包含#include <DUtil>问题：头文件包含方式不规范
1.2 escapeApplicationId函数实现inline QString escapeApplicationId(QByteArrayView utf8)问题：函数签名与调用方式不一致
1.3 unescapeToBytes函数inline QByteArray unescapeToBytes(const QStringView str, const QStringView prefix) noexcept问题：边界条件处理
2. 代码质量2.1 魔法数字result.reserve(utf8.size() * 4); // Worst case: every byte needs "\xxx" (4 chars)建议：
2.2 警告信息qCWarning(DDEAMUtils) << "String contains non-ASCII characters...";问题：
2.3 函数命名inline QString escapeApplicationId(QByteArrayView utf8)建议：
3. 代码性能3.1 字符串转换return escapeApplicationId(str.toUtf8());问题：
3.2 内存分配result.reserve(utf8.size() * 4);建议：
3.3 字符串操作result.append(uR"(\x)"_s);
result.append(QString::number(byte, 16).rightJustified(2, u'0').toLower());问题：
4. 代码安全4.1 类型转换auto byte = static_cast<unsigned char>(utf8.at(i));问题：
4.2 十六进制转换auto byte = static_cast<unsigned char>(str.mid(i + prefixLen, 2).toUShort(&ok, 16));问题：
4.3 非ASCII字符处理if (byte > 0x7F) {
hasNonAscii = true;
}问题：
5. 单元测试5.1 测试覆盖优点：
建议：
5.2 测试用例{"test\x01", "test\\x01", "Control character should be escaped"},问题：
6. 依赖管理6.1 版本要求问题：
改进建议
总结这次修改整体方向正确，将通用功能迁移到DTK库中，提高了代码复用性。主要改进点应集中在：
建议在合并前进行充分的集成测试，确保与DTK库的交互符合预期。 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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
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.
Bug: PMS-357251
Summary by Sourcery
Add UTF-8 byte-based escaping for object paths and application IDs to support non-ASCII names while remaining compatible with D-Bus and systemd requirements.
Enhancements:
Tests: