From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#47711: bug#48841: [PATCH] Add new `completion-filter-completions` API and deferred highlighting Date: Thu, 12 Aug 2021 11:00:11 +0300 Message-ID: <837dgrdrec.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1907"; mail-complaints-to="usenet@ciao.gmane.io" Cc: joaotavora@gmail.com, dgutov@yandex.ru, monnier@iro.umontreal.ca, 48841@debbugs.gnu.org, 47711@debbugs.gnu.org To: Daniel Mendler Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 12 10:01:21 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mE5eT-0000LN-K8 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 12 Aug 2021 10:01:21 +0200 Original-Received: from localhost ([::1]:45234 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mE5eS-00008h-58 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 12 Aug 2021 04:01:20 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:55284) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mE5eB-00008O-0T for bug-gnu-emacs@gnu.org; Thu, 12 Aug 2021 04:01:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:54093) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mE5eA-0002jV-Au for bug-gnu-emacs@gnu.org; Thu, 12 Aug 2021 04:01:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mE5e9-0007VN-VQ for bug-gnu-emacs@gnu.org; Thu, 12 Aug 2021 04:01:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 12 Aug 2021 08:01:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 47711 X-GNU-PR-Package: emacs Original-Received: via spool by 47711-submit@debbugs.gnu.org id=B47711.162875524128803 (code B ref 47711); Thu, 12 Aug 2021 08:01:01 +0000 Original-Received: (at 47711) by debbugs.gnu.org; 12 Aug 2021 08:00:41 +0000 Original-Received: from localhost ([127.0.0.1]:37404 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mE5do-0007UR-3t for submit@debbugs.gnu.org; Thu, 12 Aug 2021 04:00:40 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:46316) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mE5dl-0007U9-G8; Thu, 12 Aug 2021 04:00:38 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:58862) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mE5dd-0002GC-RB; Thu, 12 Aug 2021 04:00:29 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:3162 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mE5dd-0008LK-Eb; Thu, 12 Aug 2021 04:00:29 -0400 In-Reply-To: (message from Daniel Mendler on Wed, 11 Aug 2021 16:16:57 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:211646 Archived-At: [I removed emacs-devel from the CC list, please don't cross-post to both emacs-devel and the bug tracker.] > From: Daniel Mendler > Date: Wed, 11 Aug 2021 16:16:57 +0200 > Cc: 48841@debbugs.gnu.org, Dmitry Gutov , > João Távora , > Stefan Monnier , 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?