Review changes

Author: abey79

Cargo.lock

@@ -6704,6 +6704,7 @@ dependencies = [
 name = "re_ui"
 version = "0.22.0-alpha.1+dev"
 dependencies = [
+ "ahash",
  "arrow",
  "eframe",
  "egui",
@@ -6713,7 +6714,6 @@ dependencies = [
  "egui_tiles",
  "getrandom",
  "itertools 0.13.0",
- "nohash-hasher",
  "once_cell",
  "parking_lot",
  "rand",

crates/viewer/re_blueprint_tree/src/data.rs

@@ -406,6 +406,7 @@ impl DataResultData {
             NotLeaf(&'a DataResultNode),
         }
 
+        /// Temporary structure to hold local information.
         struct NodeInfo<'a> {
             leaf_or_not: LeafOrNot<'a>,
             kind: DataResultKind,
@@ -523,27 +524,30 @@ impl DataResultData {
                 }
             };
 
-            let highlight_sections = hierarchy_highlights.remove(hierarchy.len().saturating_sub(1));
-
-            // never highlight the placeholder
-            let highlight_sections =
-                if node_info.kind == DataResultKind::OriginProjectionPlaceholder {
-                    SmallVec::new()
-                } else {
-                    highlight_sections
-                        .map(Iterator::collect)
-                        .unwrap_or_default()
-                };
-
-            is_this_a_match.then(|| Self {
-                kind: node_info.kind,
-                entity_path,
-                visible,
-                view_id: view_blueprint.id,
-                label,
-                highlight_sections,
-                default_open: node_info.default_open,
-                children,
+            is_this_a_match.then(|| {
+                let highlight_sections =
+                    hierarchy_highlights.remove(hierarchy.len().saturating_sub(1));
+
+                // never highlight the placeholder
+                let highlight_sections =
+                    if node_info.kind == DataResultKind::OriginProjectionPlaceholder {
+                        SmallVec::new()
+                    } else {
+                        highlight_sections
+                            .map(Iterator::collect)
+                            .unwrap_or_default()
+                    };
+
+                Self {
+                    kind: node_info.kind,
+                    entity_path,
+                    visible,
+                    view_id: view_blueprint.id,
+                    label,
+                    highlight_sections,
+                    default_open: node_info.default_open,
+                    children,
+                }
             })
         });
 

crates/viewer/re_time_panel/src/streams_tree_data.rs

@@ -128,6 +128,7 @@ impl EntityData {
         // Gather some info about the current node…
         //
 
+        /// Temporary structure to hold local information.
         struct NodeInfo {
             is_leaf: bool,
             is_this_a_match: bool,

crates/viewer/re_ui/Cargo.toml

@@ -41,6 +41,7 @@ re_log.workspace = true
 re_log_types.workspace = true  # syntax-highlighting for EntityPath
 re_tracing.workspace = true
 
+ahash.workspace = true
 arrow = { workspace = true, optional = true }
 eframe = { workspace = true, default-features = false, features = ["wgpu"] }
 egui_commonmark = { workspace = true, features = ["pulldown_cmark"] }
@@ -51,7 +52,6 @@ getrandom.workspace = true
 itertools.workspace = true
 once_cell.workspace = true
 parking_lot.workspace = true
-nohash-hasher.workspace = true
 serde = { workspace = true, features = ["derive"] }
 serde_json.workspace = true
 smallvec.workspace = true

crates/viewer/re_ui/src/filter_widget.rs

@@ -2,7 +2,6 @@ use std::ops::Range;
 
 use egui::{Color32, NumExt as _, Widget as _};
 use itertools::Itertools;
-use nohash_hasher::IntMap;
 use smallvec::SmallVec;
 
 use crate::{list_item, UiExt as _};
@@ -226,11 +225,11 @@ impl FilterMatcher {
             None => Some(PathRanges::default()),
             Some([]) => None,
             Some(keywords) => {
-                let hierarchy = path.into_iter().map(str::to_lowercase).collect_vec();
+                let path = path.into_iter().map(str::to_lowercase).collect_vec();
 
                 let all_ranges = keywords
                     .iter()
-                    .map(|keyword| keyword.match_path(hierarchy.iter().map(String::as_str)))
+                    .map(|keyword| keyword.match_path(path.iter().map(String::as_str)))
                     .collect_vec();
 
                 // all keywords must match!
@@ -256,13 +255,12 @@ impl FilterMatcher {
 /// `match_from_first_part_start` and/or `match_to_last_part_end`, which have the same behavior as
 /// regex's `^` and `$`.
 ///
-/// If the keyword has multiple parts, the tested path must have at least one instance of contiguous
+/// If the keyword has multiple parts, e.g. "first/second", the tested path must have at least one instance of contiguous
 /// parts which match the corresponding keyword parts. In that context, the keyword parts have the
 /// following behavior:
 /// - First keyword part: `^part$` if `match_from_first_part_start`, `part$` otherwise
 /// - Last keyword part: `^part$` if `match_to_last_part_end`, `^part` otherwise
 /// - Other keyword parts: `^part$`
-
 #[derive(Debug, Clone, PartialEq)]
 struct Keyword {
     /// The parts of a keyword.
@@ -279,6 +277,8 @@ impl Keyword {
     ///
     /// The string must not contain any whitespace!
     fn new(mut keyword: &str) -> Self {
+        // Invariant: keywords are not empty
+        debug_assert!(!keyword.is_empty());
         debug_assert!(!keyword.contains(char::is_whitespace));
 
         let match_from_first_part_start = if let Some(k) = keyword.strip_prefix('/') {
@@ -307,16 +307,25 @@ impl Keyword {
     /// Match the keyword against the provided path.
     ///
     /// An empty [`PathRanges`] means that the keyword didn't match the path.
-    fn match_path<'a>(&self, path: impl IntoIterator<Item = &'a str>) -> PathRanges {
+    ///
+    /// Implementation notes:
+    /// - This function is akin to a "sliding window" of the keyword parts against the path parts,
+    ///   trying to find some "alignment" yielding a match.
+    /// - We must be thorough as we want to find _all_ match highlights (i.e., we don't early out as
+    ///   soon as we find a match).
+    fn match_path<'a>(&self, lowercase_path: impl ExactSizeIterator<Item = &'a str>) -> PathRanges {
         let mut state_machines = vec![];
 
-        for (part_index, part) in path.into_iter().enumerate() {
-            let lowercase_part = part.to_lowercase();
+        let path_length = lowercase_path.len();
 
-            state_machines.push(MatchStateMachine::new(self));
+        for (path_part_index, path_part) in lowercase_path.into_iter().enumerate() {
+            // Only start a new state machine if it has a chance to be matched entirely.
+            if self.parts.len() <= (path_length - path_part_index) {
+                state_machines.push(MatchStateMachine::new(self));
+            }
 
             for state_machine in &mut state_machines {
-                state_machine.step(&lowercase_part, part_index);
+                state_machine.process_next_path_part(path_part, path_part_index);
             }
         }
 
@@ -342,7 +351,7 @@ impl Keyword {
 /// merged when read, which only happens with [`Self::remove`].
 #[derive(Debug, Default)]
 pub struct PathRanges {
-    ranges: IntMap<usize, Vec<Range<usize>>>,
+    ranges: ahash::HashMap<usize, Vec<Range<usize>>>,
 }
 
 impl PathRanges {
@@ -435,7 +444,7 @@ impl<'a> MatchStateMachine<'a> {
         matches!(self.state, MatchState::Match)
     }
 
-    fn step(&mut self, part: &str, part_index: usize) {
+    fn process_next_path_part(&mut self, part: &str, part_index: usize) {
         if matches!(self.state, MatchState::Match | MatchState::NoMatch) {
             return;
         }
@@ -694,17 +703,17 @@ mod test {
 
     #[test]
     fn test_keyword_match_path() {
-        fn match_and_normalize(query: &str, path: &[&str]) -> Vec<Vec<Range<usize>>> {
-            let keyword = Keyword::new(query);
-            let path = path.to_vec();
-            keyword.match_path(path.clone()).into_vec(path.len())
+        fn match_and_normalize(query: &str, lowercase_path: &[&str]) -> Vec<Vec<Range<usize>>> {
+            Keyword::new(query)
+                .match_path(lowercase_path.iter().copied())
+                .into_vec(lowercase_path.len())
         }
 
         assert_eq!(match_and_normalize("a", &["a"]), vec![vec![0..1]]);
         assert_eq!(match_and_normalize("a", &["aaa"]), vec![vec![0..3]]);
 
         assert_eq!(
-            match_and_normalize("A/", &["aaa", "aaa"]),
+            match_and_normalize("a/", &["aaa", "aaa"]),
             vec![vec![2..3], vec![2..3]]
         );
 
@@ -737,31 +746,29 @@ mod test {
         );
 
         assert!(
-            match_and_normalize("a/B/c/", &["aaa", "b", "cccc"])
+            match_and_normalize("a/b/c/", &["aaa", "b", "cccc"])
                 .into_iter()
                 .flatten()
                 .count()
                 == 0,
         );
 
         assert_eq!(
-            match_and_normalize("ab/cd", &["xxxAb", "cDaB", "Cdxxx"]),
+            match_and_normalize("ab/cd", &["xxxab", "cdab", "cdxxx"]),
             vec![vec![3..5], vec![0..4], vec![0..2]]
         );
 
         assert_eq!(
-            match_and_normalize("ab/ab", &["xxxAb", "aB", "aBxxx"]),
+            match_and_normalize("ab/ab", &["xxxab", "ab", "abxxx"]),
             vec![vec![3..5], vec![0..2], vec![0..2]]
         );
     }
 
     #[test]
     fn test_match_path() {
         fn match_and_normalize(query: &str, path: &[&str]) -> Option<Vec<Vec<Range<usize>>>> {
-            let matcher = FilterMatcher::new(Some(query));
-            let path = path.to_vec();
-            matcher
-                .match_path(path.clone())
+            FilterMatcher::new(Some(query))
+                .match_path(path.iter().copied())
                 .map(|ranges| ranges.into_vec(path.len()))
         }