bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

One can pass HIST=t to completing-read in order to disable the
minibuffer history. minibuffer-complete-history fails with an error
in that case. Instead it should act as a no-op or print a meaningful
error message (No history available).

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Drew Adams]

> One can pass HIST=t to completing-read in order
> to disable the minibuffer history.

BTW, I don't see this documented anywhere -
neither in the doc string nor in the Elisp
manual, including node `Minibuffer History'.

Shouldn't this be documented?

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

On 7/17/22 16:43, Drew Adams wrote:
>> One can pass HIST=t to completing-read in order
>> to disable the minibuffer history.
> 
> BTW, I don't see this documented anywhere -
> neither in the doc string nor in the Elisp
> manual, including node `Minibuffer History'.
> 
> Shouldn't this be documented?

It is documented in the doc string of completing-read since Emacs 28:

"If HIST is t, history is not recorded."

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Drew Adams]

> It is documented in the doc string of completing-read since Emacs 28:
> "If HIST is t, history is not recorded."

Ah, so it is; thx.  I somehow missed that while
scanning it.  Sorry.

And I didn't see it in NEWS when searching for
`completing-read' or `history'.  There is tons
of stuff about changes in handling minibuffer
history, but nothing about this (disabling it).

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Juri Linkov]

close 56613 29.0.50
thanks

> One can pass HIST=t to completing-read in order to disable the
> minibuffer history. minibuffer-complete-history fails with an error
> in that case. Instead it should act as a no-op or print a meaningful
> error message (No history available).

Thanks for the bug report, now fixed in 60185819b6.

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

Thanks! Btw, I noticed that you disable sorting by let binding
completions-sort to nil. This is not the proper way to disable sorting.
The variable is meant for user configuration of the default completion UI.

In order to disable sorting from the side of the completion table, it is
better to use the completion metadata. This way the sort function will
also be picked up by all the alternative completion UIs.

(completion-in-region
 (minibuffer--completion-prompt-end) (point-max)
 (lambda (str pred action)
   (if (eq action 'metadata)
       '(metadata (display-sort-function . identity)
                  (cycle-sort-function . identity))
     (complete-with-action action completions str pred))))

On 7/17/22 20:07, Juri Linkov wrote:
> close 56613 29.0.50
> thanks
> 
>> One can pass HIST=t to completing-read in order to disable the
>> minibuffer history. minibuffer-complete-history fails with an error
>> in that case. Instead it should act as a no-op or print a meaningful
>> error message (No history available).
> 
> Thanks for the bug report, now fixed in 60185819b6.

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Juri Linkov]

> In order to disable sorting from the side of the completion table, it is
> better to use the completion metadata. This way the sort function will
> also be picked up by all the alternative completion UIs.
>
> (completion-in-region
>  (minibuffer--completion-prompt-end) (point-max)
>  (lambda (str pred action)
>    (if (eq action 'metadata)
>        '(metadata (display-sort-function . identity)
>                   (cycle-sort-function . identity))
>      (complete-with-action action completions str pred))))

Thanks for the suggestion, now fixed as well.

Stefan added this comment in minibuffer-complete-history:

    ;; FIXME: Can we make it work for CRM?

But I can't find a function that would return the
current completion boundaries to use instead of hard-coding
minibuffer--completion-prompt-end and point-max.  Then
completing-read-multiple should set locally such a function
that will use crm-separator and return a cons (BEG . END).

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

On 7/18/22 09:24, Juri Linkov wrote:
>> In order to disable sorting from the side of the completion table, it is
>> better to use the completion metadata. This way the sort function will
>> also be picked up by all the alternative completion UIs.
>>
>> (completion-in-region
>>  (minibuffer--completion-prompt-end) (point-max)
>>  (lambda (str pred action)
>>    (if (eq action 'metadata)
>>        '(metadata (display-sort-function . identity)
>>                   (cycle-sort-function . identity))
>>      (complete-with-action action completions str pred))))
> 
> Thanks for the suggestion, now fixed as well.

