* Setting `minibuffer-completion-table` buffer-locally @ 2021-04-24 2:56 Stefan Monnier 2021-04-24 11:08 ` Daniel Mendler ` (2 more replies) 0 siblings, 3 replies; 4+ messages in thread From: Stefan Monnier @ 2021-04-24 2:56 UTC (permalink / raw) To: emacs-devel Following a long discussion in bug#45474, I'm warming up to the idea to install the patch below which changes `completing-read(-default)` such that it sets the `minibuffer-completion-*` variables buffer-locally in the mini-buffer instead of let-binding them globally. I've had such a change in the back of my mind for a while, and the discussion around bug#45474 convinced me that setting the vars buffer-locally is the only right solution. It's also necessary in order for the multiple active minibuffers that can now be displayed simultaneously to work correctly. I was a bit scared of the potential for breaking existing packages, since this is an incompatible change, but my analysis of all the code I could find in MELPA and GNU ELPA suggests that there should be very little breakage, if any. It worked fine in my tests with the default UI, Icomplete, Vertico, and Ivy. If people could test it (and report here) against their favorite funky completion package with their personal configuration (Helm, Ivy, Selectrum, Icicles, Younameit, ...) I'd be most welcome. There's a "competing" patch from Gregory which does something similar but in a slightly different way (less clean but potentially a bit more backward compatible), and I think in order to decide which is the best approach we need to have a more concrete information about the risk of breakage and what we might need to do to minimize it. Stefan diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi index 7cf2fcf68f2..2d284f5b85d 100644 --- a/doc/lispref/minibuf.texi +++ b/doc/lispref/minibuf.texi @@ -1188,9 +1188,9 @@ Completion Commands @defvar minibuffer-completion-table The value of this variable is the completion table (@pxref{Basic Completion}) used for completion in the minibuffer. This is the -global variable that contains what @code{completing-read} passes to +buffer-local variable that contains what @code{completing-read} passes to @code{try-completion}. It is used by minibuffer completion commands -such as @code{minibuffer-complete-word}. +such as @code{minibuffer-complete}. @end defvar @defvar minibuffer-completion-predicate @@ -1201,7 +1201,7 @@ Completion Commands @defvar minibuffer-completion-confirm This variable determines whether Emacs asks for confirmation before -exiting the minibuffer; @code{completing-read} binds this variable, +exiting the minibuffer; @code{completing-read} sets this variable, and the function @code{minibuffer-complete-and-exit} checks the value before exiting. If the value is @code{nil}, confirmation is not required. If the value is @code{confirm}, the user may exit with an diff --git a/etc/NEWS b/etc/NEWS index 7d600eb374d..709c74b38df 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -2427,6 +2427,11 @@ This is to keep the same behavior as Eshell. \f * Incompatible Lisp Changes in Emacs 28.1 ++++ +** 'completing-read-default' sets completion variables buffer-locally. +'minibuffer-completion-table' and related variables are now set buffer-locally +in the minibuffer instead of being set via a global let-binding. + +++ ** The use of positional arguments in 'define-minor-mode' is obsolete. These were actually rendered obsolete in Emacs-21 but were never diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el index 51e0519d489..2eafc12d882 100644 --- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -3884,13 +3884,7 @@ completing-read-default ;; `read-from-minibuffer' uses 1-based index. (1+ (cdr initial-input))))) - (let* ((minibuffer-completion-table collection) - (minibuffer-completion-predicate predicate) - ;; FIXME: Remove/rename this var, see the next one. - (minibuffer-completion-confirm (unless (eq require-match t) - require-match)) - (minibuffer--require-match require-match) - (base-keymap (if require-match + (let* ((base-keymap (if require-match minibuffer-local-must-match-map minibuffer-local-completion-map)) (keymap (if (memq minibuffer-completing-file-name '(nil lambda)) @@ -3903,8 +3897,17 @@ completing-read-default ;; in minibuffer-local-filename-completion-map can ;; override bindings in base-keymap. base-keymap))) - (result (read-from-minibuffer prompt initial-input keymap - nil hist def inherit-input-method))) + (result + (minibuffer-with-setup-hook + (lambda () + (setq-local minibuffer-completion-table collection) + (setq-local minibuffer-completion-predicate predicate) + ;; FIXME: Remove/rename this var, see the next one. + (setq-local minibuffer-completion-confirm + (unless (eq require-match t) require-match)) + (setq-local minibuffer--require-match require-match)) + (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)) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Setting `minibuffer-completion-table` buffer-locally 2021-04-24 2:56 Setting `minibuffer-completion-table` buffer-locally Stefan Monnier @ 2021-04-24 11:08 ` Daniel Mendler 2021-04-24 11:15 ` Basil L. Contovounesios 2021-04-24 13:38 ` Gregory Heytings 2 siblings, 0 replies; 4+ messages in thread From: Daniel Mendler @ 2021-04-24 11:08 UTC (permalink / raw) To: Stefan Monnier, emacs-devel On 4/24/21 4:56 AM, Stefan Monnier wrote: > Following a long discussion in bug#45474, I'm warming up to the idea to > install the patch below which changes `completing-read(-default)` such > that it sets the `minibuffer-completion-*` variables buffer-locally in > the mini-buffer instead of let-binding them globally. I am in favor of such a change. In the completion systems or packages centered around completion I am involved with (Selectrum, Vertico, Marginalia, Consult, Embark, ...) we always followed the style of encapsulating minibuffer-local state inside the minibuffer. Let-binding the built-in table always seemed odd. I don't expect breakage from these packages. If some issues occur despite my expectations we will fix them promptly. It is no problem to fix issues in a backward compatible way by always accessing the minibuffer-completion-table via the minibuffer, if for some reason the variable is accessed from the context of another buffer. Daniel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Setting `minibuffer-completion-table` buffer-locally 2021-04-24 2:56 Setting `minibuffer-completion-table` buffer-locally Stefan Monnier 2021-04-24 11:08 ` Daniel Mendler @ 2021-04-24 11:15 ` Basil L. Contovounesios 2021-04-24 13:38 ` Gregory Heytings 2 siblings, 0 replies; 4+ messages in thread From: Basil L. Contovounesios @ 2021-04-24 11:15 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel Stefan Monnier <monnier@iro.umontreal.ca> writes: > If people could test it (and report here) against their favorite funky > completion package with their personal configuration (Helm, Ivy, > Selectrum, Icicles, Younameit, ...) I'd be most welcome. Ivy doesn't use or modify completing-read-default, so it should be unaffected. Some light testing of the patch with my config seems to confirm this. Thanks, -- Basil ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Setting `minibuffer-completion-table` buffer-locally 2021-04-24 2:56 Setting `minibuffer-completion-table` buffer-locally Stefan Monnier 2021-04-24 11:08 ` Daniel Mendler 2021-04-24 11:15 ` Basil L. Contovounesios @ 2021-04-24 13:38 ` Gregory Heytings 2 siblings, 0 replies; 4+ messages in thread From: Gregory Heytings @ 2021-04-24 13:38 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel [-- Attachment #1: Type: text/plain, Size: 2296 bytes --] > > There's a "competing" patch from Gregory which does something similar > but in a slightly different way (less clean but potentially a bit more > backward compatible), and I think in order to decide which is the best > approach we need to have a more concrete information about the risk of > breakage and what we might need to do to minimize it. > In the interest of fairness in the discussion and comparison, and given the length of the discussion in bug#45474, here is the "competing" patch. The five major differences between the two approaches are (m-c-* stands for the minibuffer-completion-* variables, r-f-m for read-from-minibuffer, and m-w-s-h for minibuffer-with-setup-hook): (1) SM: m-c-* variables are passed explicitly as arguments to r-f-m with a m-w-s-h, they are stricly buffer-local GH: m-c-* variables are passed implicitly as arguments to r-f-m, they automatically become buffer-local upon entering the minibuffer (2) SM: it is and it will remain necessary to use a m-w-s-h with setq-locals to have buffer-local m-c-* variables in r-f-m GH: for Emacs 28 it is necessary to let-bind minibuffer-local-completion to t around the call to r-f-m (to preserve backward compatibility), for Emacs 29 and above minibuffer-local-completion t will be assumed (or, with the strong version: the only possible behavior) (3) SM: when m-c-* variables are strictly buffer-local (i.e., when they are passed in a m-w-s-h to r-f-m), they cannot interfere with code outside of the minibuffer GH: because m-c-* variables become buffer-local only upon entering the minibuffer, they can possibly interfere with code that is executed around the call to r-f-m, between the moment they have been let-bound and the moment they become buffer-local (4) SM: the semantics of r-f-m is not changed GH: the semantics of r-f-m is changed, in Emacs 28 the m-c-* variables become buffer-local when minibuffer-local-completion is t, in Emacs 29 they become buffer-local unless minibuffer-local-completion is nil (or, with the strong version: they become buffer-local) (5) SM: the patch fixes the behavior of completing-read-default GH: the patch fixes the behavior of completing-read-default and makes, in the medium term, that new behavior the default one for all uses of r-f-m [-- Attachment #2: Type: text/x-diff, Size: 5510 bytes --] diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el index 7da3c39e6b..379dadef9d 100644 --- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -3884,7 +3884,8 @@ completing-read-default ;; `read-from-minibuffer' uses 1-based index. (1+ (cdr initial-input))))) - (let* ((minibuffer-completion-table collection) + (let* ((minibuffer-local-completion t) + (minibuffer-completion-table collection) (minibuffer-completion-predicate predicate) ;; FIXME: Remove/rename this var, see the next one. (minibuffer-completion-confirm (unless (eq require-match t) diff --git a/src/minibuf.c b/src/minibuf.c index c4482d7f1e..584afd5d04 100644 --- a/src/minibuf.c +++ b/src/minibuf.c @@ -68,6 +68,9 @@ Copyright (C) 1985-1986, 1993-2021 Free Software Foundation, Inc. /* Width of current mini-buffer prompt. Only set after display_line of the line that contains the prompt. */ +static Lisp_Object minibuffer_completion_table, minibuffer_completion_predicate, + minibuffer_completion_confirm; + static ptrdiff_t minibuf_prompt_width; static Lisp_Object nth_minibuffer (EMACS_INT depth); @@ -862,6 +865,16 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt, if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method))) call1 (Qactivate_input_method, input_method); + if (! EQ (Vminibuffer_local_completion, Qnil)) { + Fmake_local_variable (Qminibuffer_completion_table); + Fset (Qminibuffer_completion_table, minibuffer_completion_table); + Fmake_local_variable (Qminibuffer_completion_predicate); + Fset (Qminibuffer_completion_predicate, minibuffer_completion_predicate); + Fmake_local_variable (Qminibuffer_completion_confirm); + Fset (Qminibuffer_completion_confirm, minibuffer_completion_confirm); + Vminibuffer_local_completion = Qnil; + } + run_hook (Qminibuffer_setup_hook); /* Don't allow the user to undo past this point. */ @@ -1291,6 +1304,7 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer, (Lisp_Object prompt, Lisp_Object initial_contents, Lisp_Object keymap, Lisp_Object read, Lisp_Object hist, Lisp_Object default_value, Lisp_Object inherit_input_method) { Lisp_Object histvar, histpos, val; + ptrdiff_t count; barf_if_interaction_inhibited (); @@ -1315,11 +1329,25 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer, if (NILP (histpos)) XSETFASTINT (histpos, 0); + count = SPECPDL_INDEX (); + + if (! EQ (Vminibuffer_local_completion, Qnil)) { + minibuffer_completion_table = Vminibuffer_completion_table; + minibuffer_completion_predicate = Vminibuffer_completion_predicate; + minibuffer_completion_confirm = Vminibuffer_completion_confirm; + specbind (Qminibuffer_completion_table, Qnil); + specbind (Qminibuffer_completion_predicate, Qnil); + specbind (Qminibuffer_completion_confirm, Qnil); + } + val = read_minibuf (keymap, initial_contents, prompt, !NILP (read), histvar, histpos, default_value, minibuffer_allow_text_properties, !NILP (inherit_input_method)); + + unbind_to (count, Qnil); + return val; } @@ -1345,11 +1373,9 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0, Lisp_Object val; ptrdiff_t count = SPECPDL_INDEX (); - /* Just in case we're in a recursive minibuffer, make it clear that the - previous minibuffer's completion table does not apply to the new - minibuffer. - FIXME: `minibuffer-completion-table' should be buffer-local instead. */ specbind (Qminibuffer_completion_table, Qnil); + specbind (Qminibuffer_completion_predicate, Qnil); + specbind (Qminibuffer_completion_confirm, Qnil); val = Fread_from_minibuffer (prompt, initial_input, Qnil, Qnil, history, default_value, @@ -2281,6 +2307,9 @@ syms_of_minibuf (void) Fset (Qminibuffer_default, Qnil); DEFSYM (Qminibuffer_completion_table, "minibuffer-completion-table"); + DEFSYM (Qminibuffer_completion_predicate, "minibuffer-completion-predicate"); + DEFSYM (Qminibuffer_completion_confirm, "minibuffer-completion-confirm"); + DEFSYM (Qminibuffer_local_completion, "minibuffer-local-completion"); staticpro (&last_minibuf_string); @@ -2414,6 +2443,23 @@ syms_of_minibuf (void) completion commands listed in `minibuffer-confirm-exit-commands'. */); Vminibuffer_completion_confirm = Qnil; + DEFVAR_LISP ("minibuffer-local-completion", Vminibuffer_local_completion, + doc: /* Whether minibuffer completion elements should become buffer-local. +The default is nil for Emacs 28. + +[STRONG VERSION:] Setting this variable in Emacs 29 will have no effect; +the value t will be assumed. + +[WEAK VERSION:] The default will be t for Emacs 29. + +When t, `minibuffer-completion-table', `minibuffer-completion-predicate' +and `minibuffer-completion-confirm' become buffer-local upon entering the +minibuffer, and are nil in recursive invocations of the minibuffer, unless +they have been let-bound to a value. +When nil, their values is shared between the recursive invocations of the +minibuffer, unless they have been let-bound to another value. */); + Vminibuffer_local_completion = Qnil; + DEFVAR_LISP ("minibuffer-completing-file-name", Vminibuffer_completing_file_name, doc: /* Non-nil means completing file names. */); ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-24 13:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-04-24 2:56 Setting `minibuffer-completion-table` buffer-locally Stefan Monnier 2021-04-24 11:08 ` Daniel Mendler 2021-04-24 11:15 ` Basil L. Contovounesios 2021-04-24 13:38 ` Gregory Heytings
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.