unity-notifications

Merge lp://staging/~macslow/unity-notifications/fix-1335787 into lp://staging/unity-notifications

fix-1335787
Merge into trunk

Proposed by Mirco Müller on 2014-07-18

Status:

Merged

Approved by:

Michał Sawicz on 2014-09-10

Approved revision:

220

Merged at revision:

217

Proposed branch:

lp://staging/~macslow/unity-notifications/fix-1335787

Merge into:

lp://staging/unity-notifications

Diff against target:

125 lines (+64/-5)

3 files modified

include/Notification.h (+3/-0)
src/Notification.cpp (+14/-5)
test/notificationtest.cpp (+47/-0)

To merge this branch:

bzr merge lp://staging/~macslow/unity-notifications/fix-1335787

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michał Sawicz		2014-07-18	Approve on 2014-09-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-08-19
Review via email: mp+227334@code.staging.launchpad.net

Commit message

Added an internal text-filter to setSummary() and setBody()

It strips or converts HTML and other markup-text, thus no special formatting happens during rendering via QML in the Notification-renderer.

Description of the change

Added an internal text-filter to setSummary() and setBody() (with a corresponding unit-test) which strips or converts HTML and other markup-text, thus no special formatting happens during rendering via QML in the Notification-renderer.

* Are there any related MPs required for this MP to build/function as expected? Please list.
Yes. It works optimal in tandem with lp:~macslow/unity8/fix-1335787

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* Did you make sure that your branch does not contain spurious tags?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

* If you changed the UI, has there been a design review?
Not applicable.

lp://staging/~macslow/unity-notifications/fix-1335787 updated on 2014-07-18

216. By Mirco Müller on 2014-07-18: Merged with trunk and resolved conflicts.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-18:

FAILED: Continuous integration, rev:215
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/67/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-amd64-ci/8/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/8
        deb: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/8/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-i386-ci/8/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/67/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-18:

PASSED: Continuous integration, rev:216
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/69/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-i386-ci/10

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/69/rebuild

review: Approve (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-08-05:

I'm really not sure this kind of filtering belongs here :|

Why don't we just require the consumers to send us unescaped strings? UI-side we can ignore tags by using http://qt-project.org/doc/qt-5/qml-qtquick-text.html#textFormat-prop, forcing it to PlainText, which will ignore tags and such.

review: Needs Information

Revision history for this message

Mirco Müller (macslow) wrote on 2014-08-05:

Using...

textFormat: Text.PlainText

... for the summary- and body-text (in the renderer/unity8) does prevent any inlined formatting tags from being applied or URLs to be highlighted, it does not strip any of the tags-text. The only issue I see with using the Text.PlainText approach, is that no single app (neither core nor 3rd-party) knows about this and probably sends "tag-polluted" text to the notifications. Thus it will make notifications look bad and easily exceed the new two-line limit Design wants to see enforced for body-text.

Regarding special HTML-characters like...

< < < > > > & & ' &

... I'm not sure how and where to make use of the C++-class QTextDocument. I would like to leave that within the responsibility of the sending app too, since we already demand tag-free text with the Text.PlainText approach.

Revision history for this message

Mirco Müller (macslow) wrote on 2014-08-08:

Of the initial 33 text-filter tests these 7 fail when using QTextDocument...

"<img src=\"foobar.png\" />Nothing to see" -> "Nothing to see"
"><", -> "><"
"<>", -> "<>"
"< this is not a tag >" -> "< this is not a tag >"
"14 < 42" -> "14 < 42"
"First line <br dumb> \r \n Second line" -> "First line\nSecond line"
"First line\n<br /> <br>\n2nd line\r\n3rd line" -> "First line\n2nd line\n3rd line"

lp://staging/~macslow/unity-notifications/fix-1335787 updated on 2014-08-08

217. By Mirco Müller on 2014-08-08: Updated the text-filter to only use QTextDocument, thus had to skip 7 of the initial 33 test-cases.
218. By Mirco Müller on 2014-08-08: Use the data-driven approach for the text-filter test.
219. By Mirco Müller on 2014-08-08: Oops.