Enforce the rule: heuristics should never add a new view that would be completely covered by an existing view

crates/re_log_types/src/path/entity_path_filter.rs

@@ -259,6 +259,69 @@ impl EntityPathFilter {
 
         false // no matching rule and we are exclude-by-default.
     }
+
+    /// Checks whether this results of this filter "fully contain" the results of another filter.
+    ///
+    /// If this returns `true` there should not exist any [`EntityPath`] for which [`Self::is_included`]
+    /// would return `true` and the other filter would return `false` for this filter.
+    ///
+    /// This is a conservative estimate, and may return `false` in situations where the
+    /// query does in fact cover the other query. However, it should never return `true`
+    /// in a case where the other query would not be fully covered.
+    pub fn fully_contains(&self, other: &Self) -> bool {
+        // First check that we include everything included by other
+        for (other_rule, other_effect) in &other.rules {
+            match other_effect {
+                RuleEffect::Include => {
+                    if let Some((self_rule, self_effect)) = self
+                        .rules
+                        .iter()
+                        .rev()
+                        .find(|(r, _)| r.matches(&other_rule.path))
+                    {
+                        match self_effect {
+                            RuleEffect::Include => {
+                                // If the other rule includes the subtree, but the matching
+                                // rule doesn't, then we don't fully contain the other rule.
+                                if other_rule.include_subtree && !self_rule.include_subtree {
+                                    return false;
+                                }
+                            }
+                            RuleEffect::Exclude => return false,
+                        }
+                    } else {
+                        // No matching rule means this path isn't included
+                        return false;
+                    }
+                }
+                RuleEffect::Exclude => {}
+            }
+        }
+        // Next check that the other rule hasn't included something that we've excluded
+        for (self_rule, self_effect) in &self.rules {
+            match self_effect {
+                RuleEffect::Include => {}
+                RuleEffect::Exclude => {
+                    if let Some((_, other_effect)) = other
+                        .rules
+                        .iter()
+                        .rev()
+                        .find(|(r, _)| r.matches(&self_rule.path))
+                    {
+                        match other_effect {
+                            RuleEffect::Include => {
+                                return false;
+                            }
+                            RuleEffect::Exclude => {}
+                        }
+                    }
+                }
+            }
+        }
+        // If we got here, we checked every inclusion rule in `other` and they all had a more-inclusive
+        // inclusion rule and didn't hit an exclusion rule.
+        true
+    }
 }
 
 impl EntityPathRule {
@@ -432,3 +495,88 @@ fn test_entity_path_filter_subtree() {
         );
     }
 }
