Created attachment 314672 [details] [review]
chatView: Use GAppLaunchContext for URIs

In ordet to bring the new application window to front
and get indication from the shell that we are opening an
application we need to use a GAppLaunchContext.

Created attachment 314673 [details] [review]
utils: Add 'geo' URI support to urlRegexp

Created attachment 314674 [details]
Casst of opening geo: URI in Polari

Created attachment 314687 [details] [review]
utils: Add 'geo' URI support to urlRegexp

Created attachment 314688 [details] [review]
utils: Add 'geo' URI support to urlRegexp

Created attachment 314691 [details] [review]
Add openURL helper to Utils

In order to bring the new application window to front
and get indication from the shell that we are opening an
application we need to use a GAppLaunchContext.

And it would be nice to get some feedback if we got
an error from opening the URL. For instance if no
handler is present.

Created attachment 314692 [details] [review]
utils: Add 'geo' URI support to urlRegexp

Created attachment 314743 [details] [review]
Add openURL helper to Utils

In order to bring the new application window to front
and get indication from the shell that we are opening an
application we need to use a GAppLaunchContext.

And it would be nice to get some feedback if we got
an error from opening the URL. For instance if no
handler is present.

Created attachment 314744 [details] [review]
utils: Add 'geo' URI support to urlRegexp

This will let Polari handle the geo: URI scheme as
specified by RFC 5870.

GNOME Maps and KDE Marble installs handlers for
the geo: uri.

    In its simplest form it looks like:
        geo:latitude,longitude

    An Android extension to set a description is also supported:
        geo:0,0?q=latitude,longitude(description)

Review of attachment 314743 [details] [review]:

::: src/chatView.js
@@ +521,3 @@
         let iter = view.get_iter_at_location(x, y);
         let tags = iter.get_tags();
+        let timestamp = Gtk.get_current_event_time();

We do have the actual event here - use event.get_time()

::: src/utils.js
@@ +109,3 @@
+    let app = Gio.Application. get_default();
+    let scheme = GLib.uri_parse_scheme(url);
+    let appInfo = Gio.AppInfo.get_default_for_uri_scheme(scheme);

Can you leave this part out for now? I haven't quite made up my mind about how to support "fringe" URI schemes yet (i.e whether to throw an error at the user as you do here, or just not linkify them unless the scheme is supported) ...

@@ +111,3 @@
+    let appInfo = Gio.AppInfo.get_default_for_uri_scheme(scheme);
+    if (!appInfo) {
+        let msg = _("No way to open URI");

... also instead of duplicating the code, you could just use different messages in the error handling for launch_default_for_uri():

let msg;
if (e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.NOT_SUPPORTED))
    msg = _("No way to open URI");
else
    msg = _("Failed to open URI");
let n = new AppNotification.SimpleOutput(msg);

@@ +118,3 @@
+
+    let display = Gdk.Display.get_default();
+    let ctx = Gdk.Display.get_default().get_app_launch_context();

first you set a display variable, then you don't use it

@@ +119,3 @@
+    let display = Gdk.Display.get_default();
+    let ctx = Gdk.Display.get_default().get_app_launch_context();
+    let screen = display.get_default_screen();

no need to explicitly specify the screen the launch context will use anyway

Review of attachment 314744 [details] [review]:

::: src/utils.js
@@ +37,3 @@
 const _leadingJunk = '[\\s`(\\[{\'\\"<\u00AB\u201C\u2018]';
 const _notTrailingJunk = '[^\\s`!()\\[\\]{};:\'\\".,<>?\u00AB\u00BB\u201C\u201D\u2018\u2019]';
+const _float = '[-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?';

Just double-checking: .1234 is legal?

@@ +49,3 @@
             '[a-z0-9.\\-]+[.][a-z]{2,4}/' +       // foo.xx/
+            '|' +
+            'geo:' + _float + ',' + _float + 	  // 'geo' URI

