From 1c2081c10cf95138015018a76c4795c76176796e Mon Sep 17 00:00:00 2001 From: Andrew Lygin Date: Mon, 12 Feb 2024 21:01:57 +0300 Subject: [PATCH] Search bar UI enhancements (#7675) This PR introduces several enhancements (along with fixing a couple of bugs) in the search bar UI (copied from [this comment](https://github.com/zed-industries/zed/issues/7663#issuecomment-1937659091)): - Moving the Replace field under the Search field makes it easier to compare texts and avoid typos. Also, less eyes movements. - Use red (error) color to indicate that nothing matched. VSCode, IDEA do this. Again, it helps to get a quicker feedback on typos without moving your eyes. - Much less moving parts and no place for flickering. - Better fits to narrow panes. - The Close button that allows to close the search bar with the mouse. - Better keyboard handling (tab, shift+tab in the replacement mode), autofocus on the Replace field. How it looks: https://github.com/zed-industries/zed/assets/2101250/93b0edb4-4311-4a7f-9f43-b30c4d1aede5 Implementation details: - Using `Self::on_query_editor_event` looked suspicious [here](https://github.com/zed-industries/zed/blob/288013503755f0d44feff6a517169a2861b43f26/crates/search/src/buffer_search.rs#L491) because it triggered searching on the replacement text changes. I've created a separate method for the replacement editor. - These changes don't affect the project search bar. If the PR is accepted, the same changes may be implemented there. Fixed issues: - #7661 - #7663 Release Notes: - Buffer search bar UI enhancements. --- crates/search/src/buffer_search.rs | 286 ++++++++++++++++++----------- 1 file changed, 177 insertions(+), 109 deletions(-) diff --git a/crates/search/src/buffer_search.rs b/crates/search/src/buffer_search.rs index e09080e6b2..897c1186a2 100644 --- a/crates/search/src/buffer_search.rs +++ b/crates/search/src/buffer_search.rs @@ -9,13 +9,16 @@ use crate::{ ToggleCaseSensitive, ToggleReplace, ToggleWholeWord, }; use collections::HashMap; -use editor::{actions::Tab, Editor, EditorElement, EditorStyle}; +use editor::{ + actions::{Tab, TabPrev}, + Editor, EditorElement, EditorStyle, +}; use futures::channel::oneshot; use gpui::{ actions, div, impl_actions, Action, AppContext, ClickEvent, EventEmitter, FocusableView, - FontStyle, FontWeight, InteractiveElement as _, IntoElement, KeyContext, ParentElement as _, - Render, Styled, Subscription, Task, TextStyle, View, ViewContext, VisualContext as _, - WhiteSpace, WindowContext, + FontStyle, FontWeight, Hsla, InteractiveElement as _, IntoElement, KeyContext, + ParentElement as _, Render, Styled, Subscription, Task, TextStyle, View, ViewContext, + VisualContext as _, WhiteSpace, WindowContext, }; use project::search::SearchQuery; use serde::Deserialize; @@ -23,7 +26,7 @@ use settings::Settings; use std::{any::Any, sync::Arc}; use theme::ThemeSettings; -use ui::{h_flex, prelude::*, Icon, IconButton, IconName, ToggleButton, Tooltip}; +use ui::{h_flex, prelude::*, IconButton, IconName, ToggleButton, Tooltip}; use util::ResultExt; use workspace::{ item::ItemHandle, @@ -34,6 +37,9 @@ use workspace::{ pub use registrar::DivRegistrar; use registrar::{ForDeployed, ForDismissed, SearchActionsRegistrar, WithResults}; +const MIN_INPUT_WIDTH_REMS: f32 = 15.; +const MAX_INPUT_WIDTH_REMS: f32 = 25.; + #[derive(PartialEq, Clone, Deserialize)] pub struct Deploy { pub focus: bool, @@ -54,7 +60,9 @@ pub fn init(cx: &mut AppContext) { pub struct BufferSearchBar { query_editor: View, + query_editor_focused: bool, replacement_editor: View, + replacement_editor_focused: bool, active_searchable_item: Option>, active_match_index: Option, active_searchable_item_subscription: Option, @@ -72,13 +80,18 @@ pub struct BufferSearchBar { } impl BufferSearchBar { - fn render_text_input(&self, editor: &View, cx: &ViewContext) -> impl IntoElement { + fn render_text_input( + &self, + editor: &View, + color: Hsla, + cx: &ViewContext, + ) -> impl IntoElement { let settings = ThemeSettings::get_global(cx); let text_style = TextStyle { color: if editor.read(cx).read_only(cx) { cx.theme().colors().text_disabled } else { - cx.theme().colors().text + color }, font_family: settings.ui_font.family.clone(), font_features: settings.ui_font.features, @@ -161,7 +174,8 @@ impl Render for BufferSearchBar { editor.set_placeholder_text("Replace with...", cx); }); - let match_count = self + let mut match_color = Color::Default; + let match_text = self .active_searchable_item .as_ref() .and_then(|searchable_item| { @@ -171,14 +185,15 @@ impl Render for BufferSearchBar { let matches = self .searchable_items_with_matches .get(&searchable_item.downgrade())?; - let message = if let Some(match_ix) = self.active_match_index { - format!("{}/{}", match_ix + 1, matches.len()) + if let Some(match_ix) = self.active_match_index { + Some(format!("{}/{}", match_ix + 1, matches.len())) } else { - "No matches".to_string() - }; - - Some(ui::Label::new(message)) - }); + match_color = Color::Error; // No matches found + None + } + }) + .unwrap_or_else(|| "No matches".to_string()); + let match_count = Label::new(match_text).color(match_color); let should_show_replace_input = self.replace_enabled && supported_options.replacement; let in_replace = self.replacement_editor.focus_handle(cx).is_focused(cx); @@ -192,35 +207,9 @@ impl Render for BufferSearchBar { } else { cx.theme().colors().border }; - h_flex() - .w_full() + + let search_line = h_flex() .gap_2() - .key_context(key_context) - .capture_action(cx.listener(Self::tab)) - .on_action(cx.listener(Self::previous_history_query)) - .on_action(cx.listener(Self::next_history_query)) - .on_action(cx.listener(Self::dismiss)) - .on_action(cx.listener(Self::select_next_match)) - .on_action(cx.listener(Self::select_prev_match)) - .on_action(cx.listener(|this, _: &ActivateRegexMode, cx| { - this.activate_search_mode(SearchMode::Regex, cx); - })) - .on_action(cx.listener(|this, _: &ActivateTextMode, cx| { - this.activate_search_mode(SearchMode::Text, cx); - })) - .when(self.supported_options().replacement, |this| { - this.on_action(cx.listener(Self::toggle_replace)) - .when(in_replace, |this| { - this.on_action(cx.listener(Self::replace_next)) - .on_action(cx.listener(Self::replace_all)) - }) - }) - .when(self.supported_options().case, |this| { - this.on_action(cx.listener(Self::toggle_case_sensitive)) - }) - .when(self.supported_options().word, |this| { - this.on_action(cx.listener(Self::toggle_whole_word)) - }) .child( h_flex() .flex_1() @@ -229,10 +218,10 @@ impl Render for BufferSearchBar { .gap_2() .border_1() .border_color(editor_border) - .min_w(rems(384. / 16.)) + .min_w(rems(MIN_INPUT_WIDTH_REMS)) + .max_w(rems(MAX_INPUT_WIDTH_REMS)) .rounded_lg() - .child(Icon::new(IconName::MagnifyingGlass)) - .child(self.render_text_input(&self.query_editor, cx)) + .child(self.render_text_input(&self.query_editor, match_color.color(cx), cx)) .children(supported_options.case.then(|| { self.render_search_option_button( SearchOptions::CASE_SENSITIVE, @@ -308,49 +297,6 @@ impl Render for BufferSearchBar { ) }), ) - .child( - h_flex() - .gap_0p5() - .flex_1() - .when(self.replace_enabled, |this| { - this.child( - h_flex() - .flex_1() - // We're giving this a fixed height to match the height of the search input, - // which has an icon inside that is increasing its height. - .h_8() - .px_2() - .py_1() - .gap_2() - .border_1() - .border_color(cx.theme().colors().border) - .rounded_lg() - .child(self.render_text_input(&self.replacement_editor, cx)), - ) - .when(should_show_replace_input, |this| { - this.child( - IconButton::new("search-replace-next", ui::IconName::ReplaceNext) - .tooltip(move |cx| { - Tooltip::for_action("Replace next", &ReplaceNext, cx) - }) - .on_click(cx.listener(|this, _, cx| { - this.replace_next(&ReplaceNext, cx) - })), - ) - .child( - IconButton::new("search-replace-all", ui::IconName::ReplaceAll) - .tooltip(move |cx| { - Tooltip::for_action("Replace all", &ReplaceAll, cx) - }) - .on_click( - cx.listener(|this, _, cx| { - this.replace_all(&ReplaceAll, cx) - }), - ), - ) - }) - }), - ) .child( h_flex() .gap_0p5() @@ -362,7 +308,7 @@ impl Render for BufferSearchBar { Tooltip::for_action("Select all matches", &SelectAllMatches, cx) }), ) - .children(match_count) + .child(div().min_w(rems(6.)).child(match_count)) .child(render_nav_button( ui::IconName::ChevronLeft, self.active_match_index.is_some(), @@ -375,7 +321,89 @@ impl Render for BufferSearchBar { "Select next match", &SelectNextMatch, )), + ); + + let replace_line = self.replace_enabled.then(|| { + h_flex() + .gap_0p5() + .flex_1() + .child( + h_flex() + .flex_1() + // We're giving this a fixed height to match the height of the search input, + // which has an icon inside that is increasing its height. + // .h_8() + .px_2() + .py_1() + .gap_2() + .border_1() + .border_color(cx.theme().colors().border) + .rounded_lg() + .min_w(rems(MIN_INPUT_WIDTH_REMS)) + .max_w(rems(MAX_INPUT_WIDTH_REMS)) + .child(self.render_text_input( + &self.replacement_editor, + cx.theme().colors().text, + cx, + )), + ) + .when(should_show_replace_input, |this| { + this.child( + IconButton::new("search-replace-next", ui::IconName::ReplaceNext) + .tooltip(move |cx| { + Tooltip::for_action("Replace next", &ReplaceNext, cx) + }) + .on_click( + cx.listener(|this, _, cx| this.replace_next(&ReplaceNext, cx)), + ), + ) + .child( + IconButton::new("search-replace-all", ui::IconName::ReplaceAll) + .tooltip(move |cx| Tooltip::for_action("Replace all", &ReplaceAll, cx)) + .on_click(cx.listener(|this, _, cx| this.replace_all(&ReplaceAll, cx))), + ) + }) + }); + + v_flex() + .key_context(key_context) + .capture_action(cx.listener(Self::tab)) + .capture_action(cx.listener(Self::tab_prev)) + .on_action(cx.listener(Self::previous_history_query)) + .on_action(cx.listener(Self::next_history_query)) + .on_action(cx.listener(Self::dismiss)) + .on_action(cx.listener(Self::select_next_match)) + .on_action(cx.listener(Self::select_prev_match)) + .on_action(cx.listener(|this, _: &ActivateRegexMode, cx| { + this.activate_search_mode(SearchMode::Regex, cx); + })) + .on_action(cx.listener(|this, _: &ActivateTextMode, cx| { + this.activate_search_mode(SearchMode::Text, cx); + })) + .when(self.supported_options().replacement, |this| { + this.on_action(cx.listener(Self::toggle_replace)) + .when(in_replace, |this| { + this.on_action(cx.listener(Self::replace_next)) + .on_action(cx.listener(Self::replace_all)) + }) + }) + .when(self.supported_options().case, |this| { + this.on_action(cx.listener(Self::toggle_case_sensitive)) + }) + .when(self.supported_options().word, |this| { + this.on_action(cx.listener(Self::toggle_whole_word)) + }) + .gap_1() + .child( + h_flex().child(search_line.w_full()).child( + IconButton::new(SharedString::from("Close"), IconName::Close) + .tooltip(move |cx| Tooltip::for_action("Close search bar", &Dismiss, cx)) + .on_click( + cx.listener(|this, _: &ClickEvent, cx| this.dismiss(&Dismiss, cx)), + ), + ), ) + .children(replace_line) } } @@ -488,11 +516,13 @@ impl BufferSearchBar { cx.subscribe(&query_editor, Self::on_query_editor_event) .detach(); let replacement_editor = cx.new_view(|cx| Editor::single_line(cx)); - cx.subscribe(&replacement_editor, Self::on_query_editor_event) + cx.subscribe(&replacement_editor, Self::on_replacement_editor_event) .detach(); Self { query_editor, + query_editor_focused: false, replacement_editor, + replacement_editor_focused: false, active_searchable_item: None, active_searchable_item_subscription: None, active_match_index: None, @@ -765,15 +795,33 @@ impl BufferSearchBar { event: &editor::EditorEvent, cx: &mut ViewContext, ) { - if let editor::EditorEvent::Edited = event { - self.query_contains_error = false; - self.clear_matches(cx); - let search = self.update_matches(cx); - cx.spawn(|this, mut cx| async move { - search.await?; - this.update(&mut cx, |this, cx| this.activate_current_match(cx)) - }) - .detach_and_log_err(cx); + match event { + editor::EditorEvent::Focused => self.query_editor_focused = true, + editor::EditorEvent::Blurred => self.query_editor_focused = false, + editor::EditorEvent::Edited => { + self.query_contains_error = false; + self.clear_matches(cx); + let search = self.update_matches(cx); + cx.spawn(|this, mut cx| async move { + search.await?; + this.update(&mut cx, |this, cx| this.activate_current_match(cx)) + }) + .detach_and_log_err(cx); + } + _ => {} + } + } + + fn on_replacement_editor_event( + &mut self, + _: View, + event: &editor::EditorEvent, + _: &mut ViewContext, + ) { + match event { + editor::EditorEvent::Focused => self.replacement_editor_focused = true, + editor::EditorEvent::Blurred => self.replacement_editor_focused = false, + _ => {} } } @@ -911,11 +959,29 @@ impl BufferSearchBar { } fn tab(&mut self, _: &Tab, cx: &mut ViewContext) { - if let Some(item) = self.active_searchable_item.as_ref() { - let focus_handle = item.focus_handle(cx); - cx.focus(&focus_handle); - cx.stop_propagation(); - } + // Search -> Replace -> Editor + let focus_handle = if self.replace_enabled && self.query_editor_focused { + self.replacement_editor.focus_handle(cx) + } else if let Some(item) = self.active_searchable_item.as_ref() { + item.focus_handle(cx) + } else { + return; + }; + cx.focus(&focus_handle); + cx.stop_propagation(); + } + + fn tab_prev(&mut self, _: &TabPrev, cx: &mut ViewContext) { + // Search -> Replace -> Search + let focus_handle = if self.replace_enabled && self.query_editor_focused { + self.replacement_editor.focus_handle(cx) + } else if self.replacement_editor_focused { + self.query_editor.focus_handle(cx) + } else { + return; + }; + cx.focus(&focus_handle); + cx.stop_propagation(); } fn next_history_query(&mut self, _: &NextHistoryQuery, cx: &mut ViewContext) { @@ -945,10 +1011,12 @@ impl BufferSearchBar { fn toggle_replace(&mut self, _: &ToggleReplace, cx: &mut ViewContext) { if let Some(_) = &self.active_searchable_item { self.replace_enabled = !self.replace_enabled; - if !self.replace_enabled { - let handle = self.query_editor.focus_handle(cx); - cx.focus(&handle); - } + let handle = if self.replace_enabled { + self.replacement_editor.focus_handle(cx) + } else { + self.query_editor.focus_handle(cx) + }; + cx.focus(&handle); cx.notify(); } }