Created attachment 171437 [details] [review]
[misc] Add function for finding urls in string

Created attachment 171439 [details] [review]
[messageTray] make links in message banners clickable 

(g_app_info_launch_default_for_uri require gvfs. https://bugzilla.gnome.org/show_bug.cgi?id=623708#c13 )

Comment on attachment 171437 [details] [review]
[misc] Add function for finding urls in string 

The problem with this regex (especially with PATHTERM_CLASS commented out) is that it does the wrong thing with things like:

   You should look at the regex in
   http://daringfireball.net/2010/07/improved_regex_for_matching_urls, which
   does a better job.

because it thinks the "," is part of the URL (which, theoretically it *could* be, but in reality, it's much much much much more common for trailing punctuation to NOT be intended to be read as part of the URL).

Created attachment 171447 [details] [review]
[misc] Add function for finding urls in string

Comment on attachment 171447 [details] [review]
[misc] Add function for finding urls in string 

>+const url_regexp_pattern = "\\b(([a-z][\\w-]+:(/{1,3}|[a-z0-9%])|www\\d{0,3}[.]|[a-z0-9.\\-]+[.][a-z]{2,4}/)([^\\s()<>]+|\\(([^\\s()<>]+|(\\([^\\s()<>]+\\)))*\\))+(\\(([^\\s()<>]+|(\\([^\\s()<>]+\\)))*\\)|[^\\s`!()\\[\\]{};:'\".,<>?«»ââââ]))";

I think you should be able to make this a regexp literal instead of a string literal, and then you don't need _regExp as well. (Also, both url_regexp_pattern and find_urls should have camelCase names, not underscored.)

(The non-ASCII characters at the end have gotten mangled by bugzilla here, but they apply correctly in the patch.)

>+function find_urls(str, pos_inc) {

Need some docs here:

// findUrls:
// @str: string to find URLs in
//
// Searches @str for URLs and returns an array of objects with %url
// properties showing the matched URL string, and %pos properties indicating
// the position within @str where the URL was found.
//
// Return value: the list of match objects, as described above

>+        let t = find_urls(leftStr, 0);
>+        res = res.concat(t);

this is messy... If you used the "g" flag with the regex, you could use RegExp.exec() in a loop instead without needing to recurse or manually split up the string. (And then you don't need pos_inc either.)

Comment on attachment 171439 [details] [review]
[messageTray] make links in message banners clickable 

>+        body.reactive = true;

you can specify that in the constructor call

>+        function hilightUrl(pos) {

I think it would be better to underline the URLs ahead of time, rather than only on mouseover. And they should be blue as well. (A much lighter blue than normal web-browser-blue though since they'll be against a black background. It should be specified in the CSS.) Maybe there could also be an additional mouseover effect as well... if you keep the function, call it "highlightUrl" not "hilightUrl", and pull it out of addBody. We generally don't do non-anonymous inner functions.

>+            let rightStr = text.substring(url.pos + url.url.length);

might be worth including a "len" property in the data returned from find_urls, since it already has that data handy anyway.

>+            for (let i = 0; i < text.length; i++) {
>+                let [success, px, py, line_height] = body.clutter_text.position_to_coords(i);

Shouldn't "text.length" be "body.clutter_text.text.length"?

It would also presumably be more efficient to loop over each URL rather than over each character, and just check if the pointer is within the bounding box defined by the positions of the first and last characters of the URL (taking into account the non-rectangularity of the bounding box if the URL spans multiple lines).

Created attachment 171644 [details] [review]
[misc] Add function for finding urls in string

Created attachment 171645 [details] [review]
[messageTray] make links in message banners clickable 

> It would also presumably be more efficient to loop over each URL rather than over each character
No. position_to_coords is very fast.

Created attachment 172203 [details] [review]
Add function for finding urls in string

i meant like this...

(In reply to comment #9)
> Created an attachment (id=172203) [details] [review]
> i meant like this...

It look and work fine.

Comment on attachment 171645 [details] [review]
[messageTray] make links in message banners clickable 

>+.url-highlighter {
>+    link-color: #8888ff;

still too dark, i think. How about #ccccff?

>+function URLHighlighter(text, actor) {

I like that this is now independent of the rest of messageTray.js... we might eventually end up moving it into its own file, if we needed to use it elsewhere too.

>+        this._linkColor = '#888888FF';

too many "8"s there

>+                try {
>+                    Gio.app_info_launch_default_for_uri(url, global.create_app_launch_context());
>+                } catch (e) {log(e);}
>+            }
>+            return false;

you want to return true in the found-a-url case, so that the default "open empathy window" processing doesn't happen as well.

>+        this.actor.clutter_text.set_markup(text);
>+        this._urls = Utils.findUrls(this.actor.clutter_text.text);

add a comment here explaining that this.actor.clutter_text.text has the markup stripped.

>+        if (find_pos != -1) {
>+            for (let i = 0; i < this._urls.length; i++)
>+            if (find_pos >= this._urls[i].pos && this._urls[i].pos + this._urls[i].url.length > find_pos)
>+                    return i;

wrong indentation on the "if" line. also would be clearer if you added a line break after the "&&"



To do items (can be future bugs):

    * We really need a pointing-hand cursor. Clutter doesn't seem to have any
      cursor-controlling APIs though...

    * The notification protocol has a "body-hyperlinks" capability which we
      currently advertise not supporting, but we could fix that now. (It allows
      HTML-style <a href="...">...</a> links.)

    * If I type "<b>foo</b>" into gmail chat, it shows up like that in gmail and
      empathy, but as bold text in gnome-shell. So telepathyClient.js is doing
      it wrong and needs to be xml-escaping the strings it gets.

(In reply to comment #2)
> (g_app_info_launch_default_for_uri require gvfs.
> https://bugzilla.gnome.org/show_bug.cgi?id=623708#c13 )

Is this new API or is it just failing because of glib and gvfs having different install prefixes? If the latter, it would be better to fix this by setting an appropriate environment variable in the gnome-shell wrapper script.

(In reply to comment #12)
> Is this new API
No.

> or is it just failing because of glib and gvfs having different install prefixes?

No. Path to gio modules resolved at compile.
Default path can be modified with --with-gio-module-dir, but it will require generate moduleset from .in file (libdir can be different on different computers).

Created attachment 172237 [details] [review]
[messageTray] make links in message banners clickable

> * We really need a pointing-hand cursor.
done

> Clutter doesn't seem to have any cursor-controlling APIs though...
We have shell_global_set_cursor

> If I type "<b>foo</b>" into gmail chat, it shows up like that in gmail
> and empathy, but as bold text in gnome-shell.
It is by design:) (look at _cleanMarkup)

> The notification protocol has a "body-hyperlinks" capability which we
> currently advertise not supporting, but we could fix that now.
I will write patch for that.

Created attachment 172280 [details] [review]
fix variable name

(to be squashed)

Created attachment 172281 [details] [review]
messageTray: fix handling of markup vs non-markup notifications

NotificationDaemon-based notifications have markup in the banner/body,
but Telepathy-based notifications don't. (Eg, an XMPP message
containing "<b>foo</b>" should show up angle brackets and all, not as
bold.) Fix MessageTray.Notification to allow explicitly specifying
where there should and shouldn't be markup, and use that
appropriately.

Created attachment 172282 [details] [review]
gnome-shell: set GIO_EXTRA_MODULES if needed to find gvfs

In the default jhbuild, gio is in our prefix but gvfs is not, so we
need to set GIO_EXTRA_MODULES so gio can find the gvfs-based modules
(eg, for gconf-based MIME type stuff).

Comment on attachment 172237 [details] [review]
[messageTray] make links in message banners clickable

>+        this._linkColor = '#8888FF';

fix to match new css

>+                try {
>+                    Gio.app_info_launch_default_for_uri(url, global.create_app_launch_context());
>+                    return true;
>+                } catch (e) {log(e);}

i think it would be better to return true *after* the catch, so that if there's an error, the notification stays up

>+  SHELL_CURSOR_HAND

maybe should have a better name? "LINK_HAND" or "POINTING_HAND", to distinguish it from the open hand/closed hand cursors used to grab-and-drag in some programs?

if my patches look right to you then I think this is ready to go (with just the minor changes above). you can just commit+push the whole set.

(In reply to comment #17)
> Created an attachment (id=172282) [details] [review]
> gnome-shell: set GIO_EXTRA_MODULES if needed to find gvfs
> 
> In the default jhbuild, gio is in our prefix but gvfs is not, so we
> need to set GIO_EXTRA_MODULES so gio can find the gvfs-based modules
> (eg, for gconf-based MIME type stuff).

Do you think it's safe to:

 - Build a GLib that's newer than the system one
 - Use a system-built gvfs?

My assumption was that gvfs and glib have a fairly tight coupling, so I was OK with the gvfs addition to the moduleset.

well... it works :) (at least with today's glib and F14... I could imagine that some combinations of older gvfs and newer glib might fail).

I'm ok with just building gvfs if we think that's a better plan. Although with the stuff Bastien was doing, isn't newer glib/gvfs going to end up with a uri-launching API that requires newer applications/control-center in order to find the right apps?

Review of attachment 172282 [details] [review]:

It does not work.
symbol lookup error: /usr/lib64/gio/modules/libgiogconf.so: undefined symbol: g_desktop_app_info_lookup_get_type

g_desktop_app_info_lookup_* was removed in new glib.

(In reply to comment #22)
> Review of attachment 172282 [details] [review]:
> 
> It does not work.
> symbol lookup error: /usr/lib64/gio/modules/libgiogconf.so: undefined symbol:
> g_desktop_app_info_lookup_get_type
> 
> g_desktop_app_info_lookup_* was removed in new glib.

I think, the only possible solution is to add gvfs in moduleset

Maxim's patches are all reviewed, someone needs to review the patches I wrote, and we need to figure out the gvfs situation.

If adding gvfs would be problematic because of its dependencies (samba, etc), we could just cheat and exec "gvfs-open $URI" instead...

Created attachment 175071 [details] [review]
[messageTray] make links in message banners clickable

> If adding gvfs would be problematic because of its dependencies (samba, etc),
samba is optional dependencies

> we could just cheat and exec "gvfs-open $URI" instead...
done. if shell was started in 'jhbuild shell' enviroment, it will not work:(
But I think, it is rare case.

Review of attachment 172281 [details] [review]:

looks good

Review of attachment 172203 [details] [review]:

looks good

Comment on attachment 175071 [details] [review]
[messageTray] make links in message banners clickable

>+                    let command = 'gvfs-open ' + url;
>+                    let [ok, len, args] = GLib.shell_parse_argv(command);
>+                    let p = new Shell.Process({ 'args' : args });

Oof. That's doing extra work in order to create a security hole. :)

    let p = new Shell.Process({ 'args' : ['gvfs-open', url] });

>+                    log('if gnome-shell was started in jhbuild enviroment, gvfs-open will not work:(');

just remove that


I didn't actually look at any of the rest of the patch, I'm assuming it's the same as before.