Notifications: Display the link in the subject line. #6240

dragotin · 2017-12-13T21:12:26Z

Added a new data called linkText. If the link in the activity JSON has a space in it, the part before that is considered the link text. If it does not contain a space, the word [link] is used.

Note that the idea to separate the link and the link text with a space needed because the original specification does not contain a link text, just the link. I am not seeing a downside as long as spaces in the real link are encoded with %20.

This is may be a solution for #6236

dragotin · 2017-12-13T21:22:25Z

DISCLAIMER: I did not have a ownCloud available that can do notifications, so this is PR is untested. Thus label "DO NOT MERGE YET" with the kind request to review/comment and maybe test.

ogoffart · 2017-12-14T09:00:05Z

src/gui/servernotificationhandler.cpp

+            // is rendered as a link text
+            if( s.contains(QChar(' '))) {
+                const QStringList li = s.split(QChar(' '));
+                a._link = QUrl::fromEncoded(li.at(1).toLocal8Bit());


ToLatin1 or toUtf8.

toLocal8bit is for path on the filesystem, or things that we want to expose in stderr

There can only be one space?

ogoffart · 2017-12-14T09:02:24Z

src/gui/notificationwidget.cpp

+    if( !activity._link.isEmpty() ) {
+        // append a link to the message, if that is empty, to subject
+        subject.append(QString("&nbsp;<a href=\"%1\">%2</a>")
+                       .arg(activity._link.toString(QUrl::FullyEncoded))


use arg(... , ...) with two arguments. In general this is better because it works even if the first string contains a %1 or a %2.

Also, please use escapeXml to the _linkText

ogoffart · 2017-12-14T09:05:23Z

src/gui/servernotificationhandler.cpp

+                a._linkText = li.at(0);
+            } else {
+                a._link = QUrl(s);
+                a._linkText = QString("[%1]").arg(tr("link"));


(nitpick: i'd use tr("[link]"))

ogoffart · 2017-12-14T09:06:19Z

src/gui/notificationwidget.cpp

@@ -44,6 +44,16 @@ void NotificationWidget::setActivity(const Activity &activity)
    _ui._subjectLabel->setVisible(!activity._subject.isEmpty());
    _ui._messageLabel->setVisible(!activity._message.isEmpty());

+    QString subject = activity._subject;


needs to be xml escaped.

ogoffart · 2017-12-14T09:08:45Z

Why not a new button next to "Close" to open the link?

guruz · 2018-02-14T13:43:39Z

@dragotin nice PR! When do you want to continue on it?

Why not a new button next to "Close" to open the link?

If there is a long link text, this might look odd. But if you just want to have a button "Open" then I think it's fine..

Added a new data called linkText. If the link in the activity JSON has a space in it, the part before that is considered the link text. If it does not contain a space, the word [link] is used. This is maybe a solution for #6236

As the URL has to be encoded to make the whitespace-trick work, the url has to be properly assigned to QUrl.

If the Notification contains a link, it is parsed by this method. It takes care on the Link text that might be prepended. Including unittest.

dragotin · 2018-02-18T12:24:21Z

Fixed all the outstanding things (I hope) and added a unit test for splitting of the url from the link text. Thanks @ogoffart for review.

I do not like the idea of a button, because a link should not be a button, but a link. Also please note that it is already possible to add additional buttons that trigger a link via the notification specification, see here: https://github.com/owncloud/client/blob/master/src/gui/notificationwidget.cpp#L72

ogoffart · 2018-02-18T12:30:27Z

src/gui/activitydata.h

@@ -57,6 +57,7 @@ class Activity
    QString _message;
    QString _file;
    QUrl _link;
+    QString _linkText;


Maybe document which field are html escaped.

Personally, i'd keep all these string unescaped. and escape them right before putting them in some html text.

ogoffart · 2018-02-18T12:31:56Z

src/gui/activitydata.h

+            // if there is a real whitespace in the link, the part before the space
+            // is rendered as a link text
+            int charPos = s.lastIndexOf(QChar(' '));
+            if( charPos > -1 ) {


(style: we don't put spaces inside the parentheses, the space is before. You could run git-clang-format, to fix all style issues.)

ogoffart · 2018-02-18T12:33:01Z

src/gui/notificationwidget.cpp

+        }
+        subject.append( QString("&nbsp;<a href=\"%1\">%2</a>")
+                        .arg(activity._link.toString(QUrl::FullyEncoded),
+                             lText.toHtmlEscaped() ));


Am I wrong that you double html escape?

ogoffart · 2018-02-18T12:41:18Z

test/testactivitydata.cpp

+
+private slots:
+
+    void testLinkSplitEmpty() {


this kind of test would be much shorter using a testLink_data() thing

_data tests, like for example TestFolder::testFolder or TestUtility::testSanitizeForFileName_data or ...

And also QCOMPARE instead of QVERIFY

ogoffart · 2018-02-18T12:43:33Z

src/gui/notificationwidget.cpp

+        _ui._subjectLabel->setTextFormat(Qt::RichText);
+        _ui._subjectLabel->setOpenExternalLinks(true);
+
+    }
    _ui._subjectLabel->setText(activity._subject);


you mean just "subject" here!

dragotin · 2018-03-07T20:38:19Z

Thanks @ogoffart, I fixed stuff accordingly.

With the test I would need more advise ;-)

ogoffart

Code looks good.

Have you been testing that this works.

Should the notification also not include the actual text from the notificication? (or is that something else?)

ogoffart · 2018-03-09T11:12:33Z

test/testactivitydata.cpp

+
+private slots:
+
+    void testLinkSplitEmpty() {


_data tests, like for example TestFolder::testFolder or TestUtility::testSanitizeForFileName_data or ...

And also QCOMPARE instead of QVERIFY

ogoffart · 2018-03-09T11:18:48Z

I wanted to remove my "Request change" without "Approving" but apparently this is not possible.

dragotin · 2018-03-16T20:45:09Z

Should be good now.

guruz · 2018-04-09T15:31:11Z

Works for me, but let's wait if the server guys are OK with the format. I'll ping them.

We can make it more beautiful later. Maybe the link should be in a newline?

For owncloud/client#6240

DeepDiver1975 · 2018-04-09T18:26:41Z

src/gui/activitydata.h

+        if (!s.isEmpty()) {
+            // if there is a real whitespace in the link, the part before the space
+            // is rendered as a link text
+            int charPos = s.lastIndexOf(QChar(' '));


That's fishy ....

guruz

I'll take back my review too for now too

For owncloud/client#6240

guruz · 2018-04-13T14:51:17Z

Superseded by #6453 after discussion on server issue about link text. Sorry @dragotin

For owncloud/client#6240

dragotin added the DO NOT MERGE YET label Dec 13, 2017

ogoffart suggested changes Dec 14, 2017

View reviewed changes

dragotin added 5 commits February 18, 2018 11:20

Notifications: Display the link in the subject line.

d9d4230

Added a new data called linkText. If the link in the activity JSON has a space in it, the part before that is considered the link text. If it does not contain a space, the word [link] is used. This is maybe a solution for #6236

Activities: Properly encode the URL.

8e8442d

As the URL has to be encoded to make the whitespace-trick work, the url has to be properly assigned to QUrl.

Add a method to ActivityData to parse the notification link.

0d514a5

If the Notification contains a link, it is parsed by this method. It takes care on the Link text that might be prepended. Including unittest.

Unittest fixes

7f10509

Properly set the the link text if exists, otherwise use default.

7b90b75

dragotin force-pushed the fix_6236 branch from e168bab to 7b90b75 Compare February 18, 2018 12:13

HtmlEscape the subject of the notification.

cbe8eea

dragotin removed the DO NOT MERGE YET label Feb 18, 2018

ogoffart suggested changes Feb 18, 2018

View reviewed changes

Feedback from ogoffarts review: Double escape and style fixes.

f04747c

ogoffart reviewed Mar 9, 2018

View reviewed changes

ogoffart approved these changes Mar 9, 2018

View reviewed changes

dragotin added 2 commits March 16, 2018 20:29

Use QCOMPARE rather than QVERIFY

314e190

Use QLatin1String for static strings in test

68ba3bc

guruz added this to the 2.5.0 milestone Mar 19, 2018

guruz added the Feature label Mar 19, 2018

guruz self-requested a review April 7, 2018 07:55

guruz self-assigned this Apr 7, 2018

guruz approved these changes Apr 9, 2018

View reviewed changes

guruz added a commit to owncloud/notifications that referenced this pull request Apr 9, 2018

occ: Provide arguments for link and linktext

7179c08

For owncloud/client#6240

guruz mentioned this pull request Apr 9, 2018

occ: Provide arguments for link and linktext owncloud/notifications#172

Merged

DeepDiver1975 requested changes Apr 9, 2018

View reviewed changes

guruz suggested changes Apr 12, 2018

View reviewed changes

guruz added a commit to owncloud/notifications that referenced this pull request Apr 12, 2018

occ: Provide argument for link

548beaf

For owncloud/client#6240

guruz closed this Apr 13, 2018

DeepDiver1975 pushed a commit to owncloud/notifications that referenced this pull request Apr 18, 2018

occ: Provide argument for link

8d7f31b

For owncloud/client#6240

DeepDiver1975 mentioned this pull request Apr 18, 2018

occ: Provide argument for link owncloud/notifications#179

Merged

TheOneRing deleted the fix_6236 branch December 6, 2019 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications: Display the link in the subject line. #6240

Notifications: Display the link in the subject line. #6240

dragotin commented Dec 13, 2017

dragotin commented Dec 13, 2017

ogoffart Dec 14, 2017

ogoffart Dec 14, 2017

ogoffart Dec 14, 2017

ogoffart Dec 14, 2017

ogoffart commented Dec 14, 2017

guruz commented Feb 14, 2018

dragotin commented Feb 18, 2018

ogoffart Feb 18, 2018

ogoffart Feb 18, 2018

ogoffart Feb 18, 2018

ogoffart Feb 18, 2018

ogoffart Mar 9, 2018

ogoffart Feb 18, 2018

dragotin commented Mar 7, 2018

ogoffart left a comment

ogoffart Mar 9, 2018

ogoffart commented Mar 9, 2018

dragotin commented Mar 16, 2018

guruz commented Apr 9, 2018

DeepDiver1975 Apr 9, 2018

guruz left a comment

guruz commented Apr 13, 2018

Notifications: Display the link in the subject line. #6240

Notifications: Display the link in the subject line. #6240

Conversation

dragotin commented Dec 13, 2017

dragotin commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogoffart commented Dec 14, 2017

guruz commented Feb 14, 2018

dragotin commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragotin commented Mar 7, 2018

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogoffart commented Mar 9, 2018

dragotin commented Mar 16, 2018

guruz commented Apr 9, 2018

Choose a reason for hiding this comment

guruz left a comment

Choose a reason for hiding this comment

guruz commented Apr 13, 2018