diff --git a/CHANGELOG.md b/CHANGELOG.md index a3ea3499..2744f184 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] - ReleaseDate +### Changed + +- "Edit Collection" action now uses the editor set in `$VISUAL`/`$EDITOR` instead of whatever editor you have set as default for `.yaml`/`.yml` files + - In most cases this means you'll now get `vim` instead of VSCode or another GUI editor + - Closing the editor will return you to Slumber, meaning you can stay in the terminal the entire time + ## [1.4.0] - 2024-06-11 ### Added diff --git a/Cargo.lock b/Cargo.lock index 3437fe1b..f34378de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -979,25 +979,6 @@ version = "2.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f518f335dce6725a761382244631d86cf0ccb2863413590b31338feb467f9c3" -[[package]] -name = "is-docker" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "928bae27f42bc99b60d9ac7334e3a21d10ad8f1835a4e12ec3ec0464765ed1b3" -dependencies = [ - "once_cell", -] - -[[package]] -name = "is-wsl" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "173609498df190136aa7dea1a91db051746d339e18476eed5ca40521f02d7aa5" -dependencies = [ - "is-docker", - "once_cell", -] - [[package]] name = "is_terminal_polyfill" version = "1.70.0" @@ -1319,17 +1300,6 @@ version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92" -[[package]] -name = "open" -version = "5.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2eb49fbd5616580e9974662cb96a3463da4476e649a7e4b258df0de065db0657" -dependencies = [ - "is-wsl", - "libc", - "pathdiff", -] - [[package]] name = "option-ext" version = "0.2.0" @@ -1381,12 +1351,6 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" -[[package]] -name = "pathdiff" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8835116a5c179084a830efb3adc117ab007512b535bc1a21c991d3b32a6b44dd" - [[package]] name = "percent-encoding" version = "2.3.1" @@ -2041,7 +2005,6 @@ dependencies = [ "mockito", "nom", "notify", - "open", "pretty_assertions", "ratatui", "regex", diff --git a/Cargo.toml b/Cargo.toml index 218ad263..bae918d9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,6 @@ itertools = "^0.12.0" mime = "^0.3.17" nom = "7.1.3" notify = {version = "^6.1.1", default-features = false, features = ["macos_fsevent"]} -open = "5.1.1" ratatui = {version = "^0.26.0", features = ["serde", "unstable-rendered-line-info"]} reqwest = {version = "^0.12.4", default-features = false, features = ["multipart", "rustls-tls"]} rmp-serde = "^1.1.2" diff --git a/src/template.rs b/src/template.rs index 2722e6a7..39a2666d 100644 --- a/src/template.rs +++ b/src/template.rs @@ -218,12 +218,13 @@ mod tests { assert_err, by_id, header_map, temp_dir, Factory, TempDir, TestPrompter, }, + tui::test_util::EnvGuard, }; use chrono::Utc; use indexmap::indexmap; use rstest::rstest; use serde_json::json; - use std::{env, time::Duration}; + use std::time::Duration; use tokio::fs; /// Test overriding all key types, as well as missing keys @@ -964,13 +965,22 @@ mod tests { ); } + #[rstest] + #[case::present(Some("test!"), "test!")] + #[case::missing(None, "")] #[tokio::test] - async fn test_environment_success() { + async fn test_environment_success( + #[case] env_value: Option<&str>, + #[case] expected: &str, + ) { let context = TemplateContext::factory(()); - env::set_var("TEST", "test!"); - assert_eq!(render!("{{env.TEST}}", context).unwrap(), "test!"); - // Unknown gets replaced with empty string - assert_eq!(render!("{{env.UNKNOWN}}", context).unwrap(), ""); + // This prevents tests from competing for environ environment variables, + // and isolates us from the external env + let result = { + let _guard = EnvGuard::lock([("TEST", env_value)]); + render!("{{env.TEST}}", context) + }; + assert_eq!(result.unwrap(), expected); } /// Test rendering non-UTF-8 data diff --git a/src/tui.rs b/src/tui.rs index 29d6bc37..531c4804 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -16,7 +16,7 @@ use crate::{ context::TuiContext, input::Action, message::{Message, MessageSender, RequestConfig}, - util::{save_file, signals}, + util::{get_editor_command, save_file, signals}, view::{ModalPriority, PreviewPrompter, RequestState, View}, }, util::{Replaceable, ResultExt}, @@ -24,10 +24,10 @@ use crate::{ use anyhow::{anyhow, Context}; use chrono::Utc; use crossterm::{ - event::{DisableMouseCapture, EnableMouseCapture}, + event::{DisableMouseCapture, EnableMouseCapture, EventStream}, terminal::{EnterAlternateScreen, LeaveAlternateScreen}, }; -use futures::Future; +use futures::{Future, StreamExt}; use notify::{event::ModifyKind, RecursiveMode, Watcher}; use ratatui::{prelude::CrosstermBackend, Terminal}; use std::{ @@ -38,6 +38,7 @@ use std::{ time::Duration, }; use tokio::{ + select, sync::mpsc::{self, UnboundedReceiver}, time, }; @@ -126,33 +127,43 @@ impl Tui { async fn run(mut self) -> anyhow::Result<()> { // Spawn background tasks self.listen_for_signals(); - tokio::spawn( - TuiContext::get() - .input_engine - .input_loop(self.messages_tx.clone()), - ); // Hang onto this because it stops running when dropped let _watcher = self.watch_collection()?; + let input_engine = &TuiContext::get().input_engine; + // Stream of terminal input events + let mut input_stream = EventStream::new(); + // This loop is limited by the rate that messages come in, with a // minimum rate enforced by a timeout while self.should_run { // ===== Draw Phase ===== // Draw *first* so initial UI state is rendered immediately - self.terminal.draw(|f| self.view.draw(f))?; + self.draw()?; // ===== Message Phase ===== // Grab one message out of the queue and handle it. This will block // while the queue is empty so we don't waste CPU cycles. The // timeout here makes sure we don't block forever, so things like // time displays during in-flight requests will update. - let future = - time::timeout(Self::TICK_TIME, self.messages_rx.recv()); - if let Ok(message) = future.await { - // Error would indicate a very weird and fatal bug so we wanna - // know about it - let message = - message.expect("Message channel dropped while running"); + let message = select! { + event_result = input_stream.next() => { + if let Some(event) = event_result { + let event = event.expect("Error reading terminal input"); + input_engine.event_to_message(event) + } else { + // We ran out of input, just end the program + break; + } + }, + message = self.messages_rx.recv() => { + // Error would indicate a very weird and fatal bug so we + // wanna know about it + Some(message.expect("Message channel dropped while running")) + }, + _ = time::sleep(Self::TICK_TIME) => None, + }; + if let Some(message) = message { trace!(?message, "Handling message"); // If an error occurs, store it so we can show the user if let Err(error) = self.handle_message(message) { @@ -181,12 +192,9 @@ impl Tui { }); } Message::CollectionEndReload(collection) => { - self.reload_collection(collection); - } - Message::CollectionEdit => { - let path = self.collection_file.path(); - open::that_detached(path).context("Error opening {path:?}")?; + self.reload_collection(collection) } + Message::CollectionEdit => self.edit_collection()?, Message::CopyRequestUrl(request_config) => { self.copy_request_url(request_config)?; @@ -330,6 +338,43 @@ impl Tui { self.should_run = false; } + /// Draw the view onto the screen + fn draw(&mut self) -> anyhow::Result<()> { + self.terminal.draw(|frame| self.view.draw(frame))?; + Ok(()) + } + + /// Open the collection file in the user's configured editor. **This will + /// block the main thread**, because we assume we're opening a terminal + /// editor and therefore should yield the terminal to the editor. + fn edit_collection(&mut self) -> anyhow::Result<()> { + let path = self.collection_file.path(); + let mut command = get_editor_command(path)?; + let error_context = + format!("Error spawning editor with command `{command:?}`"); + + // Block while the editor runs. Useful for terminal editors since + // they'll take over the whole screen. Potentially annoying for GUI + // editors that block, but we'll just hope the command is + // fire-and-forget. If this becomes an issue we can try to detect if the + // subprocess took over the terminal and cut it loose if not, or add a + // config field for it. + self.terminal.draw(|frame| { + frame.render_widget("Waiting for editor to close...", frame.size()); + })?; + command.status().context(error_context)?; + // If the editor was terminal-based it probably dropped us out of the + // alternate screen when it closed, so get back in there. This is + // idempotent so no harm in doing it + crossterm::execute!(io::stdout(), EnterAlternateScreen)?; + // Redraw immediately. The main loop will probably be in the tick + // timeout when we go back to it, so that adds a 250ms delay to + // redrawing the screen that we want to skip. + self.draw()?; + + Ok(()) + } + /// Render URL for a request, then copy it to the clipboard fn copy_request_url( &self, diff --git a/src/tui/input.rs b/src/tui/input.rs index 6f90c4f8..ee02f757 100644 --- a/src/tui/input.rs +++ b/src/tui/input.rs @@ -1,16 +1,12 @@ //! Logic related to input handling. This is considered part of the controller. -use crate::{ - tui::message::{Message, MessageSender}, - util::Mapping, -}; +use crate::{tui::message::Message, util::Mapping}; use anyhow::{anyhow, bail}; use crossterm::event::{ - Event, EventStream, KeyCode, KeyEvent, KeyEventKind, KeyModifiers, - MediaKeyCode, MouseButton, MouseEvent, MouseEventKind, + Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers, MediaKeyCode, + MouseButton, MouseEvent, MouseEventKind, }; use derive_more::Display; -use futures::StreamExt; use indexmap::{indexmap, IndexMap}; use itertools::Itertools; use serde::{Deserialize, Serialize}; @@ -113,18 +109,6 @@ impl InputEngine { new } - /// Listen for input from the terminal, and forward relevant events to the - /// message queue. This future will run indefinitely, so it should be - /// spawned in its own task. - pub async fn input_loop(&self, messages_tx: MessageSender) { - let mut stream = EventStream::new(); - while let Some(result) = stream.next().await { - // Failure to read input is both weird and fatal, so panic - let event = result.expect("Error reading terminal input"); - self.handle_event(&messages_tx, event); - } - } - /// Get a map of all available bindings pub fn bindings(&self) -> &IndexMap { &self.bindings @@ -183,11 +167,12 @@ impl InputEngine { action } - /// Given an input event, generate and queue a corresponding message. Some - /// events will *not* generate a message, because they shouldn't get - /// handled by components. This could be because they're just useless and - /// noisy, or because they actually cause bugs (e.g. double key presses). - fn handle_event(&self, messages_tx: &MessageSender, event: Event) { + /// Given an input event, generate a corresponding message with mapped + /// action. Some events will *not* generate a message, because they + /// shouldn't get handled by components. This could be because they're just + /// useless and noisy, or because they actually cause bugs (e.g. double key + /// presses). + pub fn event_to_message(&self, event: Event) -> Option { if !matches!( event, Event::FocusGained @@ -207,7 +192,9 @@ impl InputEngine { }) ) { let action = self.action(&event); - messages_tx.send(Message::Input { event, action }); + Some(Message::Input { event, action }) + } else { + None } } } @@ -558,10 +545,7 @@ fn stringify_key_modifier(modifier: KeyModifiers) -> Cow<'static, str> { #[cfg(test)] mod tests { use super::*; - use crate::{ - test_util::{assert_err, assert_matches}, - tui::test_util::{harness, TestHarness}, - }; + use crate::test_util::{assert_err, assert_matches}; use crossterm::event::{KeyEventState, MediaKeyCode}; use rstest::rstest; use serde_test::{assert_de_tokens, assert_de_tokens_error, Token}; @@ -586,7 +570,7 @@ mod tests { }) } - /// Test that each event queues a corresponding message + /// Test events that should be handled get a message generated #[rstest] #[case::key_down_mapped( key_event(KeyEventKind::Press, KeyCode::Enter), @@ -629,16 +613,14 @@ mod tests { Some(Action::ScrollRight) )] #[case::paste(Event::Paste("hello!".into()), None)] - fn test_handle_event_queued( - mut harness: TestHarness, + fn test_to_message_handled( #[case] event: Event, #[case] expected_action: Option, ) { let engine = InputEngine::new(IndexMap::default()); - engine.handle_event(harness.messages_tx(), event.clone()); let (queued_event, queued_action) = assert_matches!( - harness.pop_message_now(), - Message::Input { event, action } => (event, action), + engine.event_to_message(event.clone()), + Some(Message::Input { event, action }) => (event, action), ); assert_eq!(queued_event, event); assert_eq!(queued_action, expected_action); @@ -653,13 +635,9 @@ mod tests { #[case::mouse_down(mouse_event(MouseEventKind::Down(MouseButton::Left)))] #[case::mouse_drag(mouse_event(MouseEventKind::Drag(MouseButton::Left)))] #[case::mouse_move(mouse_event(MouseEventKind::Moved))] - fn test_handle_event_killed( - mut harness: TestHarness, - #[case] event: Event, - ) { + fn test_handle_event_killed(#[case] event: Event) { let engine = InputEngine::new(IndexMap::default()); - engine.handle_event(harness.messages_tx(), event); - harness.assert_messages_empty(); + assert_matches!(engine.event_to_message(event), None); } #[rstest] diff --git a/src/tui/test_util.rs b/src/tui/test_util.rs index b1522a16..40d67e86 100644 --- a/src/tui/test_util.rs +++ b/src/tui/test_util.rs @@ -11,6 +11,10 @@ use crate::{ }; use ratatui::{backend::TestBackend, Terminal}; use rstest::fixture; +use std::{ + env, + sync::{Mutex, MutexGuard}, +}; use tokio::sync::mpsc::{self, UnboundedReceiver}; /// Get a test harness, with a clean terminal etc. See [TestHarness]. @@ -60,16 +64,6 @@ impl TestHarness { &self.messages_tx } - /// Assert the message queue is empty. Requires `&mut self` because it will - /// actually pop a message off the queue - pub fn assert_messages_empty(&mut self) { - let message = self.messages_rx.try_recv().ok(); - assert!( - message.is_none(), - "Expected empty queue, but had message {message:?}" - ); - } - /// Pop the next message off the queue. Panic if the queue is empty pub fn pop_message_now(&mut self) -> Message { self.messages_rx.try_recv().expect("Message queue empty") @@ -86,6 +80,69 @@ impl TestHarness { } } +/// A guard used to indicate that the current process environment is locked. +/// This should be used in all tests that access environment variables, to +/// prevent interference from external variable settings or tests conflicting +/// with each other. +pub struct EnvGuard { + previous_values: Vec<(String, Option)>, + #[allow(unused)] + guard: MutexGuard<'static, ()>, +} + +impl EnvGuard { + /// Lock the environment and set each given variable to its corresponding + /// value. The returned guard will keep the environment locked so the + /// calling test has exclusive access to it. Upon being dropped, the old + /// environment values will be restored and then the environment will be + /// unlocked. + pub fn lock( + variables: impl IntoIterator< + Item = (impl Into, Option>), + >, + ) -> Self { + /// Global mutex for accessing environment variables. Technically we + /// could break this out into a map with one mutex per variable, but + /// that adds a ton of complexity for very little value. + static MUTEX: Mutex<()> = Mutex::new(()); + + let guard = MUTEX.lock().expect("Environment lock is poisoned"); + let previous_values = variables + .into_iter() + .map(|(variable, new_value)| { + let variable: String = variable.into(); + let previous_value = env::var(&variable).ok(); + + if let Some(value) = new_value { + env::set_var(&variable, value.into()); + } else { + env::remove_var(&variable); + } + + (variable, previous_value) + }) + .collect(); + + Self { + previous_values, + guard, + } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // Restore each env var + for (variable, value) in &self.previous_values { + if let Some(value) = value { + env::set_var(variable, value); + } else { + env::remove_var(variable); + } + } + } +} + /// Assert that the event queue matches the given list of patterns macro_rules! assert_events { ($($pattern:pat),* $(,)?) => { diff --git a/src/tui/util.rs b/src/tui/util.rs index 7a6839f2..a7a385aa 100644 --- a/src/tui/util.rs +++ b/src/tui/util.rs @@ -9,9 +9,9 @@ use crate::{ }, util::ResultExt, }; -use anyhow::Context; +use anyhow::{anyhow, Context}; use futures::{future, FutureExt}; -use std::io; +use std::{env, io, path::Path, process::Command}; use tokio::{fs::OpenOptions, io::AsyncWriteExt, sync::oneshot}; use tracing::{debug, info, warn}; @@ -135,6 +135,22 @@ pub async fn save_file( Ok(()) } +/// Get a command to open the given file in the user's configured editor. Return +/// an error if the user has no editor configured +pub fn get_editor_command(file: &Path) -> anyhow::Result { + let command = env::var("VISUAL").or(env::var("EDITOR")).map_err(|_| { + anyhow!( + "No editor configured. Please set the `VISUAL` or `EDITOR` \ + environment variable" + ) + })?; + let mut splits = command.split(' '); + let editor = splits.next().expect("`split` returns at least one value"); + let mut command = Command::new(editor); + command.args(splits).arg(file); + Ok(command) +} + /// Ask the user for some text input and wait for a response. Return `None` if /// the prompt is closed with no input. async fn prompt( @@ -169,12 +185,67 @@ async fn confirm(messages_tx: &MessageSender, message: impl ToString) -> bool { mod tests { use super::*; use crate::{ - test_util::{assert_matches, temp_dir, TempDir}, - tui::test_util::{harness, TestHarness}, + test_util::{assert_err, assert_matches, temp_dir, TempDir}, + tui::test_util::{harness, EnvGuard, TestHarness}, }; + use itertools::Itertools; use rstest::rstest; + use std::ffi::OsStr; use tokio::fs; + /// Test reading editor command from VISUAL/EDITOR env vars + #[rstest] + #[case::visual(Some("ted"), Some("fred"), "ted", &[])] + #[case::editor(None, Some("fred"), "fred", &[])] + #[case::with_args(None, Some("ned --wait 60s"), "ned", &["--wait", "60s"])] + // This case is actually a bug, but I don't think it's worth the effort of + // engineering around. I added this test case for completeness + #[case::with_args_quoted( + None, Some("ned '--wait 60s'"), "ned", &["'--wait", "60s'"], + )] + fn test_get_editor( + #[case] env_visual: Option<&str>, + #[case] env_editor: Option<&str>, + #[case] expected_program: &str, + #[case] expected_args: &[&str], + ) { + let file_name = "file.yml"; + // Make sure we're not competing with the other tests that want to set + // these env vars + let command = { + let _guard = EnvGuard::lock([ + ("VISUAL", env_visual), + ("EDITOR", env_editor), + ]); + get_editor_command(Path::new(file_name)) + } + .unwrap(); + let mut expected_args = expected_args.to_owned(); + expected_args.push(file_name); + assert_eq!(command.get_program(), expected_program); + assert_eq!( + command + .get_args() + .filter_map(OsStr::to_str) + .collect_vec() + .as_slice(), + expected_args + ); + } + + /// Test when VISUAL/EDITOR env vars are empty + #[test] + fn test_get_editor_error() { + // Make sure we're not competing with the other tests that want to set + // these env vars + let result = { + let _guard = + EnvGuard::lock([("VISUAL", None::), ("EDITOR", None)]); + get_editor_command(Path::new("file.yml")) + }; + assert_err!(result, "No editor configured"); + } + /// Test various cases of save_file #[rstest] #[case::new_file(false, false)]