-
Notifications
You must be signed in to change notification settings - Fork 55
fix: unescape quotes in Bluetooth device notification #1365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds unescaping of single and double quote escape sequences in notification body text to correctly display Bluetooth device names containing quotes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider clarifying in a comment or helper function that the order of replacements (backslashes first, then quotes) is intentional and safe, since inputs like
\\"will now collapse to"and may not always reflect the original intent. - If this unescaping is specific to %q-formatted Bluetooth names, it might be safer to gate it behind a more targeted condition or helper function so that other callers of
Notifywith legitimate backslash-escaped content are not unintentionally altered.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider clarifying in a comment or helper function that the order of replacements (backslashes first, then quotes) is intentional and safe, since inputs like `\\"` will now collapse to `"` and may not always reflect the original intent.
- If this unescaping is specific to %q-formatted Bluetooth names, it might be safer to gate it behind a more targeted condition or helper function so that other callers of `Notify` with legitimate backslash-escaped content are not unintentionally altered.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Added unescaping for single and double quotes in notification body text. Log: resolve notification display issue for Bluetooth device names with quotes pms: BUG-342911
deepin pr auto review我来帮你分析这段代码的改进意见:
strBody.replace(QLatin1String("\\\\"), QLatin1String("\\"), Qt::CaseInsensitive);新代码使用正则表达式: strBody.replace(QRegularExpression("\\\\(\\\\|['\"])"), "\\1");这个改进是合理的,因为:
// 建议的改进版本
static const QRegularExpression escapePattern("\\\\(\\\\|['\"])");
uint NotificationManager::Notify(const QString &appName, uint replacesId, const QString &appIcon,
const QString &summary, const QString &body, ...)
{
if (body.isEmpty()) {
qWarning() << "Empty notification body received";
return 0;
}
QString strBody = body;
// Unescape backslashes and quotes from %q formatted strings
auto match = escapePattern.match(strBody);
if (match.hasMatch()) {
strBody.replace(escapePattern, "\\1");
}
// ... 其余代码
}这个改进版本:
总的来说,这个改动是正向的,主要改进了字符串处理的完整性,但还可以在性能和安全性方面做进一步优化。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, wyu71 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 |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
Log: resolve notification display issue for Bluetooth device names with quotes
pms: BUG-342911
Summary by Sourcery
Bug Fixes: