Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

Hi Lars,

>     Allow REQUIRE-MATCH to be a function
>     
>     * doc/lispref/minibuf.texi (Minibuffer Completion): Document it.
>     
>     * lisp/minibuffer.el (completion--complete-and-exit): Allow
>     REQUIRE-MATCH to be a function.
>     (read-file-name): Mention it.

I see the motivation is for things like `read-file-name` where we
can't specify a completion table (otherwise we could also get the same
result with an appropriate completion table whose `test-completion` is
adjusted accordingly).

But besides the `test-completion` part of the completion table, this new
feature also overlaps with the `predicate` argument.  I think it would
be good to write down somewhere how those three compare.

I wonder if that can also be used to solve the problem of completing
file names that "end in .pdf": previously using `test-completion` (via
the completion table or via `predicate`) had the problem that such
a test either ruled out completions to "foo/bar/" (which can be
indispensable on the way to the final "foo/bar/baz.pdf") or spuriously
allow hitting RET on "foo/bar/" even tho it doesn't end in `.pdf`.

> +   ;; The CONFIRM argument is a predicate.
> +   ((and (functionp minibuffer-completion-confirm)
> +         (funcall minibuffer-completion-confirm
> +                  (buffer-substring beg end)))
> +    (funcall exit-function))
> +   ;; See if we have a completion from the table.
> +   ((test-completion (buffer-substring beg end)
> +                     minibuffer-completion-table
> +                     minibuffer-completion-predicate)
> +    ;; FIXME: completion-ignore-case has various slightly
> +    ;; incompatible meanings.  E.g. it can reflect whether the user
> +    ;; wants completion to pay attention to case, or whether the
> +    ;; string will be used in a context where case is significant.
> +    ;; E.g. usually try-completion should obey the first, whereas
> +    ;; test-completion should obey the second.
> +    (when completion-ignore-case
> +      ;; Fixup case of the field, if necessary.
> +      (let* ((string (buffer-substring beg end))
> +             (compl (try-completion
> +                     string
> +                     minibuffer-completion-table
> +                     minibuffer-completion-predicate)))
> +        (when (and (stringp compl) (not (equal string compl))
> +                   ;; If it weren't for this piece of paranoia, I'd replace
> +                   ;; the whole thing with a call to do-completion.
> +                   ;; This is important, e.g. when the current minibuffer's
> +                   ;; content is a directory which only contains a single
> +                   ;; file, so `try-completion' actually completes to
> +                   ;; that file.
> +                   (= (length string) (length compl)))
> +          (completion--replace beg end compl))))
> +    (funcall exit-function))
> +   ;; The user is permitted to exit with an input that's rejected
> +   ;; by test-completion, after confirming her choice.
> +   ((memq minibuffer-completion-confirm '(confirm confirm-after-completion))
> +    (if (or (eq last-command this-command)
> +            ;; For `confirm-after-completion' we only ask for confirmation
> +            ;; if trying to exit immediately after typing TAB (this
> +            ;; catches most minibuffer typos).
> +            (and (eq minibuffer-completion-confirm 'confirm-after-completion)
> +                 (not (memq last-command minibuffer-confirm-exit-commands))))
>          (funcall exit-function)
> -        (minibuffer-message "Confirm")
> -        nil))
> +      (minibuffer-message "Confirm")
> +      nil))

I see that the `minibuffer-completion-confirm` function completely
replaces the usual handling of require-match (i.e. the attempt to fix
the case and the prompting for confirmation).

Maybe this should be better documented, and also AFAICT currently
a `minibuffer-completion-confirm` function cannot reliably reproduce this
behavior by hand because it doesn't have access to `beg` and `end`.
I think we should make this "default behavior" more easily accessible
(e.g. put it into its own function and document it as something that can
be called from `minibuffer-completion-confirm`?).


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But besides the `test-completion` part of the completion table, this new
> feature also overlaps with the `predicate` argument.  I think it would
> be good to write down somewhere how those three compare.

The PREDICATE argument is under-documented, yes -- it's just supposed to
be a filter on COLLECTION, isn't it?

> I see that the `minibuffer-completion-confirm` function completely
> replaces the usual handling of require-match (i.e. the attempt to fix
> the case and the prompting for confirmation).

(You probably noticed it, but in case not -- I didn't change that code,
I just changed the indentation.)

> Maybe this should be better documented, and also AFAICT currently
> a `minibuffer-completion-confirm` function cannot reliably reproduce this
> behavior by hand because it doesn't have access to `beg` and `end`.
> I think we should make this "default behavior" more easily accessible
> (e.g. put it into its own function and document it as something that can
> be called from `minibuffer-completion-confirm`?).

I'm pretty sure I don't understand the subtleties here, but er sure?  🧐

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

>> I see that the `minibuffer-completion-confirm` function completely
>> replaces the usual handling of require-match (i.e. the attempt to fix
>> the case and the prompting for confirmation).
> (You probably noticed it, but in case not -- I didn't change that code,
> I just changed the indentation.)

What I meant was that the branch you added to the `cond` overrides the
rest of the `cond` (the part you reindented).

>> Maybe this should be better documented, and also AFAICT currently
>> a `minibuffer-completion-confirm` function cannot reliably reproduce this
>> behavior by hand because it doesn't have access to `beg` and `end`.
>> I think we should make this "default behavior" more easily accessible
>> (e.g. put it into its own function and document it as something that can
>> be called from `minibuffer-completion-confirm`?).
>
> I'm pretty sure I don't understand the subtleties here, but er sure?  🧐

Understanding the subtleties can be difficult, but a good general rule
I try to follow in such cases is to provide a function that the new
function-hook can use to get back the old behavior.

IOW, arrange your code such that the user can provide a function
`mimic-t` where the resulting behavior is identical to providing `t`
(and same thing for the other supported values).
AFAICT it's currently virtually impossible (at least not without inside
knowledge of how the minibuffer.el code is written).


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I see that the `minibuffer-completion-confirm` function completely
>>> replaces the usual handling of require-match (i.e. the attempt to fix
>>> the case and the prompting for confirmation).
>> (You probably noticed it, but in case not -- I didn't change that code,
>> I just changed the indentation.)
>
> What I meant was that the branch you added to the `cond` overrides the
> rest of the `cond` (the part you reindented).

