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#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting Date: Fri, 13 Aug 2021 14:56:38 +0200 Message-ID: <9f59f87c-2489-aaa0-5b3f-0e911b7ffa6c@daniel-mendler.de> References: <3d3f894f-a6fa-53ae-5d50-c3aa9bffc73e@daniel-mendler.de> <56ab18b1-4348-9b2c-85bb-af9b76cd429a@daniel-mendler.de> <38a06f3c-4a7a-755c-c99b-708f91afabfa@daniel-mendler.de> 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="32095"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 47711@debbugs.gnu.org, Stefan Monnier , 48841@debbugs.gnu.org, Dmitry Gutov To: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Aug 13 14:58:11 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 1mEWlH-00083s-2f for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 14:58:11 +0200 Original-Received: from localhost ([::1]:39562 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mEWlF-0002O2-2f for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 08:58:09 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:42742) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mEWl8-0002ML-HQ for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:58:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:57508) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mEWl8-0002Q7-8h for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:58:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mEWl8-0002mG-8R for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:58: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: Fri, 13 Aug 2021 12:58:02 +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.162885942310609 (code B ref 47711); Fri, 13 Aug 2021 12:58:02 +0000 Original-Received: (at 47711) by debbugs.gnu.org; 13 Aug 2021 12:57:03 +0000 Original-Received: from localhost ([127.0.0.1]:40820 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEWk6-0002kW-V9 for submit@debbugs.gnu.org; Fri, 13 Aug 2021 08:57:03 -0400 Original-Received: from server.qxqx.de ([178.63.65.180]:56345 helo=mail.qxqx.de) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEWjw-0002k2-Bh; Fri, 13 Aug 2021 08:56:53 -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=a89yZI6TfoUJ6rLAH1MakLAhc71tO9b1C8U/CHdpsC8=; b=N4MIoo3oh97UH6pQB1pEy7f66+ 9sONgkN7OIXHdBdyqVEavbTl7vLSVNa0DJfEcPc+jLn1uMcJhu7xLnmEPYWbWDt0Zjt5Xil5g4Qt5 sgbdZcDkgu7zBVoSKBaf+jKohOVq31IKqv9Yz5dnImPysQjXCqiE1zInTEHWiTn5Mu34=; In-Reply-To: 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:211753 Archived-At: On 8/13/21 2:37 PM, João Távora wrote: > On Fri, Aug 13, 2021 at 1:22 PM Daniel Mendler wrote: > >> It is important to keep this property since this will preclude many bugs >> due to string mutation. > > I am aware of this, of course. Can you give examples of these "many bugs"? > Perhaps other than the one I already described and addressed? No, João, this is not how it goes. I don't have to prove to you that your idea introduces bugs. You have to show that mutation of the completion table strings (which are not supposed to be mutated) will not lead to bugs, which are hard to find. In contrast with the new API `completion-filter-completions` this entire class of bugs is avoided by construction of the API. Furthermore the `completion-filter-completions` API is easy to use in comparison to your idea, where "compliant" backends have to apply string manipulations to apply the highlighting and revert the strings back to their old pristine state. The only thing the API user has to do is to call the `highlight` function returned in the alist by `completion-filter-completions`. >> By separating the filtering and mutation >> (highlighting, scoring) my patch addresses the problem at hand in the >> proper way. >> [ ... ] >> Mutation would be a reasonable choice here if the problem could not be >> solved in a proper way. But in fact it can be solved in a proper way >> without mutating the strings at all as my patch shows. > > "proper" is just an reasonably empty adjective. There are different ways to > go about this, of course. What's "proper" and isn't is hard to debate > objectively. You are contradicting yourself here. You agree that string mutation is better be avoid. If we define "proper" as avoids string mutation if this is easily possible, then my patch implements a proper solution to the problem. >>> The advantage that I see is that those adaptations apart, it is a small >>> localized and effective change. >> >> Note that your idea also does not address the other issues which are >> addressed by my patch. > > That's for sure. My patch idea addresses only that single problem. > I think this is a good property of patches: to solve one thing, not many. No, this is not necessarily true. This is only good if the problem is solved in a way which is future proof. The idea of mutating the strings is a hack and not a solution. In contrast, I am presenting a future-proof new API as solution which addresses multiple problems. If you look at the patch, only 196 new lines are added to minibuffer.el. Furthermore the patch adds 213 lines of new tests. > Look, one needs to evaluate things quantitively. Your patch is not > to Vertico, it's to Emacs. I'm concerned with changes to Emacs and their > effect on all completion frontends. So trying Vertico isn't very useful. > > If you're solving a performance problem (and it seems that you are, among > other things) we really need benchmarks, a description of an experiment whose > results can be reproduced independently. It's the normal scientific method. João, you don't have to lecture me on these things. Of course I can provide such numbers. You cannot reasonably make the claim that `copy-sequence` is the problem and at the same time claim that my patch does not solve the performance issues, when in fact my patch avoids this exact string copying. >>>> The second problem addressed by the new API >>>> `completion-filter-completions` is that `completion-all-completions` is >>>> limited in what it can return. For example it cannot return the end >>>> position of the completion. >>> >>> And why is this a problem? Can you post an example of something you'd >>> like to do, but can't? Regardless, it does seem indeed like a "second" problem >>> (as you state) so perhaps something that can be addressed separately. >> >> Please look at the FIXMEs in minibuffer.el which address this. >> Currently only the beginning position of the completion boundary is >> returned, which is only half of the information. > > OK. It does seem like a separate problem, so maybe open a new bug for it? There is already a FIXME in minibuffer.el, so I assume Stefan Monnier is well aware of these issues. It is an additional win of the new API that such open problems can be fixed too. As I see it, a new API is the way to go here. Daniel