More intuitive search behaviour

src/tui/app/app.rs

@@ -7,8 +7,6 @@ use crate::tui::{
     lib::IMusicHoard,
 };
 
-use super::selection::IncSearch;
-
 pub enum AppState<BS, IS, RS, SS, ES, CS> {
     Browse(BS),
     Info(IS),
@@ -298,16 +296,20 @@ impl<MH: IMusicHoard> IAppInteractSearch for App<MH> {
         let collection = self.music_hoard.get_collection();
         let search = self.state.as_mut().unwrap_search();
         search.push(ch);
-        self.selection
+        self.selection.incremental_artist_search(collection, search);
-            .incremental_artist_search(IncSearch::Forward, 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
+        self.selection.reset_artist(&collection);
-            .incremental_artist_search(IncSearch::Reverse, collection, search);
+        self.selection.incremental_artist_search(collection, search);
     }
 
     fn finish_search(&mut self) {

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(
+    pub fn incremental_artist_search(&mut self, collection: &Collection, artist_name: &str) {
-        &mut self,
+        self.artist.incremental_search(collection, artist_name);
-        direction: IncSearch,
-        collection: &Collection,
-        artist_name: &str,
-    ) {
-        self.artist
-            .incremental_search(direction, 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,90 +232,40 @@ 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;
+        let name = Self::normalize_search(&probe.id.name, !case_sensitive, !char_sensitive);
-        match Self::normalize_search_string(probe_name, !case_sensitive, !char_sensitive)
+        let mut result = name.starts_with(search_name);
-            .cmp(search_name)
+
-        {
+        if let Some(ref probe_sort) = probe.sort {
-            std::cmp::Ordering::Less => false,
+            let name = Self::normalize_search(&probe_sort.name, !case_sensitive, !char_sensitive);
-            std::cmp::Ordering::Equal | std::cmp::Ordering::Greater => true,
+            result = result || name.starts_with(search_name);
-        }
         }
 
-    fn incremental_search(&mut self, direction: IncSearch, artists: &[Artist], artist_name: &str) {
+        result
+    }
+
+    // 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 =
+            let search = Self::normalize_search(artist_name, !case_sensitive, !char_sensitive);
-                Self::normalize_search_string(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,
-                ),
-            }
-        }
-    }
-
-    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 {
+            if let Some(slice_index) = result {
-            Some(slice_index) => index + slice_index,
+                self.select_to(artists, 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) {