Adding length tiebreak for fzf-v2 sorter - #2335
Merged
Merged
Conversation
The fzf-v2 sorting method scored every row but had no tie-breaking, and
ties are the common case: fzf's score describes only the matched region,
so every line matching a term at a word boundary scores identically.
Filtering a list of emoji names on "eyes" gives all fifteen lines a
score of 114, leaving the obvious "eyes" entry wherever it happened to
sit in the input rather than first.
Sort on fzf's own key instead of the bare negated score. fzf compares a
[4]uint16 tuple (Result.points in src/result.go) from the most
significant element down, and for its default scoring scheme that tuple
holds the inverted, uint16-clamped score followed by the length in code
points ignoring leading and trailing whitespace ({byScore, byLength} in
parseScheme, src/options.go). Packing both into one integer and
comparing numerically reproduces that ordering.
The length gets 15 bits rather than fzf's 16, which makes the widest key
exactly G_MAXINT so it still fits the int the weight is stored in and
lev_sort() can keep subtracting two keys without overflowing. Nothing is
lost in practice: rofi_scorer_fzf_v2_evaluate() rejects anything longer
than FUZZY_SCORER_MAX_LENGTH outright, so every row that matches at all
is far below the cap.
Rows tying on both criteria are left to the sort, where fzf would fall
back to the input order. g_qsort_with_data() is stable from glib 2.82,
below which such a tie is unspecified, as it already is for the other
sorting methods.
Checked against `fzf --filter --scheme=default` over 21 queries,
including multi-term ones, and the orderings match.
Rows tying on both the score and the trimmed length got identical keys, and lev_sort() reports those as equal, so their order was left to g_qsort_with_data(). That is only guaranteed stable from glib 2.82, below the 2.72 rofi builds against, so such rows could come back in any order rather than the input order fzf falls back to. Compare the row index as the final criterion, which is what fzf does (compareRanks in src/result_others.go). The values being sorted are indices into the unfiltered list, so the input position is already to hand. Done in a comparator of its own rather than in lev_sort(), so that the normal and fzf sorting methods keep resolving their own ties exactly as before. The ordering test now seeds the array in reverse, so the tied rows only come out in input order if the comparison really does fall back to the index; a stable sort alone would leave them reversed.
Cut the explanation to what the code does not already say: the fzf sources the key mirrors, why the length field is a bit narrower than fzf's, and why the tie-break cannot be left to the sort. The rest was restating the lines underneath it.
| g_qsort_with_data(state->line_map, j, sizeof(int), lev_sort, | ||
| g_qsort_with_data(state->line_map, j, sizeof(int), | ||
| config.sorting_method_enum == SORT_FZF_V2 ? fzf_v2_sort | ||
| : lev_sort, |
Collaborator
There was a problem hiding this comment.
For readability can this not be an inline ternary operator and just an if/else ?
| /* Scores tie constantly, so sort on fzf's key (inverted score plus | ||
| * a length tiebreak) rather than on the score alone. */ | ||
| t->state->distance[i] = rofi_scorer_fzf_v2_sort_key( | ||
| rofi_scorer_fzf_v2_evaluate(t->pattern, t->plen, str, slen, |
Collaborator
There was a problem hiding this comment.
can this be split, so its not nested calls.
(readability)
Reads better than a ternary spread over three lines.
| /** Code points in `str` ignoring surrounding whitespace; fzf's TrimLength(). */ | ||
| static guint16 rofi_scorer_fzf_v2_trim_length(const char *str) { | ||
| glong len; | ||
| gunichar *txt = g_utf8_to_ucs4_fast(str, -1, &len); |
Collaborator
There was a problem hiding this comment.
Not sure about performance? but do we need the extra allocation here? wouldn't a g_utf8_next_char and g_utf8_get_char work for this?
Collaborator
There was a problem hiding this comment.
g_utf8_prev_char for going backwards
Contributor
Author
There was a problem hiding this comment.
Good call, done. g_utf8_next_char forward and g_utf8_prev_char backward,
Collaborator
|
Thanks! looks ok to me. (Don't use this matcher myself, but closer to fzf is better as that is what people are used too) |
Reads better than passing one scorer call straight into the other.
Drops the UTF-32 copy in favour of g_utf8_next_char/g_utf8_prev_char, so no allocation per row. The all-whitespace case now falls out of the backward walk rather than needing its own early return. Identical results to the previous version over 200k random strings and the awkward cases (NBSP, em and ideographic space, combining marks, empty and all-whitespace input).
Contributor
Author
|
Thanks Dave! I have addressed your comments. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Follow-up to #2306.
Problem
fzf-v2scores rows but never breaks ties, and ties are the normal case: fzf's score describes only the matched region, so every line matching a term at a word boundary scores the same.Four of those lines score an identical 140, so
applelands 4th instead of 1st. Real fzf puts it first.Fix
Sort on fzf's own key rather than the bare negated score. fzf compares a
[4]uint16tuple (Result.points,src/result.go) from the most significant element down; for its default scheme that's the inverted, uint16-clamped score followed by the length in code points ignoring surrounding whitespace ({byScore, byLength}inparseScheme,src/options.go). Packing both into one integer reproduces the ordering.The length field gets 15 bits rather than fzf's 16, so the widest key is exactly
G_MAXINT. That keeps it in the existingintweight and letslev_sort()go on subtracting keys without overflow. Costs nothing in practice: the scorer rejects anything overFUZZY_SCORER_MAX_LENGTH, so any row that matches is far below the cap.Rows tying on both criteria are left to the sort, where fzf falls back to input order.
g_qsort_with_data()is stable from glib 2.82; below that such a tie is unspecified, as it already is for the other sorting methods.Scoped to
SORT_FZF_V2—normalandfzfordering is untouched, and the only change outside the new helper is one hunk infilter_elements().Testing
22 new assertions in
helper-test.c. Ordering checked againstfzf --filter --scheme=defaultover 21 queries including multi-term ones; all match.