Ah, yes, that's true.  But REQUIRE-MATCH can't both be a function and
`confirm' at the same time, and adding a new parameter for this use case
seemed a bit overboard.

(And the function can implement its own confirm-ish action if it wants
that.)

> Understanding the subtleties can be difficult, but a good general rule
> I try to follow in such cases is to provide a function that the new
> function-hook can use to get back the old behavior.
>
> IOW, arrange your code such that the user can provide a function
> `mimic-t` where the resulting behavior is identical to providing `t`
> (and same thing for the other supported values).
> AFAICT it's currently virtually impossible (at least not without inside
> knowledge of how the minibuffer.el code is written).

But is that useful?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

>> IOW, arrange your code such that the user can provide a function
>> `mimic-t` where the resulting behavior is identical to providing `t`
>> (and same thing for the other supported values).
>> AFAICT it's currently virtually impossible (at least not without inside
>> knowledge of how the minibuffer.el code is written).
> But is that useful?

If you can't reproduce the default behavior, then in my book the API
is incomplete.


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> If you can't reproduce the default behavior, then in my book the API
> is incomplete.

A t value for REQUIRE-MATCH in a `read-file-name' context would be
`file-exists-p'?  In other contexts it would require seeing whether the
string is in COLLECTION, and that's possible, too.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

Lars Ingebrigtsen [2022-06-12 11:59:05] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> If you can't reproduce the default behavior, then in my book the API
>> is incomplete.
> A t value for REQUIRE-MATCH in a `read-file-name' context would be
> `file-exists-p'?  In other contexts it would require seeing whether the
> string is in COLLECTION, and that's possible, too.

Look at the code that is run when (functionp
minibuffer-completion-confirm) is nil, and you'll see there's a lot more
to do than that if you want to reproduce the behavior faithfully.


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Look at the code that is run when (functionp
> minibuffer-completion-confirm) is nil, and you'll see there's a lot more
> to do than that if you want to reproduce the behavior faithfully.

Yeah, those are the subtleties of the completion machinery that I'm sure
I don't understand.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

Lars Ingebrigtsen [2022-06-13 14:12:32] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Look at the code that is run when (functionp
>> minibuffer-completion-confirm) is nil, and you'll see there's a lot more
>> to do than that if you want to reproduce the behavior faithfully.
>
> Yeah, those are the subtleties of the completion machinery that I'm sure
> I don't understand.  :-)

And in order not to have to understand them, you should move this code
out into its own function and call it in the same way that
`minibuffer-completion-confirm` is called (such that
`minibuffer-completion-confirm` can simply call this function to
reproduce that default behavior).


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> And in order not to have to understand them, you should move this code
> out into its own function and call it in the same way that
> `minibuffer-completion-confirm` is called (such that
> `minibuffer-completion-confirm` can simply call this function to
> reproduce that default behavior).

Sorry; I don't follow.  Feel free to change the code here as you see
fit, of course.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Stefan Monnier]

Lars Ingebrigtsen [2022-06-14 14:02:20] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> And in order not to have to understand them, you should move this code
>> out into its own function and call it in the same way that
>> `minibuffer-completion-confirm` is called (such that
>> `minibuffer-completion-confirm` can simply call this function to
>> reproduce that default behavior).
>
> Sorry; I don't follow.

The current code does more or less:

    (cond
     ((functionp minibuffer-completion-confirm)
      (funcall minibuffer-completion-confirm ...))
     <blabla>)

and we should change it to:

    (defun minibuffer-default-handle-completion-options (...)
      (cond
       <blabla>))

    [...]
    (funcall
      (if (functionp minibuffer-completion-confirm)
          minibuffer-completion-confirm
        #'minibuffer-default-handle-completion-options)
      ...)

so that we know for sure that `minibuffer-completion-confirm` can
reproduce the default behavior (i.e. that the API is not obviously
incomplete), simply by calling
`minibuffer-default-handle-completion-options`.

You don't need to understand the subtleties of <blabla>, because you
just move the code as-is and then the compiler will tell you if there's
a problem (typically because <blabla> uses vars from the context, which
you then need to pass as additional arguments).

[ For the same reason, most/all `<foo>-function` variables should
  ideally have a non-nil default value.  ]


        Stefan

Re: master 49e06183f5 1/3: Allow REQUIRE-MATCH to be a function

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> You don't need to understand the subtleties of <blabla>, because you
> just move the code as-is and then the compiler will tell you if there's
> a problem (typically because <blabla> uses vars from the context, which
> you then need to pass as additional arguments).

Sounds good to me, but I think it'd be better if you did the adjustments
here.  😀

> [ For the same reason, most/all `<foo>-function` variables should
>   ideally have a non-nil default value.  ]

I don't agree with that in general.  As a user, it's easier to reason
about nil-as-some-default-function than an explicit one.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no