API deprecation discussion

[minad]

I don't think there is an issue yet discussing potential upcoming API changes/deprecations. This is a list of things I stumbled upon. Maybe not all of these suggestions are useful and I probably missed a few changes which you are already planning.

• Deprecate and rename selectrum-read to selectrum--read in favor of completing-read (DONE in Private api #446)
• Internalize the propertizes selectrum-candidate-display-*, if they are still needed or remove them altogether?
• Remove the selectrum faces (Remove selectrum-*-highlight faces #364)
• Potentially rework some internals of the selectrum--read API, currently there seems to be a mix of using setq in a minibuffer setup hook and passing arguments to selectrum--read.
• Then there is selectrum--dynamic-candidates which could possibly be removed in favor of using a default completion-table+dynamic-flag or by rewriting the current selectrum functions which make use of this dynamic-candidates feature in other ways (probably this is difficult given the hoops I had to jump through to make Consult narrowing work etc).

[clemera]

Thanks, good to have an issue about it. Regarding the faces there is a pending PR on the prescient side. Regarding selectrum--dynamic-candidates we also rely on the dynamic input feature but that could potentially dealt with by using other means, though I'm not very motivated for this change unless we discover more compelling reasons that make it worth.

[minad]

Regarding selectrum--dynamic-candidates we also rely on the dynamic input feature but that could potentially dealt with by using a variable to pass that info, though I'm not very motivated for this change unless we discover more compelling reasons that make it worth.

Sure, I just thought I mention it. Maybe some of the internals around this feature could be simplified, but I would have to give it a try to see if there exists a potential for improvements. For example one simple change could be to just use the default minibuffer completion table (instead of a dynamic candidates function) and flag it as dynamic, such that it is called for every keypress. This dynamic flag feature could then be used for dynamic minibuffer tables (#114), if it is decided to special case some of them at some point.

[clemera]

Internalize the propertizes selectrum-candidate-display-*, if they are still needed or remove them altogether?

Maybe it's also okay to keep them? They don't conflict with the completing-read API as far as I know.

[minad]

Okay sure, my motivation to internalize them is only to discourage its use in favor of annotation functions. But maybe it is justified to keep them. I do not feel so strongly about that.

[clemera]

Okay good, I think @raxod502 wanted to keep them but maybe his opinion has changed with the new situation and generally improved eco system surrounding Selectrum.

[minad]

Some update regarding the selectrum-dependent packages:

• kiwix: This should be solved using the consult--async api, since kiwix is doing an async web query. This will be a significant improvement over the status quo, since kiwix is using a synchronous query as of now.
• org-z: This one is actually justified to use selectrum-read since it really does a synchronous completion. It calls org-ql-select which receives a query expression and returns a list of candidates synchronously. So my proposal in that case would really be to continue to use selectrum--read at least as long as Selectrum retains its dynamic candidates functionality. But as an experiment I also looked into how this could be solved using Consult. Right now there is no simple API within Consult to emulate this use case of selectrum-read, but it can be written in terms of the async functions as follows:

;;; -*- lexical-binding: t -*-

;; consult--sync wrapper, maybe make this part of Consult
(defun consult--sync (fun)
  (let ((async (consult--async-sink)))
    (lambda (action)
      (pcase action
        ('setup
         ;; consume the full input, no completion style filtering!
         (consult--split-setup (lambda (input _) (list input "" 0)))
         (funcall async 'setup))
        ((pred stringp)
         ;; flush the old candidates, generate new candidates and refresh
         (funcall async 'flush)
         (funcall async (funcall fun action))
         (funcall async 'refresh))
        (_ (funcall async action))))))

;; Usage example
(consult--read
 (consult--sync (lambda (input)
                  (list (format "%s1" input)
                        (format "%s2" input)
                        (format "%s3" input)))))

The only thing which is really hard/inefficient to emulate using Consult is this input rewriting which Selectrum offers. As long as the input string splitting is separate from the candidate generation step everything is fine (e.g. the splitting of #grep#filter in consult-grep). But as soon as you throw these two things together in a single function as in the Selectrum api, the result will be inefficient, since one would have to call this function twice, once to obtain the modified input string for filtering and once to obtain the candidates.

[minad]

For reference, the emulation would look like this:

(defun dynamic-candidates (input)
   `((candidates . (,(format "foo %s 1" input)
                    ,(format "foo %s 2" input)
                    ,(format "foo %s 3" input)))
     (input . "foo")))

(selectrum--read "Test: " #'dynamic-candidates)

(defun consult--emulate-selectrum (fun)
  (let ((async (consult--async-sink)))
    (lambda (action)
      (pcase action
        ('setup
         (consult--split-setup (lambda (input _)
                                 (let ((ninput (alist-get 'input (funcall fun input)))) ;; FIRST CALL!
                                   (list input ninput (length ninput)))))
         (funcall async 'setup))
        ((pred stringp)
         (funcall async 'flush)
         (funcall async (alist-get 'candidates (funcall fun action))) ;; SECOND CALL!
         (funcall async 'refresh))
        (_ (funcall async action))))))

(consult--read (consult--emulate-selectrum #'dynamic-candidates))

[clemera]

Hm, it doesn't look like org-z does use the dynamic input feature? In that case it should ideally just pass a dynamic table to completing-read.

[minad]

Hm, it doesn't look like org-z does use the dynamic input feature? In that case it should ideally just pass a dynamic table to completing-read.

Yes, but Selectrum does not support that, right? The consult--sync wrapper I wrote above, basically simulates dynamic completion tables.

[minad]

As I proposed before, we may want to consider is to add dynamic candidate table support to Selectrum but only enable it manually via a flag in order to avoid performance regressions. The flag could either be added to the completion metadata, by setting a flag variable before invoking completing-read or in the minibuffer setup hook.

(let ((selectrum-dynamic t))
  (completing-read ...)

The experiments I made above are basically just toy examples to check if the current Consult api is powerful enough to emulate what is missing if the Selectrum api is not to be used directly. This does not mean that people should necessarily use this in the end.

[clemera]

Yes I agree we should do that, sounds like a good compromise.

[minad]

Another idea - We could propose to Emacs upstream to add a dynamic flag to the completion metadata. This way the completion table could communicate to the completion system that it wants to be retriggered every time. Then all the dynamic completion tables everywhere would need this update. Then Selectrum or other completion systems could use this information to switch the behavior. But I think that the chances to get such a proposal accepted are low. Probably the argument will then be that the completion table can always use caching. And then the need for the detection of dynamic candidate tables is only the result of the design of Selectrum which wants to do its own filtering. Given that Selectrum uses completion styles now to do its own filtering we could also reconsider that design. Maybe it wouldn't even come with a performance penalty given optimizations like orderless-skip-highlighting. However independent on how one handles these dynamic candidate tables there is still the additional aspect of asynchronous completion tables which in any case need a refreshing function, since the candidates come in asynchronously without user interaction. Basically we have static < dynamic < asynchronous, and the Selectrum completing-read impl only covers static, the Emacs completing-read impl covers dynamic and if one takes selectrum-exhibit/icomplete-exhibit into account one gets the full asynchronous support.

[clemera]

I like the idea of a dynamic metadata flag, too but I also agree that it would probably hard to get through but we could at least try. There is also the problem with automatically using all-completions with the passed string when using complete-with-action which bypasses completion-styles that should get fixed (but this would also affect performance so probably wouldn't be accepted). I think even with skipped highlighting some built-in tables could be problematic but we could try to blacklist those.

[clemera]

Ideally a dynamic tbale should always be allowed to return any candidates but the way complete-with-action and completion-table-dynamic (which uses the former) work you only get the candidates matching the passed string (according to all-completions).

[minad]

Yes, but why would we need that? The dynamic table just takes control over the filtering itself. The only step which we are saving is this preprocessing step? Is this expensive? Maybe it is - sorting all the time. And as soon as you let the completion table do the filtering itself you are bound to use completion styles. Prescient wouldn't work anymore for example, but this could easily be rewritten as a completion style.

[clemera]

The problem is that complete-with-action is often used with the assumption of the default prefix based Emacs completion. Some tables which depend on the input need to get it passed. But when you pass the input to help--symbol-completion-table for example you only get the prefix matches with that string. Because of that we pass always the empty string and because of that can't support tables which depend on the input.

[minad]

But when you pass the input to help--symbol-completion-table for example you only get the prefix matches with that string.

This does not sound right, I mean Icomplete and default completion work with non-default completion styles like orderless, for example when calling describe-function.

[clemera]

Oops you are right, but I currently have no clue why 😆

[clemera]

I just edebuged it and it looks like they also pass the empty string for the t action in this case.

[clemera]

I wonder why then input dependent tables work where you need to pass the input?

[clemera]

Well it seems this is not supported at all, I don't know why I thought it would be the case.

[clemera]

I will look into that tomorrow I'm to tired to think right now 😴

[minad]

I just edebuged it and it looks like they also pass the empty string for the t action in this case.
I wonder why then input dependent tables work where you need to pass the input?

This is very weird then. You are right, I also checked it. Probably dynamic tables don't really work ;)
The input string is only passed for metadata and boundaries but not for the t action.

[minad]

So, these dynamic tables - is this actually only some fantasy? If that's the case then the consult--sync from above is really the way to emulate dynamic tables using the standard api it seems.

The documentation (https://www.gnu.org/software/emacs/manual/html_node/elisp/Programmed-Completion.html) is really confusing with respect to that, it talks about the passed in string for the t action. Maybe at some point this was considered as design but never implemented for whatever reason.

Obtaining the completions goes through completion-all-completions. And if one calls thins with a dynamic table, the string is not passed for the t action.

(completion-all-completions "test" (lambda (str pred action)
                             (message "%S %S %S" str pred action)
                             (complete-with-action action '(alpha beta gamma) str pred)
                             ) nil 0)

But if one uses instead all-completions which is used within the completion style implementation, then the prefix string is passed.

(all-completions "test" (lambda (str pred action)
                             (message "%S %S %S" str pred action)
                             (complete-with-action action '(alpha beta gamma) str pred)
                             )
                 nil)

EDIT: The filtering happens actually in completion-all-completions after calling the dynamic table. So the dynamic table is not doing its own filtering but the filtering is done afterwards. This is all really weird :-P

[clemera]

Hm the table could also use minibuffer-contents and use that to determine the candidates for the t action, then you would have an input dependent table.

[minad]

Yes sure. This is basically what Consult is doing. But if the minibuffer-contents are used to generate candidates, then the completion style must also somehow understand this and perform some kind of splitting similar to Consult.

[minad]

Well whatever - I guess in the end it boils down to the question if the preprocessing step is worth being separate/cached or not.

Have a good night!

[clemera]

Thanks, I was already sleeping ;) Maybe the behavior depends on completion-boundaries? In that case only tables which return an non empty string would need to be called on each input.

[minad]

I don't think boundaries have anything to do with this. This is only meant to restrict the filtering to a certain substring of the input. Does Selectrum handle this correctly? Maybe I could use this for the consult async functionality to split the string such that completion only takes the filter part of #grep#filter into account.

[clemera]

You can indeed use boundaries to depend on the input:

(defun checko (string pred action)
  (if (eq (car-safe action) 'boundaries)
      (cons 'boundaries
	    (cons 1 0))
    (if (eq action t)
	(cond ((string= string "a")
	       '("a1" "a2"))
	      ((string= string "b")
	       '("b3" "b4")))
      (complete-with-action  action nil string pred))))

(completing-read "Check: " 'checko)

[clemera]

It seems this all assumes file like completion (appending the result to the present input and proceed filtering from there) which is also the only table which depends on input which I actually know about.

[clemera]

Does Selectrum handle this correctly?

Currently not but I think we could adjust to handle completion-boundaries, but for file completions it is better to keep not using them as we do our own thing there.

[clemera]

I just noticed the example above only kind of works when using orderless.

[clemera]

On another note, the breaking changes left are the removal of the faces but I think it is okay to postpone this so I will wait now for a while if there are no other unexpected bugs introduced by recent changes and then use the current ad9d9f0 for the 4.0 release.

[minad]

It would be nice if this checko example from above would work with Selectrum. I agree with you that it is kind of weird that the returned result is the the concatenation of the prefix string and the completion. But I could actually use that instead of the current Consult async splitting. Maybe this is an improvement over the status quo, where I am using a custom completion style which performs this input splitting, simply ignoring the prefix input. However the current solution is potentially much more flexible since I can do arbitrary splitting of the input string, but in practice as of now only the suffix is used for completion filtering.

On another note, the breaking changes left are the removal of the faces but I think it is okay to postpone this so I will wait now for a while if there are no other unexpected bugs introduced by recent changes and then use the current ad9d9f0 for the 4.0 release.

Maybe there are more functions you want to internalize, I am not sure? Is there a point in having all these selectrum-read-buffer etc functions public? Maybe there is a point since users want to set these functions as advices/replacements? I think that selectrum--read is certainly the most important piece, but I guess it makes sense to think about the other functions also for a minute.

[clemera]

Maybe there are more functions you want to internalize, I am not sure? Is there a point in having all these selectrum-read-buffer etc functions public? Maybe there is a point since users want to set these functions as advices/replacements?

I also thought about that but I think it is better to have them public, they can also be used to adjust completing-read-function and such when not using selectrum-mode at all or explicitly invoke Selectrum when usually using ivy-mode or others.

[minad]

Makes sense. I wonder more about the selectrum-read-buffer, selectrum-read-library etc functions. Assuming that at some point you may want to rewrite some internals - maybe there wouldn't be the need for selectrum-read-buffer anymore. Why is this needed - I assume to handle this SPC prefix for ephemeral buffers? Maybe given some more adjustments to Selectrum internals the default read-buffer function would just work as is? In Consult I am doing this filtering by adjusting the minibuffer predicate, I wonder how the default read-buffer function handles this.

[minad]

Okay, I just checked. There is the variable read-buffer-function. So this is supposed to be overwritten. I still wonder why, but at least it is okay for Selectrum to have its own selectrum-read-buffer then. I think the criticism is only justified regarding the advices, but I guess there are good arguments to have this as is.

[clemera]

It would be nice if we could remove the custom read buffer function but I just checked and when completing internal buffers with default completion it also bypasses completion styles (probably uses all-completions), so we need to keep our custom version unfortunately.

[clemera]

selectrum-read-library-name could be private, on the other hand probably it doesn't harm to make it available.

[clemera]

Thanks for updating the wiki!

[minad]

I tried your checko example from above again and it would actually nice if there would be some limited support for completion boundaries in Selectrum.

(defun grep-test (string pred action)
  (if (eq (car-safe action) 'boundaries)
      (cons 'boundaries
	    (cons
             (if-let (pos (seq-position string ?#))
                 (1+ pos)
               (length string)) 0))
    (complete-with-action action '(alpha beta gamma) "" pred)))

(completing-read "Grep: " #'grep-test)

If you enter grep#filter, then "grep" is ignored by completion and "filter" is used for completion. This allows also to TAB cycle through the results in default completion which is an improvement over the status quo in consult-grep.

[minad]

Okay, I close this issue then!