Re: [O] [PATCH] org-protocol: Allow optional port specification

[Rasmus]

Hi Sacha,

Thanks for your patch.

Sacha Chua <address@hidden> writes:

> I was trying to get org-protocol to work on KDE Plasma 5.4.2. I set up
> my ~/.kde/share/kde4/services/org.protocol, but the standard
> org-protocol sample syntax:
>
>    org-protocol://store-link://URL/TITLE
>
> resulted in the error:
>
>    Malformed URL
>    Port field was empty; source was "..."; scheme = "org-protocol",
>    host = "store-link", path = "// ..."
>
> Modifying my Javascript to create links of the form:
>
>    org-protocol://store-link:0//URL/TITLE
>
> made org-protocol correctly pass the link to emacsclient KDE 5.4.2. This
> patch allows the optional specification of a port in the URI. What do
> you think?

First, I’m not familiar with org-protocol or messaging systems in general
so I cannot offer input on this. 

> @@ -532,7 +532,7 @@ as filename."
>          (when (string-match the-protocol fname)
>            (dolist (prolist sub-protocols)
>              (let ((proto (concat the-protocol
> -                              (regexp-quote (plist-get (cdr prolist) 
> :protocol)) ":/+")))
> +                              (regexp-quote (plist-get (cdr prolist) 
> :protocol)) ":[^/]*/+")))

This seems pretty general.  Are there any dangerous of accepting
everything but /?  Is this only meant to capture a port?  If so, is there
any disadvantage in only allowing numbers?  As said, I don’t know anything
about these topics.

> +;;; test-org-protocol.el --- tests for org-protocol.el
                                Tests

Also, I guess you should add -*- lexical-binding: t; -*- these days.

> +;;; Code:
> +
> +(ert-deftest test-org-protocol/org-protocol-check-filename-for-protocol ()
> +  "Test `org-protocol-check-filename-for-protocol' specifications."
> +  ;; Store link
> +  (let ((uri "/some/directory/org-protocol:/store-link:/URL/TITLE"))
> +    (should (null (org-protocol-check-filename-for-protocol uri (list uri) 
> nil))))
> +  (should (equal (car org-stored-links) '("URL" "TITLE")))
> +  ;; Handle multiple slashes
> +  (let ((uri "/some/directory/org-protocol://store-link://URL2//TITLE2"))
> +    (should (null (org-protocol-check-filename-for-protocol uri (list uri) 
> nil))))
> +  (should (equal (car org-stored-links) '("URL2" "TITLE2")))
> +  ;; Ignore port - useful for KDE

> +  (let ((uri "/some/directory/org-protocol:/store-link:0//URL3//TITLE3"))
> +    (should (null (org-protocol-check-filename-for-protocol uri (list uri) 
> nil))))
> +  (should (equal (car org-stored-links) '("URL3" "TITLE3"))))

I don't know org-protocol well enough to comment on your tests.  But I
guess you should add something like this, extrapolating from other test
files,

    (unless (featurep 'org-protocol)
      (signal 'missing-test-dependency "org-protocol"))

Cheers,
Rasmus

-- 
⠠⠵