From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Kitchin Subject: Re: links-9.0 v3 Date: Wed, 06 Jul 2016 19:06:08 -0400 Message-ID: References: <87oa6afmeu.fsf@saiph.selenimh> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:36542) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKvtX-0007bx-Q2 for emacs-orgmode@gnu.org; Wed, 06 Jul 2016 19:06:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKvtT-0001pn-NK for emacs-orgmode@gnu.org; Wed, 06 Jul 2016 19:06:15 -0400 Received: from mail-qk0-x230.google.com ([2607:f8b0:400d:c09::230]:35782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKvtT-0001pa-Ga for emacs-orgmode@gnu.org; Wed, 06 Jul 2016 19:06:11 -0400 Received: by mail-qk0-x230.google.com with SMTP id s126so1009929qkh.2 for ; Wed, 06 Jul 2016 16:06:11 -0700 (PDT) In-reply-to: <87oa6afmeu.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" Thanks. I responded to some below. I didn't respond to all of them. I will revise the commits accordingly and send a new version. Nicolas Goaziou writes: > Hello, > > John Kitchin writes: > >> Your version doesn't let you add properties to links with no >> existing properties, e.g. ("http") > > There was a typo (spurious `cdr'), the correct version is > > (defun org-link-set-parameters (type &rest parameters) > "Set link TYPE properties to PARAMETERS. > PARAMETERS should be :key val pairs." > (let ((data (assoc type org-link-parameters))) > (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)))) > >> and it also didn't work as expected to set properties to nil. I will give this a try. > > Not sure to understand what you mean here. Could you give an example? If I ran the previous function a few times like this (org-link-set-parameters "http" :follow nil) I would get ("http" :follow :follow nil nil nil...) probably because of the bug. I will let you know if it works. >> I think I have squashed everything together that makes sense. Let me >> know if you have further thoughts. > > OK. Some comments follow. > >> +{{{noindent}}} In-buffer completion (see [[Completion]]) can be used >> +after {{{samp([)}}} to complete link abbreviations. You may also >> +define a function that implements special (e.g., completion) support >> +for inserting such a link with {{{kbd(C-c C-l)}}}. Such a function >> +should not accept any arguments, and return the full link with >> +prefix. You can set the link completion function like this: > > Mind the spaces after sentences. I will fix these. > > In any case, updating contrib manual is not top priority, since many > section needs to be updated anyway. > >> +[fn:37] This works if a function has been defined in the :complete >> +property of a link in org-link-parameters. > > ~org-link-parameters~ > >> -(org-add-link-type "id" 'org-id-open) >> +(org-link-set-parameters "id" :follow 'org-id-open) > > Nitpick: #'org-id-open > >> -(add-hook 'org-store-link-functions 'org-w3m-store-link) >> +(org-link-set-parameters "w3m" :store 'org-w3m-store-link) > > #'org-w3m-store-link > > You get the idea, I will not comment the other occurrences. Just to make sure I get it, all functions should be sharp quoted like this? > >> + (type (save-match-data >> + (if (string-match org-plain-link-re hl) >> + (match-string-no-properties 1 hl) >> + nil))) > > Nitpick > > (save-match-data > (and (string-match org-plain-link-re hl) > (match-string-no-properties 1 hl))) > >> + (path (save-match-data >> + (if (string-match org-plain-link-re hl) >> + (match-string-no-properties 2 hl) >> + nil))) > > Ditto. ok > >> - ((assoc type org-link-parameters) >> + ((and (assoc type org-link-parameters) >> + (functionp (org-link-get-parameter type :follow))) > > I think (functionp ...) is sufficient, no need for the `and' and > (assoc type org-link-parameters), which `org-link-get-parameters' > already takes case of. So > > ((functionp (org-link-get-parameters type :follow) > ...) > >> * lisp/org.el: Replace the variable `org-store-link-functions' with a >> function by the same name. > > You need to add the name of the function being modified. > >> Update org-add-link-type >> >> * lisp/org.el org-add-link-type: deprecated and now calls >> `org-link-add'. >> >> Create a new `org-link-add' function for making links. > > You probably mean `org-link-set-parameters', since `org-link-add' would > be but a lesser version of the former. > >> + (org-link-add type :follow follow :export export) > > See above. > >> +(defun org-store-link-functions () >> + "Returns a list of functions that are called to create and store a link. >> +The functions in the variable `org-store-link-functions' come >> +first. These may be user-defined for different contexts. Then >> +comes the functions defined in the :store property of >> +org-link-parameters. >> + >> +Each function will be called in turn until one returns a non-nil >> +value. Each function should check if it is responsible for >> +creating this link (for example by looking at the major mode). If >> +not, it must exit and return nil. If yes, it should return a >> +non-nil value after a calling `org-store-link-props' with a list >> +of properties and values. Special properties are: > > Mind the spaces between sentences. > > I don't understand why we need to both preserve > `org-store-link-functions' and use :store property. Wouldn't one > location be enough? > It probably isn't necessary. I will take it out. >> +:type The link prefix, like \"http\". This must be given. >> +:link The link, like \"http://www.astro.uva.nl/~dominik\". >> + This is obligatory as well. >> +:description Optional default description for the second pair >> + of brackets in an Org-mode link. The user can still change >> + this when inserting this link into an Org-mode buffer. > > Org-mode -> Org mode > >> +In addition to these, any additional properties can be specified >> +and then used in capture templates." >> + (append >> + org-store-link-functions >> + (cl-loop for link in org-link-parameters >> + with store-func >> + do (setq store-func (org-link-get-parameter (car link) :store)) >> + if store-func >> + collect store-func))) > > The above looks dubious. You seem to be collection :store values for > every link type, in addition to user-defined functions. It doesn't make > sense to run unrelated store functions. see above. > >> - ((assoc type org-link-protocols) >> - (funcall (nth 1 (assoc type org-link-protocols)) path)) >> + ((assoc type org-link-parameters) >> + (funcall (org-link-get-parameter type :follow) path)) > > (assoc type org-link-parameters) -> (functionp (org-link-get-parameter type :follow)) > >> +(defcustom org-link-parameters >> + '(("http") ("https") ("ftp") ("mailto") >> + ("file" :complete 'org-file-complete-link) >> + ("file+emacs") ("file+sys") >> + ("news") ("shell") ("elisp") >> + ("doi") ("message") ("help")) >> + "An alist of properties that defines all the links in Org mode. >> +The key in each association is a string of the link type. >> +Subsequent optional elements make up a p-list of link properties. >> + >> +:follow - A function that takes the link path as an argument. >> + >> +:export - A function that takes the link path, description and >> +export-backend as arguments. >> + >> +:store - A function responsible for storing the link. See the >> +variable `org-store-link-functions'. >> + >> +:complete - A function that inserts a link with completion. The >> +function takes one optional prefix arg. >> + >> +:face - A face for the link, or a function that returns a face. >> +The function takes one argument which is the link path. The >> +default face is `org-link'. >> + >> +:mouse-face - The mouse-face. The default is `highlight'. >> + >> +:display - `full' will not fold the link in descriptive >> +display. Default is `org-link'. >> + >> +:help-echo - A string or function that takes (window object position) >> +as args and returns a string. >> + >> +:keymap - A keymap that is active on the link. The default is >> +`org-mouse-map'. >> + >> +:htmlize-link - A function for the htmlize-link. Defaults >> +to (list :uri \"type:path\") >> + >> +:activate-func - A function to run at the end of font-lock >> +activation. The function must accept (link-start link-end path bracketp) > ^^^ > >> +as arguments." >> + :group 'org-link >> + :type '(alist :tag "Link display paramters" >> + :value-type (plist))) > > Isn't it plist instead of (plist)? > >> +(defun org-link-get-parameter (type key) >> + "Get TYPE link property for KEY." > > We could add that TYPE is string and KEY a keyword. > >> +(defun org-link-set-parameters (type &rest parameters) >> + "Set link TYPE properties to PARAMETERS. >> + PARAMETERS should be :key val pairs." >> + (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)) >> + (push (cons type parameters) org-link-parameters) >> + (org-make-link-regexps) >> + (org-element-update-syntax)))) > > See at the beginning of the message, I didn't give up yet ;) > >> commit a4b9ed7783437838f909f3516fb53f32598e803e >> Author: John Kitchin >> Date: Tue Jul 5 10:07:10 2016 -0400 >> >> Remove org-link-types >> >> * lisp/org.el (org-link-types): Remove this variable. >> >> * testing/lisp/test-ox.el (test-org-export/custom-protocol-maybe): >> >> Remove copy-sequence in the tests. Actually it seems like the let >> statement is no longer needed. I guess it was around to protect the list >> from extra links getting defined. A downside of this is the links may >> get added during tests. That does not seem like a big deal if those are >> run in batch mode. > > The copy-sequence was used to prevent modifying `org-link-types' by > side-effect. It is not needed anymore since (org-link-types) generates > a new list every time. > > I think we're almost there, barring my stubbornness about > `org-link-set-parameters'. Thank you for this work. No problem, thanks for the detailed feedback and tips! I will send a new set of revised diffs later. > > 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