fix: restore key auto-repeat in copy mode and other non-terminal modes (PoC) #2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,9 +220,18 @@ pub(crate) struct TerminalInputTarget { | |
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub(crate) enum InputContext { | ||
| Pane, | ||
| Popup(crate::terminal::TerminalId), | ||
| NonTerminal(NonTerminalInputContext), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub(crate) enum NonTerminalInputContext { | ||
|
Owner
Author
There was a problem hiding this comment. This enum lists only the modes where we want key repeat. Modes not listed here get None from input_context(), so their repeats stay suppressed as before. I added only the four modes needed so far, but listing all modes is also possible if that is preferred. |
||
| Copy, | ||
| Navigator, | ||
| Navigate, | ||
| KeybindHelp, | ||
| } | ||
|
|
||
| pub(crate) type InputSourceId = u64; | ||
|
|
@@ -1666,11 +1675,23 @@ impl App { | |
| // --------------------------------------------------------------------------- | ||
|
|
||
| impl App { | ||
| pub(crate) fn input_context(&self) -> Option<InputContext> { | ||
| if let Some(popup) = &self.state.popup_pane { | ||
| Some(InputContext::Popup(popup.terminal_id.clone())) | ||
| } else if self.state.mode == Mode::Terminal { | ||
| Some(InputContext::Pane) | ||
| } else if self.state.mode == Mode::Copy { | ||
| Some(InputContext::NonTerminal(NonTerminalInputContext::Copy)) | ||
|
Owner
Author
There was a problem hiding this comment. This line is the direct cause of the broken repeat in copy mode. Whether repeats are allowed is decided right after a press is handled, by comparing the context before the press with the context after it (complete_press). If both are the same Some, later repeats are reprocessed; if either is None, they are all dropped. Copy mode always had None here, so its repeats were always dropped. Returning Some makes this comparison succeed. Keys that leave the mode (Enter/y/q/Esc, ...) change the context at press time, so they still do not repeat. Every repeat also re-checks that the context is still the same, so if the mode changes while the key is held, the repeat stops there. |
||
| } else if self.state.mode == Mode::Navigator { | ||
| Some(InputContext::NonTerminal( | ||
| NonTerminalInputContext::Navigator, | ||
| )) | ||
| } else if self.state.mode == Mode::Navigate { | ||
| Some(InputContext::NonTerminal(NonTerminalInputContext::Navigate)) | ||
| } else if self.state.mode == Mode::KeybindHelp { | ||
| Some(InputContext::NonTerminal( | ||
| NonTerminalInputContext::KeybindHelp, | ||
| )) | ||
| } else { | ||
| None | ||
| } | ||
|
|
@@ -1706,7 +1727,7 @@ impl App { | |
| } | ||
| continue; | ||
| } | ||
| let current_context = self.input_context(); | ||
| if !self.input_leases.reprocess_allowed( | ||
| lease_key, | ||
| &context, | ||
|
|
@@ -1715,6 +1736,10 @@ impl App { | |
| ) { | ||
| break; | ||
| } | ||
| if matches!(context, InputContext::NonTerminal(_)) { | ||
| self.handle_non_terminal_key_headless(key.clone()); | ||
| continue; | ||
| } | ||
| if let Some(target) = | ||
| self.handle_terminal_key_headless_from(source_id, key.clone()) | ||
| { | ||
|
|
@@ -1793,14 +1818,17 @@ impl App { | |
| let key = self.input_leases.normalize_press(&lease_key, key); | ||
| match key.kind { | ||
| crossterm::event::KeyEventKind::Press => { | ||
| let initial_context = self.input_context(); | ||
| let target = if matches!( | ||
| initial_context, | ||
| Some(InputContext::Pane | InputContext::Popup(_)) | ||
|
Owner
Author
There was a problem hiding this comment. This used to be is_some(), which meant "if there is a context, forward the key to the terminal". That is no longer true once NonTerminal contexts exist, so the terminal variants are matched explicitly now. The reprocess loop had the same pattern; there it is handled by the added NonTerminal branch (a hunk above in this file). |
||
| ) { | ||
| self.handle_terminal_key_headless_from(source_id, key.clone()) | ||
| } else { | ||
| self.handle_non_terminal_key_headless(key.clone()); | ||
| None | ||
| }; | ||
| let resulting_context = self.input_context(); | ||
| let plan = self.input_leases.complete_press( | ||
| lease_key, | ||
| &key, | ||
|
|
@@ -1811,7 +1839,7 @@ impl App { | |
| self.execute_repeat_plan_headless(source_id, lease_key, key, plan); | ||
| } | ||
| crossterm::event::KeyEventKind::Repeat => { | ||
| let current_context = self.input_context(); | ||
| let plan = self.input_leases.plan_repeat( | ||
| lease_key, | ||
| &key, | ||
|
|
@@ -2093,6 +2121,89 @@ mod tests { | |
| assert!(app.input_leases.is_empty()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn copy_mode_press_moves_cursor_and_forwards_nothing() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.route_client_events( | ||
| vec![raw_key( | ||
| KeyCode::Down, | ||
| KeyModifiers::empty(), | ||
| KeyEventKind::Press, | ||
| )], | ||
| false, | ||
| ); | ||
|
|
||
| assert_eq!(app.state.mode, Mode::Copy); | ||
| assert_eq!( | ||
| app.state.copy_mode.as_ref().expect("copy mode").cursor_row, | ||
| 1 | ||
| ); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn copy_mode_repeat_after_press_moves_cursor_again() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.route_client_events( | ||
| vec![ | ||
| raw_key(KeyCode::Down, KeyModifiers::empty(), KeyEventKind::Press), | ||
| raw_key(KeyCode::Down, KeyModifiers::empty(), KeyEventKind::Repeat), | ||
| ], | ||
| false, | ||
| ); | ||
|
|
||
| assert_eq!(app.state.mode, Mode::Copy); | ||
| assert_eq!( | ||
| app.state.copy_mode.as_ref().expect("copy mode").cursor_row, | ||
| 2, | ||
| "repeat should move the copy-mode cursor a second time" | ||
| ); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn copy_mode_repeat_with_repeat_count_moves_cursor_by_count() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.route_client_events( | ||
| vec![ | ||
| raw_key(KeyCode::Down, KeyModifiers::empty(), KeyEventKind::Press), | ||
| crate::raw_input::RawInputEvent::Key( | ||
| crate::input::TerminalKey::new(KeyCode::Down, KeyModifiers::empty()) | ||
| .with_kind(KeyEventKind::Repeat) | ||
| .with_repeat_count(2), | ||
| ), | ||
| ], | ||
| false, | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| app.state.copy_mode.as_ref().expect("copy mode").cursor_row, | ||
| 3, | ||
| "a single repeat_count=2 event should move the cursor two more times" | ||
| ); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn copy_mode_esc_repeat_does_not_leak_after_exit() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.route_client_events( | ||
| vec![ | ||
| raw_key(KeyCode::Esc, KeyModifiers::empty(), KeyEventKind::Press), | ||
| raw_key(KeyCode::Esc, KeyModifiers::empty(), KeyEventKind::Repeat), | ||
| ], | ||
| false, | ||
| ); | ||
|
|
||
| assert_eq!(app.state.mode, Mode::Terminal); | ||
| assert!(app.state.copy_mode.is_none()); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| fn release_notes_state() -> state::ReleaseNotesState { | ||
| state::ReleaseNotesState { | ||
| version: "0.1.0".into(), | ||
|
|
@@ -2113,6 +2224,41 @@ mod tests { | |
| ) | ||
| } | ||
|
|
||
| /// Puts a fresh app into copy mode with a channel-backed pane runtime | ||
| /// showing several lines of text, and pins the copy-mode cursor to the | ||
| /// top row so vertical cursor movement is unambiguous. | ||
| fn app_with_copy_mode() -> ( | ||
| App, | ||
| crate::layout::PaneId, | ||
| tokio::sync::mpsc::Receiver<bytes::Bytes>, | ||
| ) { | ||
| let mut app = test_app(); | ||
| let mut ws = Workspace::test_new("test"); | ||
| let pane_id = ws.tabs[0].root_pane; | ||
| let pane_infos = ws.tabs[0].layout.panes(Rect::new(0, 0, 20, 5)); | ||
| let info = pane_infos[0].clone(); | ||
| let (runtime, rx) = TerminalRuntime::test_with_channel_and_scrollback_bytes( | ||
| info.inner_rect.width, | ||
| info.inner_rect.height, | ||
| 0, | ||
| b"line-0\r\nline-1\r\nline-2\r\nline-3\r\nline-4\r\n", | ||
| 8, | ||
| ); | ||
| ws.tabs[0].runtimes.insert(pane_id, runtime); | ||
| app.state.workspaces = vec![ws]; | ||
| app.state.active = Some(0); | ||
| app.state.selected = 0; | ||
| app.state.mode = Mode::Terminal; | ||
| app.state.view.pane_infos = pane_infos; | ||
| app.state.enter_copy_mode(&app.terminal_runtimes); | ||
| app.state | ||
| .copy_mode | ||
| .as_mut() | ||
| .expect("copy mode entered") | ||
| .cursor_row = 0; | ||
| (app, pane_id, rx) | ||
| } | ||
|
|
||
| fn unique_temp_path(name: &str) -> std::path::PathBuf { | ||
| let stamp = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
|
|
@@ -4051,6 +4197,54 @@ mod tests { | |
| assert!(next_press_handled); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn monolithic_copy_mode_repeat_after_press_moves_cursor_again() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.handle_raw_input_event(raw_key( | ||
| KeyCode::Down, | ||
| KeyModifiers::empty(), | ||
| KeyEventKind::Press, | ||
| )) | ||
| .await; | ||
| app.handle_raw_input_event(raw_key( | ||
| KeyCode::Down, | ||
| KeyModifiers::empty(), | ||
| KeyEventKind::Repeat, | ||
| )) | ||
| .await; | ||
|
|
||
| assert_eq!(app.state.mode, Mode::Copy); | ||
| assert_eq!( | ||
| app.state.copy_mode.as_ref().expect("copy mode").cursor_row, | ||
| 2, | ||
| "repeat should move the copy-mode cursor a second time" | ||
| ); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn monolithic_copy_mode_esc_repeat_does_not_leak_after_exit() { | ||
| let (mut app, _pane_id, mut rx) = app_with_copy_mode(); | ||
|
|
||
| app.handle_raw_input_event(raw_key( | ||
| KeyCode::Esc, | ||
| KeyModifiers::empty(), | ||
| KeyEventKind::Press, | ||
| )) | ||
| .await; | ||
| app.handle_raw_input_event(raw_key( | ||
| KeyCode::Esc, | ||
| KeyModifiers::empty(), | ||
| KeyEventKind::Repeat, | ||
| )) | ||
| .await; | ||
|
|
||
| assert_eq!(app.state.mode, Mode::Terminal); | ||
| assert!(app.state.copy_mode.is_none()); | ||
| assert!(rx.try_recv().is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn read_only_api_requests_do_not_force_rerender() { | ||
| let read_only = crate::api::schema::Request { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed TerminalInputContext to InputContext: the enum now has a NonTerminal variant, so the name "terminal input" no longer matches what it holds. TerminalInputTarget keeps its name because it is truly terminal-only. The rename is a mechanical replace, split into the first commit.