fix: reorder batches in --tac mode (#1150)

closes #1148
This commit is contained in:
LoricAndre 2026-08-07 12:10:17 +02:00 committed by GitHub
parent d99d6cecc7
commit b17e93890a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 228 additions and 14 deletions

View file

@ -611,7 +611,10 @@ Interruption is cooperative: each chunk checks `interrupt.load(Relaxed)` before
| --- | --- |
| `Replace` | Fresh match pass (query changed, full re-sort) |
| `SortedMerge` | New items arrived during a running match (merge-insert) |
| `Append` | `--no-sort` mode |
| `Append` | Incremental `--no-sort` mode |
| `Prepend` | Incremental `--tac --no-sort` mode, preserving global reverse-input order |
`Rank::index` always records the item's stable ordinal in the original input stream. `RankBuilder` makes both configured index criteria and the implicit final index tiebreak descending under `--tac`, so normal `SortedMerge` remains valid across independently reversed batches.
---
@ -735,6 +738,7 @@ restart_matcher(force)
├─ kill existing matcher_control
├─ determine MergeStrategy
│ ├─ Replace → if query changed / force
│ ├─ Prepend → incremental --tac --no-sort
│ └─ SortedMerge / Append otherwise
└─ matcher.run(query, pool, thread_pool, processed_items, strategy, no_sort, needs_render)
→ returns new MatcherControl
@ -834,6 +838,7 @@ On each render, `ItemList::render()` checks `processed_items` and swaps them in
- `Replace`: replaces `items` entirely.
- `SortedMerge`: performs an O(n+m) merge preserving order.
- `Append`: extends `items`.
- `Prepend`: places a reversed incremental `--tac` batch before existing items; the cursor follows the head unless the user moved away from it.
**Selection state management:**

View file

@ -24,12 +24,14 @@ use tokio::sync::Notify;
#[derive(Debug)]
pub struct RankBuilder {
criterion: Vec<RankCriteria>,
tac: bool,
}
impl Default for RankBuilder {
fn default() -> Self {
Self {
criterion: vec![RankCriteria::Score, RankCriteria::Begin, RankCriteria::End],
tac: false,
}
}
}
@ -43,7 +45,13 @@ impl RankBuilder {
}
criterion.dedup();
Self { criterion }
Self { criterion, tac: false }
}
#[must_use]
pub(crate) fn tac(mut self, tac: bool) -> Self {
self.tac = tac;
self
}
/// Returns the tiebreak criteria slice.
@ -52,6 +60,28 @@ impl RankBuilder {
&self.criterion
}
fn sort_key(&self, rank: &Rank) -> [i32; 6] {
let configured = rank.sort_key(&self.criterion);
let mut key = [0; 6];
key[..5].copy_from_slice(&configured);
if self.tac {
for (priority, criterion) in self.criterion.iter().take(5).enumerate() {
key[priority] = match criterion {
RankCriteria::Index => rank.index.saturating_neg(),
RankCriteria::NegIndex => rank.index,
_ => key[priority],
};
}
}
key[5] = if self.tac {
rank.index.saturating_neg()
} else {
rank.index
};
key
}
/// Computes the byte offset of the first character after the last path separator
/// (`/` or `\`) in `text`. Returns `0` when no separator is present.
fn path_name_offset(text: &str) -> i32 {
@ -121,8 +151,9 @@ pub struct MatchedItem {
/// Range of characters that matched the pattern
pub matched_range: Option<MatchRange>,
/// Sort key precomputed at construction time from `rank` and the tiebreak
/// criteria. Caching avoids recomputing it on every comparison during sort.
sort_key: [i32; 5],
/// criteria. The sixth slot is the tac-aware implicit index tiebreak.
/// Caching avoids recomputing it on every comparison during sort.
sort_key: [i32; 6],
}
impl std::fmt::Debug for MatchedItem {
@ -162,7 +193,7 @@ impl MatchedItem {
item,
rank,
matched_range,
sort_key: rank.sort_key(rank_builder.criteria()),
sort_key: rank_builder.sort_key(&rank),
}
}
/// Merge two sorted `Vec<MatchedItem>` lists into one, preserving sort order by rank.
@ -359,9 +390,7 @@ impl PartialOrd for MatchedItem {
impl Ord for MatchedItem {
fn cmp(&self, other: &Self) -> CmpOrd {
self.sort_key
.cmp(&other.sort_key)
.then_with(|| self.rank.index.cmp(&other.rank.index))
self.sort_key.cmp(&other.sort_key)
}
}

View file

@ -59,6 +59,57 @@ fn matched_item_ordering_prefers_higher_score() {
assert!(high < low);
}
#[test]
fn matched_item_ordering_reverses_stable_input_index_for_tac() {
let old_rank = Rank {
index: 0,
..Default::default()
};
let new_rank = Rank {
index: 1,
..Default::default()
};
let normal_builder = RankBuilder::new(vec![RankCriteria::Index]);
let old = MatchedItem::new(item("old"), old_rank, None, &normal_builder);
let new = MatchedItem::new(item("new"), new_rank, None, &normal_builder);
assert!(old < new);
let tac_builder = RankBuilder::new(vec![RankCriteria::Index]).tac(true);
let old = MatchedItem::new(item("old"), old_rank, None, &tac_builder);
let new = MatchedItem::new(item("new"), new_rank, None, &tac_builder);
assert!(new < old);
assert_eq!(new.rank.index, 1);
assert_eq!(MatchedItem::sorted_merge(vec![old], vec![new])[0].text(), "new");
let tac_builder = RankBuilder::default().tac(true);
let old = MatchedItem::new(item("old"), old_rank, None, &tac_builder);
let new = MatchedItem::new(item("new"), new_rank, None, &tac_builder);
assert!(new < old);
}
#[test]
fn sorted_merge_tac_places_newer_incremental_batch_first() {
let rank_builder = RankBuilder::default().tac(true);
let make = |text: &str, index| {
MatchedItem::new(
item(text),
Rank {
index,
..Default::default()
},
None,
&rank_builder,
)
};
let existing = vec![make("c", 2), make("b", 1), make("a", 0)];
let incoming = vec![make("e", 4), make("d", 3)];
let merged = MatchedItem::sorted_merge(existing, incoming);
let indexes: Vec<_> = merged.iter().map(|item| item.rank.index).collect();
assert_eq!(indexes, [4, 3, 2, 1, 0]);
}
#[test]
fn sorted_merge_handles_empty_inputs() {
let a = vec![matched("a", 0, 10)];

View file

@ -25,6 +25,15 @@ use crate::{CaseMatching, MatchEngineFactory, SkimItem, SkimOptions};
/// flattened without sorting.
///
/// Signals `needs_render` after writing so the UI picks up the new data.
fn input_index(tac: bool, start: usize, batch_len: usize, batch_index: usize) -> usize {
debug_assert!(batch_index < batch_len);
if tac {
start + batch_len - 1 - batch_index
} else {
start + batch_index
}
}
fn merge_worker_results(
worker_results: Vec<Vec<MatchedItem>>,
no_sort: bool,
@ -60,7 +69,12 @@ fn merge_worker_results(
match &mut *guard {
Some(existing) => {
if no_sort {
existing.items.extend(items);
if matches!(merge_strategy, MergeStrategy::Prepend) {
items.append(&mut existing.items);
existing.items = items;
} else {
existing.items.extend(items);
}
} else {
// Both sides are fully sorted — one O(n+m) merge.
MatchedItem::merge_into_sorted(&mut existing.items, items);
@ -192,9 +206,9 @@ impl Matcher {
} else {
Rc::new(regex_factory)
};
(factory, Arc::new(RankBuilder::default()))
(factory, Arc::new(RankBuilder::default().tac(options.tac)))
} else {
let rank_builder = Arc::new(RankBuilder::new(options.tiebreak.clone()));
let rank_builder = Arc::new(RankBuilder::new(options.tiebreak.clone()).tac(options.tac));
log::debug!("Creating matcher for algo {:?}", options.algorithm);
let fuzzy_engine_factory = ExactOrFuzzyEngineFactory::builder()
.fuzzy_algorithm(options.algorithm)
@ -264,6 +278,7 @@ impl Matcher {
processed_items: Arc<SpinLock<Option<ProcessedItems>>>,
merge_strategy: MergeStrategy,
no_sort: bool,
tac: bool,
needs_render: Arc<AtomicBool>,
) -> MatcherControl {
let matcher_engine = self.engine_factory.create_engine_with_case(query, self.case_matching);
@ -336,7 +351,10 @@ impl Matcher {
if let Some(match_result) = matcher_engine.match_item(item.as_ref()) {
chunk_matched += 1;
let mut rank = match_result.rank;
let index = chunk_start + i + start;
let batch_index = chunk_start + i;
// `take()` reverses each tac batch, so recover the
// item's stable ordinal in the original input stream.
let index = input_index(tac, start, total, batch_index);
rank.index = i32::try_from(index).unwrap_or(i32::MAX);
local_matches.push(MatchedItem::new(
Arc::clone(item),
@ -506,4 +524,46 @@ mod tests {
let guard = processed.lock();
assert_eq!(guard.as_ref().unwrap().items.len(), 2);
}
#[test]
fn tac_input_index_spans_incremental_batches() {
let first_batch: Vec<_> = (0..3).map(|index| input_index(true, 0, 3, index)).collect();
let second_batch: Vec<_> = (0..2).map(|index| input_index(true, 3, 2, index)).collect();
assert_eq!(first_batch, [2, 1, 0]);
assert_eq!(second_batch, [4, 3]);
assert_eq!(input_index(false, 3, 2, 0), 3);
assert_eq!(input_index(false, 3, 2, 1), 4);
}
#[test]
fn merge_worker_results_prepend_no_sort_places_new_batch_first() {
let processed = SpinLock::new(None);
let needs_render = AtomicBool::new(false);
merge_worker_results(
vec![vec![matched("c", 2), matched("b", 1), matched("a", 0)]],
true,
&processed,
MergeStrategy::Prepend,
&needs_render,
);
merge_worker_results(
vec![vec![matched("e", 4), matched("d", 3)]],
true,
&processed,
MergeStrategy::Prepend,
&needs_render,
);
let guard = processed.lock();
let indexes: Vec<i32> = guard
.as_ref()
.unwrap()
.items
.iter()
.map(|item| item.rank.index)
.collect();
assert_eq!(indexes, [4, 3, 2, 1, 0]);
}
}

View file

@ -1409,6 +1409,8 @@ impl App {
let merge_strategy = if force {
MergeStrategy::Replace
} else if no_sort && self.options.tac {
MergeStrategy::Prepend
} else if no_sort {
MergeStrategy::Append
} else {
@ -1422,6 +1424,7 @@ impl App {
self.item_list.processed_items.clone(),
merge_strategy,
no_sort,
self.options.tac,
self.needs_render.clone(),
);
// A new search is in flight; arm the `result`/`zero`/`one` events to

View file

@ -28,6 +28,8 @@ pub(crate) enum MergeStrategy {
SortedMerge,
/// Append to existing list without sorting (for --no-sort)
Append,
/// Prepend to existing list without sorting (for --tac --no-sort)
Prepend,
}
/// Processed items ready for rendering
@ -125,6 +127,27 @@ impl ItemList {
self.showing_stale_items = false;
}
/// Prepends a batch while preserving either the head-following behavior or
/// the item currently focused by a user who has moved away from the head.
fn prepend(&mut self, mut items: Vec<MatchedItem>) {
if items.is_empty() {
return;
}
let added = items.len();
let follows_head = self.current == 0;
items.append(&mut self.items);
self.items = items;
if follows_head {
self.offset = 0;
self.sub_offset = 0;
} else {
self.current = self.current.saturating_add(added);
self.offset = self.offset.saturating_add(added);
}
}
/// Toggles the selection state of the item at the given index
pub fn toggle_at(&mut self, index: usize) {
if self.items.is_empty() {
@ -485,8 +508,11 @@ impl SkimWidget for ItemList {
}
let initial_current = this.selected();
// Check for pre-processed items from background thread (non-blocking)
let items_updated = if let Some(processed) = this.processed_items.lock().take() {
// Check for pre-processed items from background thread (non-blocking).
// Bind the result separately so the lock guard is dropped before a merge
// mutates the item list.
let processed = this.processed_items.lock().take();
let items_updated = if let Some(processed) = processed {
debug!("Render: Got {} processed items", processed.items.len());
// Check if items are empty or blank for no_clear_if_empty handling
@ -513,6 +539,9 @@ impl SkimWidget for ItemList {
MergeStrategy::Append => {
this.items.extend(processed.items);
}
MergeStrategy::Prepend => {
this.prepend(processed.items);
}
}
this.showing_stale_items = false;

View file

@ -273,6 +273,43 @@ fn render_applies_sorted_merge_strategy() {
assert_eq!(il.items.len(), 2);
}
#[test]
fn render_prepends_tac_batch_and_follows_head() {
let mut il = ItemList::default();
let mut base = vec![matched("c", 2), matched("b", 1), matched("a", 0)];
il.append(&mut base);
set_processed(&il, vec![matched("e", 4), matched("d", 3)], MergeStrategy::Prepend);
render_list(&mut il, 20, 5);
let texts: Vec<_> = il.items.iter().map(|item| item.item.text().into_owned()).collect();
assert_eq!(texts, ["e", "d", "c", "b", "a"]);
assert_eq!(il.current, 0);
assert_eq!(
il.selected().as_ref().map(|item| item.item.text().into_owned()),
Some("e".into())
);
}
#[test]
fn render_prepend_preserves_focus_away_from_head() {
let mut il = ItemList::default();
let mut base = vec![matched("c", 2), matched("b", 1), matched("a", 0)];
il.append(&mut base);
il.current = 1;
il.offset = 1;
set_processed(&il, vec![matched("e", 4), matched("d", 3)], MergeStrategy::Prepend);
render_list(&mut il, 20, 5);
assert_eq!(il.current, 3);
assert_eq!(il.offset, 3);
assert_eq!(
il.selected().as_ref().map(|item| item.item.text().into_owned()),
Some("b".into())
);
}
#[test]
fn render_empty_list_does_not_panic() {
let mut il = ItemList::default();