From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: [PATCH] * org-insert-link: use ido when inserting links Date: Fri, 12 Oct 2012 14:50:08 +0200 Message-ID: <87a9vr3ldr.fsf@gmail.com> References: <04D0E787-A8A1-4246-8DD2-D607E38D61BA@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([208.118.235.92]:50109) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMekU-0005es-9N for emacs-orgmode@gnu.org; Fri, 12 Oct 2012 08:53:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TMekS-0002MX-Tx for emacs-orgmode@gnu.org; Fri, 12 Oct 2012 08:53:54 -0400 Received: from mail-wg0-f49.google.com ([74.125.82.49]:48523) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMekS-0002MT-KN for emacs-orgmode@gnu.org; Fri, 12 Oct 2012 08:53:52 -0400 Received: by mail-wg0-f49.google.com with SMTP id gg4so1792326wgb.30 for ; Fri, 12 Oct 2012 05:53:51 -0700 (PDT) In-Reply-To: <04D0E787-A8A1-4246-8DD2-D607E38D61BA@gmail.com> (tony day's message of "Fri, 12 Oct 2012 14:58:29 +1100") 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-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: tony day Cc: emacs-orgmode@gnu.org Hello, Thanks for considering my suggestions. So here are new ones: tony day writes: > org.el (org-insert-link): removed a list within the list of link > creation that was causing a bug when using ido. Nitpick: "Removed a list...". Judging by your test case, it's the (mapcar 'cadr org-stored-links) part that cause problems, isn't it? I'm not sure why it should belong to the provided collection. Wouldn't dropping it solve the ido problem? > Removed the hard coded iswitch and ido switches. Just wondering: what will happen if an user wants to use iswitchb? > Changed the order of prefixes so http came up first. Please do not add unrelated "features" during a patch review. By the way, I'm not sure to agree with you: it /is/ meaningful to have user-defined link abbrevs before default types. > (org-iread-file-name): created a function that can use > ido-read-file-name if flagged as ok. Nitpick: "Created..." > (org-file-complete-link): referenced org-iread-file-name. Nitpick: "Referenced..." > --- > lisp/org.el | 44 ++++++++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/lisp/org.el b/lisp/org.el > index bdfc919..41c2572 100644 > --- a/lisp/org.el > +++ b/lisp/org.el > @@ -9311,24 +9311,22 @@ Use TAB to complete link prefixes, then RET for type-specific completion support > ;; Fake a link history, containing the stored links. > (setq tmphist (append (mapcar 'car org-stored-links) > org-insert-link-history)) > - (setq all-prefixes (append (mapcar 'car abbrevs) > - (mapcar 'car org-link-abbrev-alist) > - org-link-types)) > + (setq all-prefixes (append org-link-types > + (mapcar 'car abbrevs) > + (mapcar 'car > org-link-abbrev-alist))) For now, you can remove that part, unless it is tied to the current patch. But having http first doesn't sound convincing. > (unwind-protect > (progn > (setq link > - (let ((org-completion-use-ido nil) > - (org-completion-use-iswitchb nil)) > - (org-completing-read > - "Link: " > - (append > - (mapcar (lambda (x) (list (concat x ":"))) > - all-prefixes) > - (mapcar 'car org-stored-links) > - (mapcar 'cadr org-stored-links)) > - nil nil nil > - 'tmphist > - (caar org-stored-links)))) > + (org-completing-read > + "Link: " > + (append > + (mapcar 'cadr org-stored-links) > + (mapcar (lambda (x) (concat x ":")) > + all-prefixes) Why do you need to change that order? > + (mapcar 'car org-stored-links)) > + nil nil nil > + 'tmphist > + (cadr org-stored-links))) It means the default value will be the second stored link, if any? Is it intended? > (if (not (string-match "\\S-" link)) > (error "No link selected")) > (mapc (lambda(l) > @@ -9422,7 +9420,7 @@ Use TAB to complete link prefixes, then RET for type-specific completion support > (defun org-file-complete-link (&optional arg) > "Create a file link using completion." > (let (file link) > - (setq file (read-file-name "File: ")) > + (setq file (org-iread-file-name "File: ")) > (let ((pwd (file-name-as-directory (expand-file-name "."))) > (pwd1 (file-name-as-directory (abbreviate-file-name > (expand-file-name "."))))) > @@ -9440,6 +9438,20 @@ Use TAB to complete link prefixes, then RET for type-specific completion support > (t (setq link (concat "file:" file))))) > link)) > > +(defun org-iread-file-name (&rest args) > + "Read-file-name using `ido-mode' speedup if available. > +ARGS are arguments that may be passed to `ido-read-file-name' or `read-file-name'. > +See `read-file-name' for a description of parameters. > +" > + (org-without-partial-completion > + (if (and org-completion-use-ido > + (fboundp 'ido-read-file-name) > + (boundp 'ido-mode) ido-mode > + (listp (second args))) > + (let ((ido-enter-matching-directory nil)) > + (apply 'ido-read-file-name args)) > + (apply 'read-file-name args)))) > + > (defun org-completing-read (&rest args) > "Completing-read with SPACE being a normal character." > (let ((enable-recursive-minibuffers t) It looks good. You can answer to this mail, no need to post the patch on another thread. Regards, -- Nicolas Goaziou