[PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[vibhavp]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

If browse-url-browser-function is defined as a list of pairs
(REGEXP . FUNCTION), like:
(setq browse-url-browser-function '(("i\.imgur\.com" browse-url-emacs)
                                    ("." browse-url-chromium)))
browse-url (with an "i.imgur.com" url) will print an error saying that
"(invalid-function (browse-url-chromium))". This is probably because of
how browse-url handles such pairs:

;; The `function' can be an alist; look down it for first match
;; and apply the function (which might be a lambda).
(catch 'done
  (dolist (bf function)
    (when (string-match (car bf) url)
      (apply (cdr bf) url args)
              ^^^^^^ (cdr bf) returns (browse-url-emacs) instead of browse-url-emacs
      (throw 'done t)))

This can be easily fixed by replacing "(cdr bf)" with "(cadr bf)". I
have attached a patch which does the same. If it looks good, I'll push
it to master.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: browse-url-fix.patch --]
[-- Type: text/x-diff, Size: 469 bytes --]

diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index 3f8cb84..fbf2b46 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -792,7 +792,7 @@ The default is to pass `browse-url-new-window-flag'."
 	(catch 'done
 	  (dolist (bf function)
 	    (when (string-match (car bf) url)
-	      (apply (cdr bf) url args)
+	      (apply (cadr bf) url args)
 	      (throw 'done t)))
 	  (error "No browse-url-browser-function matching URL %s"
 		 url))

[-- Attachment #3: Type: text/plain, Size: 50 bytes --]


Thanks,
Vibhav
-- 
Vibhav Pant
vibhavp@gmail.com

Re: [PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[vibhavp]

Are there any objections to the patch?

-- 
Vibhav Pant
vibhavp@gmail.com

Re: [PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[Eli Zaretskii]

> From: vibhavp@gmail.com
> Date: Sat, 25 Apr 2015 21:44:29 +0530
> 
> Are there any objections to the patch?

But the way browse-url-browser-function is documented, it should be a
list of conses, not a list of lists.  With your change, if
browse-url-browser-function is an alist, like the doc string implies,
the function will now fail.  So other users out there might suffer.

I'd be okay with a backward compatible change that would work with
both forms, but such a change will have to be accompanied by a
suitable change in documentation.

Thanks.

Re: [PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[vibhavp]

Eli Zaretskii <eliz@gnu.org> writes:

> But the way browse-url-browser-function is documented, it should be a
> list of conses, not a list of lists.  With your change, if
> browse-url-browser-function is an alist, like the doc string implies,
> the function will now fail.  So other users out there might suffer.

I phrased that wrong. Making browse-url-browse-function a list of lists
will not make sense to being with. The way browse-url works is by
iterating thorough every (REGEXP . FUNCTION) pair in
browse-url-browse-function, and applying string-match to the REGEXP
(which is the car of the pair) to the url. If the url matches the
REGEXP, browse-url opens the URL with it's associated function (the cdr
of the pair).

However (for some reason), (car bf) returns (browser-function) instead
of browse-function, which can be demonstrated with this piece of code:

(defun abc () 
  (dolist (bf '(("i.imgur.com" browse-url-emacs)
		("youtube.com" browse-url-firefox)
		("." browse-url-chromium)))
    (print (cdr bf))))

The output of this is:

(browse-url-emacs)

(browse-url-firefox)

(browse-url-chromium)

nil

This is not what we want, because applying (browse-url-emacs) to the URL
results in an "invalid function" error. If the "car" is replaced with a
"cadr", this is the output:

"browse-url-emacs"

"browse-url-youtube"

"browse-url-chromium"

nil

This is the desired outcome. So, I'm not sure if this change requires a
documentation change, because it is merely fixing a bug with how
browse-url function handles the variable.

Thanks,
Vibhav
-- 
Vibhav Pant
vibhavp@gmail.com

Re: [PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[Eli Zaretskii]

> From: vibhavp@gmail.com
> Date: Sat, 25 Apr 2015 22:40:14 +0530
> Cc: emacs-devel@gnu.org
> 
> However (for some reason), (car bf) returns (browser-function) instead
> of browse-function, which can be demonstrated with this piece of code:
> 
> (defun abc () 
>   (dolist (bf '(("i.imgur.com" browse-url-emacs)
> 		("youtube.com" browse-url-firefox)
> 		("." browse-url-chromium)))
>     (print (cdr bf))))

Shouldn't this be

(defun abc () 
  (dolist (bf '(("i.imgur.com" . browse-url-emacs)
		("youtube.com" . browse-url-firefox)
		("." . browse-url-chromium)))
    (print (cdr bf))))

instead?

> The output of this is:
> 
> (browse-url-emacs)
> 
> (browse-url-firefox)
> 
> (browse-url-chromium)
> 
> nil
> 
> This is not what we want, because applying (browse-url-emacs) to the URL
> results in an "invalid function" error.

Yes, because you used a list of lists instead of a list of conses,
like the doc string of browse-url-browser-function requires.

> If the "car" is replaced with a
> "cadr", this is the output:
> 
> "browse-url-emacs"
> 
> "browse-url-youtube"
> 
> "browse-url-chromium"
> 
> nil
> 
> This is the desired outcome.

But it will fail with a list of cons cells.

Re: [PATCH] Fix browse-url not working when browse-url-browser-function is a list (regexp . function) pairs

[vibhavp]

Ah, I was confused between conses and lists. Sorry for all the confusion.
-- 
Vibhav Pant
vibhavp@gmail.com