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 16:03:21 +0200 Message-ID: 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> <9f59f87c-2489-aaa0-5b3f-0e911b7ffa6c@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="1699"; 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 16:04:17 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 1mEXnF-0000At-BR for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 16:04:17 +0200 Original-Received: from localhost ([::1]:46940 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mEXnE-0007GC-4U for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 10:04:16 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:56860) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mEXn0-0007Fm-K8 for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 10:04:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:59465) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mEXn0-00086o-E8 for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 10:04:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mEXn0-0005LF-AE for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 10:04: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 14:04: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.162886342420493 (code B ref 47711); Fri, 13 Aug 2021 14:04:02 +0000 Original-Received: (at 47711) by debbugs.gnu.org; 13 Aug 2021 14:03:44 +0000 Original-Received: from localhost ([127.0.0.1]:42776 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEXme-0005KK-Tz for submit@debbugs.gnu.org; Fri, 13 Aug 2021 10:03:44 -0400 Original-Received: from server.qxqx.de ([178.63.65.180]:52349 helo=mail.qxqx.de) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEXmV-0005Jq-Ns; Fri, 13 Aug 2021 10:03:36 -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=U+7jHgcxqSUV0RHeOu3QFSoRtGe0iLwvU6TC36yrcVI=; b=EcPIMUnEp4uE8O01Mqno6vFAsl rUQgFBQ6At+ydR0XwESZnD/Q1mhooK6GNCJmS6WcUPXOPRR//qIGlTuFIPoxXHXt5ps4KrGh8Fs0W muuQwa8s1CF0tqEG9JJpzW/GomDGoT7mDRug2LZ73hg4xE0AWTjiLc32Ot2ecxPYC9EI=; 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:211769 Archived-At: On 8/13/21 3:36 PM, João Távora wrote: >> 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. > > I didn't say it's better avoided, though of course I will avoid _any_ change if > I can. I said I have identified one drawback with doing it. Then I > have addressed > that drawback. So that's what I said. > > I am unaware of _other_ drawbacks. They might exist, but I am unaware of > them. Perhaps you are, and indeed you state they exist, but you refuse to > let me know about them. Or perhaps others know of them and will let me know. > In my long-running discussion with Dmitry they were not presented (again, > except for the one I identified). In the discussion with Dmitry, I already pointed out that there is an alternative principled approach implemented by my Vertico UI, which is in fact the same approach as implemented in this patch. If there are other useful conclusions from the discussion I will adopt them here for this 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. > > OK, but what thing of the future, real or academic, do you envision that > would bring back the problem, or create other problems? > >> The idea of mutating the strings is a hack and not a solution. > > Without facts to back it up, I have to take this as gratuitous disparagement. > Nicht so gut. João, your whole answers are "nicht so gut" or not useful. What is your point? Please give constructive technical feedback instead of such empty phrases. >> In contrast, I am presenting a >> future-proof new API as solution which addresses multiple problems. > > That's the issue. The completion system is very complex and there are many > good ideas, different, floated by many people. But if you make a patch to > address "multiple" fuzzily-described problems, it's hard to judge how good > your ideas even are! Maybe they are indeed very good, I never said > they weren't. No need to get worked up about it! > > Again, my proposal is to first focus on the performance problems caused by > string allocation. _That_ problem is well understood, at least by me (but it > would help to settle on convenient benchmarks understood by others, too). > Then we can go from there. No, it is not the correct approach to fix larger issues by applying localized patches. We both have identified the string allocations and highlighting as problem. My patch resolves the problem, by exposing just the right pieces of the already existing completion machinery. More about this below. >> you look at the patch, only 196 new lines are added to minibuffer.el. >> Furthermore the patch adds 213 lines of new tests. > > It's a large patch, over 1000 lines. One does not review a patch > merely by looking at > lines added, when one needs to read much more, to understand implications, etc. > It needs documentation, for one, much more than just docstrings, on > how to use the > new API. I suggest you take a step back here and try to understand the high-level idea first. It seems that you are misjudging the complexity of the patch. The minibuffer completion machinery is already constructed such that filtering and highlighting are separate. If you look at `completion-basic-all-completions` for example, there is first a filtering step and then the highlighting is applied in a second step by the function `completion-hilit-commonality`. This separation exists for all completion styles. My patch does nothing else than separating these two processing steps. The new API `completion-filter-completions` returns the filtered list and a function to apply highlighting afterwards only to the actually displayed candidates where highlighting is needed. In contrast your idea totally misses this. > Maybe others review patches on other aspects that's fine. Maybe > others will. Eli reviewed on minor formatting and documentation aspects. I am looking forward to more reviews by other people. Your desire for benchmarks is understandable, but I doubt that it will lead to progress in the discussion here and I doubt that it will convince you. The outcome of the benchmark is the following - my patch only filters and does not mutate the strings, so it will be slightly faster than your idea where the strings are mutated first and afterwards the mutation has to be undone again. However the mutations are of course not expensive, so the differences will be small. The discussion we should be having here is about technical details and internals and not about the numbers which won't give any guidance in this case regarding the correct API design. You seem to always come back to the "scientific method". Note that there is not only statistics, there is only "scientific reasoning" and mathematics, which allows to reason about transformations and drawing conclusions from that. If you don't do this, you are only doing half of the science. Daniel