GNOME Bugzilla – Bug 692968
Add support to handle external URIs
Last modified: 2013-02-13 15:51:58 UTC
Currently, Documents don't handle links. The patch add support to handle (only) external URIs, leaving the anothers EV_LINK_ACTION_TYPE_* unimplemented, at least for now.
Created attachment 234937 [details] [review] Add support to handle external URIs
Review of attachment 234937 [details] [review]: Thanks for the patch! This looks mostly good - I have some comments on the code below. ::: src/preview.js @@ +185,3 @@ }, + _launch_external_uri: function(widget, action) { We use camel case for function names - should be _launchExternalUri @@ +194,3 @@ + context.set_timestamp(Gtk.get_current_event_time()); + + let context = screen.get_display().get_app_launch_context(); It's cleaner to use uri.startsWith(), here and below for the 'www.' check, @@ +205,3 @@ + uri = parent.get_uri() + uri; + else + if (uri.indexOf("://") == -1 && uri.substring(0, 6) != "mailto:") { I don't really understand what this piece of code is trying to do, but I see Evince performs similar checks. Would be nice to: - comment the code a bit explaining why this URI rewriting is necessary - split the URI rewriting in a separate function @@ +210,3 @@ + + if (!Gio.AppInfo.launch_default_for_uri(uri, context, error)) + } else { This is not the correct way to use GErrors in JS - you don't pass the error structure down to the function but use try/catch blocks for functions that can throw exceptions (GErrors are automatically translated into thrown exceptions by the binding) So code should do something like try { Gio.AppInfo.launch_default_for_uri(uri, context); } catch (e) { log('Unable... ' + e.message); }
(In reply to comment #2) > Review of attachment 234937 [details] [review]: > > Thanks for the patch! This looks mostly good - I have some comments on the code > below. > > ::: src/preview.js > @@ +185,3 @@ > }, > > + _launch_external_uri: function(widget, action) { > > We use camel case for function names - should be _launchExternalUri Right. > > @@ +194,3 @@ > + context.set_timestamp(Gtk.get_current_event_time()); > + > + let context = screen.get_display().get_app_launch_context(); > > It's cleaner to use uri.startsWith(), here and below for the 'www.' check, When you say "here" are you referring the "mailto" part, right? > > @@ +205,3 @@ > + uri = parent.get_uri() + uri; > + else > + if (uri.indexOf("://") == -1 && uri.substring(0, 6) != "mailto:") { > > I don't really understand what this piece of code is trying to do, but I see > Evince performs similar checks. Would be nice to: > - comment the code a bit explaining why this URI rewriting is necessary > - split the URI rewriting in a separate function Right. > > @@ +210,3 @@ > + > + if (!Gio.AppInfo.launch_default_for_uri(uri, context, error)) > + } else { > > This is not the correct way to use GErrors in JS - you don't pass the error > structure down to the function but use try/catch blocks for functions that can > throw exceptions (GErrors are automatically translated into thrown exceptions > by the binding) Nice. It was one of my doubts :-) I really have zero (actually, 2, this patch and the another one) experience with JS at all. > > So code should do something like > try { > Gio.AppInfo.launch_default_for_uri(uri, context); > } catch (e) { > log('Unable... ' + e.message); > } Right. Thank you for your comments and I will re-up the patch as soon as possible.
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 234937 [details] [review] [details]: > > > > Thanks for the patch! This looks mostly good - I have some comments on the code > > below. > > > > ::: src/preview.js > > @@ +185,3 @@ > > }, > > > > + _launch_external_uri: function(widget, action) { > > > > We use camel case for function names - should be _launchExternalUri > > Right. > > > > > @@ +194,3 @@ > > + context.set_timestamp(Gtk.get_current_event_time()); > > + > > + let context = screen.get_display().get_app_launch_context(); > > > > It's cleaner to use uri.startsWith(), here and below for the 'www.' check, > > When you say "here" are you referring the "mailto" part, right? Now I remember why didn't use uri.startsWith() neither uri.contains(). JS ERROR: !!! Exception was: TypeError: uri.contains is not a function JS ERROR: !!! message = '"uri.contains is not a function"' JS ERROR: !!! fileName = '"/home/fidencio/dev/share/gnome-documents/js/preview.js"' JS ERROR: !!! lineNumber = '229' JS ERROR: !!! stack = '"([object GObject_Object],[object GObject_Object])@/home/fidencio/dev/share/gnome-documents/js/preview.js:229 JS ERROR: !!! Exception was: TypeError: uri.startsWith is not a function JS ERROR: !!! message = '"uri.startsWith is not a function"' JS ERROR: !!! fileName = '"/home/fidencio/dev/share/gnome-documents/js/preview.js"' JS ERROR: !!! lineNumber = '226' JS ERROR: !!! stack = '"([object GObject_Object],[object GObject_Object])@/home/fidencio/dev/share/gnome-documents/js/preview.js:226 > > > > > @@ +205,3 @@ > > + uri = parent.get_uri() + uri; > > + else > > + if (uri.indexOf("://") == -1 && uri.substring(0, 6) != "mailto:") { > > > > I don't really understand what this piece of code is trying to do, but I see > > Evince performs similar checks. Would be nice to: > > - comment the code a bit explaining why this URI rewriting is necessary > > - split the URI rewriting in a separate function > > Right. > > > > > @@ +210,3 @@ > > + > > + if (!Gio.AppInfo.launch_default_for_uri(uri, context, error)) > > + } else { > > > > This is not the correct way to use GErrors in JS - you don't pass the error > > structure down to the function but use try/catch blocks for functions that can > > throw exceptions (GErrors are automatically translated into thrown exceptions > > by the binding) > > Nice. It was one of my doubts :-) > I really have zero (actually, 2, this patch and the another one) experience > with JS at all. > > > > > So code should do something like > > try { > > Gio.AppInfo.launch_default_for_uri(uri, context); > > } catch (e) { > > log('Unable... ' + e.message); > > } > > Right. > > Thank you for your comments and I will re-up the patch as soon as possible.
I tried applying your patch and changing it to use startsWith() and it seemed to work fine for a document with a simple http URL...can you post the patch with the new code? For debugging you can also find the type of an object with the typeof(object) function.
Created attachment 235381 [details] [review] Add support to handle external URIs
Review of attachment 235381 [details] [review]: Generally, in Documents code I always use single quotes for strings that are not translated and double quotes for translated strings. ::: src/preview.js @@ +186,3 @@ + _uriRewrite: function(uri) { + if (uri.substring(0, 6) != "mailto:") { Shouldn't you check for www. instead of mailto here?
(In reply to comment #7) > Review of attachment 235381 [details] [review]: > > Generally, in Documents code I always use single quotes for strings that are > not translated and double quotes for translated strings. Right, I'll change it. > > ::: src/preview.js > @@ +186,3 @@ > > + _uriRewrite: function(uri) { > + if (uri.substring(0, 6) != "mailto:") { > > Shouldn't you check for www. instead of mailto here? Yes! It was a wrong sed. :-p
Created attachment 235383 [details] [review] Add support to handle external URIs
Review of attachment 235383 [details] [review]: Looks good with this minor comment fixed. ::: src/preview.js @@ +232,3 @@ + Gio.AppInfo.launch_default_for_uri(uri, context) + } catch (e) { + _launchExternalUri: function(widget, action) { Should be e.message
Created attachment 235900 [details] [review] Add support to handle external URIs
Attachment 235900 [details] pushed as 8718f18 - Add support to handle external URIs