Use terminal editor for Edit Collection action

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

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"

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

src/tui.rs

@@ -16,18 +16,18 @@ 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},
 };
 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,

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<Action, InputBinding> {
         &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<Message> {
         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<Action>,
     ) {
         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]