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

[Aaron Ecay]

Hi Sacha,

Thanks for your patch.

2015ko abenudak 2an, Sacha Chua-ek idatzi zuen:
> 
> 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

I think that the original format is an ad-hoc manipulation of the url
format which tries to pack two PROTOCOL:// sequences into one string.
Rather than adding a bogus port which just doubles down on this, a
better solution IMO would be to make org-protocol links valid urls in
another way, using the query string format:

org-protocol://store-link?url=[...]&title=[...]

This corresponds better to the url format: the protocol is org-protocol,
which determines emacs shall handle this link.  The location is
store-link, which indicates a handler function which is an element of
‘org-protocol-protocol-alist’, and the query string gives the arguments
to this function.

Does that make sense?

Aaron

-- 
Aaron Ecay

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

[Rasmus]

Hi Sacha,

Thanks for your patch.

Sacha Chua  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

-- 
⠠⠵

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

[Sacha Chua]

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?

>From 0533b2e76a9cb965ea8f4ea5b3804d17c2bd3b5d Mon Sep 17 00:00:00 2001
From: Sacha Chua 
Date: Wed, 2 Dec 2015 10:53:07 -0500
Subject: [PATCH] org-protocol: Allow optional port specification

* lisp/org-protocol.el (org-protocol-check-filename-for-protocol):
  Recognize and ignore ports specified as part of the protocol or
  sub-protocol. This seems to be necessary to avoid "Port field was
  empty" errors in newer versions of KDE.
* testing/lisp/test-org-protocol.el: New file with a test for
  `org-protocol-check-filename-for-protocol'.
---
 lisp/org-protocol.el  |  2 +-
 testing/lisp/test-org-protocol.el | 39 +++
 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 testing/lisp/test-org-protocol.el

diff --git a/lisp/org-protocol.el b/lisp/org-protocol.el
index 339f2b7..150f458 100644
--- a/lisp/org-protocol.el
+++ b/lisp/org-protocol.el
@@ -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)) ":[^/]*/+")))
   (when (string-match proto fname)
 (let* ((func (plist-get (cdr prolist) :function))
(greedy (plist-get (cdr prolist) :greedy))
diff --git a/testing/lisp/test-org-protocol.el b/testing/lisp/test-org-protocol.el
new file mode 100644
index 000..94be520
--- /dev/null
+++ b/testing/lisp/test-org-protocol.el
@@ -0,0 +1,39 @@
+;;; test-org-protocol.el --- tests for org-protocol.el
+
+;; Copyright (c)  Sacha Chua
+;; Authors: Sacha Chua
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see .
+
+;;; 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"
+
+
+;;; test-org-protocol.el ends here
-- 
2.6.3


(Copyright papers are on file.)

Sacha