Re: [O] links-9.0 v3

[John Kitchin]

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 <address@hidden> 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