Search woes (#6698)

Piotr Osiewicz and Kirill created

Fixes #6441 
Release Notes:
- Fixed "SelectNextMatch", "SelectPrevMatch" and "SelectAllMatches"
actions not working when search bar is not visible.
- Fixed "SelectPrevMatch" not being bound in project search.

---------

Co-authored-by: Kirill <kirill@zed.dev>

Change summary

crates/search/src/buffer_search.rs           | 226 ++-------------------
crates/search/src/buffer_search/registrar.rs | 172 ++++++++++++++++
crates/search/src/project_search.rs          |   8 
3 files changed, 209 insertions(+), 197 deletions(-)

Detailed changes

crates/search/src/buffer_search.rs 🔗

@@ -1,3 +1,5 @@
+mod registrar;
+
 use crate::{
     history::SearchHistory,
     mode::{next_mode, SearchMode},
@@ -29,6 +31,9 @@ use workspace::{
     ToolbarItemEvent, ToolbarItemLocation, ToolbarItemView, Workspace,
 };
 
+pub use registrar::DivRegistrar;
+use registrar::{ForDeployed, ForDismissed, SearchActionsRegistrar, WithResults};
+
 #[derive(PartialEq, Clone, Deserialize)]
 pub struct Deploy {
     pub focus: bool,
@@ -422,230 +427,59 @@ impl ToolbarItemView for BufferSearchBar {
     }
 }
 
-/// Registrar inverts the dependency between search and its downstream user, allowing said downstream user to register search action without knowing exactly what those actions are.
-pub trait SearchActionsRegistrar {
-    fn register_handler<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    );
-
-    fn register_handler_for_dismissed_search<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    );
-}
-
-type GetSearchBar<T> =
-    for<'a, 'b> fn(&'a T, &'a mut ViewContext<'b, T>) -> Option<View<BufferSearchBar>>;
-
-/// Registers search actions on a div that can be taken out.
-pub struct DivRegistrar<'a, 'b, T: 'static> {
-    div: Option<Div>,
-    cx: &'a mut ViewContext<'b, T>,
-    search_getter: GetSearchBar<T>,
-}
-
-impl<'a, 'b, T: 'static> DivRegistrar<'a, 'b, T> {
-    pub fn new(search_getter: GetSearchBar<T>, cx: &'a mut ViewContext<'b, T>) -> Self {
-        Self {
-            div: Some(div()),
-            cx,
-            search_getter,
-        }
-    }
-    pub fn into_div(self) -> Div {
-        // This option is always Some; it's an option in the first place because we want to call methods
-        // on div that require ownership.
-        self.div.unwrap()
-    }
-}
-
-impl<T: 'static> SearchActionsRegistrar for DivRegistrar<'_, '_, T> {
-    fn register_handler<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    ) {
-        let getter = self.search_getter;
-        self.div = self.div.take().map(|div| {
-            div.on_action(self.cx.listener(move |this, action, cx| {
-                let should_notify = (getter)(this, cx)
-                    .clone()
-                    .map(|search_bar| {
-                        search_bar.update(cx, |search_bar, cx| {
-                            if search_bar.is_dismissed()
-                                || search_bar.active_searchable_item.is_none()
-                            {
-                                false
-                            } else {
-                                callback(search_bar, action, cx);
-                                true
-                            }
-                        })
-                    })
-                    .unwrap_or(false);
-                if should_notify {
-                    cx.notify();
-                } else {
-                    cx.propagate();
-                }
-            }))
-        });
-    }
-
-    fn register_handler_for_dismissed_search<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    ) {
-        let getter = self.search_getter;
-        self.div = self.div.take().map(|div| {
-            div.on_action(self.cx.listener(move |this, action, cx| {
-                let should_notify = (getter)(this, cx)
-                    .clone()
-                    .map(|search_bar| {
-                        search_bar.update(cx, |search_bar, cx| {
-                            if search_bar.is_dismissed() {
-                                callback(search_bar, action, cx);
-                                true
-                            } else {
-                                false
-                            }
-                        })
-                    })
-                    .unwrap_or(false);
-                if should_notify {
-                    cx.notify();
-                } else {
-                    cx.propagate();
-                }
-            }))
-        });
-    }
-}
-
-/// Register actions for an active pane.
-impl SearchActionsRegistrar for Workspace {
-    fn register_handler<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    ) {
-        self.register_action(move |workspace, action: &A, cx| {
-            if workspace.has_active_modal(cx) {
-                cx.propagate();
-                return;
-            }
-
-            let pane = workspace.active_pane();
-            pane.update(cx, move |this, cx| {
-                this.toolbar().update(cx, move |this, cx| {
-                    if let Some(search_bar) = this.item_of_type::<BufferSearchBar>() {
-                        let should_notify = search_bar.update(cx, move |search_bar, cx| {
-                            if search_bar.is_dismissed()
-                                || search_bar.active_searchable_item.is_none()
-                            {
-                                false
-                            } else {
-                                callback(search_bar, action, cx);
-                                true
-                            }
-                        });
-                        if should_notify {
-                            cx.notify();
-                        } else {
-                            cx.propagate();
-                        }
-                    }
-                })
-            });
-        });
-    }
-
-    fn register_handler_for_dismissed_search<A: Action>(
-        &mut self,
-        callback: fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>),
-    ) {
-        self.register_action(move |workspace, action: &A, cx| {
-            if workspace.has_active_modal(cx) {
-                cx.propagate();
-                return;
-            }
-
-            let pane = workspace.active_pane();
-            pane.update(cx, move |this, cx| {
-                this.toolbar().update(cx, move |this, cx| {
-                    if let Some(search_bar) = this.item_of_type::<BufferSearchBar>() {
-                        let should_notify = search_bar.update(cx, move |search_bar, cx| {
-                            if search_bar.is_dismissed() {
-                                callback(search_bar, action, cx);
-                                true
-                            } else {
-                                false
-                            }
-                        });
-                        if should_notify {
-                            cx.notify();
-                        } else {
-                            cx.propagate();
-                        }
-                    }
-                })
-            });
-        });
-    }
-}
-
 impl BufferSearchBar {
     pub fn register(registrar: &mut impl SearchActionsRegistrar) {
-        registrar.register_handler(|this, action: &ToggleCaseSensitive, cx| {
+        registrar.register_handler(ForDeployed(|this, action: &ToggleCaseSensitive, cx| {
             if this.supported_options().case {
                 this.toggle_case_sensitive(action, cx);
             }
-        });
-        registrar.register_handler(|this, action: &ToggleWholeWord, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, action: &ToggleWholeWord, cx| {
             if this.supported_options().word {
                 this.toggle_whole_word(action, cx);
             }
-        });
-        registrar.register_handler(|this, action: &ToggleReplace, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, action: &ToggleReplace, cx| {
             if this.supported_options().replacement {
                 this.toggle_replace(action, cx);
             }
-        });
-        registrar.register_handler(|this, _: &ActivateRegexMode, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, _: &ActivateRegexMode, cx| {
             if this.supported_options().regex {
                 this.activate_search_mode(SearchMode::Regex, cx);
             }
-        });
-        registrar.register_handler(|this, _: &ActivateTextMode, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, _: &ActivateTextMode, cx| {
             this.activate_search_mode(SearchMode::Text, cx);
-        });
-        registrar.register_handler(|this, action: &CycleMode, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, action: &CycleMode, cx| {
             if this.supported_options().regex {
                 // If regex is not supported then search has just one mode (text) - in that case there's no point in supporting
                 // cycling.
                 this.cycle_mode(action, cx)
             }
-        });
-        registrar.register_handler(|this, action: &SelectNextMatch, cx| {
+        }));
+        registrar.register_handler(WithResults(|this, action: &SelectNextMatch, cx| {
             this.select_next_match(action, cx);
-        });
-        registrar.register_handler(|this, action: &SelectPrevMatch, cx| {
+        }));
+        registrar.register_handler(WithResults(|this, action: &SelectPrevMatch, cx| {
             this.select_prev_match(action, cx);
-        });
-        registrar.register_handler(|this, action: &SelectAllMatches, cx| {
+        }));
+        registrar.register_handler(WithResults(|this, action: &SelectAllMatches, cx| {
             this.select_all_matches(action, cx);
-        });
-        registrar.register_handler(|this, _: &editor::actions::Cancel, cx| {
+        }));
+        registrar.register_handler(ForDeployed(|this, _: &editor::actions::Cancel, cx| {
             this.dismiss(&Dismiss, cx);
-        });
+        }));
 
         // register deploy buffer search for both search bar states, since we want to focus into the search bar
         // when the deploy action is triggered in the buffer.
-        registrar.register_handler(|this, deploy, cx| {
+        registrar.register_handler(ForDeployed(|this, deploy, cx| {
             this.deploy(deploy, cx);
-        });
-        registrar.register_handler_for_dismissed_search(|this, deploy, cx| {
+        }));
+        registrar.register_handler(ForDismissed(|this, deploy, cx| {
             this.deploy(deploy, cx);
-        })
+        }))
     }
 
     pub fn new(cx: &mut ViewContext<Self>) -> Self {
@@ -930,7 +764,7 @@ impl BufferSearchBar {
         event: &editor::EditorEvent,
         cx: &mut ViewContext<Self>,
     ) {
-        if let editor::EditorEvent::Edited { .. } = event {
+        if let editor::EditorEvent::Edited = event {
             self.query_contains_error = false;
             self.clear_matches(cx);
             let search = self.update_matches(cx);

crates/search/src/buffer_search/registrar.rs 🔗

@@ -0,0 +1,172 @@
+use gpui::{div, Action, Div, InteractiveElement, View, ViewContext};
+use workspace::Workspace;
+
+use crate::BufferSearchBar;
+
+/// Registrar inverts the dependency between search and its downstream user, allowing said downstream user to register search action without knowing exactly what those actions are.
+pub trait SearchActionsRegistrar {
+    fn register_handler<A: Action>(&mut self, callback: impl ActionExecutor<A>);
+}
+
+type SearchBarActionCallback<A> = fn(&mut BufferSearchBar, &A, &mut ViewContext<BufferSearchBar>);
+
+type GetSearchBar<T> =
+    for<'a, 'b> fn(&'a T, &'a mut ViewContext<'b, T>) -> Option<View<BufferSearchBar>>;
+
+/// Registers search actions on a div that can be taken out.
+pub struct DivRegistrar<'a, 'b, T: 'static> {
+    div: Option<Div>,
+    cx: &'a mut ViewContext<'b, T>,
+    search_getter: GetSearchBar<T>,
+}
+
+impl<'a, 'b, T: 'static> DivRegistrar<'a, 'b, T> {
+    pub fn new(search_getter: GetSearchBar<T>, cx: &'a mut ViewContext<'b, T>) -> Self {
+        Self {
+            div: Some(div()),
+            cx,
+            search_getter,
+        }
+    }
+    pub fn into_div(self) -> Div {
+        // This option is always Some; it's an option in the first place because we want to call methods
+        // on div that require ownership.
+        self.div.unwrap()
+    }
+}
+
+impl<T: 'static> SearchActionsRegistrar for DivRegistrar<'_, '_, T> {
+    fn register_handler<A: Action>(&mut self, callback: impl ActionExecutor<A>) {
+        let getter = self.search_getter;
+        self.div = self.div.take().map(|div| {
+            div.on_action(self.cx.listener(move |this, action, cx| {
+                let should_notify = (getter)(this, cx)
+                    .clone()
+                    .map(|search_bar| {
+                        search_bar.update(cx, |search_bar, cx| {
+                            callback.execute(search_bar, action, cx)
+                        })
+                    })
+                    .unwrap_or(false);
+                if should_notify {
+                    cx.notify();
+                } else {
+                    cx.propagate();
+                }
+            }))
+        });
+    }
+}
+
+/// Register actions for an active pane.
+impl SearchActionsRegistrar for Workspace {
+    fn register_handler<A: Action>(&mut self, callback: impl ActionExecutor<A>) {
+        self.register_action(move |workspace, action: &A, cx| {
+            if workspace.has_active_modal(cx) {
+                cx.propagate();
+                return;
+            }
+
+            let pane = workspace.active_pane();
+            let callback = callback.clone();
+            pane.update(cx, |this, cx| {
+                this.toolbar().update(cx, move |this, cx| {
+                    if let Some(search_bar) = this.item_of_type::<BufferSearchBar>() {
+                        let should_notify = search_bar.update(cx, move |search_bar, cx| {
+                            callback.execute(search_bar, action, cx)
+                        });
+                        if should_notify {
+                            cx.notify();
+                        } else {
+                            cx.propagate();
+                        }
+                    }
+                })
+            });
+        });
+    }
+}
+
+type DidHandleAction = bool;
+/// Potentially executes the underlying action if some preconditions are met (e.g. buffer search bar is visible)
+pub trait ActionExecutor<A: Action>: 'static + Clone {
+    fn execute(
+        &self,
+        search_bar: &mut BufferSearchBar,
+        action: &A,
+        cx: &mut ViewContext<BufferSearchBar>,
+    ) -> DidHandleAction;
+}
+
+/// Run an action when the search bar has been dismissed from the panel.
+pub struct ForDismissed<A>(pub(super) SearchBarActionCallback<A>);
+impl<A> Clone for ForDismissed<A> {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+}
+
+impl<A: Action> ActionExecutor<A> for ForDismissed<A> {
+    fn execute(
+        &self,
+        search_bar: &mut BufferSearchBar,
+        action: &A,
+        cx: &mut ViewContext<BufferSearchBar>,
+    ) -> DidHandleAction {
+        if search_bar.is_dismissed() {
+            self.0(search_bar, action, cx);
+            true
+        } else {
+            false
+        }
+    }
+}
+
+/// Run an action when the search bar is deployed.
+pub struct ForDeployed<A>(pub(super) SearchBarActionCallback<A>);
+impl<A> Clone for ForDeployed<A> {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+}
+
+impl<A: Action> ActionExecutor<A> for ForDeployed<A> {
+    fn execute(
+        &self,
+        search_bar: &mut BufferSearchBar,
+        action: &A,
+        cx: &mut ViewContext<BufferSearchBar>,
+    ) -> DidHandleAction {
+        if search_bar.is_dismissed() || search_bar.active_searchable_item.is_none() {
+            false
+        } else {
+            self.0(search_bar, action, cx);
+            true
+        }
+    }
+}
+
+/// Run an action when the search bar has any matches, regardless of whether it
+/// is visible or not.
+pub struct WithResults<A>(pub(super) SearchBarActionCallback<A>);
+impl<A> Clone for WithResults<A> {
+    fn clone(&self) -> Self {
+        Self(self.0)
+    }
+}
+
+impl<A: Action> ActionExecutor<A> for WithResults<A> {
+    fn execute(
+        &self,
+        search_bar: &mut BufferSearchBar,
+        action: &A,
+        cx: &mut ViewContext<BufferSearchBar>,
+    ) -> DidHandleAction {
+        if search_bar.active_match_index.is_some() {
+            self.0(search_bar, action, cx);
+            true
+        } else {
+            false
+        }
+    }
+}

crates/search/src/project_search.rs 🔗

@@ -88,6 +88,12 @@ pub fn init(cx: &mut AppContext) {
         register_workspace_action(workspace, move |search_bar, action: &CycleMode, cx| {
             search_bar.cycle_mode(action, cx)
         });
+        register_workspace_action(
+            workspace,
+            move |search_bar, action: &SelectPrevMatch, cx| {
+                search_bar.select_prev_match(action, cx)
+            },
+        );
         register_workspace_action(
             workspace,
             move |search_bar, action: &SelectNextMatch, cx| {
@@ -1550,7 +1556,7 @@ impl ProjectSearchBar {
         }
     }
 
-    pub fn select_next_match(&mut self, _: &SelectNextMatch, cx: &mut ViewContext<Self>) {
+    fn select_next_match(&mut self, _: &SelectNextMatch, cx: &mut ViewContext<Self>) {
         if let Some(search) = self.active_project_search.as_ref() {
             search.update(cx, |this, cx| {
                 this.select_match(Direction::Next, cx);