From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Daniel Mendler Newsgroups: gmane.emacs.bugs Subject: bug#48841: [PATCH] Add new `completion-filter-completions` API and deferred highlighting Date: Mon, 16 Aug 2021 11:42:22 +0200 Message-ID: References: <837dgrdrec.fsf@gnu.org> <83mtpkbky3.fsf@gnu.org> 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="34876"; mail-complaints-to="usenet@ciao.gmane.io" Cc: dgutov@yandex.ru, monnier@iro.umontreal.ca, joaotavora@gmail.com, 48841@debbugs.gnu.org, 47711@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Aug 16 11:43:18 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 1mFZ9I-0008sP-OK for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 16 Aug 2021 11:43:16 +0200 Original-Received: from localhost ([::1]:38036 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mFZ9E-0004vq-UU for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 16 Aug 2021 05:43:14 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:57614) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mFZ94-0004vK-WF for bug-gnu-emacs@gnu.org; Mon, 16 Aug 2021 05:43:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:37014) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mFZ94-0000nF-Pl for bug-gnu-emacs@gnu.org; Mon, 16 Aug 2021 05:43:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mFZ94-0004vG-NG for bug-gnu-emacs@gnu.org; Mon, 16 Aug 2021 05:43:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Daniel Mendler Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 16 Aug 2021 09:43:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 48841 X-GNU-PR-Package: emacs Original-Received: via spool by 48841-submit@debbugs.gnu.org id=B48841.162910696818890 (code B ref 48841); Mon, 16 Aug 2021 09:43:02 +0000 Original-Received: (at 48841) by debbugs.gnu.org; 16 Aug 2021 09:42:48 +0000 Original-Received: from localhost ([127.0.0.1]:48559 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mFZ8c-0004uK-KL for submit@debbugs.gnu.org; Mon, 16 Aug 2021 05:42:47 -0400 Original-Received: from server.qxqx.de ([178.63.65.180]:43069 helo=mail.qxqx.de) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mFZ8Y-0004ts-8W; Mon, 16 Aug 2021 05:42:31 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=qxqx.de; s=mail1392553390; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=C6FYAFTRxF50v0M2jWCYcMTMDZouKfxCvDwJMzz16sk=; b=FKUNVDxD41IMLfHD0mdoT3ASsD n3JSH3Cw1ZWQxsEb4xxFSb6GITypglkL0jp1mlqd/9S4aom7WFdGcGAWDwvHSV8H39mwsQ19mtL0i lUt0Wz1Rm2H835cMHlsoIZuWmt8e9ySFM5KF6+UFScatCxCtVJ3u0ijJPUX5qTV2hHcM=; In-Reply-To: <83mtpkbky3.fsf@gnu.org> Content-Language: en-US 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:211976 Archived-At: Hello Eli, here I respond to the comments you sent out after I've already sent the overhauled patch. On 8/14/21 8:27 AM, Eli Zaretskii wrote: >> The function 'completion-filter-completions' receives a completion table >> as argument. The strings produced by this table are returned >> unmodified, but of course the completion table has to produce them. For >> a static completion table (e.g., in the simplest case a list of strings) >> the completion table itself will not allocate strings. In this scenario >> 'completion-filter-completions' will not perform any string allocations, >> only the list will be allocated. This is what leads to major >> performance gains. > > My point was that at least some of this should be in the description, > otherwise it will leave the reader wondering. I agree with this. The documentation should be improved. >>>> +(defvar completion--filter-completions nil >>>> + "Enable the new completions return value format. >>> >>> 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? >> >> (As I understood the style guide '-p' is not a good idea for boolean >> variables, since a value is not a predicate in a strict sense.) >> >> To address your technical comment - this variable is precisely what one >> of the technical difficulties mentioned in my other mail is about. The >> question is how we can retain backward compatibility of the completion >> style 'all' functions, e.g., 'completion-basic-all-completions', while >> still allowing the function to return the newly introduced alist format >> with more data, which enables 'completion-filter-completions' to perform >> the efficient deferred highlighting. > > I understand, but given that we provide this for other packages, it > shouldn't be an internal variable. No, we explicitly don't provide this variable for other packages. It is explicitly only meant to be used for the existing completion styles emacs21, emacs22, basic, substring, partial-completion, initials and flex to opt-in in a backward-compatible/calling convention preserving way to the alist return format. The idea is to keep the existing APIs fully backward compatible. Other packages should select the format returned from the completion styles differently. They should return the alist format on Emacs 28 or if the API 'completion-filter-completions' API is present. In the not so near future external packages which support only Emacs 28 and upwards will then only return the alist format and don't have to perform any detection anymore. >>> 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".) >> >> The new API 'completion-filter-completions' will substitute the existing >> API 'completion-all-completions'. > > That's your hope, and I understand. But we as a project didn't yet > decide to deprecate the original APIs, so talking about superseding is > premature. It is not the hope - it is the explicit goal. The API has been designed to replace the existing API 'completion-all-completions'. We can of course tone this down. However I, as a package author, would appreciate if Emacs tells me when a newer API aims to replace another API and when the documentation is explicit about it. Of course if you decide to have the doc strings written in a different tone, I will adapt my patch accordingly. Here I am just explaining why I chose the word "superseded" instead of a more neutral word. >>> 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. >> >> "Filter" seems like exactly the right word to me. The function takes a >> list of strings (or a completion table) and returns a subset of matching >> completion strings without further modifications to the strings. See >> above what I wrote about allocations. > > But the name says "filter completions". Which would mean you take a > list of completions and filter out some of them. A completion table > is much more general object than a list of strings. Thus, I think > using "filter" here is sub-optimal. Okay, you are right about this. But I think even if the name 'completion-filter-completions' is not 100% precise it still conveys what the API is about. 'completion-completions-alist' or 'completion-all-completions-as-alist' are valid names of course, but I dislike them for their verbosity. Already 'completion-all-completions' is quite verbose. A strong argument to use this long name is that the completion style functions are still called 'completion-basic-all-completions' etc. But if we accept that the new API 'completion-filter-completions' will actually supersede the existing API 'completion-all-completions' it makes sense to use a name which will not hurt our eyes in the long run. However this is of course just a personal preference of mine. I don't want to spent much time with name bikeshedding discussions. If you decide on a name, I will adapt my patch accordingly. >>>> +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? >> >> Base position is a fixed term which is already used in minibuffer.el for >> completions. See also 'completion-base-position' for example. > > Well, we don't have to keep bad habits indefinitely. It's okay to > lose them and use better terminology. Or at least to explain that > terminology in parentheses the first time it is used in some context. Okay, I agree. However I tried to avoid including superfluous changes with my patch set. We should add more context and documentation and then rename the variables in another patch if we decide that we want to go through with it. >>> Are we really losing the completion-score property here? If so, why? >> >> Yes, the property is removed in the current patch. It is not actually >> used for anything in the new implementation. But it is possible to >> restore the property such that 'completion-all-completions' always >> returns scored candidates as it does now. See my other mail regarding >> the caveats of the current patch. > > I'd prefer not to lose existing features, because that'd potentially > make the changes backward-incompatible. The overhauled patch (version 2) ensures that no features are lost. The patch is fully backward compatible. There is a potential performance regression in 'completion-all-completions' as identified by João. I have yet to confirm this regression. In any case, it should be fixable since the refactored 'completion-all-completions' API does precisely the same amount of work as it does now. Furthermore more documentation should be added to my patch. As of now 'completion-all-completions' is not mentioned in the info manual and 'completion-filter-completions' is also not added there. We may want to improve the documentation of that. But for now I would like to limit my documentation improvements to expanding the doc strings of the functions involved in my patch. Daniel