+
+#[test]
+fn test_fully_contains() {
+    struct TestCase {
+        filter: &'static str,
+        contains: Vec<&'static str>,
+        not_contains: Vec<&'static str>,
+    }
+
+    let cases = [
+        TestCase {
+            filter: "+ /**",
+            contains: [
+                "",
+                "+ /**",
+                "+ /a/**",
+                r#"+ /a/**
+                   + /b/c
+                   + /b/d
+                   "#,
+            ]
+            .into(),
+            not_contains: [].into(),
+        },
+        TestCase {
+            filter: "+ /a/**",
+            contains: ["+ /a/**", "+ /a", "+ /a/b/**"].into(),
+            not_contains: [
+                "+ /**",
+                "+ /b/**",
+                "+ /b",
+                r#"+ /a/b
+                   + /b
+                   "#,
+            ]
+            .into(),
+        },
+        TestCase {
+            filter: r#"
+                + /a
+                + /b/c
+                "#,
+            contains: ["+ /a", "+ /b/c"].into(),
+            not_contains: ["+ /a/**", "+ /b/**", "+ /b/c/d"].into(),
+        },
+        TestCase {
+            filter: r#"
+                + /**
+                - /b/c
+                "#,
+            contains: ["+ /a", "+ /a/**", "+ /b", "+ /b/c/d"].into(),
+            not_contains: ["+ /b/**", "+ /b/c"].into(),
+        },
+        TestCase {
+            filter: r#"
+                + /**
+                - /b/c/**
+                "#,
+            contains: ["+ /a", "+ /a/**", "+ /b"].into(),
+            not_contains: ["+ /b/**", "+ /b/c", "+ /b/c/d"].into(),
+        },
+    ];
+
+    for case in &cases {
+        let filter = EntityPathFilter::parse_forgiving(case.filter);
+        for contains in &case.contains {
+            let contains_filter = EntityPathFilter::parse_forgiving(contains);
+            assert!(
+                filter.fully_contains(&contains_filter),
+                "Expected {:?} to fully contain {:?}, but it didn't",
+                filter.formatted(),
+                contains_filter.formatted(),
+            );
+        }
+        for not_contains in &case.not_contains {
+            let not_contains_filter = EntityPathFilter::parse_forgiving(not_contains);
+            assert!(
+                !filter.fully_contains(&not_contains_filter),
+                "Expected {:?} to NOT fully contain {:?}, but it did",
+                filter.formatted(),
+                not_contains_filter.formatted(),
+            );
+        }
+    }
+}

crates/re_space_view/src/data_query_blueprint.rs

@@ -48,6 +48,25 @@ impl DataQueryBlueprint {
             .eq(&other.space_view_class_identifier)
             && self.entity_path_filter.eq(&other.entity_path_filter)
     }
+
+    /// Checks whether the results of this query "fully contains" the results of another query.
+    ///
+    /// If this returns `true` then the [`DataQueryResult`] returned by this query should always
+    /// contain any [`EntityPath`] that would be included in the results of the other query.
+    ///
+    /// This is a conservative estimate, and may return `false` in situations where the
+    /// query does in fact cover the other query. However, it should never return `true`
+    /// in a case where the other query would not be fully covered.
+    pub fn fully_contains(&self, other: &DataQueryBlueprint) -> bool {
+        // A query can't fully contain another if their space-view classes don't match
+        if self.space_view_class_identifier != other.space_view_class_identifier {
+            return false;
+        }
+
+        // Anything included by the other query is also included by this query
+        self.entity_path_filter
+            .fully_contains(&other.entity_path_filter)
+    }
 }
 
 impl DataQueryBlueprint {

crates/re_space_view/src/space_view.rs

@@ -515,6 +515,23 @@ impl SpaceViewBlueprint {
             .map(|q| q.entity_path_filter.clone())
             .sum()
     }
+
+    pub fn contains_all_entities_from(&self, other: &Self) -> bool {
+        // TODO(jleibs): Handle multi-query-aggregation
+
+        // If other has no query, by definition we contain all entities from it.
+        let Some(q_other) = other.queries.first() else {
+            return true;
+        };
+
+        // If other has any query, but self has no query, we clearly can't contain it
+        let Some(q_self) = self.queries.first() else {
+            return false;
+        };
+
+        // If this query fully contains the other, then we have all its entities
+        q_self.fully_contains(q_other)
+    }
 }
 
 #[cfg(test)]

crates/re_viewport/src/viewport.rs

@@ -350,18 +350,13 @@ impl<'a, 'b> Viewport<'a, 'b> {
             .values()
             .chain(already_added.iter())
         {
-            if existing_view.space_origin == space_view_candidate.space_origin {
+            if existing_view.class_identifier() == space_view_candidate.class_identifier() {
                 if existing_view.entities_determined_by_user {
                     // Since the user edited a space view with the same space path, we can't be sure our new one isn't redundant.
                     // So let's skip that.
                     return false;
                 }
-                if existing_view
-                    .queries
-                    .iter()
-                    .zip(space_view_candidate.queries.iter())
-                    .all(|(q1, q2)| q1.is_equivalent(q2))
-                {
+                if existing_view.contains_all_entities_from(space_view_candidate) {
                     // This space view wouldn't add anything we haven't already
                     return false;
                 }