Thanks! Since you added this function twice (for history and defaults
completion) you could also introduce a helper function. I use such
helpers in my projects. Maybe such a function would be useful at other
places in the Emacs code base? I haven't grepped but I think this
pattern appears often.

(defun completion-table-presorted (table &rest metadata)
  "Disable sorting in the completion UI for TABLE which is already sorted."
  (lambda (string pred action)
    (if (eq action 'metadata)
        `(metadata (display-sort-function . identity)
                   (cycle-sort-function . identity)
                   ,@metadata)
      (complete-with-action action table string pred))))

Or maybe the more general function would be useful too?

(defun completion-table-with-metadata (table &rest metadata)
  "Return completion TABLE with METADATA."
  (lambda (string pred action)
    (if (eq action 'metadata)
        `(metadata ,@metadata)
      (complete-with-action action table string pred))))

> Stefan added this comment in minibuffer-complete-history:
> 
>     ;; FIXME: Can we make it work for CRM?
> 
> But I can't find a function that would return the
> current completion boundaries to use instead of hard-coding
> minibuffer--completion-prompt-end and point-max.  Then
> completing-read-multiple should set locally such a function
> that will use crm-separator and return a cons (BEG . END).

Doesn't it work if you retrieve the boundaries first with
`completion-boundaries` from `minibuffer-completion-table`? In my
Vertico package I use the following code and it works without issues
with CRM. The API requires a little bit of a ceremony.

(let* ((content (minibuffer-contents-no-properties))
       (pt (max 0 (- (point) (minibuffer-prompt-end))))
       (before (substring content 0 pt))
       (after (substring content pt))
       (bounds (completion-boundaries
                before minibuffer-completion-table
                minibuffer-completion-predicate after)))
  ...)

Daniel

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> But I can't find a function that would return the
> current completion boundaries to use instead of hard-coding
> minibuffer--completion-prompt-end and point-max.  Then
> completing-read-multiple should set locally such a function
> that will use crm-separator and return a cons (BEG . END).

I can't remember what hacks are used in CRM, but we do have "a function
that would return the current completion boundaries to use": it's what
`completion-at-point-functions` is for.

I think the long-term direction is clear: `completion-at-point` should
be used not just "in buffer" but in the minibuffer as well.
We could start that journey by making use of it for CRM.


        Stefan

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Doesn't it work if you retrieve the boundaries first with
> `completion-boundaries` from `minibuffer-completion-table`? In my
> Vertico package I use the following code and it works without issues
> with CRM. The API requires a little bit of a ceremony.

I suspect it won't work correctly when CRM is used with a completion
table that itself uses boundaries (e.g. when completing a list of file
names).

It's quite unusual, so in practice your approach probably works fine,
but fundamentally I think it's "wrong" (tho I don't have much better to
offer right now, except by adding CRM-specific hacks).


        Stefan

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

On 7/18/22 17:04, Stefan Monnier wrote:
>> Doesn't it work if you retrieve the boundaries first with
>> `completion-boundaries` from `minibuffer-completion-table`? In my
>> Vertico package I use the following code and it works without issues
>> with CRM. The API requires a little bit of a ceremony.
> 
> I suspect it won't work correctly when CRM is used with a completion
> table that itself uses boundaries (e.g. when completing a list of file
> names).
> 
> It's quite unusual, so in practice your approach probably works fine,
> but fundamentally I think it's "wrong" (tho I don't have much better to
> offer right now, except by adding CRM-specific hacks).
Yes, I've suspected that this is an issue. But CRM of
completion-file-name-table works well with Vertico.

