Any objection to adding completing-read-function?

Any objection to adding completing-read-function?

[Leo]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

Hello all,

I have been locally using a variable like that to replace
completing-read with ido-completing-read and it appears to me to improve
efficiency in many places.

So I wonder if there is any objection to adding a new variable
completing-read-function that when set replaces completing-read? Let me
know if I should submit it to the bug tracker.

After applying the attached patch, one may customise it like this:

(setq completing-read-function 'ido-completing-read*)
(defun ido-completing-read* (prompt choices &optional predicate require-match
                                    initial-input hist def inherit-input-method)
  (if (and (listp choices) (not (functionp choices)))
      (ido-completing-read prompt (if (listp (car choices))
                                      (mapcar 'car choices)
                                    choices)
                           predicate require-match initial-input hist def inherit-input-method)
    (let ((completing-read-function nil))
      (completing-read prompt choices predicate require-match
                       initial-input hist def inherit-input-method))))

Here are some screenshots:

I switching to a bookmark:

[-- Attachment #2: bookmark.png --]
[-- Type: image/png, Size: 18926 bytes --]

[-- Attachment #3: Type: text/plain, Size: 33 bytes --]


Switching to a branch in magit:

[-- Attachment #4: emacs.png --]
[-- Type: image/png, Size: 18752 bytes --]

[-- Attachment #5: Type: text/plain, Size: 32 bytes --]


Loading lisp systems in slime:

[-- Attachment #6: slime.png --]
[-- Type: image/png, Size: 18975 bytes --]

[-- Attachment #7: Type: text/plain, Size: 8 bytes --]


Patch:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: crf.diff --]
[-- Type: text/x-diff, Size: 3807 bytes --]

diff --git a/lisp/ido.el b/lisp/ido.el
index e52a753..de12c8a 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2011,11 +2011,12 @@ If INITIAL is non-nil, it specifies the initial input string."
 	(setq ido-exit nil)
 	(setq ido-final-text
 	      (catch 'ido
-		(completing-read
-		 (ido-make-prompt item prompt)
-		 '(("dummy" . 1)) nil nil ; table predicate require-match
-		 (prog1 ido-text-init (setq ido-text-init nil))	;initial-contents
-		 history))))
+		(let ((completing-read-function nil))
+		  (completing-read
+		   (ido-make-prompt item prompt)
+		   '(("dummy" . 1)) nil nil ; table predicate require-match
+		   (prog1 ido-text-init (setq ido-text-init nil)) ;initial-contents
+		   history)))))
       (ido-trace "completing-read" ido-final-text)
       (if (get-buffer ido-completion-buffer)
 	  (kill-buffer ido-completion-buffer))
@@ -4835,7 +4836,7 @@ See `read-directory-name' for additional parameters."
 	  (concat ido-current-directory filename)))))
 
 ;;;###autoload
-(defun ido-completing-read (prompt choices &optional predicate require-match initial-input hist def)
+(defun ido-completing-read (prompt choices &optional predicate require-match initial-input hist def inherit-input-method)
   "Ido replacement for the built-in `completing-read'.
 Read a string in the minibuffer with ido-style completion.
 PROMPT is a string to prompt with; normally it ends in a colon and a space.
diff --git a/src/minibuf.c b/src/minibuf.c
index 564346f..930d50f 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -132,6 +132,7 @@ Lisp_Object Vminibuffer_completion_table, Qminibuffer_completion_table;
 Lisp_Object Vminibuffer_completion_predicate, Qminibuffer_completion_predicate;
 Lisp_Object Vminibuffer_completion_confirm, Qminibuffer_completion_confirm;
 Lisp_Object Vminibuffer_completing_file_name;
+Lisp_Object Vcompleting_read_function;
 
 Lisp_Object Quser_variable_p;
 
@@ -1721,6 +1722,9 @@ with a space are ignored unless STRING itself starts with a space.  */)
 \f
 DEFUN ("completing-read", Fcompleting_read, Scompleting_read, 2, 8, 0,
        doc: /* Read a string in the minibuffer, with completion.
+If `completing-read-function' is non-nil, call it with all arguments passed
+to `completing-read'.
+
 PROMPT is a string to prompt with; normally it ends in a colon and a space.
 COLLECTION can be a list of strings, an alist, an obarray or a hash table.
 COLLECTION can also be a function to do the completion itself.
@@ -1779,9 +1783,25 @@ Completion ignores case if the ambient value of
      Lisp_Object hist, def, inherit_input_method;
 {
   Lisp_Object val, histvar, histpos, position;
+  Lisp_Object args[9];
   Lisp_Object init;
   int pos = 0;
   int count = SPECPDL_INDEX ();
+
+  if (! NILP (Vcompleting_read_function))
+    {
+      args[0] = Vcompleting_read_function;
+      args[1] = prompt;
+      args[2] = collection;
+      args[3] = predicate;
+      args[4] = require_match;
+      args[5] = initial_input;
+      args[6] = hist;
+      args[7] = def;
+      args[8] = inherit_input_method;
+      return Ffuncall (9, args);
+    }
+
   struct gcpro gcpro1;
 
   init = initial_input;
@@ -2216,6 +2236,12 @@ If the value is `confirm-after-completion', the user may exit with an
 	       doc: /* Non-nil means completing file names.  */);
   Vminibuffer_completing_file_name = Qnil;
 
+  DEFVAR_LISP ("completing-read-function",
+               &Vcompleting_read_function,
+               doc: /* Non-nil means `completing-read' does its work by calling this function.
+The function will receive all arguments passed to `completing-read'.  */);
+  Vcompleting_read_function = Qnil;
+
   DEFVAR_LISP ("minibuffer-help-form", &Vminibuffer_help_form,
 	       doc: /* Value that `help-form' takes on inside the minibuffer.  */);
   Vminibuffer_help_form = Qnil;

[-- Attachment #9: Type: text/plain, Size: 13 bytes --]


Cheers,
Leo

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> So I wonder if there is any objection to adding a new variable
> completing-read-function that when set replaces completing-read? Let me
> know if I should submit it to the bug tracker.

There's been such requests in the past, which I've usually resisted.
But I guess it's OK to do such a thing.
A few points to note, tho:

> Here are some screenshots:
> I switching to a bookmark:
[...]
> Switching to a branch in magit:
[...]
> Loading lisp systems in slime:
[...]

You can get similar results with M-x icomplete-mode, possibly combined
with changing completion-styles (e.g. to add substring matching).

Generally, I'd much rather see the default completion improved than
side-stepped.

One other thing: a variable completing-read-function should not allow
a nil value, instead its default value should be
`completing-read-default' which is the current completing-read, so you
can always funcall completing-read-function without checking if
it's nil.


        Stefan

RE: Any objection to adding completing-read-function?

[Drew Adams]

> I have been locally using a variable like that to replace
> completing-read with ido-completing-read and it appears to me 
> to improve efficiency in many places.
> 
> So I wonder if there is any objection to adding a new variable
> completing-read-function that when set replaces 
> completing-read? Let me know if I should submit it to the
> bug tracker.

Thank you, Leo.  I strongly support such a change (I've requested it in the
past).

It lets you do for `completing-read' what you have been able to do for
`read-file-name' since it was moved to Lisp: write your own completion function
yet have ordinary, existing calls to `completing-read' pick that up.

No need to redefine or advise anything.  No need to limit your changes to what
is allowed/foreseen by the implementation of `completing-read'.

And of course you can scope such a behavior change within a minor mode or using
`let' instead of `setq'.

Very simple, clean.  Thanks.

RE: Any objection to adding completing-read-function?

[Drew Adams]

> There's been such requests in the past, which I've usually resisted.
> But I guess it's OK to do such a thing.

Good.  Let's please do it.

> a variable completing-read-function should not allow
> a nil value, instead its default value should be
> `completing-read-default' which is the current completing-read, so you
> can always funcall completing-read-function without checking if
> it's nil.

Then that should have been the approach for `read-file-name-function' as well.
These two should be kept similar (purpose, behavior), IMO.

There are already places (both in vanilla Emacs code and in 3rd-party libraries)
that bind `read-file-name-function' to nil to get the default behavior.

So we have a choice of asymmetry between `read-file-name' and `completing-read'
or breaking existing code and backward compatibility.  Or just doing what Leo
did: use nil for the default behavior.

I agree with what you say about not having to check for nil.  But that's where
we are at present.

Re: Any objection to adding completing-read-function?

[Leo]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

On 2010-12-28 14:34 +0000, Stefan Monnier wrote:
>> So I wonder if there is any objection to adding a new variable
>> completing-read-function that when set replaces completing-read? Let me
>> know if I should submit it to the bug tracker.
>
> There's been such requests in the past, which I've usually resisted.
> But I guess it's OK to do such a thing.
> A few points to note, tho:

OK. I'll submit it to the bug tracker later.

>> Here are some screenshots:
>> I switching to a bookmark:
> [...]
>> Switching to a branch in magit:
> [...]
>> Loading lisp systems in slime:
> [...]
>
> You can get similar results with M-x icomplete-mode, possibly combined
> with changing completion-styles (e.g. to add substring matching).
>
> Generally, I'd much rather see the default completion improved than
> side-stepped.

I agree.

> One other thing: a variable completing-read-function should not allow
> a nil value, instead its default value should be
> `completing-read-default' which is the current completing-read, so you
> can always funcall completing-read-function without checking if it's
> nil.

I have modified the patch to be like this. Since there are quite a few
libraries both in emacs and 3rd party calling completing-read directly,
in the patch the old completing-read is now completing-read-default and
the new completing-read calls completing-read-function, which defaults
to completing-read-default. Is this OK? Thanks.

>
>         Stefan

Leo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: crf.diff --]
[-- Type: text/x-diff, Size: 4574 bytes --]

diff --git a/lisp/ido.el b/lisp/ido.el
index e52a753..0f464d3 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -2011,7 +2011,7 @@
 	(setq ido-exit nil)
 	(setq ido-final-text
 	      (catch 'ido
-		(completing-read
+		(completing-read-default
 		 (ido-make-prompt item prompt)
 		 '(("dummy" . 1)) nil nil ; table predicate require-match
 		 (prog1 ido-text-init (setq ido-text-init nil))	;initial-contents
@@ -4835,7 +4835,7 @@ See `read-directory-name' for additional parameters."
 	  (concat ido-current-directory filename)))))
 
 ;;;###autoload
