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

[Daniel Mendler <mail@daniel-mendler.de>]

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