From 8dab76867e0bcce2c1d4873bc8cf09b6e5568d37 Mon Sep 17 00:00:00 2001 From: thomasschafer Date: Sun, 1 Dec 2024 16:40:43 +0000 Subject: [PATCH 1/4] Show regex errors in popup --- src/app.rs | 62 +++++++++++++++++++++++++++++++++++-------------- src/fields.rs | 29 +++++++++++------------ src/ui.rs | 52 +++++++++++++++++++++++++++++++++++++---- tests/fields.rs | 1 + 4 files changed, 107 insertions(+), 37 deletions(-) diff --git a/src/app.rs b/src/app.rs index 27db4e2..fd2d1bc 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1,6 +1,5 @@ use ignore::WalkState; use itertools::Itertools; -use log::info; use parking_lot::{ MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard, RwLockWriteGuard, }; @@ -22,7 +21,7 @@ use tokio::{ use crate::{ event::{AppEvent, BackgroundProcessingEvent, ReplaceResult, SearchResult}, - fields::{CheckboxField, Field, TextField}, + fields::{CheckboxField, Field, FieldError, TextField}, parsed_fields::{ParsedFields, SearchType}, utils::relative_path_from, EventHandlingResult, @@ -175,6 +174,7 @@ pub const NUM_SEARCH_FIELDS: usize = 4; pub struct SearchFields { pub fields: [SearchField; NUM_SEARCH_FIELDS], pub highlighted: usize, + pub show_error_popup: bool, } macro_rules! define_field_accessor { @@ -256,6 +256,7 @@ impl SearchFields { }, ], highlighted: 0, + show_error_popup: false, } } @@ -272,10 +273,17 @@ impl SearchFields { (self.highlighted + self.fields.len().saturating_sub(1)) % self.fields.len(); } - pub fn clear_errors(&mut self) { + pub fn errors(&self) -> Vec<(&str, FieldError)> { self.fields - .iter_mut() - .for_each(|field| field.field.write().clear_error()) + .iter() + .filter_map(|field| { + field + .field + .read() + .error() + .map(|err| (field.name.title(), err)) + }) + .collect::>() } pub fn search_type(&self) -> anyhow::Result { @@ -290,6 +298,11 @@ impl SearchFields { } } +enum ValidatedField { + Parsed(T), + Error, +} + pub struct App { pub current_screen: Screen, pub search_fields: SearchFields, @@ -473,7 +486,7 @@ impl App { } fn handle_key_searching(&mut self, key: &KeyEvent) -> bool { - self.search_fields.clear_errors(); + self.search_fields.show_error_popup = false; match (key.code, key.modifiers) { (KeyCode::Enter, _) => { self.app_event_sender.send(AppEvent::PerformSearch).unwrap(); @@ -544,10 +557,17 @@ impl App { match (key.code, key.modifiers) { (KeyCode::Esc, _) | (KeyCode::Char('c'), KeyModifiers::CONTROL) => { + let exit = match &self.current_screen { + Screen::SearchFields if self.search_fields.show_error_popup => { + self.search_fields.show_error_popup = false; + false + } + _ => true, + }; return Ok(EventHandlingResult { - exit: true, + exit, rerender: true, - }) + }); } (KeyCode::Char('r'), KeyModifiers::CONTROL) => { self.reset(); @@ -574,37 +594,43 @@ impl App { } fn validate_fields( - &self, + &mut self, background_processing_sender: UnboundedSender, ) -> anyhow::Result> { let search_pattern = match self.search_fields.search_type() { Err(e) => { if e.downcast_ref::().is_some() { - info!("Error when parsing search regex {}", e); self.search_fields .search_mut() - .set_error("Couldn't parse regex".to_owned()); - return Ok(None); + .set_error("Couldn't parse regex".to_owned(), e.to_string()); + ValidatedField::Error } else { return Err(e); } } - Ok(p) => p, + Ok(p) => ValidatedField::Parsed(p), }; let path_pattern_text = self.search_fields.path_pattern().text(); let path_pattern = if path_pattern_text.is_empty() { - None + ValidatedField::Parsed(None) } else { match Regex::new(path_pattern_text.as_str()) { Err(e) => { - info!("Error when parsing filname pattern regex {}", e); self.search_fields .path_pattern_mut() - .set_error("Couldn't parse regex".to_owned()); - return Ok(None); + .set_error("Couldn't parse regex".to_owned(), e.to_string()); + ValidatedField::Error } - Ok(r) => Some(r), + Ok(r) => ValidatedField::Parsed(Some(r)), + } + }; + + let (search_pattern, path_pattern) = match (search_pattern, path_pattern) { + (ValidatedField::Parsed(s), ValidatedField::Parsed(p)) => (s, p), + _ => { + self.search_fields.show_error_popup = true; + return Ok(None); } }; diff --git a/src/fields.rs b/src/fields.rs index 0aa5090..e996933 100644 --- a/src/fields.rs +++ b/src/fields.rs @@ -7,11 +7,17 @@ use ratatui::{ Frame, }; +#[derive(Clone)] +pub struct FieldError { + pub short: String, + pub long: String, +} + #[derive(Default)] pub struct TextField { pub text: String, pub cursor_idx: usize, - pub error: Option, + pub error: Option, } impl TextField { @@ -151,8 +157,8 @@ impl TextField { self.cursor_idx = 0; } - pub fn set_error(&mut self, error: String) { - self.error = Some(error); + pub fn set_error(&mut self, short: String, long: String) { + self.error = Some(FieldError { short, long }); } pub fn clear_error(&mut self) { @@ -213,7 +219,7 @@ impl TextField { pub struct CheckboxField { pub checked: bool, - pub error: Option, // TODO: render this + pub error: Option, // Not used currently so not rendered } impl CheckboxField { @@ -246,6 +252,7 @@ impl Field { } pub fn handle_keys(&mut self, code: KeyCode, modifiers: KeyModifiers) { + self.clear_error(); match self { Field::Text(f) => f.handle_keys(code, modifiers), Field::Checkbox(f) => f.handle_keys(code, modifiers), @@ -259,14 +266,6 @@ impl Field { } } - #[allow(dead_code)] - pub fn set_error(&mut self, error: String) { - match self { - Field::Text(f) => f.set_error(error), - Field::Checkbox(_) => todo!(), - } - } - pub fn clear_error(&mut self) { match self { Field::Text(f) => f.clear_error(), @@ -274,7 +273,7 @@ impl Field { } } - fn error(&self) -> Option { + pub fn error(&self) -> Option { match self { Field::Text(f) => f.error.clone(), Field::Checkbox(f) => f.error.clone(), @@ -320,9 +319,9 @@ impl Field { } } - if let Some(error_string) = self.error() { + if let Some(error) = self.error() { frame.render_widget( - Paragraph::new(Text::styled(format!("Error: {error_string}"), Color::Red)), + Paragraph::new(Text::styled(format!("Error: {}", error.short), Color::Red)), outer_chunks[1], ); }; diff --git a/src/ui.rs b/src/ui.rs index fc9cfbc..c212263 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -1,10 +1,9 @@ use itertools::Itertools; use ratatui::{ - layout::Constraint, - layout::{Alignment, Direction, Flex, Layout, Rect}, - style::{Color, Style}, + layout::{Alignment, Constraint, Direction, Flex, Layout, Rect}, + style::{Color, Style, Stylize}, text::{Line, Span, Text}, - widgets::{Block, List, ListItem, Paragraph}, + widgets::{Block, Clear, List, ListItem, Paragraph, Wrap}, Frame, }; use similar::{Change, ChangeTag, TextDiff}; @@ -59,6 +58,51 @@ fn render_search_view(frame: &mut Frame<'_>, app: &App, rect: Rect) { highlighted_area.y + 1, ) } + + if app.search_fields.show_error_popup { + let error_lines: Vec> = app + .search_fields + .errors() + .iter() + .flat_map(|(name, error)| { + let name_line = Line::from(vec![Span::styled(*name, Style::default().bold())]); + + let error_lines: Vec> = error + .long + .lines() + .map(|line| { + Line::from(vec![Span::styled( + format!(" {line}"), + Style::default().fg(Color::Red), + )]) + }) + .collect(); + + std::iter::once(name_line) + .chain(error_lines) + .chain(std::iter::once(Line::from(""))) + .collect::>() + }) + .collect(); + + let content_height = error_lines.len() as u16 + 1; + + let popup_area = center( + area, + Constraint::Percentage(80), + Constraint::Length(content_height), + ); + + let popup = Paragraph::new(error_lines) + .block( + Block::bordered() + .title("Errors") + .title_alignment(Alignment::Center), + ) + .wrap(Wrap { trim: true }); + frame.render_widget(Clear, popup_area); + frame.render_widget(popup, popup_area); + } } #[derive(Debug, PartialEq, Eq, Clone)] diff --git a/tests/fields.rs b/tests/fields.rs index 61bac3f..b934d93 100644 --- a/tests/fields.rs +++ b/tests/fields.rs @@ -95,6 +95,7 @@ fn test_search_fields() { }, ], highlighted: 0, + show_error_popup: false, }; // Test focus navigation From 94cb8e600d00c19b2023898a1b00bf577aa9b4d6 Mon Sep 17 00:00:00 2001 From: thomasschafer Date: Sun, 1 Dec 2024 16:54:27 +0000 Subject: [PATCH 2/4] Clear search error when toggling fixed strings --- src/app.rs | 14 +++++++++++++- src/main.rs | 1 - src/ui.rs | 15 +++++++-------- tests/fields.rs | 3 --- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/app.rs b/src/app.rs index fd2d1bc..d819a15 100644 --- a/src/app.rs +++ b/src/app.rs @@ -260,8 +260,16 @@ impl SearchFields { } } + fn highlighted_field_impl(&self) -> &SearchField { + &self.fields[self.highlighted] + } + pub fn highlighted_field(&self) -> &Arc> { - &self.fields[self.highlighted].field + &self.highlighted_field_impl().field + } + + pub fn highlighted_field_name(&self) -> &FieldName { + &self.highlighted_field_impl().name } pub fn focus_next(&mut self) { @@ -498,6 +506,10 @@ impl App { self.search_fields.focus_next(); } (code, modifiers) => { + if let FieldName::FixedStrings = self.search_fields.highlighted_field_name() { + // TODO: ideally this should only happen when the field is checked, but for now this will do + self.search_fields.search_mut().clear_error(); + }; self.search_fields .highlighted_field() .write() diff --git a/src/main.rs b/src/main.rs index 39eaee3..7746a84 100644 --- a/src/main.rs +++ b/src/main.rs @@ -46,7 +46,6 @@ fn parse_log_level(s: &str) -> Result { LevelFilter::from_str(s).map_err(|_| format!("Invalid log level: {}", s)) } -// In main(), update the logging setup: #[tokio::main] async fn main() -> anyhow::Result<()> { let args = Args::parse(); diff --git a/src/ui.rs b/src/ui.rs index c212263..62178c9 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -51,14 +51,6 @@ fn render_search_view(frame: &mut Frame<'_>, app: &App, rect: Rect) { ) }); - let highlighted_area = areas[app.search_fields.highlighted]; - if let Some(cursor_idx) = app.search_fields.highlighted_field().read().cursor_idx() { - frame.set_cursor( - highlighted_area.x + cursor_idx as u16 + 1, - highlighted_area.y + 1, - ) - } - if app.search_fields.show_error_popup { let error_lines: Vec> = app .search_fields @@ -102,6 +94,13 @@ fn render_search_view(frame: &mut Frame<'_>, app: &App, rect: Rect) { .wrap(Wrap { trim: true }); frame.render_widget(Clear, popup_area); frame.render_widget(popup, popup_area); + } else if let Some(cursor_idx) = app.search_fields.highlighted_field().read().cursor_idx() { + let highlighted_area = areas[app.search_fields.highlighted]; + + frame.set_cursor( + highlighted_area.x + cursor_idx as u16 + 1, + highlighted_area.y + 1, + ) } } diff --git a/tests/fields.rs b/tests/fields.rs index b934d93..eae423f 100644 --- a/tests/fields.rs +++ b/tests/fields.rs @@ -17,7 +17,6 @@ fn test_text_field_operations() { assert_eq!(field.text(), "Hello"); assert_eq!(field.cursor_idx(), 5); - // Test cursor movement field.move_cursor_left(); assert_eq!(field.cursor_idx(), 4); field.move_cursor_right(); @@ -27,7 +26,6 @@ fn test_text_field_operations() { field.move_cursor_end(); assert_eq!(field.cursor_idx(), 5); - // Test word movement field.clear(); for c in "Hello world".chars() { field.enter_char(c); @@ -68,7 +66,6 @@ fn test_checkbox_field() { field.handle_keys(KeyCode::Char(' '), KeyModifiers::empty()); assert!(!field.checked); - // Test that other keys don't affect the checkbox field.handle_keys(KeyCode::Enter, KeyModifiers::empty()); assert!(!field.checked); } From c54b0dc8b2488f98b182291821e09bef603ed223 Mon Sep 17 00:00:00 2001 From: thomasschafer Date: Mon, 2 Dec 2024 14:31:47 +0000 Subject: [PATCH 3/4] Add test for popup --- src/event.rs | 1 + tests/app.rs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/event.rs b/src/event.rs index e15dfcb..b8d4c5d 100644 --- a/src/event.rs +++ b/src/event.rs @@ -50,6 +50,7 @@ pub struct EventHandler { pub app_event_sender: mpsc::UnboundedSender, } +#[derive(Debug)] pub struct EventHandlingResult { pub exit: bool, pub rerender: bool, diff --git a/tests/app.rs b/tests/app.rs index 8dcd516..69f5656 100644 --- a/tests/app.rs +++ b/tests/app.rs @@ -121,6 +121,42 @@ async fn test_back_from_results() { assert!(matches!(app.current_screen, Screen::SearchFields)); } +// TODO: replace this (and other tests?) with end-to-end tests +#[tokio::test] +async fn test_error_popup() { + let events = EventHandler::new(); + let mut app = App::new(None, false, events.app_event_sender.clone()); + app.current_screen = Screen::SearchFields; + app.search_fields = + SearchFields::with_values("search invalid regex(", "replacement", false, ""); + + let res = app.perform_search_if_valid(); + assert!(!res.exit); + assert!(matches!(app.current_screen, Screen::SearchFields)); + assert!(app.search_fields.show_error_popup); + + let res = app + .handle_key_events(&KeyEvent { + code: KeyCode::Esc, + modifiers: KeyModifiers::NONE, + kind: KeyEventKind::Press, + state: KeyEventState::NONE, + }) + .unwrap(); + assert!(!res.exit); + assert!(!app.search_fields.show_error_popup); + + let res = app + .handle_key_events(&KeyEvent { + code: KeyCode::Esc, + modifiers: KeyModifiers::NONE, + kind: KeyEventKind::Press, + state: KeyEventState::NONE, + }) + .unwrap(); + assert!(res.exit); +} + macro_rules! create_test_files { ($($name:expr => {$($line:expr),+ $(,)?}),+ $(,)?) => { { From a7651af00fd7677ff2a4300bb446685b184b2b79 Mon Sep 17 00:00:00 2001 From: thomasschafer Date: Mon, 2 Dec 2024 14:42:09 +0000 Subject: [PATCH 4/4] Allow enter to close popup --- src/app.rs | 56 ++++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/src/app.rs b/src/app.rs index d819a15..3c63f29 100644 --- a/src/app.rs +++ b/src/app.rs @@ -494,26 +494,29 @@ impl App { } fn handle_key_searching(&mut self, key: &KeyEvent) -> bool { - self.search_fields.show_error_popup = false; - match (key.code, key.modifiers) { - (KeyCode::Enter, _) => { - self.app_event_sender.send(AppEvent::PerformSearch).unwrap(); - } - (KeyCode::BackTab, _) | (KeyCode::Tab, KeyModifiers::ALT) => { - self.search_fields.focus_prev(); - } - (KeyCode::Tab, _) => { - self.search_fields.focus_next(); - } - (code, modifiers) => { - if let FieldName::FixedStrings = self.search_fields.highlighted_field_name() { - // TODO: ideally this should only happen when the field is checked, but for now this will do - self.search_fields.search_mut().clear_error(); - }; - self.search_fields - .highlighted_field() - .write() - .handle_keys(code, modifiers); + if self.search_fields.show_error_popup { + self.search_fields.show_error_popup = false; + } else { + match (key.code, key.modifiers) { + (KeyCode::Enter, _) => { + self.app_event_sender.send(AppEvent::PerformSearch).unwrap(); + } + (KeyCode::BackTab, _) | (KeyCode::Tab, KeyModifiers::ALT) => { + self.search_fields.focus_prev(); + } + (KeyCode::Tab, _) => { + self.search_fields.focus_next(); + } + (code, modifiers) => { + if let FieldName::FixedStrings = self.search_fields.highlighted_field_name() { + // TODO: ideally this should only happen when the field is checked, but for now this will do + self.search_fields.search_mut().clear_error(); + }; + self.search_fields + .highlighted_field() + .write() + .handle_keys(code, modifiers); + } } }; false @@ -568,16 +571,11 @@ impl App { } match (key.code, key.modifiers) { - (KeyCode::Esc, _) | (KeyCode::Char('c'), KeyModifiers::CONTROL) => { - let exit = match &self.current_screen { - Screen::SearchFields if self.search_fields.show_error_popup => { - self.search_fields.show_error_popup = false; - false - } - _ => true, - }; + (KeyCode::Esc, _) | (KeyCode::Char('c'), KeyModifiers::CONTROL) + if !self.search_fields.show_error_popup => + { return Ok(EventHandlingResult { - exit, + exit: true, rerender: true, }); }