-(defun ido-completing-read (prompt choices &optional predicate require-match initial-input hist def)
+(defun ido-completing-read (prompt choices &optional predicate require-match initial-input hist def inherit-input-method)
   "Ido replacement for the built-in `completing-read'.
 Read a string in the minibuffer with ido-style completion.
 PROMPT is a string to prompt with; normally it ends in a colon and a space.
diff --git a/src/minibuf.c b/src/minibuf.c
index 564346f..6e7e18b 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -132,6 +132,7 @@ Lisp_Object Vminibuffer_completion_table, Qminibuffer_completion_table;
 Lisp_Object Vminibuffer_completion_predicate, Qminibuffer_completion_predicate;
 Lisp_Object Vminibuffer_completion_confirm, Qminibuffer_completion_confirm;
 Lisp_Object Vminibuffer_completing_file_name;
+Lisp_Object Qcompleting_read_default, Vcompleting_read_function;
 
 Lisp_Object Quser_variable_p;
 
@@ -1721,6 +1722,27 @@ with a space are ignored unless STRING itself starts with a space.  */)
 \f
 DEFUN ("completing-read", Fcompleting_read, Scompleting_read, 2, 8, 0,
        doc: /* Read a string in the minibuffer, with completion.
+This function calls `completing-read-function' to do the work, which
+defaults to `completing-read-default' (which see).  */)
+     (prompt, collection, predicate, require_match, initial_input, hist, def, inherit_input_method)
+     Lisp_Object prompt, collection, predicate, require_match, initial_input;
+     Lisp_Object hist, def, inherit_input_method;
+{
+  Lisp_Object args[9];
+  args[0] = Vcompleting_read_function;
+  args[1] = prompt;
+  args[2] = collection;
+  args[3] = predicate;
+  args[4] = require_match;
+  args[5] = initial_input;
+  args[6] = hist;
+  args[7] = def;
+  args[8] = inherit_input_method;
+  return Ffuncall (9, args);
+}
+
+DEFUN ("completing-read-default", Fcompleting_read_default, Scompleting_read_default, 2, 8, 0,
+       doc: /* Default function for `completing-read-function'.
 PROMPT is a string to prompt with; normally it ends in a colon and a space.
 COLLECTION can be a list of strings, an alist, an obarray or a hash table.
 COLLECTION can also be a function to do the completion itself.
@@ -1741,9 +1763,9 @@ REQUIRE-MATCH can take the following values:
 - anything else behaves like t except that typing RET does not exit if it
   does non-null completion.
 
-If the input is null, `completing-read' returns DEF, or the first element
-of the list of default values, or an empty string if DEF is nil,
-regardless of the value of REQUIRE-MATCH.
+If the input is null, `completing-read-default' returns DEF, or the
+first element of the list of default values, or an empty string if DEF
+is nil, regardless of the value of REQUIRE-MATCH.
 
 If INITIAL-INPUT is non-nil, insert it in the minibuffer initially,
   with point positioned at the end.
@@ -2077,6 +2099,9 @@ syms_of_minibuf ()
   minibuf_save_list = Qnil;
   staticpro (&minibuf_save_list);
 
+  Qcompleting_read_default = intern_c_string ("completing-read-default");
+  staticpro (&Qcompleting_read_default);
+
   Qcompletion_ignore_case = intern_c_string ("completion-ignore-case");
   staticpro (&Qcompletion_ignore_case);
 
@@ -2216,6 +2241,11 @@ If the value is `confirm-after-completion', the user may exit with an
 	       doc: /* Non-nil means completing file names.  */);
   Vminibuffer_completing_file_name = Qnil;
 
+  DEFVAR_LISP ("completing-read-function",
+               &Vcompleting_read_function,
+               doc: /* The function to be called by `completing-read'.  */);
+  Vcompleting_read_function = Qcompleting_read_default;
+
   DEFVAR_LISP ("minibuffer-help-form", &Vminibuffer_help_form,
 	       doc: /* Value that `help-form' takes on inside the minibuffer.  */);
   Vminibuffer_help_form = Qnil;
@@ -2291,6 +2321,7 @@ properties.  */);
   defsubr (&Stest_completion);
   defsubr (&Sassoc_string);
   defsubr (&Scompleting_read);
