-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Checking for GNOME before making HTML escapes in title of desktop notification #28936
base: dev
Are you sure you want to change the base?
Conversation
: u"<b>%1</b>\n%2"_q.arg( | ||
subtitle.toHtmlEscaped(), | ||
text.toHtmlEscaped()).toStdString(); | ||
if (qEnvironmentVariable("XDG_CURRENT_DESKTOP").toLower().contains("gnome")) { |
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.
This doesn't look correct, there's already HasCapability("body-markup")
check for this, you're duplicating the code path
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.
https://specifications.freedesktop.org/notification-spec/latest/protocol.html
"body-markup" Supports markup in the body text. If marked up text is sent to a server that does not give this cap, the markup will show through as regular text so must be stripped clientside.
: subtitle.toStdString() + " (" + title.toStdString() + ')'; | ||
|
||
_body = text.toStdString(); | ||
? title.toPlainText().toStdString() |
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.
Again, this doesn't look correct, I don't see toPlainText in QString documentation
subtitle.toHtmlEscaped(), | ||
msg.toHtmlEscaped()).toStdString(); | ||
} | ||
if (HasCapability("body-markup") && !qEnvironmentVariable("XDG_CURRENT_DESKTOP").toLower().contains("gnome")) { |
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.
This still doesn't look correct. There's already HasCapability("body-markup")
, you don't need any XDG_CURRENT_DESKTOP checks
Hello, I have the same issue as here (27940). Explanation: GNOME does not support HTML formatting in desktop notifications. I'm not sure that this fragment of code can fix this, but you can try.