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

[Mirco Müller]

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.

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:~macslow/unity-notifications/fix-1335787]

216. By Mirco Müller on 2014-07-18

Merged with trunk and resolved conflicts.

[PS Jenkins bot (ps-jenkins)]

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)

[PS Jenkins bot (ps-jenkins)]

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)

[Michał Sawicz (saviq)]

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

[Mirco Müller (macslow)]

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.

[Mirco Müller (macslow)]

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:~macslow/unity-notifications/fix-1335787]

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.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:217
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/72/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-amd64-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-i386-ci/13/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:219
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/74/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-amd64-ci/15/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-armhf-ci/15/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-utopic-i386-ci/15/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

See inline

[lp:~macslow/unity-notifications/fix-1335787]

220. By Mirco Müller on 2014-09-10

Use tag-names for text-filter unit-tests.

[Michał Sawicz (saviq)]

Yup.

review: Approve

=== modified file 'include/Notification.h'
--- include/Notification.h	2014-07-14 13:54:41 +0000
+++ include/Notification.h	2014-09-10 15:07:11 +0000
@@ -118,6 +118,9 @@
     Q_INVOKABLE void close();
 
     bool operator<(const Notification &n) const; // Order by "interestingness".
+
+private:
+    QString filterText(const QString& text);
 };
 
 #endif /* NOTIFICATION_HPP_ */

=== modified file 'src/Notification.cpp'
--- src/Notification.cpp	2014-07-21 16:59:41 +0000
+++ src/Notification.cpp	2014-09-10 15:07:11 +0000
@@ -21,6 +21,7 @@
 #include "NotificationServer.h"
 #include "Notification.h"
 #include <string>
+#include <QTextDocument>
 
 using namespace std;
 
@@ -81,9 +82,10 @@
 }
 
 void Notification::setBody(const QString &text) {
-    if(p->body != text) {
-        p->body = text;
-        Q_EMIT bodyChanged(text);
+    QString filtered = filterText(text);
+    if(p->body != filtered) {
+        p->body = filtered;
+        Q_EMIT bodyChanged(p->body);
         Q_EMIT dataChanged(p->id);
     }
 }
@@ -153,8 +155,9 @@
 }
 
 void Notification::setSummary(const QString &summary) {
-    if(p->summary != summary) {
-        p->summary = summary;
+    QString filtered = filterText(summary);
+    if(p->summary != filtered) {
+        p->summary = filtered;
         Q_EMIT summaryChanged(p->summary);
         Q_EMIT dataChanged(p->id);
     }
@@ -248,3 +251,9 @@
 void Notification::close() {
     Q_EMIT completed(p->id);
 }
+
+QString Notification::filterText(const QString& text) {
+    QTextDocument filtered;
+    filtered.setHtml(text);
+    return filtered.toPlainText();
+}

=== modified file 'test/notificationtest.cpp'
--- test/notificationtest.cpp	2014-02-18 14:51:44 +0000
+++ test/notificationtest.cpp	2014-09-10 15:07:11 +0000
@@ -16,6 +16,8 @@
         void testVisualSDQueueWithoutCritical();
         void testVisualSDQueueWithCritical();
         void testVisualSDQueueMax();
+        void testTextFilter();
+        void testTextFilter_data();
 };
 
 void TestNotifications::testSimpleInsertion() {
@@ -179,5 +181,50 @@
     QCOMPARE(m.getDisplayedNotification(1)->getBody(), QString("snap-decision-4"));
 }
 
+void TestNotifications::testTextFilter_data() {
+    QTest::addColumn<QString>("string");
+    QTest::addColumn<QString>("result");
+
+    QTest::newRow("URL") << "<a href=\"http://www.ubuntu.com/\">Ubuntu</a>" << "Ubuntu";
+    QTest::newRow("as is") << "Don't rock the boat" <<                         "Don't rock the boat";
+    QTest::newRow("italic") << "<i>Not italic</i>" <<                          "Not italic";
+    QTest::newRow("unterline") << "<u>Test</u>" <<                             "Test";
+    QTest::newRow("bold") << "<b>Bold</b>" <<                                  "Bold";
+    QTest::newRow("span") << "<span>Span</span>" <<                            "Span";
+    QTest::newRow("s-tag") << "<s>E-flat</s>" <<                               "E-flat";
+    QTest::newRow("sub") << "<sub>Sandwich</sub>" <<                           "Sandwich";
+    QTest::newRow("small") << "<small>Fry</small>" <<                          "Fry";
+    QTest::newRow("tt") << "<tt>Testing tag</tt>" <<                           "Testing tag";
+    QTest::newRow("html") << "<html>Surrounded by html</html>" <<              "Surrounded by html";
+    QTest::newRow("qt") << "<qt>Surrounded by qt</qt>" <<                      "Surrounded by qt";
+    QTest::newRow("quotes") << "\"Film spectators are quiet vampires.\"" <<    "\"Film spectators are quiet vampires.\"";
+    QTest::newRow("> 1/4") << "7 > 3" <<                                       "7 > 3";
+    QTest::newRow("> 2/4") << "7 &gt; 3" <<                                    "7 > 3";
+    QTest::newRow("> 3/4") << "7 &#62; 3" <<                                   "7 > 3";
+    QTest::newRow("> 4/4") << "7 &#x3e; 3" <<                                  "7 > 3";
+    QTest::newRow("< 1/3") << "14 &lt; 42" <<                                  "14 < 42";
+    QTest::newRow("< 2/3") << "14 &#60; 42" <<                                 "14 < 42";
+    QTest::newRow("< 3/3") << "14 &#x3c; 42" <<                                "14 < 42";
+    QTest::newRow("& 1/4") << "War & Peace" <<                                 "War & Peace";
+    QTest::newRow("& 2/4") << "Law &#38; Order" <<                             "Law & Order";
+    QTest::newRow("& 3/4") << "Love &#x26; War" <<                             "Love & War";
+    QTest::newRow("& 4/4") << "Peace &amp; Love" <<                            "Peace & Love";
+    QTest::newRow("apostrophe") << "Kick him while he&apos;s down" <<          "Kick him while he's down";
+    QTest::newRow("mixed tags") << "<b>So broken</i>" <<                       "So broken";
+}
+
+void TestNotifications::testTextFilter() {
+    const int timeout = 1000;
+
+    Notification n(new Notification(1, timeout, Notification::Low, "notification", Notification::Ephemeral));
+
+    QFETCH(QString, string);
+    QFETCH(QString, result);
+    n.setSummary(string);
+    QCOMPARE(n.getSummary(), result);
+    n.setBody(string);
+    QCOMPARE(n.getBody(), result);
+}
+
 QTEST_MAIN(TestNotifications)
 #include "notificationtest.moc"