ranked match: prefer input order over alphabetical order for user-specified completions #5035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krobelus
force-pushed
the
prefer-input-order-over-alphabet
branch
2 times, most recently
from
838d7cd
to
5806523
Compare
mawww
reviewed
Dec 1, 2023
src/insert_completer.hh
Outdated
| String completion; | ||
| String on_select; | ||
| DisplayLine menu_entry; | ||
|
|
||
| bool operator==(const Candidate& other) const { return completion == other.completion; } | ||
| auto operator<=>(const Candidate& other) const { return completion <=> other.completion; } | ||
| auto operator<=>(const Candidate& other) const { return std::tie(completion, input_order) <=> std::tie(other.completion, other.input_order); } |
There was a problem hiding this comment.
This is a bit weird, equality and ordering are not consistent with each other.
There was a problem hiding this comment.
true. Happily we don't need this change at all because the comparison is only used for deduplicating full-line completions. For those I don't think we care about preserving the input order.
And if we do care, we could use a stable sort for <c-x>l completions.
Same for buffer completions, I don't have a strong opinion on whether they should respect input order.
I'm leaning towards no but if we do want it it'd be
diff --git a/src/commands.cc b/src/commands.cc
index fb0b790dd..097895c98 100644
--- a/src/commands.cc
+++ b/src/commands.cc
@@ -185,8 +185,8 @@ static Completions complete_buffer_name(const Context& context, CompletionFlags
if (RankedMatch match{bufname, query})
matches.emplace_back(match, buffer.get());
}
- std::sort(filename_matches.begin(), filename_matches.end());
- std::sort(matches.begin(), matches.end());
+ std::stable_sort(filename_matches.begin(), filename_matches.end());
+ std::stable_sort(matches.begin(), matches.end());
CandidateList res;
for (auto& match : filename_matches)While at it, remove a needless reserve() call and reserve an extra slot because "InsertCompleter::try_complete" might add one more element.
krobelus
force-pushed
the
prefer-input-order-over-alphabet
branch
from
5806523
to
d6b5774
Compare
krobelus
force-pushed
the
prefer-input-order-over-alphabet
branch
2 times, most recently
from
df698f6
to
658c338
Compare
…cified completions When using either of set-option g completers option=my_option prompt -shell-script-candidates ... While the search text is empty, the completions will be sorted alphabetically. This is bad because it means the most important entries are not listed first, making them harder to select or even spot. Let's apply input order before resorting to sorting alphabetically. In theory there is a more elegant solution: sort candidates (except if they're user input) before passing them to RankedMatch, and then always use stable sort. However that doesn't work because we use a heap which doesn't support stable sort. Closes mawww#1709, mawww#4813
|
updated the builtin uses of |
mawww
reviewed
Dec 3, 2023
| @@ -25,7 +25,7 @@ define-command -params ..1 \ | |||
| cut -f 1 "$tags" | grep -v '^!' | uniq > "$namecache" | |||
| fi | |||
| cat "$namecache" | |||
| done} \ | |||
| done | sort } \ | |||
There was a problem hiding this comment.
Piping into sort means we wont get partial results in shell-script-candidates, which is a shame in the cases such as doc or man where we could get candidates progressively out of the find command. I wonder if sorting really is better than random order.
krobelus
added a commit
to krobelus/kakoune
that referenced
this pull request
Jan 13, 2024
As of recently, shell script candidate completions are computed in the background, and displayed incrementally. Sorting completions means we can't show partial results. We do this for "ctags-search"; but if there is only one ctags file there is already a sensible order (which is slightly different than what GNU sort does). So let's preserve the order in that case. The number of completions is probably too high for an order to be useful. Similarly, for "man", sorting is probably not very helpful because there are so many results. See mawww#5035 (comment)
evannjohnson
pushed a commit
to evannjohnson/kakoune-ev
that referenced
this pull request
Jan 25, 2024
As of recently, shell script candidate completions are computed in the background, and displayed incrementally. Sorting completions means we can't show partial results. We do this for "ctags-search"; but if there is only one ctags file there is already a sensible order (which is slightly different than what GNU sort does). So let's preserve the order in that case. The number of completions is probably too high for an order to be useful. Similarly, for "man", sorting is probably not very helpful because there are so many results. See mawww#5035 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using either of
While the search text is empty, the completions will be sorted
alphabetically.
This is bad because it means the most important entries are not listed
first, making them harder to select or even spot.
Let's compare input order before sorting alphabetically.
In theory there is a more elegant solution: sort candidates (except if they're user input) before passing them to RankedMatch, and only use stable sort. However that doesn't work because we use a heap which doesn't support stable sort.
Closes #1709, #4813