emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed

From: Rasmus <rasmus@gmx.us>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH] org-protocol: Allow optional port specification
Date: Wed, 02 Dec 2015 20:32:32 +0100	[thread overview]
Message-ID: <87zixswsrz.fsf@pank.eu> (raw)
In-Reply-To: 87wpswrg7q.fsf@sachachua.com

Hi Sacha,

Thanks for your patch.

Sacha Chua <sacha@sachachua.com> 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

-- 
⠠⠵

next prev parent reply	other threads:[~2015-12-02 19:32 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-02 16:02 [PATCH] org-protocol: Allow optional port specification Sacha Chua
2015-12-02 19:32 ` Rasmus [this message]
2015-12-03 19:01 ` Aaron Ecay
2015-12-04 22:55   ` [PATCH] org-protocol: Allow key=val&key2=value2-style URLs Sacha Chua
2015-12-05 13:35     ` Aaron Ecay
2015-12-07 17:52       ` Sacha Chua
2015-12-07 23:07         ` Sacha Chua
2015-12-18 21:49           ` Sacha Chua
2015-12-20 15:48             ` Nicolas Goaziou
2015-12-21 21:12               ` Sacha Chua
2015-12-22 12:45                 ` Nicolas Goaziou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zixswsrz.fsf@pank.eu \
    --to=rasmus@gmx.us \
    --cc=emacs-orgmode@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).