[klipper] [Bug 390375] New: Klipper notifications visually broken since plasma 5.12

[Till Schäfer]

https://bugs.kde.org/show_bug.cgi?id=390375

Bug ID: 390375
Summary: Klipper notifications visually broken since plasma
5.12
Product: klipper
Version: 5.12.0
Platform: Gentoo Packages
OS: Linux
Status: UNCONFIRMED
Severity: normal
Priority: NOR
Component: general
Assignee: ***@kde.org
Reporter: ***@uni-dortmund.de
Target Milestone: ---

Created attachment 110607
--> https://bugs.kde.org/attachment.cgi?id=110607&action=edit
screenshot of visually broken notification

After upgrading to plasma 5.12, the klipper notifications, which are issued
while changing the clipboard content to the next/previous history item are
visually broken. It seems that the newlines and the horizontal alignment are
gone and all text is just written in one line.

See the attached screenshot.

[Martin Flöser]

https://bugs.kde.org/show_bug.cgi?id=390375

Martin Flöser <***@kde.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |1.0
CC| |plasma-***@kde.org
Component|general |Notifications
Assignee|***@kde.org |***@privat.broulik.de
Product|klipper |plasmashell

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=390375

David Edmundson <***@davidedmundson.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@davidedmundson.co.uk

--- Comment #1 from David Edmundson <***@davidedmundson.co.uk> ---
Klipper is sending a table as text. See Klipper::cycleText

They aren't HTML tags that were ever permissable in the notification
specification. Klipper is in the wrong.

Since 5.12 we filter to only the tags that are allowed.

From a security POV, we could re-allow the table with the attribute filtering
we now have and it'd be safe. But I don't know if I want to.

[Till Schäfer]

https://bugs.kde.org/show_bug.cgi?id=390375

Till Schäfer <***@uni-dortmund.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Ever confirmed|0 |1
Status|UNCONFIRMED |CONFIRMED

--- Comment #2 from Till Schäfer <***@uni-dortmund.de> ---
do we have a classical "diffusion of responsibility" situation here? :-)

If the table does not introduce any security holes, are there any reasons to
not allow it? Maybe the notification specification should be updated
accordingly then...

Nevertheless, either way should be OK, i.g., fixing Klipper or Plasma, from my
POV.

[Achilleas Koutsou]

https://bugs.kde.org/show_bug.cgi?id=390375

Achilleas Koutsou <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

[Till Schäfer]

https://bugs.kde.org/show_bug.cgi?id=390375

--- Comment #3 from Till Schäfer <***@uni-dortmund.de> ---
Can you point me to a notification specification for KDE? I will try to fix
that bug myself and create a phabricator ticket.

I only found this document [1], but it seems rather outdated. The gnome
documentation [2] only mentions markdown that modifies the font appearance,
creates links or embeds an image. However, there is nothing to actually formal
the layout. So it would be nice to get some hint, if there are other
possibilities to layout a notification.

[1] https://community.kde.org/Plasma/Notifications
[2] https://developer.gnome.org/notification-spec/

[David Edmundson]

https://bugs.kde.org/show_bug.cgi?id=390375

--- Comment #4 from David Edmundson <***@davidedmundson.co.uk> ---
We tend to follow [2]

However, there is nothing to actually formal the layout

Yes, there's nothing, which is why klipper's attempts to use something doesn't
work and gets stripped out.

If we want to add support in our notifications just modify
notificationsanitizer.cpp:49 allowedTags = {..} to include the 3 releavant
table ones
But then we're going off spec.

[Till Schäfer]

https://bugs.kde.org/show_bug.cgi?id=390375

--- Comment #5 from Till Schäfer <***@uni-dortmund.de> ---
created phabricator ticket [1] and added you as reviewer (if that's alright
:-))

[1] https://phabricator.kde.org/D11754

[Kai Uwe Broulik]

https://bugs.kde.org/show_bug.cgi?id=390375

Kai Uwe Broulik <***@privat.broulik.de> changed:

What |Removed |Added
----------------------------------------------------------------------------
Version Fixed In| |5.12.5
Status|CONFIRMED |RESOLVED
Resolution|--- |FIXED
Latest Commit| |https://commits.kde.org/pla
| |sma-workspace/896950a819ea7
| |6800007cad7393adacac7f9bf20

--- Comment #6 from Kai Uwe Broulik <***@privat.broulik.de> ---
Git commit 896950a819ea76800007cad7393adacac7f9bf20 by Kai Uwe Broulik, on
behalf of Till Schäfer.
Committed on 28/03/2018 at 10:11.
Pushed by broulik into branch 'Plasma/5.12'.

fix: Klipper notifications visually broken since plasma 5.12
FIXED-IN: 5.12.5

Differential Revision: https://phabricator.kde.org/D11754

M +1 -1 dataengines/notifications/notificationsanitizer.cpp
M +3 -1 dataengines/notifications/notificationsanitizer.h

https://commits.kde.org/plasma-workspace/896950a819ea76800007cad7393adacac7f9bf20