[Notifications] Fix grouping
ClosedPublic

Authored by broulik on Feb 5 2018, 8:25 AM.

Details

Summary

Sanitize the body before doing anything else.
Cleanup grouping logic.

Test Plan

Unit test still passes

notify-send "Test" "Hello"; notify-send "Test" "Hello"

resulted in a single popup with "Test" summary and body

Hello

notify-send "Test" "Hello"; notify-send "Test" "Darkness"

resulted in a single popup with "Test" summary and body

Hello
Darkness

(previously the notification would become empty)
Verified that non-whitelisted tags like <h1> were still filtered out in both cases

notify-send "Test" "Hello"; notify-send "Test2" "Hello"

resulted in two separate popups. Also couldn't observe any "notification contained invalid XML" warnings on console.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Feb 5 2018, 8:25 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 5 2018, 8:25 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Feb 5 2018, 8:25 AM
fvogt added inline comments.Feb 5 2018, 8:35 AM
dataengines/notifications/notificationsengine.cpp
250

I don't like magic numbers, maybe sizeof("<?xml version="1.0"><html></html>")-1?

broulik updated this revision to Diff 26561.Feb 5 2018, 8:39 AM
  • Get rid of magic numbers

Note that the unit test doesn't touch any of the code you modified, it passing isn't a sign of anything.

Does 5.8 need fixing?

dataengines/notifications/notificationsengine.cpp
250

strlen not sizeof

fvogt added inline comments.Feb 5 2018, 8:54 AM
dataengines/notifications/notificationsengine.cpp
214

This results in ...Text<br/><?xml version="1.0"?><html>Some text... AFAICT.

This also means that the if condition is always false.

fvogt added inline comments.Feb 5 2018, 9:12 AM
dataengines/notifications/notificationsengine.cpp
214

According to @broulik the first part is expected, which IMO needs a comment as it does very much look like a bug.

The latter part is false though.

broulik updated this revision to Diff 26570.Feb 5 2018, 12:39 PM
  • Add comment
  • Use strlen
davidedmundson accepted this revision.Feb 5 2018, 12:40 PM
This revision is now accepted and ready to land.Feb 5 2018, 12:40 PM
This revision was automatically updated to reflect the committed changes.