fix: TUI selection underflow and filtered-results desync
Three related TUI correctness bugs: - download_selector navigation (select_previous/next/last) computed items.len() - 1, panicking with an arithmetic underflow when the list was empty (e.g. a filter matched nothing). Index math is now in wrap-safe helpers that return no selection for an empty list. - update_search_results overwrote the filtered results view with the full unfiltered list on every ~100ms poll, so an active filter silently stopped working as new results streamed in. It now re-derives the filtered view from the current query. - The results pane rendered selection checkboxes by testing the display-row index against a selection set that stores unfiltered indices, so under an active filter the wrong rows showed as checked. It now maps the display index back through the filtered-index table. Filtering and index-mapping logic extracted into pure, unit-tested helpers.
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
#[derive(Clone)]
|
||||
#[derive(Clone, Default)]
|
||||
pub struct FileDisplayData {
|
||||
pub filename: String,
|
||||
pub size: u64,
|
||||
|
||||
@@ -254,28 +254,44 @@ impl FileSelector {
|
||||
}
|
||||
}
|
||||
|
||||
fn select_previous(&mut self) {
|
||||
let i = match self.state.selected() {
|
||||
/// Previous index with wrap-around, or None when the list is empty.
|
||||
fn wrapping_prev(selected: Option<usize>, len: usize) -> Option<usize> {
|
||||
if len == 0 {
|
||||
return None;
|
||||
}
|
||||
Some(match selected {
|
||||
Some(i) if i > 0 => i - 1,
|
||||
_ => self.items.len() - 1,
|
||||
};
|
||||
self.state.select(Some(i));
|
||||
_ => len - 1,
|
||||
})
|
||||
}
|
||||
|
||||
/// Next index with wrap-around, or None when the list is empty.
|
||||
fn wrapping_next(selected: Option<usize>, len: usize) -> Option<usize> {
|
||||
if len == 0 {
|
||||
return None;
|
||||
}
|
||||
Some(match selected {
|
||||
Some(i) if i < len - 1 => i + 1,
|
||||
_ => 0,
|
||||
})
|
||||
}
|
||||
|
||||
fn select_previous(&mut self) {
|
||||
let i = Self::wrapping_prev(self.state.selected(), self.items.len());
|
||||
self.state.select(i);
|
||||
}
|
||||
|
||||
fn select_next(&mut self) {
|
||||
let i = match self.state.selected() {
|
||||
Some(i) if i < self.items.len() - 1 => i + 1,
|
||||
_ => 0,
|
||||
};
|
||||
self.state.select(Some(i));
|
||||
let i = Self::wrapping_next(self.state.selected(), self.items.len());
|
||||
self.state.select(i);
|
||||
}
|
||||
|
||||
fn select_first(&mut self) {
|
||||
self.state.select(Some(0));
|
||||
self.state.select((!self.items.is_empty()).then_some(0));
|
||||
}
|
||||
|
||||
fn select_last(&mut self) {
|
||||
self.state.select(Some(self.items.len() - 1));
|
||||
self.state.select(self.items.len().checked_sub(1));
|
||||
}
|
||||
|
||||
fn apply_filter(&mut self) {
|
||||
@@ -585,3 +601,30 @@ impl FileSelector {
|
||||
frame.render_widget(controls_widget, area);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::FileSelector;
|
||||
|
||||
// Navigating an empty list (e.g. a filter that matched nothing) must not
|
||||
// underflow `len - 1`; it should yield no selection.
|
||||
#[test]
|
||||
fn wrapping_prev_on_empty_list_is_none() {
|
||||
assert_eq!(FileSelector::wrapping_prev(None, 0), None);
|
||||
assert_eq!(FileSelector::wrapping_prev(Some(0), 0), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wrapping_next_on_empty_list_is_none() {
|
||||
assert_eq!(FileSelector::wrapping_next(None, 0), None);
|
||||
assert_eq!(FileSelector::wrapping_next(Some(0), 0), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wrapping_navigation_wraps_around() {
|
||||
assert_eq!(FileSelector::wrapping_prev(Some(0), 3), Some(2));
|
||||
assert_eq!(FileSelector::wrapping_prev(Some(2), 3), Some(1));
|
||||
assert_eq!(FileSelector::wrapping_next(Some(2), 3), Some(0));
|
||||
assert_eq!(FileSelector::wrapping_next(Some(1), 3), Some(2));
|
||||
}
|
||||
}
|
||||
|
||||
+104
-31
@@ -162,12 +162,18 @@ impl MainTui {
|
||||
self.state.results_pane_area = Some(right_chunks[0]);
|
||||
self.state.downloads_pane_area = Some(downloads_chunks[0]);
|
||||
|
||||
// Render Results pane
|
||||
let results_items = if self.state.results_filter_query.is_empty() {
|
||||
&self.state.results_items
|
||||
} else {
|
||||
&self.state.results_filtered_items
|
||||
};
|
||||
// Render Results pane. When a filter is active the rendered rows are a
|
||||
// subset, so the pane also needs the mapping back to unfiltered indices
|
||||
// to render the selection checkboxes correctly.
|
||||
let (results_items, results_original_indices) =
|
||||
if self.state.results_filter_query.is_empty() {
|
||||
(&self.state.results_items, None)
|
||||
} else {
|
||||
(
|
||||
&self.state.results_filtered_items,
|
||||
Some(self.state.results_filtered_indices.as_slice()),
|
||||
)
|
||||
};
|
||||
|
||||
let active_search_query = self
|
||||
.state
|
||||
@@ -182,6 +188,7 @@ impl MainTui {
|
||||
items: results_items,
|
||||
table_state: &mut self.state.results_table_state,
|
||||
selected_indices: &self.state.results_selected_indices,
|
||||
original_indices: results_original_indices,
|
||||
filter_query: &self.state.results_filter_query,
|
||||
is_filtering: self.state.results_is_filtering,
|
||||
focused: self.state.focused_pane == FocusedPane::Results,
|
||||
@@ -815,27 +822,17 @@ impl MainTui {
|
||||
self.state.results_is_filtering = false;
|
||||
}
|
||||
|
||||
fn recompute_results_filter(&mut self) {
|
||||
let (items, indices) = filter_results(
|
||||
&self.state.results_items,
|
||||
&self.state.results_filter_query,
|
||||
);
|
||||
self.state.results_filtered_items = items;
|
||||
self.state.results_filtered_indices = indices;
|
||||
}
|
||||
|
||||
fn apply_filter(&mut self) {
|
||||
let query = self.state.results_filter_query.to_lowercase();
|
||||
if query.is_empty() {
|
||||
self.state.results_filtered_items =
|
||||
self.state.results_items.clone();
|
||||
self.state.results_filtered_indices =
|
||||
(0..self.state.results_items.len()).collect();
|
||||
} else {
|
||||
self.state.results_filtered_items.clear();
|
||||
self.state.results_filtered_indices.clear();
|
||||
|
||||
for (idx, item) in self.state.results_items.iter().enumerate() {
|
||||
if item.filename.to_lowercase().contains(&query)
|
||||
|| item.username.to_lowercase().contains(&query)
|
||||
{
|
||||
self.state.results_filtered_items.push(item.clone());
|
||||
self.state.results_filtered_indices.push(idx);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
self.recompute_results_filter();
|
||||
if !self.state.results_filtered_items.is_empty() {
|
||||
self.state.results_table_state.select(Some(0));
|
||||
}
|
||||
@@ -931,15 +928,20 @@ impl MainTui {
|
||||
}
|
||||
}
|
||||
|
||||
// Update selected search if this is the active one
|
||||
// Update selected search if this is the active one. Re-derive
|
||||
// the filtered view from the current query so an active
|
||||
// filter is preserved as new results stream in, rather than
|
||||
// being clobbered by the full unfiltered list.
|
||||
if let Some(selected_idx) = selected_search_index
|
||||
&& selected_idx == idx
|
||||
{
|
||||
self.state.results_items = search.results.clone();
|
||||
self.state.results_filtered_items =
|
||||
search.results.clone();
|
||||
self.state.results_filtered_indices =
|
||||
(0..search.results.len()).collect();
|
||||
let (items, indices) = filter_results(
|
||||
&self.state.results_items,
|
||||
&self.state.results_filter_query,
|
||||
);
|
||||
self.state.results_filtered_items = items;
|
||||
self.state.results_filtered_indices = indices;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1117,3 +1119,74 @@ fn visible_input_at_cursor(
|
||||
|
||||
(visible_input, cursor_column as u16)
|
||||
}
|
||||
|
||||
/// Filter `items` by a case-insensitive substring match on filename or
|
||||
/// username. Returns the matching items alongside their indices in the original
|
||||
/// list, so callers can translate a filtered display index back to the
|
||||
/// unfiltered results. An empty query returns everything (identity mapping).
|
||||
fn filter_results(
|
||||
items: &[FileDisplayData],
|
||||
query: &str,
|
||||
) -> (Vec<FileDisplayData>, Vec<usize>) {
|
||||
let query = query.to_lowercase();
|
||||
if query.is_empty() {
|
||||
return (items.to_vec(), (0..items.len()).collect());
|
||||
}
|
||||
|
||||
let mut filtered_items = Vec::new();
|
||||
let mut filtered_indices = Vec::new();
|
||||
for (idx, item) in items.iter().enumerate() {
|
||||
if item.filename.to_lowercase().contains(&query)
|
||||
|| item.username.to_lowercase().contains(&query)
|
||||
{
|
||||
filtered_items.push(item.clone());
|
||||
filtered_indices.push(idx);
|
||||
}
|
||||
}
|
||||
(filtered_items, filtered_indices)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::filter_results;
|
||||
use crate::models::FileDisplayData;
|
||||
|
||||
fn file(filename: &str, username: &str) -> FileDisplayData {
|
||||
FileDisplayData {
|
||||
filename: filename.to_string(),
|
||||
username: username.to_string(),
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_query_returns_identity_mapping() {
|
||||
let items = vec![file("a.mp3", "bob"), file("b.flac", "amy")];
|
||||
let (filtered, indices) = filter_results(&items, "");
|
||||
assert_eq!(filtered.len(), 2);
|
||||
assert_eq!(indices, vec![0, 1]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn query_matches_filename_and_username_and_maps_indices() {
|
||||
let items = vec![
|
||||
file("track.mp3", "bob"),
|
||||
file("song.flac", "alice"),
|
||||
file("alice_demo.mp3", "carol"),
|
||||
];
|
||||
// "alice" matches item 1 (username) and item 2 (filename).
|
||||
let (filtered, indices) = filter_results(&items, "alice");
|
||||
assert_eq!(filtered.len(), 2);
|
||||
assert_eq!(indices, vec![1, 2]);
|
||||
assert_eq!(filtered[0].filename, "song.flac");
|
||||
assert_eq!(filtered[1].filename, "alice_demo.mp3");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn query_is_case_insensitive() {
|
||||
let items = vec![file("The Weeknd.mp3", "dj")];
|
||||
let (filtered, indices) = filter_results(&items, "WEEKND");
|
||||
assert_eq!(filtered.len(), 1);
|
||||
assert_eq!(indices, vec![0]);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,12 +19,33 @@ pub struct ResultsPaneParams<'a> {
|
||||
pub items: &'a [FileDisplayData],
|
||||
pub table_state: &'a mut TableState,
|
||||
pub selected_indices: &'a HashSet<usize>,
|
||||
/// Maps a rendered row index to its index in the unfiltered results list.
|
||||
/// `None` means the rendered rows are the unfiltered list (identity map).
|
||||
pub original_indices: Option<&'a [usize]>,
|
||||
pub filter_query: &'a str,
|
||||
pub is_filtering: bool,
|
||||
pub focused: bool,
|
||||
pub active_search_query: Option<&'a str>,
|
||||
}
|
||||
|
||||
/// Whether the rendered row `display_idx` is selected. `selected_indices` holds
|
||||
/// indices into the *unfiltered* results, so under an active filter the display
|
||||
/// index must be translated through `original_indices` first.
|
||||
fn row_is_selected(
|
||||
display_idx: usize,
|
||||
original_indices: Option<&[usize]>,
|
||||
selected_indices: &HashSet<usize>,
|
||||
) -> bool {
|
||||
let original = match original_indices {
|
||||
Some(map) => match map.get(display_idx) {
|
||||
Some(&o) => o,
|
||||
None => return false,
|
||||
},
|
||||
None => display_idx,
|
||||
};
|
||||
selected_indices.contains(&original)
|
||||
}
|
||||
|
||||
pub fn render_results_pane(
|
||||
frame: &mut Frame,
|
||||
area: Rect,
|
||||
@@ -34,6 +55,7 @@ pub fn render_results_pane(
|
||||
items,
|
||||
table_state,
|
||||
selected_indices,
|
||||
original_indices,
|
||||
filter_query,
|
||||
is_filtering,
|
||||
focused,
|
||||
@@ -83,11 +105,12 @@ No results. Select a search from the Searches pane [1]. Or start new search [s
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(idx, file)| {
|
||||
let checkbox = if selected_indices.contains(&idx) {
|
||||
"[✓]"
|
||||
} else {
|
||||
"[ ]"
|
||||
};
|
||||
let checkbox =
|
||||
if row_is_selected(idx, original_indices, selected_indices) {
|
||||
"[✓]"
|
||||
} else {
|
||||
"[ ]"
|
||||
};
|
||||
|
||||
let bitrate_str = file
|
||||
.bitrate
|
||||
@@ -147,3 +170,34 @@ No results. Select a search from the Searches pane [1]. Or start new search [s
|
||||
|
||||
frame.render_stateful_widget(table, area, table_state);
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::row_is_selected;
|
||||
use std::collections::HashSet;
|
||||
|
||||
#[test]
|
||||
fn identity_mapping_used_when_no_filter() {
|
||||
let selected: HashSet<usize> = [1].into_iter().collect();
|
||||
assert!(row_is_selected(1, None, &selected));
|
||||
assert!(!row_is_selected(0, None, &selected));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filtered_rows_resolve_through_original_indices() {
|
||||
// Filter shows original rows 2, 5, 7 as display rows 0, 1, 2.
|
||||
// The user selected original row 5.
|
||||
let original_indices = [2usize, 5, 7];
|
||||
let selected: HashSet<usize> = [5].into_iter().collect();
|
||||
assert!(!row_is_selected(0, Some(&original_indices), &selected));
|
||||
assert!(row_is_selected(1, Some(&original_indices), &selected));
|
||||
assert!(!row_is_selected(2, Some(&original_indices), &selected));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn out_of_range_display_index_is_not_selected() {
|
||||
let original_indices = [2usize, 5];
|
||||
let selected: HashSet<usize> = [2, 5].into_iter().collect();
|
||||
assert!(!row_is_selected(9, Some(&original_indices), &selected));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user