Clear search results on invalid query input (#7958)

Andrew Lygin created 2 years ago

Fixes a bug in the buffer search bar: Clears results of a previous
successfull search when the user enters invalid search request.

Steps to reproduce the bug:
1. Switch to Regex search mode.
2. Enter a valid search query that produces several matches.
3. Add a symbol to the search query that makes it an invalid regexp.
4. Switch to the editor and walk through the code back and forth.

Expected result: All the match highlightings after step 2 are cleared,
search bar indicates absence of the last search matches.

Actual: The results from the last valid search are highlighted, search
bar indicates presence of matches.

Potentially, the same effect may occur when searching in the simple text
mode, or when clearing the search query in some circumstances, so I made
the fix for all those cases, though I wasn't able to reproduce them
manually.

The bug:


https://github.com/zed-industries/zed/assets/2101250/1c50b98c-ae8e-4a9c-8ff5-1e5c63027ec3

After the fix:


https://github.com/zed-industries/zed/assets/2101250/e3eedf8c-2e3e-41ee-81cc-c2c9d919fba3

Release Notes:
- Clear search results on invalid query input

Change summary

crates/search/src/buffer_search.rs | 64 ++++++++++++++++++++++++++++---
1 file changed, 57 insertions(+), 7 deletions(-)

Detailed changes

crates/search/src/buffer_search.rs

@@ -182,11 +182,13 @@ impl Render for BufferSearchBar {
                 if self.query(cx).is_empty() {
                     return None;
                 }
-                let matches = self
+                let matches_count = self
                     .searchable_items_with_matches
-                    .get(&searchable_item.downgrade())?;
+                    .get(&searchable_item.downgrade())
+                    .map(Vec::len)
+                    .unwrap_or(0);
                 if let Some(match_ix) = self.active_match_index {
-                    Some(format!("{}/{}", match_ix + 1, matches.len()))
+                    Some(format!("{}/{}", match_ix + 1, matches_count))
                 } else {
                     match_color = Color::Error; // No matches found
                     None
@@ -843,6 +845,16 @@ impl BufferSearchBar {
     fn toggle_whole_word(&mut self, _: &ToggleWholeWord, cx: &mut ViewContext<Self>) {
         self.toggle_search_option(SearchOptions::WHOLE_WORD, cx)
     }
+
+    fn clear_active_searchable_item_matches(&mut self, cx: &mut WindowContext) {
+        if let Some(active_searchable_item) = self.active_searchable_item.as_ref() {
+            self.active_match_index = None;
+            self.searchable_items_with_matches
+                .remove(&active_searchable_item.downgrade());
+            active_searchable_item.clear_matches(cx);
+        }
+    }
+
     fn clear_matches(&mut self, cx: &mut ViewContext<Self>) {
         let mut active_item_matches = None;
         for (searchable_item, matches) in self.searchable_items_with_matches.drain() {
@@ -869,8 +881,7 @@ impl BufferSearchBar {
         if let Some(active_searchable_item) = self.active_searchable_item.as_ref() {
             self.query_contains_error = false;
             if query.is_empty() {
-                self.active_match_index.take();
-                active_searchable_item.clear_matches(cx);
+                self.clear_active_searchable_item_matches(cx);
                 let _ = done_tx.send(());
                 cx.notify();
             } else {
@@ -886,7 +897,7 @@ impl BufferSearchBar {
                         Ok(query) => query.with_replacement(self.replacement(cx)),
                         Err(_) => {
                             self.query_contains_error = true;
-                            self.active_match_index = None;
+                            self.clear_active_searchable_item_matches(cx);
                             cx.notify();
                             return done_rx;
                         }
@@ -903,7 +914,7 @@ impl BufferSearchBar {
                         Ok(query) => query.with_replacement(self.replacement(cx)),
                         Err(_) => {
                             self.query_contains_error = true;
-                            self.active_match_index = None;
+                            self.clear_active_searchable_item_matches(cx);
                             cx.notify();
                             return done_rx;
                         }
@@ -1910,4 +1921,43 @@ mod tests {
             .unindent()
         );
     }
+
+    #[gpui::test]
+    async fn test_invalid_regexp_search_after_valid(cx: &mut TestAppContext) {
+        let (editor, search_bar, cx) = init_test(cx);
+        let display_points_of = |background_highlights: Vec<(Range<DisplayPoint>, Hsla)>| {
+            background_highlights
+                .into_iter()
+                .map(|(range, _)| range)
+                .collect::<Vec<_>>()
+        };
+        // Search using valid regexp
+        search_bar
+            .update(cx, |search_bar, cx| {
+                search_bar.activate_search_mode(SearchMode::Regex, cx);
+                search_bar.search("expression", None, cx)
+            })
+            .await
+            .unwrap();
+        editor.update(cx, |editor, cx| {
+            assert_eq!(
+                display_points_of(editor.all_text_background_highlights(cx)),
+                &[
+                    DisplayPoint::new(0, 10)..DisplayPoint::new(0, 20),
+                    DisplayPoint::new(1, 9)..DisplayPoint::new(1, 19),
+                ],
+            );
+        });
+
+        // Now, the expression is invalid
+        search_bar
+            .update(cx, |search_bar, cx| {
+                search_bar.search("expression (", None, cx)
+            })
+            .await
+            .unwrap_err();
+        editor.update(cx, |editor, cx| {
+            assert!(display_points_of(editor.all_text_background_highlights(cx)).is_empty(),);
+        });
+    }
 }