Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No filtering when constructing the candidate list during completion #1463

Closed
wants to merge 5 commits into from

Conversation

roosephu
Copy link

@roosephu roosephu commented Jan 1, 2022

No unit test, though. Only tested completing variables ($edit:re) and paths.

Notes:

  1. The initial candidate list is longer filtered by prefix. Filtering is done by ComboBox and considers substrings by default.
  2. Smart-start is unchanged. it is triggered iff. all candidates with the same prefix share a longer common prefix. That is, it's possible that smart-start is triggered even if the initial candidate list contains an item not starting with the seed.

@krader1961
Copy link
Contributor

@roosephu, Your change fails on two of the CI environments. It also fails on all eight of my systems that I use to test Elvish changes. So it appears your change needs more work to update existing unit tests even if you don't add tests for the new functionality you're introducing. Also, a change to the public behavior should, ideally, be associated with an open issue that documents why such a change is warranted and where there has been, at least in theory, a discussion about the merits of such a change. Especially since you're changing user visible behavior as reflected in failures of the existing unit tests.

@xiaq
Copy link
Member

xiaq commented Jan 1, 2022

As we discussed offline, I like the idea of outsourcing all filtering to ComboBox. This PR is in the right direction but it leaves the completion system in an inconsistent state:

  • When using a customer completion matcher, inserting the completion seed to ComboBox is not the correct behavior under customer matchers For example, if the completion matcher is edit:match-subseq - fr will match foobar and froo, but inserting fr into ComboBox means that only froo gets shown.

  • The behavior of the completion widget after clearing the ComboBox filter now depends on whether a customer matcher is used. If it is used, clearing the filter still only shows the candidates that match the custom matcher. If it is not used, clearing the filter shows all the candidates (as if the completion seed is empty).

  • There is also now a behavior change: the default completion matching behavior is now substring, not prefix.

Fully outsourcing all filtering to ComboBox requires a reworking of the completion matcher mechanism. Here is one way this can work:

  • Instead of actually performing the matching, custom matchers should generate filters.
  • The generated filter can then be copied verbatim to the ComboBox.
  • For example, a prefix matcher should generate [re foo] (see filter DSL) for seed foo; a substring matcher should generate [re 'f.*o.*o'] for seed foo.

However, this will result in some loss of expressiveness - the possible filters are now fully constrained to what the filter DSL supports. This will make things such as my smart matcher impossible, which is undesirable.

I don't have a better design at this moment, but it's something I'll keep thinking about.

@xiaq
Copy link
Member

xiaq commented May 22, 2022

Closing since there hasn't been any activity recently. If anyone is willing to pick this up, please refer to my last comment.

@xiaq xiaq closed this May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants