From: John Kitchin <jkitchin@andrew.cmu.edu>
To: Nicolas Goaziou <mail@nicolasgoaziou.fr>
Cc: "emacs-orgmode@gnu.org" <emacs-orgmode@gnu.org>
Subject: Re: links-9.0 v3
Date: Thu, 07 Jul 2016 15:17:07 -0400 [thread overview]
Message-ID: <m2poqpl0lo.fsf@Johns-MacBook-Air.local> (raw)
In-Reply-To: <8737nlfqdv.fsf@saiph.selenimh>
Let me preface my reply that I think your last suggestion:
> (org-link-get-parameter (if app (concat type "+" app) type) :follow)
Is the thing to do for this set of changes for now. I think it would
wrap up this set of changes. I will send a new set of diffs that
implement this shortly after this mail.
I have some other responses below because there are some things I don't
totally understand yet.
Nicolas Goaziou writes:
> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
>>> Here is the gotcha. `type' is "file", not "file+sys" or "file+emacs",
>>> so, when checking `dedicated-function' first, we cannot tell the
>>> difference between "file+sys" and "file+emacs".
>>
>> I don't follow this. Why can't these be three types?
>
> The type is "file". "sys" or "emacs" are indications about how to follow
> them. "docview" is also an indication, but it really points to a "file".
>
>> They define 3 different follow actions right? Those are basically
>> equal to pressing RET, C-u RET and C-u C-u RET on a link. We could
>> just define three :follow functions, and one :export function for
>> them.
>
> It doesn't mean they cannot have an entry in `org-link-parameters'.
> Actually they should.
>
> However `org-link-protocols' keys are applications, not types. So
I don't understand what you mean here. The contents of org-link
protocols (in master) looks a lot like a list of (link-type :follow
:export), e.g.
#+BEGIN_SRC emacs-lisp :results code
(assoc "bbdb" org-link-protocols)
#+END_SRC
#+RESULTS:
#+BEGIN_SRC emacs-lisp
("bbdb" org-bbdb-open org-bbdb-export)
#+END_SRC
There doesn't seem to be a "sys" or "emacs" defined in
org-link-protocols in master. So there is no dedicated function being
called as far as I can tell.
#+BEGIN_SRC emacs-lisp :results code
(assoc "sys" org-link-protocols)
#+END_SRC
#+RESULTS:
#+BEGIN_SRC emacs-lisp
nil
#+END_SRC
> (org-link-get-parameter type :follow)
>
> is not a drop-in replacement for
>
> (nth 1 (assoc app org-link-protocols))
I see that these are not the same, since type != app.
With app, this would try to look up (org-link-get-parameter "sys"
:follow), which currently would return nil. That seems consistent with
what is currently in master too. I concede that if a "sys" link was
defined it would do something, but that isn't currently the case AFAICT.
Your solution solves this nicely though.
>> With the patches I sent, the three types actually work as they used to,
>> e.g. file:some.pptx opens the powerpoint file in emacs (not convenient
>> ;), file+sys:some.pptx opens it in powerpoint. This seems to be because
>> org-element-context (via org-element-link-parser) sees file+sys and
>> file+emacs as a file type.
>
> Which is correct.
>
>> Overall, this is an inconsistency to me. On one hand, we need file+sys
>> and file+emacs in org-link-parameters so that they are built into the
>> link regexps (or they would have to be hard-coded in the function that
>> makes the regexp.
>
> [...]
>
>> On the other hand, the org-open-at-point
>> function bypasses the link properties, so it is not possible to change
>> the :follow function for file+sys and file+emacs.
>
> Of course it is possible. I suggested two solutions already.
>
>>> One solution is to swap the logic order. First, if app is non-nil, we
>>> use it. If it isn't, we look after `dedicated-function'.
>>>
>>> Another solution is to add an optional parameter to the signature of
>>> the :follow function, which would be the "app" (e.g. "emacs", "sys",
>>> "docview"...) to use. I tend to think this solution is slightly better,
>>> since it doesn't require to hard-code logic in `org-open-at-point'.
>>>
>>> WDYT?
>>
>> I am not crazy about this solution,
>
> Which one? I suggested two of them.
I was referring to the optional parameter, although I reconsider it
here. I don't understand how does the "application" get to the
follow functions of links other than file+sys and file+emacs. It seems
like we would need to allow type+application:path as a link syntax and
extend the link-parser to get the application. Right now it looks like
the parser only adds application properties to file type links.
>
>> since it only applies to one type of link,
>
> Does it? Every type can benefit from this change, i.e. instead of
> calling follow function with a single argument, it is called with two,
> the second one being the application (e.g., "sys", "emacs"...) or nil.
I don't mind this (it makes links more flexible after all ;) OTOH, we
would have to "register" each type+application for the link regexp, and
then each type can have its own follow function, so it seems unnecessary
to me.
I would leave this on the table for future consideration, but consider
it outside the current scope of this set of changes. I think with your
final suggestion it isn't necessary to consider this now.
>
>> and I can't see how to use it for other follow functions. It would be
>> better IMO to define :follow functions maybe like this:
>>
>> "file" :follow #'org-open-at-point
>> "file+sys" :follow (lambda (_) (org-open-at-point '(4)))
>> "file+emacs" :follow (lambda (_) (org-open-at-point '(16)))
>>
>> and make them be honored in org-open-at-point. Then we could eliminate
>> the logic code in org-open-at-point.
>
> You may be misunderstanding the problem.
My understanding of your statement of the problem is that the
link-parser recognizes file:path, file+sys:path, and file+emacs:path as
links of type "file", with different "application" properties. In the
implementation of org-open-at-point on master, there is a cond branch
for the "file" type link, and inside that the application is checked.
Hence, without your suggestion at the end, there is not a way to access
the :follow function of file+sys or file+emacs.
To me this seems to be an unnecessary distinction from a link point of
view since those three file links could just be defined as regular links
with different follow/export functions. OTOH, perhaps there are other
places in org-mode that rely on being able to tell when a link is a
file, even if they are labeled file+sys or file+emacs that I am not
aware of?
If I compare this to what exists in org-ref, for example, there
are close to 40 different types of citations one can use, but they are
all fundamentally "cite" types. They all share a common follow action,
but have different export functions. When defined as separate links, I
use them like cite:key citenum:key, citet:key, autocite:key, etc...
Even here while I can see some utility for an application, e.g. perhaps
to open the key in zotero, or mendeley or bibtex, I would normally
associate that action with the :follow function. Am I missing something?
> The issue is that `org-open-at-point', at the moment, cannot call
> the :follow function for "file+emacs" or "file+sys" since the common
> type is "file", even if `org-link-parameters' distinguish them. IOW the
> first :follow function would always be called.
>
> Also, `org-open-at-point' shouldn't be part of a follow function, since
> `org-open-at-point' calls follow functions. It can call `org-open-file',
> tho, as currently done in `org-open-at-point'.
> Actually, I can think of a third solution, which may well follow the
> path of least resistance. Instead of calling
>
> (org-link-get-parameter type :follow)
>
> we would call
>
> (org-link-get-parameter (if app (concat type "+" app) type) :follow)
This seems like a good solution to me. The next revision of patches
implements this, along with follow functions for file+sys and file+emacs
that use org-open-file.
> and get the appropriate follow function. This solution is local to
> `org-open-at-point', but I don't think other places need :follow
> function.
It sounds good to me.
>
>>>> (let ((data (assoc type org-link-parameters)))
>>>> - (if data
>>>> - (cl-loop for (key val) on parameters by #'cddr
>>>> - do
>>>> - (setf (cl-getf (cdr data) key)
>>>> - val))
>>>> + (if data (setcdr data (org-combine-plists (cdr data) parameters))
>>>> (push (cons type parameters) org-link-parameters)
>>>> (org-make-link-regexps)
>>>> (org-element-update-syntax))))
>>>
>>> This change can be merged with `org-link-set-parameters' definition.
>>
>> I am not sure how to do that. It is like a hunk in one commit that I
>> want to move to another commit.
>
> I would edit the commit defining `org-link-set-parameters' and install
> that change there. Then, upon rebasing, I would make sure this change is
> preserved.
I think I figured this out.
>
>>>> +(defcustom org-link-parameters
>>>> + '(("http") ("https") ("ftp") ("mailto")
>>>> + ("file" :complete 'org-file-complete-link)
>>>> + ("file+emacs") ("file+sys")
>>>> + ("news") ("shell") ("elisp")
>>>> + ("doi") ("message") ("help"))
>>>
>>> See above about "file+emacs" and "file+sys", which are not valid types.
>>
>> Those either need to be here for link regexps, or hard-coded somewhere
>> else though.
>
> You're right, they need to be here, but there is still an issue.
hopefully it is solved in the next version.
>
>>Speaking of which, should coderef be a link type, so it can
>> be removed as a hard-coded string that I referenced above?
>
> No it cannot. "coderef" is not a valid link type, since there is no
> [[coderef:something]]. It shouldn't belong to the link regexp.
I only mentioned it because it seems to be in there in master via this line:
(regexp-opt (cons "coderef" org-link-types)
but it looks like it is in there in a different sort of way. It doesn't
seem important here.
Ok, that's it. Thanks for reading down this far!
>
> Regards,
--
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu
next prev parent reply other threads:[~2016-07-07 19:17 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-06 14:41 links-9.0 v3 John Kitchin
2016-07-06 22:10 ` Nicolas Goaziou
2016-07-06 23:06 ` John Kitchin
2016-07-07 1:55 ` John Kitchin
2016-07-07 8:20 ` Nicolas Goaziou
2016-07-07 13:27 ` John Kitchin
2016-07-07 14:56 ` Nicolas Goaziou
2016-07-07 19:17 ` John Kitchin [this message]
2016-07-08 21:32 ` Nicolas Goaziou
2016-07-07 19:21 ` John Kitchin
2016-07-08 21:48 ` Nicolas Goaziou
2016-07-08 22:04 ` John Kitchin
2016-07-09 13:27 ` John Kitchin
2016-07-18 12:02 ` Nicolas Goaziou
2016-07-15 1:12 ` John Kitchin
2016-07-18 11:48 ` Nicolas Goaziou
2016-07-18 15:20 ` John Kitchin
2016-07-18 16:05 ` Nicolas Goaziou
2016-07-18 16:55 ` John Kitchin
2016-07-18 21:59 ` Nicolas Goaziou
2016-07-18 23:37 ` John Kitchin
2016-08-06 1:14 ` [PATCH] " Matt Lundin
2016-08-08 0:12 ` John Kitchin
2016-08-08 5:30 ` Robert Klein
2016-08-08 9:21 ` Nicolas Goaziou
2016-08-08 9:08 ` 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=m2poqpl0lo.fsf@Johns-MacBook-Air.local \
--to=jkitchin@andrew.cmu.edu \
--cc=emacs-orgmode@gnu.org \
--cc=mail@nicolasgoaziou.fr \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).