consult-line is not putting the symbol at point in future history under default completion or icomplete

[oantolin]

Under Selectrum this works just fine: when you call consult-line, the first M-n inserts the symbol at point, the second M-n inserts the isearch string. Under default completion and icomplete, the first M-n inserts the current line, which I guess is inevitable since it is the default, but the second M-n seems to insert the isearch string skipping the symbol at point!

[minad]

Hmm. Any idea what the problem is?

[oantolin]

Not yet, but I'm digging around. I'll report any findings.

[oantolin]

OK, this is weird: under default completion or icomplete, as I said M-n inserts first the current line, then the isearch string. If right after you insert the isearch string you press M-p then you get the symbol at point!

[oantolin]

So I think the future history is correct, but somehow the index into it starts 1 higher than it should be.

[oantolin]

Well, indeed minibuffer-history-position is 0 for Selectrum and -1 for default completion at the time your custom minibuffer-default-add-function is run. I guess probably the first M-n inserting the default value (which is the current line) does the increment. I'm not sure how the API is meant to be used, I mean we could just set minibuffer-history-position to 0, but I doubt that's what your are supposed to do. Maybe the minibuffer-default-add-function is supposed to include the default at the beginning of the list it returns?

[minad]

The problem seems to be that goto-history-element has some logic which also checks minibuffer-default, so the default is really coupled to the history :( This means the add-history idea might not work. But why does it work in selectrum then. No idea.

Maybe the minibuffer-default-add-function is supposed to include the default at the beginning of the list it returns?

This is what I suspect too.

[oantolin]

Look at the default value for minibuffer-default-add-function, namely minibuffer-default-add-completions:

(defun minibuffer-default-add-completions ()
  "Return a list of all completions without the default value.
This function is used to add all elements of the completion table to
the end of the list of defaults just after the default value."
  (let ((def minibuffer-default)
    (all (all-completions ""
                  minibuffer-completion-table
                  minibuffer-completion-predicate)))
    (if (listp def)
    (append def all)
      (cons def (delete def all)))))

It puts the defaults at the beginning of the list.

[oantolin]

You know, I think it is safe to set minibuffer-history-position to 0 inside the custom minibuffer-default-add-function. The default completion UI makes the default value(s) be the first future history element(s) and so expects minibuffer-default-add-function to include them in its return value. In Selectrum the default value is used differently, its not meant to be inserted by the first M-n, so for Selectrum you wouldn't want to include the default value in the return value of minibuffer-default-add-function.

[oantolin]

You know, I think it is safe to set minibuffer-history-position to 0

Forget it, it may be safe but it doesn't work. 😆 It seems to make no difference in the behavior.

[oantolin]

I can just remember to do an extra M-p. 🤷

[minad]

I can just write my default-add-function such that it adds first the default, then the additional items, then the completions. This seems to work in icomplete. The behavior in selectrum is slightly different since minibuffer-default is not set for some reason. I will report a bug.

[oantolin]

This expression seems to do the trick:

(consult--remove-dups
 (append
  (if (listp minibuffer-default)
      minibuffer-default
    (list minibuffer-default))
  items
  (seq-drop (funcall orig)
            (if (listp minibuffer-default)
                (length minibuffer-default)
              1))))

That gives the behavior I would expect in both default completion and in Selectum. It relies on minibuffer-default being nil in Selectrum, so I wouldn't report a bug. 😛

[minad]

I don't want to go with such a hack. I pushed a fix. What do you think?

[oantolin]

Won't that add a nil under Selectrum at the beginning of the list?

[minad]

No, why? (append nil ...)

[oantolin]

Oh, sorry, of course. That was silly, it's the same trick I was proposing to use, in fact. 🤦

[minad]

Right :laughing:

[oantolin]

So you've decided to ignore the value of minibuffer-default-add-function? I guess that makes sense, you want to control the future history completely.

[minad]

You agree that this is better? We should go with the Emacs standard behavior otherwise we will only get problems later on. And this ugly goto-history-element function really seems to expect exactly this.

So you've decided to ignore the value of minibuffer-default-add-function? I guess that makes sense, you want to control the future history completely.

Yes, and our function is mostly compliant. It pushes only our new items in between. I guess that is okay.

[oantolin]

I think it's good, and it does do what you'd expect both under Selectrum and icomplete. Well, at least if you don't change the default minibuffer-default-add-function. I wonder what people that change it change it to and what for?

[oantolin]

Also, thanks for the delete "", the old version would add the isearch-string even if empty.

[minad]

Yes nil and blanks are removed now. This was bad before. I could also not do this and just require the :add-history to not contain empty values, but this way it is slightly easier for the api users.

[minad]

Well, at least if you don't change the default minibuffer-default-add-function. I wonder what people that change it change it to and what for?

Well, in the case of consult, if people want to add their own elements they can overwrite :add-history via consult-config!

[clemera]

@oantolin

Well, at least if you don't change the default minibuffer-default-add-function. I wonder what people that change it change it to and what for?

It helps to guess the value for some common cases, we have a simple enhanced version on the selectrum wiki.