After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 636252 - aggressive linking in notifications
aggressive linking in notifications
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Marina Zhurakhinskaya
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-12-01 20:43 UTC by William Jon McCann
Modified: 2011-09-08 13:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (25.59 KB, image/png)
2011-01-06 18:59 UTC, William Jon McCann
  Details
findUrl: document the URL-matching regex (2.32 KB, patch)
2011-04-13 13:52 UTC, Dan Winship
committed Details | Review
findUrl: don't match non-:// URLs (1007 bytes, patch)
2011-04-13 13:52 UTC, Dan Winship
committed Details | Review
findUrl: be pickier about what can precede a URL (1.72 KB, patch)
2011-04-13 13:52 UTC, Dan Winship
committed Details | Review
tests: add a unit test for Misc.findUrls() (3.23 KB, patch)
2011-07-05 18:14 UTC, Dan Winship
committed Details | Review

Description William Jon McCann 2010-12-01 20:43:01 UTC
Awesome to have live links in notifications now.  However, it seems the matching is a bit too aggressive.

Evolution has notifications that include "Mail from mccann@redhat.com"  any chat can have text like "One of:this, that".  Those both appear as links currently.
Comment 1 William Jon McCann 2010-12-01 20:43:55 UTC
I should note that the first example the link wasn't a mailto:mccann@redhat.com but a url link to redhat.com.  And the second example the link was "of:this".
Comment 2 Marina Zhurakhinskaya 2010-12-01 21:56:25 UTC
I've noticed that long chat notifications that have a link in them have the ellipses underlined even if the link itself is not a part of the text shown in the banner. I've also seen Gwibber notifications that don't have anything in their banner other then ellipses in the end.

I just have to say though that having the links in the notifications is awesome :)!
Comment 3 Dan Winship 2011-01-06 18:49:38 UTC
(In reply to comment #0)
> Evolution has notifications that include "Mail from mccann@redhat.com"

and you're seeing "mccann@redhat.com" get URLified? it doesn't happen if I send myself that with notify-send...
Comment 4 William Jon McCann 2011-01-06 18:59:15 UTC
Created attachment 177691 [details]
screenshot

Sorry, I was mistaken about the part that was turned into a link.

Regardless of this bug the message from Evo should be improved...
Comment 5 Dan Winship 2011-04-13 13:52:28 UTC
Created attachment 185867 [details] [review]
findUrl: document the URL-matching regex

Explode the regex onto multiple lines, and add comments explaining the
pieces. Also, change ()s to (?:)s (non-capturing groups) where
appropriate, and replace the UTF-8 characters with \u escapes so that
they actually work.
Comment 6 Dan Winship 2011-04-13 13:52:32 UTC
Created attachment 185868 [details] [review]
findUrl: don't match non-:// URLs

Although "x:5" could theoretically be a URL with scheme "x" and path
"5", it probably isn't. Only URLify strings that use the "authority"
syntax ("foo://").
Comment 7 Dan Winship 2011-04-13 13:52:35 UTC
Created attachment 185869 [details] [review]
findUrl: be pickier about what can precede a URL

findUrl() was seeing strings like "You have 1 new message in
foo@example.com/Inbox" and finding the URL
"[http://]example.com/Inbox". Require that URLs either start at the
start of the string, or are preceded by whitespace or an open
paren/quote/etc.

(Since JS doesn't have look-behind assertions like perl does, we have
to actually match the URL-preceding character in the regex, and then
adjust the result findUrl returns accordingly.)
Comment 8 Dan Winship 2011-05-16 10:33:58 UTC
poke
Comment 9 Dan Winship 2011-07-05 18:14:07 UTC
Created attachment 191347 [details] [review]
tests: add a unit test for Misc.findUrls()

depends on the unit test fixage from bug 650298
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-08-03 19:35:25 UTC
Review of attachment 185867 [details] [review]:

This is increasingly reminding me of http://www.ex-parrot.com/pdw/Mail-RFC822-Address.html, but sure.
Comment 11 Jasper St. Pierre (not reading bugmail) 2011-08-03 19:36:55 UTC
Review of attachment 185868 [details] [review]:

The only scheme that I can think of in major use without an "://" is mailto, but I doubt that's going to be in plaintext in a notification.
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-08-03 19:41:20 UTC
Review of attachment 185869 [details] [review]:

::: js/misc/util.js
@@ +13,3 @@
 // http://daringfireball.net/2010/07/improved_regex_for_matching_urls
 const _balancedParens = '\\((?:[^\\s()<>]+|(?:\\(?:[^\\s()<>]+\\)))*\\)';
+const _leadingJunk = '[\\s`(\\[{\'\\"<\u00AB\u201C\u2018]';

IMHO [;:.,] should be allowed too.
Comment 13 Jasper St. Pierre (not reading bugmail) 2011-08-03 19:44:32 UTC
Review of attachment 191347 [details] [review]:

"No newline at end of file"

::: tests/unit/url.js
@@ +36,3 @@
+      output: [ { url: 'http://www.gnome.org:99/port', pos: 10 } ] },
+    { input: 'This is an ftp://www.gnome.org/ test.',
+      output: [ { url: 'ftp://www.gnome.org/', pos: 11 } ] },

Another mistake thrown on IRC a lot would help:

  'This is http://www.gnome.org.'
  'This is http://www.gnome.org/.'

@@ +50,3 @@
+    { input: 'This is not:/a.url/ test.',
+      output: [ ] },
+    { input: 'This is not@a.url/ test.',

Including Jon's origin test would be useful:

  'This is surely@not.a/url  test.'
Comment 14 Dan Winship 2011-09-08 13:11:56 UTC
> The only scheme that I can think of in major use without an "://" is mailto,
> but I doubt that's going to be in plaintext in a notification.

Yeah, added a comment about that to the commit message

> +const _leadingJunk = '[\\s`(\\[{\'\\"<\u00AB\u201C\u2018]';
> 
> IMHO [;:.,] should be allowed too.

given that this bug is about URLifying things that shouldn't be
urlified, I'm going to stick with the original regexp there. If this
turns out to miss important cases, then we can revisit it later.

> Another mistake thrown on IRC a lot would help:

(I think you're referring to the fact that xchat mistakenly includes a
trailing "," in a URL, which we don't do.)

> Including Jon's origin test would be useful:

I added both of the test cases you suggested.

Attachment 185867 [details] pushed as 92a8507 - findUrl: document the URL-matching regex
Attachment 185868 [details] pushed as 5632216 - findUrl: don't match non-:// URLs
Attachment 185869 [details] pushed as e2898be - findUrl: be pickier about what can precede a URL
Attachment 191347 [details] pushed as 6496205 - tests: add a unit test for Misc.findUrls()