(completing-read-multiple "Files: " #'completion-file-name-table)

On the other hand read-file-name-internal or completion--file-name-table
do not work. I get some args-out-of-range errors due to the quoting
mechanism.

(completing-read-multiple "Files: " #'read-file-name-internal)

Daniel

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

On 7/18/22 17:01, Stefan Monnier wrote:
>> But I can't find a function that would return the
>> current completion boundaries to use instead of hard-coding
>> minibuffer--completion-prompt-end and point-max.  Then
>> completing-read-multiple should set locally such a function
>> that will use crm-separator and return a cons (BEG . END).
> 
> I can't remember what hacks are used in CRM, but we do have "a function
> that would return the current completion boundaries to use": it's what
> `completion-at-point-functions` is for.
> 
> I think the long-term direction is clear: `completion-at-point` should
> be used not just "in buffer" but in the minibuffer as well.
> We could start that journey by making use of it for CRM.

Not sure if I agree with this in full generality. The question is if you
define completion only as completion of text or if you also accept the
selection paradigm, where are a bunch of candidates are offered and
filtered. The selection paradigm is used in most popular applications
(web browser history, form filling). In Emacs we have Icomplete, Ivy,
Vertico, etc. which implement that paradigm.

For CRM I agree that completion-at-point makes sense, since there we are
indeed doing step wise completion of multiple components. But I am not a
fan of CRM and I don't like its API design. It is not widely used and
the resulting UI is quite inconvenient. For example if you want to
select among a bunch of very long candidates the CRM UI doesn't work at
all. For this reason, org-cite and the Citar package instead call
completing-read in a loop to select multiple candidates. I've also
replaced most of my potential CRM use cases with completing-read in
combination with Embark. Embark can be used to act on multiple
candidates. Using single candidate completing-read as basis leads to a
system which is more composable overall.

Anyway because I am critical of CRM I would be cautious to use it as an
example which guides a potential redesign of the completion mechanism.

Daniel

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I think the long-term direction is clear: `completion-at-point` should
>> be used not just "in buffer" but in the minibuffer as well.
>> We could start that journey by making use of it for CRM.
>
> Not sure if I agree with this in full generality. The question is if you
> define completion only as completion of text or if you also accept the
> selection paradigm, where are a bunch of candidates are offered and
> filtered. The selection paradigm is used in most popular applications
> (web browser history, form filling). In Emacs we have Icomplete, Ivy,
> Vertico, etc. which implement that paradigm.

I wasn't careful enough in what I wrote.  I didn't mean to say that CRM
should use the `completion-at-point` UI, but that it should rely on CAPF
as the way to complete each element.

How to make that interact correctly with packages like `icomplete-mode`
which have been designed specifically for minibuffer completion is part
of the implementation issues.


        Stefan

bug#56613: 29; minibuffer-complete-history throws an error for minibuffer-history-variable=t

[Daniel Mendler]

On 7/18/22 18:19, Stefan Monnier wrote:
>>> I think the long-term direction is clear: `completion-at-point` should
>>> be used not just "in buffer" but in the minibuffer as well.
>>> We could start that journey by making use of it for CRM.
>>
>> Not sure if I agree with this in full generality. The question is if you
>> define completion only as completion of text or if you also accept the
>> selection paradigm, where are a bunch of candidates are offered and
>> filtered. The selection paradigm is used in most popular applications
>> (web browser history, form filling). In Emacs we have Icomplete, Ivy,
>> Vertico, etc. which implement that paradigm.
> 
> I wasn't careful enough in what I wrote.  I didn't mean to say that CRM
> should use the `completion-at-point` UI, but that it should rely on CAPF
> as the way to complete each element.

Oh, I already agreed that Capfs could work for CRM behind the scenes.
However I would be careful to not violate the abstraction barriers too
much, which we have in place right now. In the end the completion table
has to be passed through completing-read or completion-in-region to the
frontend.

(completing-read-multiple installs its crm--choose-completion-string
hook, which is not great. But besides that, it is kind of nice that
completing-read and completing-read-multiple just work as is with
Vertico. Vertico doesn't have any kind of CRM special casing.)

Daniel