From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Kitchin Subject: Re: links-9.0 v3 Date: Thu, 07 Jul 2016 15:17:07 -0400 Message-ID: References: <87oa6afmeu.fsf@saiph.selenimh> <87k2gxg8qm.fsf@saiph.selenimh> <8737nlfqdv.fsf@saiph.selenimh> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:39769) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLEnU-0001Pd-9n for emacs-orgmode@gnu.org; Thu, 07 Jul 2016 15:17:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLEnP-0002mj-SP for emacs-orgmode@gnu.org; Thu, 07 Jul 2016 15:17:15 -0400 Received: from mail-qk0-x22d.google.com ([2607:f8b0:400d:c09::22d]:33738) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLEnP-0002mN-MK for emacs-orgmode@gnu.org; Thu, 07 Jul 2016 15:17:11 -0400 Received: by mail-qk0-x22d.google.com with SMTP id e3so23209864qkd.0 for ; Thu, 07 Jul 2016 12:17:11 -0700 (PDT) In-reply-to: <8737nlfqdv.fsf@saiph.selenimh> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: Nicolas Goaziou Cc: "emacs-orgmode@gnu.org" 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 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