Re: [O] [PATCH] org-insert-link: allow ido usage when inserting links

[tony day]

On 11 Oct 2012, at 23:23, Nicolas Goaziou <address@hidden> wrote:

> Thanks for submitting a patch. Here are a few comments.

Hi Nicolas, thanks for taking the time to go through the code.  I will resubmit 
the patch in a separate mail (I didn't know whether I could respond to your 
suggestions and submit a new patch in the same mail).

> Entries should end with a period (not the title, though). Also, if you
> haven't signed FSF papers yet, you should append "TINYCHANGE" on a line
> on its own.

I have signed the FSF papers and they have been processed.

> I don't see the interest of this change nor how it is related to
> allowing ido usage to insert links. Can
> 
>  (append
>    (mapcar (lambda (x) (list (concat x ":"))) all-prefixes)
>            (mapcar 'car org-stored-links)
>            (mapcar 'cadr org-stored-links))
> 
> contain nil values?
> 
> If so, adding a (delq nil (append ...)) should be enough. This should be
> a separate patch anyway.

The problem is actualy the list bit, which causes a bug when using ido (but not 
when using normal completion).

Having gone through it again, I don't think the append can contain nil values, 
so I removed that bit.

> Shouldn't `read-file-name' become
> `org-iread-file-name'? 

Agreed and changed.

I don't think the patch can be split into two - the bug from list is only a bug 
if ido is used.

Here's some test code it case it helps:

- unit test
    #+begin_src emacs-lisp
      ;;(setq org-stored-links nil)
      (setq org-stored-links 
            '((#("file:~/stuff/org/bugz.org::*current debugging" 28 35 
(fontified t org-category "bugz" line-prefix #("*" 0 1 (face org-hide)) 
wrap-prefix #("    " 0 4 (face org-indent)) face org-level-2) 36 45 (fontified 
t org-category "bugz" line-prefix #("*" 0 1 (face org-hide)) wrap-prefix #("    
" 0 4 (face org-indent)) face org-level-2)) "current debugging")))
      ;;(setq org-stored-links
      ;;      '((#("file:~/stuff/org/bugz.org::*test link 2" 28 32 (fontified t 
line-prefix #("**" 0 2 (face org-hide)) wrap-prefix #("      " 0 6 (face 
org-indent)) face org-level-3) 33 37 (fontified t line-prefix #("**" 0 2 (face 
org-hide)) wrap-prefix #("      " 0 6 (face org-indent)) face org-level-3) 38 
39 (fontified t line-prefix #("**" 0 2 (face org-hide)) wrap-prefix #("      " 
0 6 (face org-indent)) face org-level-3)) "test link 2") 
(#("file:~/stuff/org/bugz.org::*test link 1" 28 32 (fontified t line-prefix 
#("**" 0 2 (face org-hide)) wrap-prefix #("      " 0 6 (face org-indent)) face 
org-level-3) 33 37 (fontified t line-prefix #("**" 0 2 (face org-hide)) 
wrap-prefix #("      " 0 6 (face org-indent)) face org-level-3) 38 39 
(fontified t line-prefix #("**" 0 2 (face org-hide)) wrap-prefix #("      " 0 6 
(face org-indent)) face org-level-3)) "test link 1")))
      (setq abbrevs org-link-abbrev-alist-local)
      (setq all-prefixes (append org-link-types
                                 (mapcar 'car abbrevs)
                                 (mapcar 'car org-link-abbrev-alist)))
      (setq all-links (append
                       (mapcar 'cadr org-stored-links)
                       (mapcar (lambda (x) (concat x ":"))
                               all-prefixes)
                       (mapcar 'car org-stored-links)))
      ;;(setq all-links (delete nil all-links))
      (print (loop for link in all-links
                   collect
                   (list link)))  
       
  #+end_src


Tony