+  defsubr (&Scompleting_read_default);
 }
 
 /* arch-tag: 8f69b601-fba3-484c-a6dd-ceaee54a7a73

Re: Any objection to adding completing-read-function?

[Leo]

On 2010-12-28 16:17 +0000, Drew Adams wrote:
> Thank you, Leo.  I strongly support such a change (I've requested it in the
> past).

Thanks.

> It lets you do for `completing-read' what you have been able to do for
> `read-file-name' since it was moved to Lisp: write your own completion function
> yet have ordinary, existing calls to `completing-read' pick that up.
>
> No need to redefine or advise anything.  No need to limit your changes to what
> is allowed/foreseen by the implementation of `completing-read'.
>
> And of course you can scope such a behavior change within a minor mode or using
> `let' instead of `setq'.
>
> Very simple, clean.  Thanks.

I agree with these merits, precisely the reason that prompted me to do
this.

Leo

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> Then that should have been the approach for `read-file-name-function'
> as well.

Yes.  Feel free to go back to travel back in time and fix it there.
In the mean time we can do the right thing for new `foo-function's.


        Stefan

RE: Any objection to adding completing-read-function?

[Drew Adams]

> > Then that should have been the approach for 
> > `read-file-name-function' as well.
> 
> Yes.  Feel free to go back to travel back in time and fix it there.
> In the mean time we can do the right thing for new `foo-function's.

How about fixing `read-file-name-function' the same way now, so the two will be
symmetric?  In the vanilla Emacs sources only Ido uses `read-file-name-function'
at present.  3rd parties can adjust accordingly.

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> I have modified the patch to be like this. Since there are quite a few
> libraries both in emacs and 3rd party calling completing-read directly,
> in the patch the old completing-read is now completing-read-default and
> the new completing-read calls completing-read-function, which defaults
> to completing-read-default. Is this OK? Thanks.

Yes, looks good.  Feel free to install it, after fixing the details below:

>  DEFUN ("completing-read", Fcompleting_read, Scompleting_read, 2, 8, 0,
>         doc: /* Read a string in the minibuffer, with completion.
> +This function calls `completing-read-function' to do the work, which
> +defaults to `completing-read-default' (which see).  */)

The docstring needs to describe the meaning of all arguments, just as it
did before (i.e. it should stay unmodified, except for mentioning
`completing-read-function').  OTOH it doesn't need to mention the
default value of `completing-read-function'.

> +DEFUN ("completing-read-default", Fcompleting_read_default, Scompleting_read_default, 2, 8, 0,
> +       doc: /* Default function for `completing-read-function'.

This docstring, OTOH, can refer to `completing-read' for the meaning of
its arguments.

> +  DEFVAR_LISP ("completing-read-function",
> +               &Vcompleting_read_function,
> +               doc: /* The function to be called by `completing-read'.  */);

The text should say something like "should accept the same arguments as
`completing-read'".


        Stefan

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

>> > Then that should have been the approach for 
>> > `read-file-name-function' as well.
>> Yes.  Feel free to go back to travel back in time and fix it there.
>> In the mean time we can do the right thing for new `foo-function's.

> How about fixing `read-file-name-function' the same way now, so the
> two will be symmetric?  In the vanilla Emacs sources only Ido uses
> `read-file-name-function' at present.  3rd parties can
> adjust accordingly.

Fine by me,


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2010-12-28 22:03 +0000, Stefan Monnier wrote:
>> How about fixing `read-file-name-function' the same way now, so the
>> two will be symmetric?  In the vanilla Emacs sources only Ido uses
>> `read-file-name-function' at present.  3rd parties can
>> adjust accordingly.
>
> Fine by me,

Is the following patch (against 24, not touching ido.el yet) something
people have in mind? - Leo

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 8d09d5d..7038f1c 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1474,8 +1474,9 @@ except that it passes the file name through `substitute-in-file-name'."
                             'completion--file-name-table)
   "Internal subroutine for `read-file-name'.  Do not call this.")
 
-(defvar read-file-name-function nil
-  "If this is non-nil, `read-file-name' does its work by calling this function.")
+(defvar read-file-name-function 'read-file-name-default
+  "The function called by `read-file-name' to do its work.
+It should accept the same arguments as `read-file-name'.")
 
 (defcustom read-file-name-completion-ignore-case
   (if (memq system-type '(ms-dos windows-nt darwin cygwin))
@@ -1513,7 +1514,7 @@ such as making the current buffer visit no file in the case of
 (declare-function x-file-dialog "xfns.c"
                   (prompt dir &optional default-filename mustmatch only-dir-p))
 
-(defun read-file-name-defaults (&optional dir initial)
+(defun read-file-name--defaults (&optional dir initial)
   (let ((default
 	  (cond
 	   ;; With non-nil `initial', use `dir' as the first default.
@@ -1539,6 +1540,141 @@ such as making the current buffer visit no file in the case of
      (if (listp minibuffer-default) minibuffer-default (list minibuffer-default))
      (if (listp default) default (list default)))))
 
