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

[Daniel Mendler]

On 8/16/21 12:15 PM, João Távora wrote:
> On Mon, Aug 16, 2021 at 10:09 AM Daniel Mendler <mail@daniel-mendler.de> 
> wrote:
> 
>> There are serious drawbacks of attaching "private" string properties to
>> arbitrary strings. For once it complicates debugging seriously if there
>> are suddenly string properties attached to symbol names. It also leads
>> to a potential memory leak.
> 
> Please, in the name of the sanity of this discussion, justify these two
> statements with examples or follow them with a clause like "because...".

João, I am giving hard examples here. What is not an example about
"memory leak" or making debugging output verbose thanks to the attached
string properties? You always repeat your demands but whatever I write
it is not satisfactory for you. Is it possible to convince you? Can you
try to interpret my arguments in a positive and constructive light
somehow and fill them with meaning such that it makes sense to you? My
goal is not to be right here just for the sake of being right. As Eli
said, nobody has to prove anything.

>> 3. The `completion-filter-completions` API is the fastest possible API
> 
> Again that's quite a statement that I cannot evaluate the veracity of
> without hard proof.

As I said, I will ensure that my API does not introduce performance
regressions. And since my new API performs strictly less work than your
proposal it will necessarily be faster if you consider only the
filtering, which is what matters for incrementally updating UIs.

>> 4. If 'completion-all-completions' does indeed get slower thanks to my
>> patch, it is a performance regression of my patch. I will fix this. And
>> I thank you João for bringing this to my attention. However one should
>> also consider that in the end, 'fido-mode' and 'icomplete-mode' should
>> move to the new API 'completion-filter-completions' such that they
>> benefit from the fast filtering and only copy and highlight the actually
>> displayed strings.
> 
> Maybe they will, maybe they will.  But it's still quite early to decide if
> we're going to move all frontends to that API.  There's no manual
> documentation for it. Conceivably, if you appreciate your API so, you
> could demonstrate in practice us how easy it is to use by providing
> a separate patch that adapts icomplete-mode and fido-mode to use it.

There is also no manual documentation of 'completion-all-completions'.
Of course you are right that it is early to make these decisions, but
the new API 'completion-filter-completions' is designed in a way to
allow adoption by 'icomplete-mode'. It is important to design the API
such that adoption is possible. I have a patch for Vertico which shows
this. I can provide patches for 'icomplete-mode' separately later on.

> In the meantime, there is a patch with a measured and documented
> performance boost where fido-mode and icomplete-mode move
> opt-in to a new `completion-lazy-hilit` feature in the "old" API with
> a total or four 1-line changes.  That patch lives in the branch
> scratch/icomplete-lazy-highlight-attempt-2.

As argued multiple times here now, the change you are proposing is
fragile. It will lead to problems later on. The goal is not to find the
smallest change which leads to a performance boost, all API violations
allowed. Attaching "private" string properties to arbitrary strings is
an API violation which we will regret later and which will make Emacs
harder to debug and harder to use.

> I think we should move to that, solving the bug#48841 while we
> do the evaluation of all aspects of your contributions.

No, we should not merge this problematic patch of yours. See the many
arguments against this proposal.

Daniel