Only request keyboard protocols once we know if kitty kbd is supported

Today we might
1. enable modifyOtherKeys
2. get a reply indicating the kitty keyboard protocol is supported
3. because of 2, we never turn off modifyOtherKeys again

Let's get rid of this weird issue by enabling either modifyOtherKeys
or the kitty enhancements only after we know whether the kitty protocol
is supported.

This means we need to call terminal_protocols_enable_ifn() before every
call to readch() until the querying is done.  Fortunately, this is
already in place in read_normal_chars(); there are other places that
call readch() but none of those is executed until querying has completed.

src/builtins/fish_key_reader.rs

@@ -7,7 +7,7 @@
 //!
 //! Type "exit" or "quit" to terminate the program.
 
-use std::{io::Write, ops::ControlFlow, os::unix::prelude::OsStrExt};
+use std::{ops::ControlFlow, os::unix::prelude::OsStrExt};
 
 use libc::{STDIN_FILENO, TCSANOW, VEOF, VINTR};
 
@@ -19,13 +19,11 @@ use crate::{
     env::env_init,
     input::input_terminfo_get_name,
     input_common::{
-        enable_kitty_progressive_enhancements, kitty_progressive_enhancements_query,
+        kitty_progressive_enhancements_query, terminal_protocol_hacks,
-        terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, ImplicitEvent,
+        terminal_protocols_enable_ifn, CharEvent, InputEventQueue, InputEventQueuer,
-        InputEventQueue, InputEventQueuer, KITTY_KEYBOARD_SUPPORTED,
     },
     key::{char_to_symbol, Key},
     nix::isatty,
-    output::Outputter,
     panic::panic_handler,
     print_help::print_help,
     proc::set_interactive_session,
@@ -96,22 +94,12 @@ fn process_input(streams: &mut IoStreams, continuous_mode: bool, verbose: bool)
     let mut recent_chars2 = vec![];
     streams.err.appendln("Press a key:\n");
 
-    terminal_protocols_enable_ifn();
     while (!first_char_seen || continuous_mode) && !check_exit_loop_maybe_warning(None) {
+        terminal_protocols_enable_ifn();
         let evt = queue.readch();
 
-        let kevt = match evt {
+        let CharEvent::Key(kevt) = evt else {
-            CharEvent::Key(kevt) => kevt,
-            CharEvent::Readline(_) | CharEvent::Command(_) => continue,
-            CharEvent::Implicit(ImplicitEvent::PrimaryDeviceAttribute) => {
-                if KITTY_KEYBOARD_SUPPORTED.load() {
-                    enable_kitty_progressive_enhancements(
-                        Outputter::stdoutput().borrow_mut().by_ref(),
-                    );
-                }
             continue;
-            }
-            CharEvent::Implicit(_) => continue,
         };
         let c = kevt.key.codepoint;
         if verbose {

src/input_common.rs

@@ -2,7 +2,7 @@ use libc::STDOUT_FILENO;
 
 use crate::common::{
     fish_reserved_codepoint, is_windows_subsystem_for_linux, read_blocked, shell_modes,
-    str2wcstring, write_loop, WSL,
+    str2wcstring, write_loop, ScopeGuard, WSL,
 };
 use crate::env::{EnvStack, Environment};
 use crate::fd_readable_set::{FdReadableSet, Timeout};
@@ -24,7 +24,7 @@ use std::ops::ControlFlow;
 use std::os::fd::RawFd;
 use std::os::unix::ffi::OsStrExt;
 use std::ptr;
-use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering};
 
 // The range of key codes for inputrc-style keyboard functions.
 pub const R_END_INPUT_FUNCTIONS: usize = (ReadlineCmd::ReverseRepeatJump as usize) + 1;
@@ -444,23 +444,24 @@ pub fn update_wait_on_sequence_key_ms(vars: &EnvStack) {
 }
 
 static TERMINAL_PROTOCOLS: AtomicBool = AtomicBool::new(false);
+static BRACKETED_PASTE: AtomicBool = AtomicBool::new(false);
 
 pub(crate) static SCROLL_FORWARD_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 pub(crate) static CURSOR_UP_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 
-pub(crate) static KITTY_KEYBOARD_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
+#[repr(u8)]
+pub(crate) enum Capability {
+    Unknown,
+    Supported,
+    NotSupported,
+}
+pub(crate) static KITTY_KEYBOARD_SUPPORTED: AtomicU8 = AtomicU8::new(Capability::Unknown as _);
 
 pub(crate) static SYNCHRONIZED_OUTPUT_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 
 pub(crate) static CURSOR_POSITION_REPORTING_SUPPORTED: RelaxedAtomicBool =
     RelaxedAtomicBool::new(false);
 
-macro_rules! kitty_progressive_enhancements {
-    () => {
-        "\x1b[=5u"
-    };
-}
-
 pub fn kitty_progressive_enhancements_query() -> &'static [u8] {
     if std::env::var_os("TERM").is_some_and(|term| term.as_os_str().as_bytes() == b"st-256color") {
         return b"";
@@ -468,14 +469,6 @@ pub fn kitty_progressive_enhancements_query() -> &'static [u8] {
     b"\x1b[?u"
 }
 
-pub(crate) fn enable_kitty_progressive_enhancements(out: &mut impl std::io::Write) -> bool {
-    if IN_MIDNIGHT_COMMANDER_PRE_CSI_U.load() || IN_ITERM_PRE_CSI_U.load() {
-        return false;
-    }
-    let _ = out.write(kitty_progressive_enhancements!().as_bytes());
-    true
-}
-
 static IS_TMUX: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 pub static IN_MIDNIGHT_COMMANDER_PRE_CSI_U: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 static IN_ITERM_PRE_CSI_U: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
@@ -515,56 +508,72 @@ fn test_parse_version() {
 }
 
 pub fn terminal_protocols_enable_ifn() {
+    let did_write = RelaxedAtomicBool::new(false);
+    let _save_screen_state = ScopeGuard::new((), |()| {
+        if did_write.load() {
+            reader_current_data().map(|data| data.save_screen_state());
+        }
+    });
+    if !BRACKETED_PASTE.load(Ordering::Relaxed) {
+        BRACKETED_PASTE.store(true, Ordering::Release);
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[?2004h");
+        if IS_TMUX.load() {
+            let _ = write_loop(&STDOUT_FILENO, "\x1b[?1004h".as_bytes()); // focus reporting
+        }
+        did_write.store(true);
+    }
+    if IN_MIDNIGHT_COMMANDER_PRE_CSI_U.load() {
+        return;
+    }
+    let kitty_keyboard_supported = KITTY_KEYBOARD_SUPPORTED.load(Ordering::Relaxed);
+    if kitty_keyboard_supported == Capability::Unknown as _ {
+        return;
+    }
     if TERMINAL_PROTOCOLS.load(Ordering::Relaxed) {
         return;
     }
     TERMINAL_PROTOCOLS.store(true, Ordering::Release);
-    let sequences = if IN_MIDNIGHT_COMMANDER_PRE_CSI_U.load() {
+    FLOG!(term_protocols, "Enabling extended keys");
-        "\x1b[?2004h"
+    if kitty_keyboard_supported == Capability::NotSupported as _ || IN_ITERM_PRE_CSI_U.load() {
-    } else if !KITTY_KEYBOARD_SUPPORTED.load() || IN_ITERM_PRE_CSI_U.load() {
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[>4;1m"); // XTerm's modifyOtherKeys
-        concat!("\x1b[?2004h", "\x1b[>4;1m", "\x1b=",)
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b="); // set application keypad mode, so the keypad keys send unique codes
     } else {
-        concat!(
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[=5u"); // kitty progressive enhancements
-            "\x1b[?2004h", // Bracketed paste
-            // "\x1b[>4;1m",  // XTerm's modifyOtherKeys - no use in doing this with kitty protocol
-            kitty_progressive_enhancements!(),
-            "\x1b=", // set application keypad mode, so the keypad keys send unique codes
-        )
-    };
-    FLOG!(term_protocols, "Enabling extended keys and bracketed paste");
-    let _ = write_loop(&STDOUT_FILENO, sequences.as_bytes());
-    if IS_TMUX.load() {
-        let _ = write_loop(&STDOUT_FILENO, "\x1b[?1004h".as_bytes());
     }
-    reader_current_data().map(|data| data.save_screen_state());
+    did_write.store(true);
 }
 
 pub(crate) fn terminal_protocols_disable_ifn() {
-    if !TERMINAL_PROTOCOLS.load(Ordering::Acquire) {
+    let did_write = RelaxedAtomicBool::new(false);
-        return;
+    let _save_screen_state = is_main_thread().then(|| {
+        ScopeGuard::new((), |()| {
+            if did_write.load() {
+                reader_current_data().map(|data| data.save_screen_state());
             }
-    let sequences = if !KITTY_KEYBOARD_SUPPORTED.load() || IN_ITERM_PRE_CSI_U.load() {
+        })
-        concat!("\x1b[?2004l", "\x1b[>4;0m", "\x1b>",)
+    });
-    } else {
+    if BRACKETED_PASTE.load(Ordering::Acquire) {
-        concat!(
+        BRACKETED_PASTE.store(false, Ordering::Release);
-            "\x1b[?2004l", // Bracketed paste
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[?2004l");
-            // "\x1b[>4;0m",  // XTerm's modifyOtherKeys
-            "\x1b[=0u", // CSI u with kitty progressive enhancement
-            "\x1b>",    // application keypad mode
-        )
-    };
-    FLOG_SAFE!(
-        term_protocols,
-        "Disabling extended keys and bracketed paste"
-    );
-    let _ = write_loop(&STDOUT_FILENO, sequences.as_bytes());
         if IS_TMUX.load() {
             let _ = write_loop(&STDOUT_FILENO, "\x1b[?1004l".as_bytes());
         }
-    if is_main_thread() {
+        did_write.store(true);
-        reader_current_data().map(|data| data.save_screen_state());
+    }
+    if !TERMINAL_PROTOCOLS.load(Ordering::Acquire) {
+        return;
+    }
+    FLOG_SAFE!(term_protocols, "Disabling extended keys");
+    let kitty_keyboard_supported = KITTY_KEYBOARD_SUPPORTED.load(Ordering::Acquire);
+    assert_ne!(kitty_keyboard_supported, Capability::Unknown as _);
+    if kitty_keyboard_supported == Capability::NotSupported as _ || IN_ITERM_PRE_CSI_U.load() {
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[>4;0m"); // XTerm's modifyOtherKeys
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b>"); // application keypad mode
+    } else {
+        let _ = write_loop(&STDOUT_FILENO, b"\x1b[=0u"); // kitty progressive enhancements
     }
     TERMINAL_PROTOCOLS.store(false, Ordering::Release);
+    did_write.store(true);
 }
 
 fn parse_mask(mask: u32) -> Modifiers {
@@ -1160,7 +1169,7 @@ pub trait InputEventQueuer {
                         reader,
                         "Received kitty progressive enhancement flags, marking as supported"
                     );
-                    KITTY_KEYBOARD_SUPPORTED.store(true);
+                    KITTY_KEYBOARD_SUPPORTED.store(Capability::Supported as _, Ordering::Release);
                     return None;
                 }
 

src/reader.rs

@@ -81,9 +81,9 @@ use crate::history::{
     SearchType,
 };
 use crate::input::init_input;
-use crate::input_common::enable_kitty_progressive_enhancements;
 use crate::input_common::kitty_progressive_enhancements_query;
 use crate::input_common::BlockingWait;
+use crate::input_common::Capability;
 use crate::input_common::CursorPositionWait;
 use crate::input_common::ImplicitEvent;
 use crate::input_common::InputEventQueuer;
@@ -2506,22 +2506,18 @@ impl<'a> Reader<'a> {
                     match stage {
                         Queried::NotYet => panic!(),
                         Queried::Once => {
-                            let mut out = Outputter::stdoutput().borrow_mut();
+                            if KITTY_KEYBOARD_SUPPORTED.load(Ordering::Relaxed)
-                            out.begin_buffering();
+                                == Capability::Unknown as _
-                            let mut querying = false;
+                            {
-                            if KITTY_KEYBOARD_SUPPORTED.load() {
+                                KITTY_KEYBOARD_SUPPORTED
-                                enable_kitty_progressive_enhancements(out.by_ref());
+                                    .store(Capability::NotSupported as _, Ordering::Release);
-                                querying = true;
                             }
                             if SYNCHRONIZED_OUTPUT_SUPPORTED.load() {
+                                let mut out = Outputter::stdoutput().borrow_mut();
+                                out.begin_buffering();
                                 query_capabilities_via_dcs(out.by_ref());
-                                querying = true;
-                            }
-                            if querying {
                                 let _ = out.write(QUERY_PRIMARY_DEVICE_ATTRIBUTE);
-                            }
                                 out.end_buffering();
-                            if querying {
                                 self.save_screen_state();
                                 self.blocking_wait = Some(BlockingWait::Startup(Queried::Twice));
                                 return ControlFlow::Continue(());