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

[Leo]

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

=== 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  <address@hidden>
+
+       * minibuffer.el (completing-read-function):
+       (completing-read-default): Move from minibuf.c
+
 2011-06-21  Lars Magne Ingebrigtsen  <address@hidden>
 
        * 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)))))
+
+(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))
 
 ;; 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  <address@hidden>
+
+       * minibuf.c (Fcompleting_read_default):
+       (Vcompleting_read_function): Move to minibuffer.el.
+
 2011-06-21  Paul Eggert  <address@hidden>
 
        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));
-}
 
 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);
 }