From f8815982f40de6ae4c2a29991e3a05d6d3caad70 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Mon, 12 Feb 2024 20:11:18 +0100 Subject: [PATCH] More intuitive search behaviour --- src/tui/app/app.rs | 14 +++--- src/tui/app/selection.rs | 105 ++++++++------------------------------- 2 files changed, 30 insertions(+), 89 deletions(-) diff --git a/src/tui/app/app.rs b/src/tui/app/app.rs index aded90a..b9e58f7 100644 --- a/src/tui/app/app.rs +++ b/src/tui/app/app.rs @@ -7,8 +7,6 @@ use crate::tui::{ lib::IMusicHoard, }; -use super::selection::IncSearch; - pub enum AppState { Browse(BS), Info(IS), @@ -298,16 +296,20 @@ impl IAppInteractSearch for App { let collection = self.music_hoard.get_collection(); let search = self.state.as_mut().unwrap_search(); search.push(ch); - self.selection - .incremental_artist_search(IncSearch::Forward, collection, search); + self.selection.incremental_artist_search(collection, search); } + // Removing a character restarts the search from scratch. For now, the performance impact of + // this is not noticeable. If it does become noticeable, some form of memoization should be used + // as an optimisation. The most intuitive behaviour when removing a character is to return to + // the previous search position. However, it is difficult to construct a search predicate in + // selection.rs that will always correctly reverse to the previous position. fn remove_character(&mut self) { let collection = self.music_hoard.get_collection(); let search = self.state.as_mut().unwrap_search(); search.pop(); - self.selection - .incremental_artist_search(IncSearch::Reverse, collection, search); + self.selection.reset_artist(&collection); + self.selection.incremental_artist_search(collection, search); } fn finish_search(&mut self) { diff --git a/src/tui/app/selection.rs b/src/tui/app/selection.rs index b1afdb2..b95ac1b 100644 --- a/src/tui/app/selection.rs +++ b/src/tui/app/selection.rs @@ -61,11 +61,6 @@ impl Delta { } } -pub enum IncSearch { - Forward, - Reverse, -} - impl Selection { pub fn new(artists: &[Artist]) -> Self { Selection { @@ -100,14 +95,8 @@ impl Selection { }; } - pub fn incremental_artist_search( - &mut self, - direction: IncSearch, - collection: &Collection, - artist_name: &str, - ) { - self.artist - .incremental_search(direction, collection, artist_name); + pub fn incremental_artist_search(&mut self, collection: &Collection, artist_name: &str) { + self.artist.incremental_search(collection, artist_name); } pub fn increment_selection(&mut self, collection: &Collection, delta: Delta) { @@ -204,7 +193,7 @@ impl ArtistSelection { } // FIXME: use aho_corasick for normalization - fn normalize_search_string(search: &str, lowercase: bool, asciify: bool) -> String { + fn normalize_search(search: &str, lowercase: bool, asciify: bool) -> String { let normalized = if lowercase { search.to_lowercase() } else { @@ -243,92 +232,42 @@ impl ArtistSelection { artist_name.chars().any(|ch| special_chars.contains(&ch)) } - // FIXME: compare against both sort key and actual name fn incremental_search_predicate( case_sensitive: bool, char_sensitive: bool, search_name: &String, probe: &Artist, ) -> bool { - let probe_name = &probe.get_sort_key().name; - match Self::normalize_search_string(probe_name, !case_sensitive, !char_sensitive) - .cmp(search_name) - { - std::cmp::Ordering::Less => false, - std::cmp::Ordering::Equal | std::cmp::Ordering::Greater => true, + let name = Self::normalize_search(&probe.id.name, !case_sensitive, !char_sensitive); + let mut result = name.starts_with(search_name); + + if let Some(ref probe_sort) = probe.sort { + let name = Self::normalize_search(&probe_sort.name, !case_sensitive, !char_sensitive); + result = result || name.starts_with(search_name); } + + result } - fn incremental_search(&mut self, direction: IncSearch, artists: &[Artist], artist_name: &str) { + // FIXME: Use memoization - the only way to have correct behaviour + fn incremental_search(&mut self, artists: &[Artist], artist_name: &str) { if let Some(index) = self.state.list.selected() { let case_sensitive = Self::is_case_sensitive(artist_name); let char_sensitive = Self::is_char_sensitive(artist_name); - let search_name = - Self::normalize_search_string(artist_name, !case_sensitive, !char_sensitive); + let search = Self::normalize_search(artist_name, !case_sensitive, !char_sensitive); - match direction { - IncSearch::Forward => self.forward_incremental_search( - artists, - index, - case_sensitive, - char_sensitive, - &search_name, - ), - IncSearch::Reverse => self.reverse_incremental_search( - artists, - index, - case_sensitive, - char_sensitive, - &search_name, - ), + let slice = &artists[index..]; + + let result = slice.iter().position(|probe| { + Self::incremental_search_predicate(case_sensitive, char_sensitive, &search, probe) + }); + + if let Some(slice_index) = result { + self.select_to(artists, index + slice_index); } } } - fn forward_incremental_search( - &mut self, - artists: &[Artist], - index: usize, - case_sensitive: bool, - char_sensitive: bool, - search: &String, - ) { - let slice = &artists[index..]; - - let result = slice.iter().position(|probe| { - Self::incremental_search_predicate(case_sensitive, char_sensitive, &search, probe) - }); - - let new_index = match result { - Some(slice_index) => index + slice_index, - None => artists.len(), - }; - self.select_to(artists, new_index); - } - - fn reverse_incremental_search( - &mut self, - artists: &[Artist], - index: usize, - case_sensitive: bool, - char_sensitive: bool, - search: &String, - ) { - let slice = &artists[..(index + 1)]; - - // We search using opposite predicate in the reverse direction because what matters is the - // point at which the predicate flips value. - let result = slice.iter().rev().position(|probe| { - !Self::incremental_search_predicate(case_sensitive, char_sensitive, &search, probe) - }); - - let new_index = match result { - Some(slice_index) => index - slice_index + 1, - None => 0, - }; - self.select_to(artists, new_index); - } - fn increment_by(&mut self, artists: &[Artist], by: usize) { if let Some(index) = self.state.list.selected() { let result = index.saturating_add(by);