+(defun read-file-name-default (prompt &optional dir default-filename mustmatch initial predicate)
+  "Default method for reading file names.
+See `read-file-name' for the meaning of the arguments."
+  (unless dir (setq dir default-directory))
+  (unless (file-name-absolute-p dir) (setq dir (expand-file-name dir)))
+  (unless default-filename
+    (setq default-filename (if initial (expand-file-name initial dir)
+                             buffer-file-name)))
+  ;; If dir starts with user's homedir, change that to ~.
+  (setq dir (abbreviate-file-name dir))
+  ;; Likewise for default-filename.
+  (if default-filename
+      (setq default-filename
+	    (if (consp default-filename)
+		(mapcar 'abbreviate-file-name default-filename)
+	      (abbreviate-file-name default-filename))))
+  (let ((insdef (cond
+                 ((and insert-default-directory (stringp dir))
+                  (if initial
+                      (cons (minibuffer--double-dollars (concat dir initial))
+                            (length (minibuffer--double-dollars dir)))
+                    (minibuffer--double-dollars dir)))
+                 (initial (cons (minibuffer--double-dollars initial) 0)))))
+    
+    (let ((completion-ignore-case read-file-name-completion-ignore-case)
+	  (minibuffer-completing-file-name t)
+	  (pred (or predicate 'file-exists-p))
+	  (add-to-history nil))
+
+      (let* ((val
+	      (if (or (not (next-read-file-uses-dialog-p))
+		      ;; Graphical file dialogs can't handle remote
+		      ;; files (Bug#99).
+		      (file-remote-p dir))
+		  ;; We used to pass `dir' to `read-file-name-internal' by
+		  ;; abusing the `predicate' argument.  It's better to
+		  ;; just use `default-directory', but in order to avoid
+		  ;; changing `default-directory' in the current buffer,
+		  ;; we don't let-bind it.
+		  (lexical-let ((dir (file-name-as-directory
+				      (expand-file-name dir))))
+		    (minibuffer-with-setup-hook
+			(lambda ()
+			  (setq default-directory dir)
+			  ;; When the first default in `minibuffer-default'
+			  ;; duplicates initial input `insdef',
+			  ;; reset `minibuffer-default' to nil.
+			  (when (equal (or (car-safe insdef) insdef)
+				       (or (car-safe minibuffer-default)
+					   minibuffer-default))
+			    (setq minibuffer-default
+				  (cdr-safe minibuffer-default)))
+			  ;; On the first request on `M-n' fill
+			  ;; `minibuffer-default' with a list of defaults
+			  ;; relevant for file-name reading.
+			  (set (make-local-variable 'minibuffer-default-add-function)
+			       (lambda ()
+				 (with-current-buffer
+				     (window-buffer (minibuffer-selected-window))
+				   (read-file-name--defaults dir initial)))))
+		      (completing-read prompt 'read-file-name-internal
+				       pred mustmatch insdef
+				       'file-name-history default-filename)))
+		;; If DEFAULT-FILENAME not supplied and DIR contains
+		;; a file name, split it.
+		(let ((file (file-name-nondirectory dir))
+		      ;; When using a dialog, revert to nil and non-nil
+		      ;; interpretation of mustmatch. confirm options
+		      ;; need to be interpreted as nil, otherwise
+		      ;; it is impossible to create new files using
+		      ;; dialogs with the default settings.
+		      (dialog-mustmatch
+		       (not (memq mustmatch
+				  '(nil confirm confirm-after-completion)))))
+		  (when (and (not default-filename)
+			     (not (zerop (length file))))
+		    (setq default-filename file)
+		    (setq dir (file-name-directory dir)))
+		  (when default-filename
+		    (setq default-filename
+			  (expand-file-name (if (consp default-filename)
+						(car default-filename)
+					      default-filename)
+					    dir)))
+		  (setq add-to-history t)
+		  (x-file-dialog prompt dir default-filename
+				 dialog-mustmatch
+				 (eq predicate 'file-directory-p)))))
+
+	     (replace-in-history (eq (car-safe file-name-history) val)))
+	;; If completing-read returned the inserted default string itself
+	;; (rather than a new string with the same contents),
+	;; it has to mean that the user typed RET with the minibuffer empty.
+	;; In that case, we really want to return ""
+	;; so that commands such as set-visited-file-name can distinguish.
+	(when (consp default-filename)
+	  (setq default-filename (car default-filename)))
+	(when (eq val default-filename)
+	  ;; In this case, completing-read has not added an element
+	  ;; to the history.  Maybe we should.
+	  (if (not replace-in-history)
+	      (setq add-to-history t))
+	  (setq val ""))
+	(unless val (error "No file name specified"))
+
+	(if (and default-filename
+		 (string-equal val (if (consp insdef) (car insdef) insdef)))
+	    (setq val default-filename))
+	(setq val (substitute-in-file-name val))
+
+	(if replace-in-history
+	    ;; Replace what Fcompleting_read added to the history
+	    ;; with what we will actually return.  As an exception,
+	    ;; if that's the same as the second item in
+	    ;; file-name-history, it's really a repeat (Bug#4657).
+	    (let ((val1 (minibuffer--double-dollars val)))
+	      (if history-delete-duplicates
+		  (setcdr file-name-history
+			  (delete val1 (cdr file-name-history))))
+	      (if (string= val1 (cadr file-name-history))
+		  (pop file-name-history)
+		(setcar file-name-history val1)))
+	  (if add-to-history
+	      ;; Add the value to the history--but not if it matches
+	      ;; the last value already there.
+	      (let ((val1 (minibuffer--double-dollars val)))
+		(unless (and (consp file-name-history)
+			     (equal (car file-name-history) val1))
+		  (setq file-name-history
+			(cons val1
+			      (if history-delete-duplicates
+				  (delete val1 file-name-history)
+				file-name-history)))))))
+	val))))
+
 (defun read-file-name (prompt &optional dir default-filename mustmatch initial predicate)
   "Read file name, prompting with PROMPT and completing in directory DIR.
 Value is not expanded---you must call `expand-file-name' yourself.
@@ -1580,140 +1716,8 @@ treated as equivalent to nil.
 
 See also `read-file-name-completion-ignore-case'
 and `read-file-name-function'."
-  (unless dir (setq dir default-directory))
-  (unless (file-name-absolute-p dir) (setq dir (expand-file-name dir)))
-  (unless default-filename
-    (setq default-filename (if initial (expand-file-name initial dir)
-                             buffer-file-name)))
-  ;; If dir starts with user's homedir, change that to ~.
-  (setq dir (abbreviate-file-name dir))
-  ;; Likewise for default-filename.
-  (if default-filename
-      (setq default-filename
-	    (if (consp default-filename)
-		(mapcar 'abbreviate-file-name default-filename)
-	      (abbreviate-file-name default-filename))))
-  (let ((insdef (cond
-                 ((and insert-default-directory (stringp dir))
-                  (if initial
-                      (cons (minibuffer--double-dollars (concat dir initial))
-                            (length (minibuffer--double-dollars dir)))
-                    (minibuffer--double-dollars dir)))
-                 (initial (cons (minibuffer--double-dollars initial) 0)))))
-
-    (if read-file-name-function
-        (funcall read-file-name-function
-                 prompt dir default-filename mustmatch initial predicate)
-      (let ((completion-ignore-case read-file-name-completion-ignore-case)
-            (minibuffer-completing-file-name t)
-            (pred (or predicate 'file-exists-p))
-            (add-to-history nil))
-
-        (let* ((val
-                (if (or (not (next-read-file-uses-dialog-p))
-			;; Graphical file dialogs can't handle remote
-			;; files (Bug#99).
-			(file-remote-p dir))
-                    ;; We used to pass `dir' to `read-file-name-internal' by
-                    ;; abusing the `predicate' argument.  It's better to
-                    ;; just use `default-directory', but in order to avoid
-                    ;; changing `default-directory' in the current buffer,
-                    ;; we don't let-bind it.
-                    (lexical-let ((dir (file-name-as-directory
-                                        (expand-file-name dir))))
-                      (minibuffer-with-setup-hook
-                          (lambda ()
-			    (setq default-directory dir)
-			    ;; When the first default in `minibuffer-default'
-			    ;; duplicates initial input `insdef',
-			    ;; reset `minibuffer-default' to nil.
-			    (when (equal (or (car-safe insdef) insdef)
-					 (or (car-safe minibuffer-default)
-					     minibuffer-default))
-			      (setq minibuffer-default
-				    (cdr-safe minibuffer-default)))
-			    ;; On the first request on `M-n' fill
-			    ;; `minibuffer-default' with a list of defaults
-			    ;; relevant for file-name reading.
-			    (set (make-local-variable 'minibuffer-default-add-function)
-				 (lambda ()
-				   (with-current-buffer
-				       (window-buffer (minibuffer-selected-window))
-				     (read-file-name-defaults dir initial)))))
-                        (completing-read prompt 'read-file-name-internal
-                                         pred mustmatch insdef
-                                         'file-name-history default-filename)))
-                  ;; If DEFAULT-FILENAME not supplied and DIR contains
-                  ;; a file name, split it.
-                  (let ((file (file-name-nondirectory dir))
-			;; When using a dialog, revert to nil and non-nil
-			;; interpretation of mustmatch. confirm options
-			;; need to be interpreted as nil, otherwise
-			;; it is impossible to create new files using
-			;; dialogs with the default settings.
-			(dialog-mustmatch
-                         (not (memq mustmatch
-                                    '(nil confirm confirm-after-completion)))))
-                    (when (and (not default-filename)
-			       (not (zerop (length file))))
-                      (setq default-filename file)
-                      (setq dir (file-name-directory dir)))
-                    (when default-filename
-		      (setq default-filename
-			    (expand-file-name (if (consp default-filename)
-						  (car default-filename)
-						default-filename)
-					      dir)))
-                    (setq add-to-history t)
-                    (x-file-dialog prompt dir default-filename
-				   dialog-mustmatch
-                                   (eq predicate 'file-directory-p)))))
-
-               (replace-in-history (eq (car-safe file-name-history) val)))
-          ;; If completing-read returned the inserted default string itself
-          ;; (rather than a new string with the same contents),
-          ;; it has to mean that the user typed RET with the minibuffer empty.
-          ;; In that case, we really want to return ""
-          ;; so that commands such as set-visited-file-name can distinguish.
-	  (when (consp default-filename)
-	    (setq default-filename (car default-filename)))
-          (when (eq val default-filename)
-            ;; In this case, completing-read has not added an element
-            ;; to the history.  Maybe we should.
-            (if (not replace-in-history)
-                (setq add-to-history t))
-            (setq val ""))
-          (unless val (error "No file name specified"))
-
-          (if (and default-filename
-                   (string-equal val (if (consp insdef) (car insdef) insdef)))
-              (setq val default-filename))
-          (setq val (substitute-in-file-name val))
-
-          (if replace-in-history
-              ;; Replace what Fcompleting_read added to the history
-              ;; with what we will actually return.  As an exception,
-              ;; if that's the same as the second item in
-              ;; file-name-history, it's really a repeat (Bug#4657).
-              (let ((val1 (minibuffer--double-dollars val)))
-                (if history-delete-duplicates
-                    (setcdr file-name-history
-                            (delete val1 (cdr file-name-history))))
-		(if (string= val1 (cadr file-name-history))
-		    (pop file-name-history)
-		  (setcar file-name-history val1)))
-            (if add-to-history
-                ;; Add the value to the history--but not if it matches
-                ;; the last value already there.
-                (let ((val1 (minibuffer--double-dollars val)))
-                  (unless (and (consp file-name-history)
-                               (equal (car file-name-history) val1))
-                    (setq file-name-history
-                          (cons val1
-                                (if history-delete-duplicates
-                                    (delete val1 file-name-history)
-                                  file-name-history)))))))
-          val)))))
+  (funcall read-file-name-function
+           prompt dir default-filename mustmatch initial predicate))
 
 (defun internal-complete-buffer-except (&optional buffer)
   "Perform completion on all buffers excluding BUFFER.

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> Is the following patch (against 24, not touching ido.el yet) something
> people have in mind? - Leo

I think so, tho it's hard to tell since the diff shows a big removal and
a big addition: if you place read-file-name-default after
read-file-name, then the diff will be more readable, I think.


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2010-12-29 02:51 +0000, Stefan Monnier wrote:
>> Is the following patch (against 24, not touching ido.el yet) something
>> people have in mind? - Leo
>
> I think so, tho it's hard to tell since the diff shows a big removal and
> a big addition: if you place read-file-name-default after
> read-file-name, then the diff will be more readable, I think.
>
>
>         Stefan

I thought C-c C-w in diff ignore all spaces but it turned out only for a
hunk. Here is the diff again:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 8d09d5d..4b006c7 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1474,8 +1474,9 @@
                             'completion--file-name-table)
   "Internal subroutine for `read-file-name'.  Do not call this.")
 
-(defvar read-file-name-function nil
-  "If this is non-nil, `read-file-name' does its work by calling this function.")
+(defvar read-file-name-function 'read-file-name-default
+  "The function called by `read-file-name' to do its work.
+It should accept the same arguments as `read-file-name'.")
 
 (defcustom read-file-name-completion-ignore-case
   (if (memq system-type '(ms-dos windows-nt darwin cygwin))
@@ -1513,7 +1514,7 @@ such as making the current buffer visit no file in the case of
 (declare-function x-file-dialog "xfns.c"
                   (prompt dir &optional default-filename mustmatch only-dir-p))
 
-(defun read-file-name-defaults (&optional dir initial)
+(defun read-file-name--defaults (&optional dir initial)
   (let ((default
 	  (cond
 	   ;; With non-nil `initial', use `dir' as the first default.
@@ -1580,6 +1580,12 @@
 
 See also `read-file-name-completion-ignore-case'
 and `read-file-name-function'."
+  (funcall read-file-name-function
+           prompt dir default-filename mustmatch initial predicate))
+
+(defun read-file-name-default (prompt &optional dir default-filename mustmatch initial predicate)
+  "Default method for reading file names.
+See `read-file-name' for the meaning of the arguments."
   (unless dir (setq dir default-directory))
   (unless (file-name-absolute-p dir) (setq dir (expand-file-name dir)))
   (unless default-filename
@@ -1601,9 +1601,6 @@
                     (minibuffer--double-dollars dir)))
                  (initial (cons (minibuffer--double-dollars initial) 0)))))
 
-    (if read-file-name-function
-        (funcall read-file-name-function
-                 prompt dir default-filename mustmatch initial predicate)
       (let ((completion-ignore-case read-file-name-completion-ignore-case)
             (minibuffer-completing-file-name t)
             (pred (or predicate 'file-exists-p))
@@ -1639,7 +1636,7 @@
 				 (lambda ()
 				   (with-current-buffer
 				       (window-buffer (minibuffer-selected-window))
-				     (read-file-name-defaults dir initial)))))
+				   (read-file-name--defaults dir initial)))))
                         (completing-read prompt 'read-file-name-internal
                                          pred mustmatch insdef
                                          'file-name-history default-filename)))
@@ -1713,7 +1713,7 @@
                                 (if history-delete-duplicates
                                     (delete val1 file-name-history)
                                   file-name-history)))))))
-          val)))))
+	val))))
 
 (defun internal-complete-buffer-except (&optional buffer)
   "Perform completion on all buffers excluding BUFFER.

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> I thought C-c C-w in diff ignore all spaces but it turned out only for a
> hunk.

The issue is not so much whitespace, but the fact that you moved code
around, so diff can either show that code removed+added or show
(alternatively) show the intervening code as removed+added, depending on
which of the two is smaller.

> +  (funcall read-file-name-function
> +           prompt dir default-filename mustmatch initial predicate))

Maybe (or read-file-name-function #'read-file-name-default) is in order
here for backward compatibility, in case external code used
a let-binding to nil.


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2010-12-29 15:39 +0000, Stefan Monnier wrote:
>> +  (funcall read-file-name-function
>> +           prompt dir default-filename mustmatch initial predicate))
>
> Maybe (or read-file-name-function #'read-file-name-default) is in order
> here for backward compatibility, in case external code used
> a let-binding to nil.

OK, I'll add that. Incidentally, there is no need to fix ido.

Leo

Re: Any objection to adding completing-read-function?

[Leo]

On 2010-12-29 23:39 +0800, Stefan Monnier wrote:
>> +  (funcall read-file-name-function
>> +           prompt dir default-filename mustmatch initial predicate))
>
> Maybe (or read-file-name-function #'read-file-name-default) is in order
> here for backward compatibility, in case external code used
> a let-binding to nil.

Hi Stefan,

Any objection to this patch that changes read-file-name-function to a
non-nil default?

Leo

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2011-03-17 00:43:54 +0000
+++ lisp/minibuffer.el	2011-03-20 13:01:47 +0000
@@ -1486,8 +1486,9 @@
                             'completion--file-name-table)
   "Internal subroutine for `read-file-name'.  Do not call this.")
 
-(defvar read-file-name-function nil
-  "If this is non-nil, `read-file-name' does its work by calling this function.")
+(defvar read-file-name-function 'read-file-name-default
+  "The function called by `read-file-name' to do its work.
+It should accept the same arguments as `read-file-name'.")
 
 (defcustom read-file-name-completion-ignore-case
   (if (memq system-type '(ms-dos windows-nt darwin cygwin))
@@ -1525,7 +1526,7 @@
 (declare-function x-file-dialog "xfns.c"
                   (prompt dir &optional default-filename mustmatch only-dir-p))
 
-(defun read-file-name-defaults (&optional dir initial)
+(defun read-file-name--defaults (&optional dir initial)
   (let ((default
 	  (cond
 	   ;; With non-nil `initial', use `dir' as the first default.
@@ -1592,6 +1593,12 @@
 
 See also `read-file-name-completion-ignore-case'
 and `read-file-name-function'."
+  (funcall (or read-file-name-function #'read-file-name-default)
+           prompt dir default-filename mustmatch initial predicate))
+
+(defun read-file-name-default (prompt &optional dir default-filename mustmatch initial predicate)
+  "Default method for reading file names.
+See `read-file-name' for the meaning of the arguments."
   (unless dir (setq dir default-directory))
   (unless (file-name-absolute-p dir) (setq dir (expand-file-name dir)))
   (unless default-filename
@@ -1613,9 +1620,6 @@
                     (minibuffer--double-dollars dir)))
                  (initial (cons (minibuffer--double-dollars initial) 0)))))
 
-    (if read-file-name-function
-        (funcall read-file-name-function
-                 prompt dir default-filename mustmatch initial predicate)
       (let ((completion-ignore-case read-file-name-completion-ignore-case)
             (minibuffer-completing-file-name t)
             (pred (or predicate 'file-exists-p))
@@ -1651,7 +1655,7 @@
 				 (lambda ()
 				   (with-current-buffer
 				       (window-buffer (minibuffer-selected-window))
-				     (read-file-name-defaults dir initial)))))
+				   (read-file-name--defaults dir initial)))))
                         (completing-read prompt 'read-file-name-internal
                                          pred mustmatch insdef
                                          'file-name-history default-filename)))
@@ -1725,7 +1729,7 @@
                                 (if history-delete-duplicates
                                     (delete val1 file-name-history)
                                   file-name-history)))))))
-          val)))))
+	val))))
 
 (defun internal-complete-buffer-except (&optional buffer)
   "Perform completion on all buffers excluding BUFFER.

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

>>> +  (funcall read-file-name-function
>>> +           prompt dir default-filename mustmatch initial predicate))
>> Maybe (or read-file-name-function #'read-file-name-default) is in order
>> here for backward compatibility, in case external code used
>> a let-binding to nil.

> Hi Stefan,

> Any objection to this patch that changes read-file-name-function to a
> non-nil default?

Go for it,


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2011-03-21 22:22 +0800, Stefan Monnier wrote:
> Go for it,

Done.

Leo

RE: Any objection to adding completing-read-function?

[Drew Adams]

1. Leo's original patch said this near the beginning of the doc string for
`completing-read':

"This function calls `completing-read-function' to do the work"

That's very clear.  What finally got implemented removed that explanation and
just added this to the very end of the (very long) doc string:

"See also `completing-read-function'."

That's insufficiently clear, IMO.  There is a big difference between the two in
terms of communication to users.  The former makes clear what
`completing-read-function' is about - its role for `completing-read'.  The
latter relegates `completing-read-function' to a "see also".

I have the same comment wrt `read-file-name'.  IMO it would be preferable to say
that it calls `read-file-name-function' to do the work, instead of just adding a
"see also" at the very end.


2. I would also prefer it if the code for `completing-read-function' and its
treatment were in Lisp, not C.  No, I won't be submitting a patch. ;-)  Dunno
how easy it would be to move this to Lisp, but IMO it would be preferable if it,
just like `read-file-name-function', were in minibuffer.el.  These two variables
are parallel.

At this stage in the game it doesn't seem like we should be adding completion
code to the C sources.  Ideally we should be moving the last remaining pieces of
the completion code (e.g., `completing-read') completely to Lisp instead.  How
about putting this on the TODO list?

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> 2. I would also prefer it if the code for `completing-read-function' and its
> treatment were in Lisp, not C.

Full agreement.  I'd like to turn the `initial' arg into a callback
(which can do initialization in a similar way to
minibuffer-with-setup-hook, but more robust) but doing it in C is
more painful.

> No, I won't be submitting a patch. ;-)  Dunno

Damn!


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On 2011-05-31 23:51 +0800, Stefan Monnier wrote:
>> 2. I would also prefer it if the code for `completing-read-function' and its
>> treatment were in Lisp, not C.
>
> Full agreement.  I'd like to turn the `initial' arg into a callback
> (which can do initialization in a similar way to
> minibuffer-with-setup-hook, but more robust) but doing it in C is
> more painful.

Will look into this later on.

>> No, I won't be submitting a patch. ;-)  Dunno
>
> Damn!

Preliminary patch attached that moves completing-read-function and
completing-read-default to elisp. Comments welcome.

Since completing-read-default in the patch calls read-from-minibuffer,
what to do with this note:

,----[ read-from-minibuffer ]
| *Note* that this behavior differs from the way such arguments are used
| in `completing-read' and some related functions, which use
| zero-indexing for POSITION.
`----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: crf.diff --]
[-- Type: text/x-diff, Size: 5922 bytes --]

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2011-06-01 15:34:41 +0000
+++ lisp/minibuffer.el	2011-06-01 18:48:10 +0000
@@ -2699,7 +2699,30 @@
   (let ((newstr (completion-initials-expand string table pred)))
     (when newstr
       (completion-pcm-try-completion newstr table pred (length newstr)))))
+\f
+(defvar completing-read-function 'completing-read-default
+  "The function called by `completing-read' to do its work.
+It should accept the same arguments as `completing-read'.")
 
+(defun completing-read-default (prompt collection &optional predicate require-match
+                                       initial-input hist def inherit-input-method)
+  "Default method for reading from the minibuffer with completion.
+See `completing-read' for the meaning of the arguments."
+  (let* ((minibuffer-completion-table collection)
+         (minibuffer-completion-predicate predicate)
+         (minibuffer-completion-confirm (not require-match))
+         (keymap (if require-match
+                     (if minibuffer-completing-file-name
+                         minibuffer-local-filename-must-match-map
+                       minibuffer-local-must-match-map)
+                   (if minibuffer-completing-file-name
+                       minibuffer-local-filename-completion-map
+                     minibuffer-local-completion-map)))
+         (result (read-from-minibuffer prompt initial-input keymap
+                                       nil hist def inherit-input-method)))
+    (when (and (equal result "") def)
+      (setq result (if (consp def) (car def) def)))
+    result))
 \f
 ;; Miscellaneous
 

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2011-06-01 15:34:41 +0000
+++ src/minibuf.c	2011-06-01 18:51:33 +0000
@@ -72,7 +72,6 @@
 static Lisp_Object Qminibuffer_completion_table;
 static Lisp_Object Qminibuffer_completion_predicate;
 static Lisp_Object Qminibuffer_completion_confirm;
-static Lisp_Object Qcompleting_read_default;
 static Lisp_Object Quser_variable_p;
 
 static Lisp_Object Qminibuffer_default;
@@ -1686,7 +1685,7 @@
   (Lisp_Object prompt, Lisp_Object collection, Lisp_Object predicate, Lisp_Object require_match, Lisp_Object initial_input, Lisp_Object hist, Lisp_Object def, Lisp_Object inherit_input_method)
 {
   Lisp_Object args[9];
-  args[0] = Vcompleting_read_function;
+  args[0] = Fsymbol_value (intern ("completing-read-function"));
   args[1] = prompt;
   args[2] = collection;
   args[3] = predicate;
@@ -1697,76 +1696,6 @@
   args[8] = inherit_input_method;
   return Ffuncall (9, args);
 }
-
-DEFUN ("completing-read-default", Fcompleting_read_default, Scompleting_read_default, 2, 8, 0,
-       doc: /* Default method for reading from the minibuffer with completion.
-See `completing-read' for the meaning of the arguments.  */)
-  (Lisp_Object prompt, Lisp_Object collection, Lisp_Object predicate, Lisp_Object require_match, Lisp_Object initial_input, Lisp_Object hist, Lisp_Object def, Lisp_Object inherit_input_method)
-{
-  Lisp_Object val, histvar, histpos, position;
-  Lisp_Object init;
-  int pos = 0;
-  int count = SPECPDL_INDEX ();
-  struct gcpro gcpro1;
-
-  init = initial_input;
-  GCPRO1 (def);
-
-  specbind (Qminibuffer_completion_table, collection);
-  specbind (Qminibuffer_completion_predicate, predicate);
-  specbind (Qminibuffer_completion_confirm,
-	    EQ (require_match, Qt) ? Qnil : require_match);
-
-  position = Qnil;
-  if (!NILP (init))
-    {
-      if (CONSP (init))
-	{
-	  position = Fcdr (init);
-	  init = Fcar (init);
-	}
-      CHECK_STRING (init);
-      if (!NILP (position))
-	{
-	  CHECK_NUMBER (position);
-	  /* Convert to distance from end of input.  */
-	  pos = XINT (position) - SCHARS (init);
-	}
-    }
-
-  if (SYMBOLP (hist))
-    {
-      histvar = hist;
-      histpos = Qnil;
-    }
-  else
-    {
-      histvar = Fcar_safe (hist);
-      histpos = Fcdr_safe (hist);
-    }
-  if (NILP (histvar))
-    histvar = Qminibuffer_history;
-  if (NILP (histpos))
-    XSETFASTINT (histpos, 0);
-
-  val = read_minibuf (NILP (require_match)
-		      ? (NILP (Vminibuffer_completing_file_name)
-			 || EQ (Vminibuffer_completing_file_name, Qlambda)
-			 ? Vminibuffer_local_completion_map
-			 : Vminibuffer_local_filename_completion_map)
-		      : (NILP (Vminibuffer_completing_file_name)
-			 || EQ (Vminibuffer_completing_file_name, Qlambda)
-			 ? Vminibuffer_local_must_match_map
-			 : Vminibuffer_local_filename_must_match_map),
-		      init, prompt, make_number (pos), 0,
-		      histvar, histpos, def, 0,
-		      !NILP (inherit_input_method));
-
-  if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (def))
-    val = CONSP (def) ? XCAR (def) : def;
-
-  RETURN_UNGCPRO (unbind_to (count, val));
-}
 \f
 Lisp_Object Fassoc_string (register Lisp_Object key, Lisp_Object list, Lisp_Object case_fold);
 
@@ -2005,7 +1934,6 @@
   minibuf_save_list = Qnil;
   staticpro (&minibuf_save_list);
 
-  DEFSYM (Qcompleting_read_default, "completing-read-default");
   DEFSYM (Qcompletion_ignore_case, "completion-ignore-case");
   DEFSYM (Qread_file_name_internal, "read-file-name-internal");
   DEFSYM (Qminibuffer_default, "minibuffer-default");
@@ -2124,12 +2052,6 @@
 	       doc: /* Non-nil means completing file names.  */);
   Vminibuffer_completing_file_name = Qnil;
 
-  DEFVAR_LISP ("completing-read-function",
-	       Vcompleting_read_function,
-	       doc: /* The function called by `completing-read' to do the work.
-It should accept the same arguments as `completing-read'.  */);
-  Vcompleting_read_function = Qcompleting_read_default;
-
   DEFVAR_LISP ("minibuffer-help-form", Vminibuffer_help_form,
 	       doc: /* Value that `help-form' takes on inside the minibuffer.  */);
   Vminibuffer_help_form = Qnil;
@@ -2205,5 +2127,4 @@
   defsubr (&Stest_completion);
   defsubr (&Sassoc_string);
   defsubr (&Scompleting_read);
-  defsubr (&Scompleting_read_default);
 }


[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


Leo

Re: Any objection to adding completing-read-function?

[Leo]

On 2011-06-02 03:03 +0800, Leo wrote:
>> Full agreement.  I'd like to turn the `initial' arg into a callback
>> (which can do initialization in a similar way to
>> minibuffer-with-setup-hook, but more robust) but doing it in C is
>> more painful.
>
> Will look into this later on.

OK, I am going to focus on moving some of the code to elisp first.

> Preliminary patch attached that moves completing-read-function and
> completing-read-default to elisp. Comments welcome.

No comments so far. Any objection if I put the patch in trunk?

Leo

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

>>> Full agreement.  I'd like to turn the `initial' arg into a callback
>>> (which can do initialization in a similar way to
>>> minibuffer-with-setup-hook, but more robust) but doing it in C is
>>> more painful.
>> Will look into this later on.
> OK, I am going to focus on moving some of the code to elisp first.

Of course.  First move the code to C reproducing the behavior as
faithfully as possible.

>> Preliminary patch attached that moves completing-read-function and
>> completing-read-default to elisp. Comments welcome.
> No comments so far. Any objection if I put the patch in trunk?

I haven't had a chance yet to look at it.  Maybe next week.


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2011-06-09 23:16 +0800, Stefan Monnier wrote:
>>> Preliminary patch attached that moves completing-read-function and
>>> completing-read-default to elisp. Comments welcome.
>> No comments so far. Any objection if I put the patch in trunk?
>
> I haven't had a chance yet to look at it.  Maybe next week.

ping?

Leo

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

> +(defun completing-read-default (prompt collection &optional predicate require-match
> +                                       initial-input hist def inherit-input-method)

Please stay within 80 columns.

> +  "Default method for reading from the minibuffer with completion.
> +See `completing-read' for the meaning of the arguments."
> +  (let* ((minibuffer-completion-table collection)
> +         (minibuffer-completion-predicate predicate)
> +         (minibuffer-completion-confirm (not require-match))

This `not' doesn't preserve the behavior when `confirm' is not t.

> +         (keymap (if require-match
> +                     (if minibuffer-completing-file-name

The C code checks (memq minibuffer-completing-file-name '(nil
lambda)) instead.  Are you sure your code is correct, and if so why?

> +         (result (read-from-minibuffer prompt initial-input keymap
> +                                       nil hist def inherit-input-method)))

The C code does some funny dance with the initial-input before passing
it to read_minibuf, which read-from-minibuffer (and your code) doesn't
do, so I suspect that your code does not preserve the
corresponding behavior.


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

On 2011-06-22 09:50 +0800, Stefan Monnier wrote:
>> +(defun completing-read-default (prompt collection &optional predicate require-match
>> +                                       initial-input hist def inherit-input-method)
>
> Please stay within 80 columns.

Done.

>> + "Default method for reading from the minibuffer with completion.
>> +See `completing-read' for the meaning of the arguments." + (let*
>> ((minibuffer-completion-table collection) +
>> (minibuffer-completion-predicate predicate) +
>> (minibuffer-completion-confirm (not require-match))
>
> This `not' doesn't preserve the behavior when `confirm' is not t.

Fixed.

>> +         (keymap (if require-match
>> +                     (if minibuffer-completing-file-name
>
> The C code checks (memq minibuffer-completing-file-name '(nil
> lambda)) instead.  Are you sure your code is correct, and if so why?

The value 'lambda is not documented anywhere. What's its meaning?

>> +         (result (read-from-minibuffer prompt initial-input keymap
>> +                                       nil hist def inherit-input-method)))
>
> The C code does some funny dance with the initial-input before passing
> it to read_minibuf, which read-from-minibuffer (and your code) doesn't
> do, so I suspect that your code does not preserve the
> corresponding behavior.
>
>
>         Stefan

Please review the attached updated patch. Thanks very much.

Leo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: crf.diff --]
[-- Type: text/x-diff, Size: 7072 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2011-06-21 22:55:52 +0000
+++ lisp/ChangeLog	2011-06-22 04:04:32 +0000
@@ -1,3 +1,8 @@
+2011-06-22  Leo Liu  <sdl.web@gmail.com>
+
+	* minibuffer.el (completing-read-function):
+	(completing-read-default): Move from minibuf.c
+
 2011-06-21  Lars Magne Ingebrigtsen  <larsi@gnus.org>
 
 	* mail/smtpmail.el (smtpmail-via-smtp): Set

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2011-06-20 20:16:20 +0000
+++ lisp/minibuffer.el	2011-06-22 04:11:40 +0000
@@ -2710,7 +2710,40 @@
   (let ((newstr (completion-initials-expand string table pred)))
     (when newstr
       (completion-pcm-try-completion newstr table pred (length newstr)))))
+\f
+(defvar completing-read-function 'completing-read-default
+  "The function called by `completing-read' to do its work.
+It should accept the same arguments as `completing-read'.")
+
+(defun completing-read-default (prompt collection &optional predicate
+                                       require-match initial-input
+                                       hist def inherit-input-method)
+  "Default method for reading from the minibuffer with completion.
+See `completing-read' for the meaning of the arguments."
+
+  (when (consp initial-input)
+    (setq initial-input
+          (cons (car initial-input)
+                ;; `completing-read' uses 0-based index while
+                ;; `read-from-minibuffer' uses 1-based index.
+                (1+ (cdr initial-input)))))
 
+  (let* ((minibuffer-completion-table collection)
+         (minibuffer-completion-predicate predicate)
+         (minibuffer-completion-confirm (unless (eq require-match t)
+                                          require-match))
+         (keymap (if require-match
+                     (if (memq minibuffer-completing-file-name '(nil lambda))
+                         minibuffer-local-must-match-map
+                       minibuffer-local-filename-must-match-map)
+                   (if (memq minibuffer-completing-file-name '(nil lambda))
+                       minibuffer-local-completion-map
+                     minibuffer-local-filename-completion-map)))
+         (result (read-from-minibuffer prompt initial-input keymap
+                                       nil hist def inherit-input-method)))
+    (when (and (equal result "") def)
+      (setq result (if (consp def) (car def) def)))
+    result))
 \f
 ;; Miscellaneous
 

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-06-21 16:47:56 +0000
+++ src/ChangeLog	2011-06-22 04:05:59 +0000
@@ -1,3 +1,8 @@
+2011-06-22  Leo Liu  <sdl.web@gmail.com>
+
+	* minibuf.c (Fcompleting_read_default):
+	(Vcompleting_read_function): Move to minibuffer.el.
+
 2011-06-21  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Port to Sun C.

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2011-06-10 20:05:21 +0000
+++ src/minibuf.c	2011-06-22 04:02:48 +0000
@@ -72,7 +72,6 @@
 static Lisp_Object Qminibuffer_completion_table;
 static Lisp_Object Qminibuffer_completion_predicate;
 static Lisp_Object Qminibuffer_completion_confirm;
-static Lisp_Object Qcompleting_read_default;
 static Lisp_Object Quser_variable_p;
 
 static Lisp_Object Qminibuffer_default;
@@ -1694,7 +1693,7 @@
   (Lisp_Object prompt, Lisp_Object collection, Lisp_Object predicate, Lisp_Object require_match, Lisp_Object initial_input, Lisp_Object hist, Lisp_Object def, Lisp_Object inherit_input_method)
 {
   Lisp_Object args[9];
-  args[0] = Vcompleting_read_function;
+  args[0] = Fsymbol_value (intern ("completing-read-function"));
   args[1] = prompt;
   args[2] = collection;
   args[3] = predicate;
@@ -1705,76 +1704,6 @@
   args[8] = inherit_input_method;
   return Ffuncall (9, args);
 }
-
-DEFUN ("completing-read-default", Fcompleting_read_default, Scompleting_read_default, 2, 8, 0,
-       doc: /* Default method for reading from the minibuffer with completion.
-See `completing-read' for the meaning of the arguments.  */)
-  (Lisp_Object prompt, Lisp_Object collection, Lisp_Object predicate, Lisp_Object require_match, Lisp_Object initial_input, Lisp_Object hist, Lisp_Object def, Lisp_Object inherit_input_method)
-{
-  Lisp_Object val, histvar, histpos, position;
-  Lisp_Object init;
-  int pos = 0;
-  int count = SPECPDL_INDEX ();
-  struct gcpro gcpro1;
-
-  init = initial_input;
-  GCPRO1 (def);
-
-  specbind (Qminibuffer_completion_table, collection);
-  specbind (Qminibuffer_completion_predicate, predicate);
-  specbind (Qminibuffer_completion_confirm,
-	    EQ (require_match, Qt) ? Qnil : require_match);
-
-  position = Qnil;
-  if (!NILP (init))
-    {
-      if (CONSP (init))
-	{
-	  position = Fcdr (init);
-	  init = Fcar (init);
-	}
-      CHECK_STRING (init);
-      if (!NILP (position))
-	{
-	  CHECK_NUMBER (position);
-	  /* Convert to distance from end of input.  */
-	  pos = XINT (position) - SCHARS (init);
-	}
-    }
-
-  if (SYMBOLP (hist))
-    {
-      histvar = hist;
-      histpos = Qnil;
-    }
-  else
-    {
-      histvar = Fcar_safe (hist);
-      histpos = Fcdr_safe (hist);
-    }
-  if (NILP (histvar))
-    histvar = Qminibuffer_history;
-  if (NILP (histpos))
-    XSETFASTINT (histpos, 0);
-
-  val = read_minibuf (NILP (require_match)
-		      ? (NILP (Vminibuffer_completing_file_name)
-			 || EQ (Vminibuffer_completing_file_name, Qlambda)
-			 ? Vminibuffer_local_completion_map
-			 : Vminibuffer_local_filename_completion_map)
-		      : (NILP (Vminibuffer_completing_file_name)
-			 || EQ (Vminibuffer_completing_file_name, Qlambda)
-			 ? Vminibuffer_local_must_match_map
-			 : Vminibuffer_local_filename_must_match_map),
-		      init, prompt, make_number (pos), 0,
-		      histvar, histpos, def, 0,
-		      !NILP (inherit_input_method));
-
-  if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (def))
-    val = CONSP (def) ? XCAR (def) : def;
-
-  RETURN_UNGCPRO (unbind_to (count, val));
-}
 \f
 Lisp_Object Fassoc_string (register Lisp_Object key, Lisp_Object list, Lisp_Object case_fold);
 
@@ -2013,7 +1942,6 @@
   minibuf_save_list = Qnil;
   staticpro (&minibuf_save_list);
 
-  DEFSYM (Qcompleting_read_default, "completing-read-default");
   DEFSYM (Qcompletion_ignore_case, "completion-ignore-case");
   DEFSYM (Qread_file_name_internal, "read-file-name-internal");
   DEFSYM (Qminibuffer_default, "minibuffer-default");
@@ -2132,12 +2060,6 @@
 	       doc: /* Non-nil means completing file names.  */);
   Vminibuffer_completing_file_name = Qnil;
 
-  DEFVAR_LISP ("completing-read-function",
-	       Vcompleting_read_function,
-	       doc: /* The function called by `completing-read' to do the work.
-It should accept the same arguments as `completing-read'.  */);
-  Vcompleting_read_function = Qcompleting_read_default;
-
   DEFVAR_LISP ("minibuffer-help-form", Vminibuffer_help_form,
 	       doc: /* Value that `help-form' takes on inside the minibuffer.  */);
   Vminibuffer_help_form = Qnil;
@@ -2214,5 +2136,4 @@
   defsubr (&Stest_completion);
   defsubr (&Sassoc_string);
   defsubr (&Scompleting_read);
-  defsubr (&Scompleting_read_default);
 }

Re: Any objection to adding completing-read-function?

[Stefan Monnier]

>> The C code checks (memq minibuffer-completing-file-name '(nil
>> lambda)) instead.  Are you sure your code is correct, and if so why?
> The value 'lambda is not documented anywhere. What's its meaning?

It's an internal hack and should mostly be invisible to the rest of the
Lisp code.  It's explained in a comment in minibuf.c (search for
Qlambda).
Passing an initialization-function to read_minibuf (e.g. via the
initial-input arg) would make it possible to get rid of this hack, BTW.

BTW, your patch looks OK now, so if your tests indicate that it works
correctly, feel free to install it.


        Stefan

Re: Any objection to adding completing-read-function?

[Leo]

On 2011-06-23 05:19 +0800, Stefan Monnier wrote:
> It's an internal hack and should mostly be invisible to the rest of the
> Lisp code.  It's explained in a comment in minibuf.c (search for
> Qlambda).
> Passing an initialization-function to read_minibuf (e.g. via the
> initial-input arg) would make it possible to get rid of this hack, BTW.

Thanks.

> BTW, your patch looks OK now, so if your tests indicate that it works
> correctly, feel free to install it.

Installed so that more people can test it.

Leo