bug#47711: bug#48841: [PATCH] Add new `completion-filter-completions` API and deferred highlighting

[Eli Zaretskii <eliz@gnu.org>]

[I removed emacs-devel from the CC list, please don't cross-post to
both emacs-devel and the bug tracker.]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Wed, 11 Aug 2021 16:16:57 +0200
> Cc: 48841@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>,
>  João Távora <joaotavora@gmail.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, 47711@debbugs.gnu.org
> 
> I prepared a patch which provides the API
> `completion-filter-completions`. This function supports deferred
> highlighting and returns additional data with the list of matching
> completion candidates. The API supersedes the existing function
> `completion-all-completions`.

Thanks.  The discussion of this is still going on, and I don't
consider myself an expert in this area of Emacs, so below please find
only comments for minor formatting and documentation aspects.

> Add a new `completion-filter-completions` API, which supersedes
> `completion-all-completions`.  The new API returns the matching
> completion candidates and additional data.  The return value is an
> alist, with the keys `completions`, `base`, `end` and `highlight`.
> The API can be extended in a backward compatible way later on thanks
> to the use of an alist as return value.

Please don't use Markdown-style quoting `like this` in our comments
and log messages.  We quote 'like this' in these places.

> The `completions` value is the list of completion strings *without*
> applied highlighting.  The completion strings are returned unmodified,
> which avoids allocations and results in performance gains for

This is unclear: how can you return a list of strings which you
produce without allocating the strings?

>         The value `base` is the base position of the completion.

"Base position" where, or relative to what object?

> Correspondingly the value `end` specifies the end position of the
> completion counted from the beginning of the input strng.

So the base position is also relative to the beginning of the input
string?  If so, please say so explicitly.

>                                                    Finally the
> `highlight` value is a function taking a list of completion strings
> and returns a new list of new strings with highlighting applied.

If you say "taking a list...", then for consistent style please also
say "...and returning a new list...".

> A continously updating UI can use the highlighting function to apply
> highlighting only to the visible completions.

Not, "visible", but "displayed", right?  So I'd rephrase

  ...to apply highlighting only to the completions that are actually
  displayed.

> (completion-basic-all-completions,
> completion-emacs21-all-completions,
> completion-emacs22-all-completions): Use it.

That's incorrect format, I guess you did it by hand rather than
letting change-log-mode do it for you?  The correct format looks like
this:

  (completion-basic-all-completions)
  (completion-emacs21-all-completions)
  (completion-emacs22-all-completions): use it.

IOW, each line ends with a closing parentheses, not a comma.