As mentioned in the other review, I'm still undecided on whether to unconditionally turn those URIs into links.
Independent from that, I'm not really comfortable with making an already tricky regex ever-more complicated (see bug 755563 and its dupes for a list of other schemes we've already been asked to support). So I think I'd prefer something like:

const _defaultUrlRegex = '(^|' + ...
const _geoUrlRegex = '(^|' + ...
const _urlRegexp = new RegExp(_defaultUrlRegex + '|' + _geoUrlRegex);

(or create the RegExp object lazily and only append the geoUrl handling if that scheme is supported)

Review of attachment 314743 [details] [review]:

Thanks for the review!

::: src/chatView.js
@@ +521,3 @@
         let iter = view.get_iter_at_location(x, y);
         let tags = iter.get_tags();
+        let timestamp = Gtk.get_current_event_time();

Of course!

::: src/utils.js
@@ +109,3 @@
+    let app = Gio.Application. get_default();
+    let scheme = GLib.uri_parse_scheme(url);
+    let appInfo = Gio.AppInfo.get_default_for_uri_scheme(scheme);

Yes, understand! Will do!

@@ +111,3 @@
+    let appInfo = Gio.AppInfo.get_default_for_uri_scheme(scheme);
+    if (!appInfo) {
+        let msg = _("No way to open URI");

Ah, thank you!

Review of attachment 314744 [details] [review]:

::: src/utils.js
@@ +37,3 @@
 const _leadingJunk = '[\\s`(\\[{\'\\"<\u00AB\u201C\u2018]';
 const _notTrailingJunk = '[^\\s`!()\\[\\]{};:\'\\".,<>?\u00AB\u00BB\u201C\u201D\u2018\u2019]';
+const _float = '[-+]?[0-9]*\\.?[0-9]+(?:[eE][-+]?[0-9]+)?';

No it is not, and I checked that it is not linkified. The RFC says it needs 1*DIGIT before dot.

@@ +49,3 @@
             '[a-z0-9.\\-]+[.][a-z]{2,4}/' +       // foo.xx/
+            '|' +
+            'geo:' + _float + ',' + _float + 	  // 'geo' URI

Yeah, I understand completely. So Maybe this should be abandoned and a solution should be added to 755563 instead?

Created attachment 314829 [details] [review]
Add openURL helper to Utils

In order to bring the new application window to front
and get indication from the shell that we are opening an
application we need to use a GAppLaunchContext.

And it would be nice to get some feedback if we got
an error from opening the URL. For instance if no
handler is present.

Review of attachment 314829 [details] [review]:

::: src/chatView.js
@@ +488,3 @@
+        let timestamp = Gtk.get_current_event_time();
+        item.connect('activate', function() {
+            Utils.openURL(url, timestamp);

This looks wrong - why would we want the timestamp from the time the menu was popped up (which btw is available in the @time parameter) rather than when the menu item was activated?

::: src/utils.js
@@ +112,3 @@
+        Gio.AppInfo.launch_default_for_uri(url, ctx);
+    } catch(e) {
+        let n = new AppNotifications.SimpleOutput(_("Failed to open URI"));

"Failed to open link" - URI is a bit too technical for my taste for something we throw at the user, and "link" is consistent with the context menu.

@@ +114,3 @@
+        let n = new AppNotifications.SimpleOutput(_("Failed to open URI"));
+        app.notificationQueue.addNotification(n);
+        debug("failed top open URI: %s".format(e.message));

typo: s/top/to/ - also might be helpful to use "failed to open %s: %s".format(url, e.message)"?

Created attachment 314929 [details] [review]
Add openURL helper to Utils

In order to bring the new application window to front
and get indication from the shell that we are opening an
application we need to use a GAppLaunchContext.

And it would be nice to get some feedback if we got
an error from opening the URL. For instance if no
handler is present.

Review of attachment 314929 [details] [review]:

::: src/chatView.js
@@ -484,3 +484,2 @@
     _showUrlContextMenu: function(url, button, time) {
         let menu = new Gtk.Menu();
-

Spurious whitespace removal, otherwise LGTM

Attachment 314929 [details] pushed as 305fca7 - Add openURL helper to Utils