diff --git a/sql-cli/src/app_state_container.rs b/sql-cli/src/app_state_container.rs index d8f666a8..54775035 100644 --- a/sql-cli/src/app_state_container.rs +++ b/sql-cli/src/app_state_container.rs @@ -6267,6 +6267,46 @@ impl AppStateContainer { } } +impl Default for AppStateContainer { + fn default() -> Self { + // Create a minimal AppStateContainer for testing + // Uses CommandHistory::default() which handles errors gracefully + let command_history = CommandHistory::default(); + let mut widgets = WidgetStates::new(); + widgets.set_history(HistoryWidget::new(command_history.clone())); + + Self { + buffers: BufferManager::new(), + current_buffer_id: 0, + command_input: RefCell::new(InputState::new()), + search: RefCell::new(SearchState::new()), + filter: RefCell::new(FilterState::new()), + column_search: RefCell::new(ColumnSearchState::new()), + history_search: RefCell::new(HistorySearchState::new()), + sort: RefCell::new(SortState::new()), + selection: RefCell::new(SelectionState::new()), + completion: RefCell::new(CompletionState::default()), + widgets, + cache_list: CacheListState::new(), + column_stats: ColumnStatsState::new(), + jump_to_row: JumpToRowState::new(), + navigation: RefCell::new(NavigationState::new()), + command_history: RefCell::new(command_history), + key_press_history: RefCell::new(KeyPressHistory::new(100)), + results: RefCell::new(ResultsState::default()), + clipboard: RefCell::new(ClipboardState::default()), + chord: RefCell::new(ChordState::default()), + undo_redo: RefCell::new(UndoRedoState::default()), + scroll: RefCell::new(ScrollState::default()), + results_cache: ResultsCache::new(100), + mode_stack: Vec::new(), + debug_enabled: false, + debug_service: RefCell::new(None), + help: RefCell::new(HelpState::new()), + } + } +} + impl fmt::Debug for AppStateContainer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("AppStateContainer") diff --git a/sql-cli/src/ui/enhanced_tui.rs b/sql-cli/src/ui/enhanced_tui.rs index 13812a9e..eef351c1 100644 --- a/sql-cli/src/ui/enhanced_tui.rs +++ b/sql-cli/src/ui/enhanced_tui.rs @@ -880,15 +880,27 @@ impl EnhancedTuiApp { } NextSearchMatch => { - // Use VimSearchManager for search navigation - self.vim_search_next(); + // n key: navigate to next search match only if search is active (not after Escape) + // Use StateCoordinator to determine if search navigation should be handled + use crate::ui::state_coordinator::StateCoordinator; + if StateCoordinator::should_handle_next_match( + &self.state_container, + Some(&self.vim_search_adapter), + ) { + self.vim_search_next(); + } else { + debug!("NextSearchMatch: No active search (or cancelled with Escape), ignoring 'n' key"); + } Ok(ActionResult::Handled) } PreviousSearchMatch => { - // Shift+N behavior: search navigation if search is active, otherwise toggle row numbers - if self.vim_search_adapter.borrow().is_active() - || self.vim_search_adapter.borrow().get_pattern().is_some() - { + // Shift+N behavior: search navigation only if vim search is active, otherwise toggle row numbers + // Use StateCoordinator to determine if search navigation should be handled + use crate::ui::state_coordinator::StateCoordinator; + if StateCoordinator::should_handle_previous_match( + &self.state_container, + Some(&self.vim_search_adapter), + ) { self.vim_search_previous(); } else { // Delegate to the ToggleRowNumbers action for consistency @@ -2585,14 +2597,60 @@ impl EnhancedTuiApp { } fn handle_results_input(&mut self, key: crossterm::event::KeyEvent) -> Result { - // Simple fix: If Escape is pressed and there's an active search, clear it - if key.code == KeyCode::Esc && !self.state_container.get_search_pattern().is_empty() { - info!("Escape pressed in Results mode with active search - clearing search"); - self.state_container.set_search_pattern(String::new()); - self.state_container.clear_search(); - self.state_container - .set_status_message("Search cleared".to_string()); - return Ok(false); + // Log all keys in Results mode to debug Escape handling + debug!( + "handle_results_input: Processing key {:?} in Results mode", + key + ); + + // Check if vim search is active/navigating + let is_vim_navigating = self.vim_search_adapter.borrow().is_navigating(); + let vim_is_active = self.vim_search_adapter.borrow().is_active(); + let has_search_pattern = !self.state_container.get_search_pattern().is_empty(); + + debug!( + "Search state check: vim_navigating={}, vim_active={}, has_pattern={}, pattern='{}'", + is_vim_navigating, + vim_is_active, + has_search_pattern, + self.state_container.get_search_pattern() + ); + + // If Escape is pressed and there's an active search or vim navigation, handle it properly + if key.code == KeyCode::Esc { + info!("ESCAPE KEY DETECTED in Results mode!"); + + if is_vim_navigating || vim_is_active || has_search_pattern { + info!("Escape pressed with active search - clearing via StateCoordinator"); + debug!( + "Pre-clear state: vim_navigating={}, vim_active={}, pattern='{}'", + is_vim_navigating, + vim_is_active, + self.state_container.get_search_pattern() + ); + + // Use StateCoordinator to handle ALL search cancellation logic + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::cancel_search_with_refs( + &mut self.state_container, + &self.shadow_state, + Some(&self.vim_search_adapter), + ); + + // Verify clearing worked + let post_pattern = self.state_container.get_search_pattern(); + let post_vim_active = self.vim_search_adapter.borrow().is_active(); + info!( + "Post-clear state: pattern='{}', vim_active={}", + post_pattern, post_vim_active + ); + + self.state_container + .set_status_message("Search cleared".to_string()); + return Ok(false); + } else { + info!("Escape pressed but no active search to clear"); + } } let selection_mode = self.state_container.get_selection_mode(); @@ -3155,10 +3213,15 @@ impl EnhancedTuiApp { } // ALWAYS switch back to Results mode after Apply for all search modes - self.state_container.set_mode(AppMode::Results); - self.shadow_state - .borrow_mut() - .observe_mode_change(AppMode::Results, "filter_applied"); + // Use StateCoordinator to properly complete search (keeps pattern for n/N) + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::complete_search_with_refs( + &mut self.state_container, + &self.shadow_state, + Some(&self.vim_search_adapter), + AppMode::Results, + "search_applied", + ); // Show status message let filter_msg = match mode { @@ -3228,22 +3291,14 @@ impl EnhancedTuiApp { debug!(target: "search", "Cancel: No saved SQL from widget"); } - // Clear all search navigation state (so n/N keys work properly after escape) - self.state_container.clear_search(); - self.vim_search_adapter.borrow_mut().cancel_search(); - - // Observe search end - self.shadow_state - .borrow_mut() - .observe_search_end("search_cancelled"); - - // Switch back to Results mode - self.state_container.set_mode(AppMode::Results); - - // Observe mode change - self.shadow_state - .borrow_mut() - .observe_mode_change(AppMode::Results, "return_from_search"); + // Use StateCoordinator to properly cancel search and restore state + // StateCoordinator handles clearing vim search adapter too + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::cancel_search_with_refs( + &mut self.state_container, + &self.shadow_state, + Some(&self.vim_search_adapter), + ); } SearchModesAction::NextMatch => { debug!(target: "search", "NextMatch action, current_mode={:?}, widget_mode={:?}", diff --git a/sql-cli/src/ui/state_coordinator.rs b/sql-cli/src/ui/state_coordinator.rs index 54094714..91f728ed 100644 --- a/sql-cli/src/ui/state_coordinator.rs +++ b/sql-cli/src/ui/state_coordinator.rs @@ -3,7 +3,6 @@ use std::rc::Rc; use crate::app_state_container::AppStateContainer; use crate::buffer::{AppMode, Buffer, BufferAPI, BufferManager}; -use crate::data::data_view::DataView; use crate::sql::hybrid_parser::HybridParser; use crate::ui::viewport_manager::ViewportManager; use crate::widgets::search_modes_widget::SearchMode; @@ -186,6 +185,152 @@ impl StateCoordinator { trigger.to_string() } + // ========== SEARCH CANCELLATION ========== + + /// Cancel search and properly restore state + /// This handles all the complex state synchronization when Escape is pressed during search + pub fn cancel_search(&mut self) -> (Option, Option) { + debug!("StateCoordinator::cancel_search: Canceling search and restoring state"); + + // Clear search state in state container + self.state_container.clear_search(); + + // Observe search end in shadow state + self.shadow_state + .borrow_mut() + .observe_search_end("search_cancelled"); + + // Switch back to Results mode with proper synchronization + self.sync_mode(AppMode::Results, "search_cancelled"); + + // Return saved SQL and cursor position for restoration + // This would come from search widget's saved state + (None, None) + } + + /// Static version for delegation pattern with vim search adapter + pub fn cancel_search_with_refs( + state_container: &mut AppStateContainer, + shadow_state: &RefCell, + vim_search_adapter: Option<&RefCell>, + ) { + debug!("StateCoordinator::cancel_search_with_refs: Canceling search and clearing all search state"); + + // Clear vim search adapter if provided + if let Some(adapter) = vim_search_adapter { + debug!("Clearing vim search adapter state"); + adapter.borrow_mut().clear(); + } + + // Clear search pattern in state container + state_container.set_search_pattern(String::new()); + state_container.clear_search(); + + // Observe search end in shadow state + shadow_state + .borrow_mut() + .observe_search_end("search_cancelled"); + + // Sync back to Results mode + Self::sync_mode_with_refs( + state_container, + shadow_state, + AppMode::Results, + "vim_search_cancelled", + ); + } + + /// Check if 'n' key should navigate to next search match + /// Returns true only if there's an active search (not cancelled with Escape) + pub fn should_handle_next_match( + state_container: &AppStateContainer, + vim_search_adapter: Option<&RefCell>, + ) -> bool { + // 'n' should only work if there's a search pattern AND it hasn't been cancelled + let has_search = !state_container.get_search_pattern().is_empty(); + let pattern = state_container.get_search_pattern(); + + // Check if vim search is active or navigating + // After Escape, this will be false + let vim_active = if let Some(adapter) = vim_search_adapter { + let adapter_ref = adapter.borrow(); + adapter_ref.is_active() || adapter_ref.is_navigating() + } else { + false + }; + + debug!( + "StateCoordinator::should_handle_next_match: pattern='{}', vim_active={}, result={}", + pattern, + vim_active, + has_search && vim_active + ); + + // Only handle if search exists AND hasn't been cancelled with Escape + has_search && vim_active + } + + /// Check if 'N' key should navigate to previous search match + /// Returns true only if there's an active search (not cancelled with Escape) + pub fn should_handle_previous_match( + state_container: &AppStateContainer, + vim_search_adapter: Option<&RefCell>, + ) -> bool { + // 'N' should only work if there's a search pattern AND it hasn't been cancelled + let has_search = !state_container.get_search_pattern().is_empty(); + let pattern = state_container.get_search_pattern(); + + // Check if vim search is active or navigating + // After Escape, this will be false + let vim_active = if let Some(adapter) = vim_search_adapter { + let adapter_ref = adapter.borrow(); + adapter_ref.is_active() || adapter_ref.is_navigating() + } else { + false + }; + + debug!( + "StateCoordinator::should_handle_previous_match: pattern='{}', vim_active={}, result={}", + pattern, vim_active, has_search && vim_active + ); + + // Only handle if search exists AND hasn't been cancelled with Escape + has_search && vim_active + } + + /// Complete a search operation (after Apply/Enter is pressed) + /// This keeps the pattern for n/N navigation but marks search as complete + pub fn complete_search_with_refs( + state_container: &mut AppStateContainer, + shadow_state: &RefCell, + vim_search_adapter: Option<&RefCell>, + mode: AppMode, + trigger: &str, + ) { + debug!( + "StateCoordinator::complete_search_with_refs: Completing search, switching to {:?}", + mode + ); + + // Note: We intentionally DO NOT clear the search pattern here + // The pattern remains available for n/N navigation + + // Mark vim search adapter as not actively searching + // but keep the matches for navigation + if let Some(adapter) = vim_search_adapter { + debug!("Marking vim search as complete but keeping matches"); + adapter.borrow_mut().mark_search_complete(); + } + + // Observe search completion in shadow state + shadow_state + .borrow_mut() + .observe_search_end("search_completed"); + + // Switch to the target mode (usually Results) + Self::sync_mode_with_refs(state_container, shadow_state, mode, trigger); + } + // ========== QUERY EXECUTION SYNCHRONIZATION ========== /// Switch to Results mode after successful query execution diff --git a/sql-cli/src/ui/vim_search_adapter.rs b/sql-cli/src/ui/vim_search_adapter.rs index 2f69219d..88c27418 100644 --- a/sql-cli/src/ui/vim_search_adapter.rs +++ b/sql-cli/src/ui/vim_search_adapter.rs @@ -83,19 +83,20 @@ impl VimSearchAdapter { } /// Handle a key press through AppStateContainer (simplified interface) - /// Note: This version requires the caller to handle viewport management + /// Returns true if key was handled, false to let it fall through pub fn handle_key(&mut self, key: KeyCode, state: &mut AppStateContainer) -> bool { let mode = state.get_mode(); - // Handle Escape key based on current mode + // Special handling for Escape key if key == KeyCode::Esc { match mode { - AppMode::Results if self.is_active => { - // In Results mode with active search: just exit search, stay in Results - info!("VimSearchAdapter: Exiting search navigation in Results mode"); - state.set_status_message("Search mode exited".to_string()); - self.clear(); - return true; // We handled it + AppMode::Results if self.is_active || self.is_navigating() => { + // In Results mode with active search: Signal that Escape was pressed + // But DON'T handle it here - return false to let StateCoordinator handle it + info!("VimSearchAdapter: Escape detected in Results mode with active search - signaling for StateCoordinator"); + // Return false so it falls through to handle_results_input + // where StateCoordinator will properly clear everything + return false; } AppMode::Search => { // In Search mode: exit to Results @@ -249,6 +250,15 @@ impl VimSearchAdapter { self.manager.exit_navigation(); } + /// Mark search as complete (after Apply/Enter) + /// Keeps matches for n/N navigation and stays active + pub fn mark_search_complete(&mut self) { + info!("VimSearchAdapter: Marking search as complete, keeping matches for navigation"); + // Keep is_active = true so n/N continue to work + // The manager stays active with matches + // Only clear() or cancel_search() will deactivate + } + /// Navigate to next match pub fn next_match( &mut self, diff --git a/sql-cli/test_search_escape.sh b/sql-cli/test_search_escape.sh new file mode 100755 index 00000000..bbe621d4 --- /dev/null +++ b/sql-cli/test_search_escape.sh @@ -0,0 +1,23 @@ +#!/bin/bash +# Test script to verify search and Escape behavior + +echo "Testing search and Escape behavior..." +echo "" +echo "Steps to test:" +echo "1. Load a CSV file" +echo "2. Press '/' to start vim search" +echo "3. Type a search term (e.g., 'unalloc')" +echo "4. Press Enter to apply search" +echo "5. Press 'n' - should navigate to next match" +echo "6. Press 'N' - should toggle line numbers (NOT navigate)" +echo "7. Press Escape - should clear search completely" +echo "8. Press 'n' - should do nothing (no search active)" +echo "9. Press 'N' - should toggle line numbers" +echo "" +echo "Starting sql-cli..." + +# Enable debug logging for search and StateCoordinator +export RUST_LOG=sql_cli::ui::state_coordinator=debug,sql_cli::ui::vim_search_adapter=info,search=debug + +# Run with a sample file +./target/release/sql-cli ../trades_20k.csv \ No newline at end of file diff --git a/sql-cli/tests/test_buffer_state_refactor.rs b/sql-cli/tests/test_buffer_state_refactor.rs index dfd442f3..fd02dd6c 100644 --- a/sql-cli/tests/test_buffer_state_refactor.rs +++ b/sql-cli/tests/test_buffer_state_refactor.rs @@ -156,9 +156,9 @@ fn test_buffer_state_preserved_on_switch() { #[test] fn test_proxy_with_no_buffer() { - // Create empty BufferManager - let buffer_manager = BufferManager::new(); - let state = AppStateContainer::new(buffer_manager).unwrap(); + // Create AppStateContainer with empty BufferManager using Default impl + // This avoids file system access issues in tests + let state = AppStateContainer::default(); // Test that proxies return defaults when no buffer exists let nav_proxy = state.navigation_proxy();