> +(defvar completion--filter-completions nil
> +  "Enable the new completions return value format.

Using genitive construction should be limited to just 2 words.  Here
you have 4, which produces an awkward, hard to interpret phrase.
Suggest to reword:

  If non-nil, return completions in `completion-filter-completions' format.

Note that I also dropped the "new" part (which generally doesn't stand
the test of time), and instead gave a hint as to what that format is.

Btw, why is this an internal variable?  Shouldn't all completion
styles ideally support it?  If so, it should be a public variable,
documented in the ELisp manual.  And the name should also end with -p,
since it's a boolean.  How about completion-filter-completions-format-p?

> +            New completion style functions may always return their
> +results in the new alist format, since `completion-all-completions'
> +transparently converts back to the old improper list of completions
> +with base size in the last cdr.")

"may" and "always" are a kind of contradiction.

Also, I'd drop the "improper" part, as it sounds like a derogatory
adjective.

>  (defun completion-try-completion (string table pred point &optional metadata)
>    "Try to complete STRING using completion table TABLE.
>  Only the elements of table that satisfy predicate PRED are considered.
> -POINT is the position of point within STRING.
> -The return value can be either nil to indicate that there is no completion,
> -t to indicate that STRING is the only possible completion,
> -or a pair (NEWSTRING . NEWPOINT) of the completed result string together with
> -a new position for point."
> +POINT is the position of point within STRING.  The return value can be
> +either nil to indicate that there is no completion, t to indicate that
> +STRING is the only possible completion, or a pair (NEWSTRING . NEWPOINT)
> +of the completed result string together with a new position for point.
> +The METADATA may be modified by the completion style."

Here you changed whitespace by filling, and that ruined the
intentionally formatted doc string, which made it easy to find the
various forms of the return value and the important parts of the doc
string.  Please keep the original formatting.

>  (defun completion-all-completions (string table pred point &optional metadata)
>    "List the possible completions of STRING in completion table TABLE.
>  Only the elements of table that satisfy predicate PRED are considered.
> -POINT is the position of point within STRING.
> -The return value is a list of completions and may contain the base-size
> -in the last `cdr'."
> -  ;; FIXME: We need to additionally return the info needed for the
> -  ;; second part of completion-base-position.
> -  (completion--nth-completion 2 string table pred point metadata))
> +POINT is the position of point within STRING.  The return value is a
> +list of completions and may contain the base-size in the last `cdr'.
> +The METADATA may be modified by the completion style.  This function
> +has been superseded by `completion-filter-completions', which returns
> +richer information and supports deferred candidate highlighting."

Likewise here.

Also, the "This function has been superseded..." part should be a new
paragraph, so that it stands out.  (And I'm not yet sure we indeed
want to say "superseded" here, but that's part of the on-going
discussion.  maybe use a more neutral language here, like "See also".)

> +    (if (and result (consp (car result)))
> +        ;; Give the completion styles some freedom!
> +        ;; If they are targeting Emacs 28 upwards only, they
> +        ;; may always return a result with deferred
> +        ;; highlighting.  We convert back to the old format
> +        ;; here by applying the highlighting eagerly.

"May always" again.  How about "can always" instead?

> +        (nconc (funcall (cdr (assq 'highlight result))
> +                        (cdr (assq 'completions result)))
> +               (cdr (assq 'base result)))
> +      result)))
> +
> +(defun completion-filter-completions (string table pred point metadata)
> +  "Filter the possible completions of STRING in completion table TABLE.

Is "filter" really the right word here (in the doc string)?  "Filer"
means you take a sequence and produce another sequence with some
members removed.  That's not what this API does, is it?  Suggest to
use a different name, like completion-completions-alist or
completion-all-completions-as-alist.

> +Only the elements of table that satisfy predicate PRED are considered.
> +POINT is the position of point within STRING.  The METADATA may be
> +modified by the completion style.  The return value is a alist with
> +the keys:
> +
> +- base: Base position of the completion (from the start of STRING)

"Base" here means the beginning?  If so, why not call it "beg" or
somesuch?

> +This function supersedes the function `completion-all-completions'."

Again, "supersedes" is too strong, IMO.  I would say "is a variant of"
instead, and explain why this variant could be better suited to some
use cases.  IOW, explain the upsides (and downsides, if any), and let
the programmers decide whether they want this, instead of more-or-less
forcing them to use it.

> +        ;; Deferred highlighting has been requested, but the completion
> +        ;; style returned a non-deferred result. Convert the result to the
                                                  ^^
two spaces between sentences, please.

> +        ;; new alist format.

"New" is not a good word here.

> +      ;; added by the completion machinery.

Please start comments with a capital letter.

> +(defun completion--deferred-hilit (completions prefix-len base end)
> +  "Return completions in old format or new alist format.
> +If `completion--filter-completions' is non-nil use the new format."

Again, please don't use "old" and "new" here, but instead describe
explicitly the differences between them, or provide a hyperlink to
where that is described.

> +                      ;; Apply highlighting

Please end each sentence in a comment with a period.

> +(defun completion-pcm--deferred-hilit (pattern completions base end)
> +  "Return completions in old format or new alist format.
> +If `completion--filter-completions' is non-nil use the new format."

"Old" and "new" again.

>  (defun completion-pcm--hilit-commonality (pattern completions)
>    "Show where and how well PATTERN matches COMPLETIONS.
>  PATTERN, a list of symbols and strings as seen
>  `completion-pcm--merge-completions', is assumed to match every
>  string in COMPLETIONS.  Return a deep copy of COMPLETIONS where
> -each string is propertized with `completion-score', a number
> -between 0 and 1, and with faces `completions-common-part',
> +each string is propertized with faces `completions-common-part',
>  `completions-first-difference' in the relevant segments."

Are we really losing the completion-score property here?  If so, why?

> +           ;; If `pattern' doesn't have an explicit trailing any,

This is confusing: what do you mean by "explicit trailing any" in the
context of patterns?

> +(defun completion--flex-score (pattern completions)
> +  "Compute how well PATTERN matches COMPLETIONS.
> +PATTERN, a list of strings is assumed to match every string in
> +COMPLETIONS.

Is PATTERN really a list?  It would be strange for a list to be called
PATTERN, and how can a list "match every string in COMPLETIONS"?

>               Return a copy of COMPLETIONS where each element is
> +a pair of a score and the completion string.

What is "the completion string" in this case? is it the same string
from COMPLETIONS, or is it something else?  The doc string leaves that
unclear.

>                                                The score lies in
> +the range between -1 and 0, where -1 corresponds to the full
> +match."

What score could a partial match have, and what is the meaning of the
numerical value for a partial match?