From 6d9b8cc5950cec7bede4e287ec0ebdf3aeeb4e5d Mon Sep 17 00:00:00 2001 From: Andrew Lygin Date: Sun, 18 Feb 2024 09:01:06 +0300 Subject: [PATCH] Clear search results on invalid query input (#7958) 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 --- crates/search/src/buffer_search.rs | 64 ++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/crates/search/src/buffer_search.rs b/crates/search/src/buffer_search.rs index 4e11c208895313b7dffbbf8d19b8ccfafff6f793..e86de37322be4572666f5d4a1132ab64898400d5 100644 --- a/crates/search/src/buffer_search.rs +++ b/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.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) { 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, Hsla)>| { + background_highlights + .into_iter() + .map(|(range, _)| range) + .collect::>() + }; + // 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(),); + }); + } }