From 829709c9c4dffd4359fd05fff90e2c616a398f25 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Fri, 25 Apr 2025 21:06:37 +0200 Subject: [PATCH] Replace synchronized update workaround Old versions of ConHost and Putty can't parse DCS sequences. For this reason, we briefly switch to the alternate screen buffer while sending DCS-format (e.g. XTGETTCAP) queries. For extra paranoia, we wrapped this procedure in a synchronized update. This doesn't seem to be needed; neither ConHost nor Putty show glitches when the synchronized update is omitted. As of today, every terminal that implements XTGETTCAP also implements synchronized updates but that might change. Let's remove it, to reduce surprise for users and terminal developers. As a bonus, this also fixes a glitch on Terminal.app which fails to parse the synchronized-update query (`printf '\x1b[?2026$p'`) and echoes the "p" (bug report ID FB17141059). Else we could work around this with another alternate screen buffer. Unfortunately, this change surfaces two issues with GNU screen. For one, they don't allow apps to use the alternate screen features (the user may allow it with "altscreen on"). Second, screen unconditionally echoes the payload of DCS commands. A possible fix has been suggested at https://lists.gnu.org/archive/html/screen-devel/2025-04/msg00010.html I think this combination of behaviors is unique among terminals. I'm sure there are more terminals that don't parse DCS commands yet, but I think almost all terminals implement alternate screen buffer. Probably only terminal multiplexers are prone to this issue. AFAICT very few multiplexers exists, so we can work around those until they are fixed. Disable XTGETTCAP queries for GNU screen specifically. Unfortunately screen does not implement XTVERSION, so I don't know how to reliably identify it. Instead, check STY and some commonly-used values TERM values. This has false negatives in some edge cases. But the worst thing that happens is that "+q696e646e" will be echoed once at startup, which is easy to ignore, or work around with something like function workaround --on-event fish_prompt commandline -f repaint functions --erase workaround end which I don't think we should apply by default, because it can mask other issues. We should give screen more time to respond. I guess I'll open an issue so we don't forget. In doubt, we can always go back to the previous approach (but implement it in fish script). Alternative workaround: instead of the alternative screen buffer, we could try something like clr_eol/clr_bol to erase the spuriously echoed text. I tried to do this in various ways but (surprisingly) failed. --- CHANGELOG.rst | 5 ++-- doc_src/terminal-compatibility.rst | 8 ------- src/input_common.rs | 14 +++-------- src/reader.rs | 38 +++++++++++------------------- src/terminal.rs | 9 ------- 5 files changed, 19 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b7a46ff77..958cd9140 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -55,9 +55,8 @@ New or improved bindings - :kbd:`ctrl-z` (undo) after executing a command will restore the previous cursor position instead of placing the cursor at the end of the command line. - The OSC 133 prompt marking feature has learned about kitty's ``click_events=1`` flag, which allows moving fish's cursor by clicking. - :kbd:`ctrl-l` now pushes all text located above the prompt to the terminal's scrollback, before clearing and redrawing the screen (via a new special input function ``scrollback-push``). - This feature depends on the terminal advertising via XTGETTCAP support for the ``indn`` terminfo capability, - and on the terminal supporting Synchronized Output (which is currently used by fish to detect features). - If any is missing, the binding falls back to ``clear-screen``. + For compatibility with terminals that do not provide the scroll-forward command, + this is only enabled by default if the terminal advertises support for the ``indn`` capability via XTGETTCAP. - Bindings using shift with non-ASCII letters (such as :kbd:`ctrl-shift-ä`) are now supported. If there is any modifier other than shift, this is the recommended notation (as opposed to :kbd:`ctrl-Ä`). diff --git a/doc_src/terminal-compatibility.rst b/doc_src/terminal-compatibility.rst index 630d5d944..c9113522e 100644 --- a/doc_src/terminal-compatibility.rst +++ b/doc_src/terminal-compatibility.rst @@ -234,14 +234,6 @@ Optional Commands - - Disable bracketed paste. - XTerm - * - ``\e[?2026h`` - - - - Begin synchronized output. - - contour - * - ``\e[?2026l`` - - - - End synchronized output. - - contour * - ``\e]0; Pt \x07`` - ts - Set window title (OSC 0). diff --git a/src/input_common.rs b/src/input_common.rs index 647430ec9..e51a1ca74 100644 --- a/src/input_common.rs +++ b/src/input_common.rs @@ -20,7 +20,7 @@ }; use crate::terminal::{ Capability, Output, Outputter, KITTY_KEYBOARD_SUPPORTED, SCROLL_FORWARD_SUPPORTED, - SCROLL_FORWARD_TERMINFO_CODE, SYNCHRONIZED_OUTPUT_SUPPORTED, + SCROLL_FORWARD_TERMINFO_CODE, }; use crate::threads::{iothread_port, is_main_thread}; use crate::universal_notifier::default_notifier; @@ -763,8 +763,7 @@ pub enum CursorPositionWait { #[derive(Eq, PartialEq)] pub enum Queried { NotYet, - Once, - Twice, + Yes, } #[derive(Eq, PartialEq)] @@ -1078,14 +1077,7 @@ fn parse_csi(&mut self, buffer: &mut Vec) -> Option { let key = match c { b'$' => { if next_char(self) == b'y' { - if private_mode == Some(b'?') { - // DECRPM - if params[0][0] == 2026 && matches!(params[1][0], 1 | 2) { - FLOG!(reader, "Synchronized output is supported"); - SYNCHRONIZED_OUTPUT_SUPPORTED.store(true); - } - } - // DECRQM + // DECRPM/DECRQM return None; } match params[0][0] { diff --git a/src/reader.rs b/src/reader.rs index fcccdaf18..6e6c33dcb 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -128,18 +128,15 @@ use crate::terminal::Outputter; use crate::terminal::TerminalCommand::DecrstAlternateScreenBuffer; use crate::terminal::TerminalCommand::DecrstMouseTracking; -use crate::terminal::TerminalCommand::DecrstSynchronizedUpdate; use crate::terminal::TerminalCommand::DecsetAlternateScreenBuffer; use crate::terminal::TerminalCommand::DecsetShowCursor; -use crate::terminal::TerminalCommand::DecsetSynchronizedUpdate; use crate::terminal::TerminalCommand::QueryCursorPosition; use crate::terminal::TerminalCommand::{ ClearScreen, Osc0WindowTitle, Osc133CommandStart, QueryKittyKeyboardProgressiveEnhancements, - QueryPrimaryDeviceAttribute, QuerySynchronizedOutput, QueryXtgettcap, QueryXtversion, + QueryPrimaryDeviceAttribute, QueryXtgettcap, QueryXtversion, }; use crate::terminal::{ Capability, KITTY_KEYBOARD_SUPPORTED, SCROLL_FORWARD_SUPPORTED, SCROLL_FORWARD_TERMINFO_CODE, - SYNCHRONIZED_OUTPUT_SUPPORTED, }; use crate::termsize::{termsize_invalidate_tty, termsize_last, termsize_update}; use crate::text_face::parse_text_face; @@ -2198,16 +2195,15 @@ fn readline(&mut self, nchars: Option) -> Option { if is_dumb() || IN_MIDNIGHT_COMMANDER.load() || IN_DVTM.load() { *self.blocking_wait() = None; } else { - *self.blocking_wait() = Some(BlockingWait::Startup(Queried::Once)); + *self.blocking_wait() = Some(BlockingWait::Startup(Queried::Yes)); let mut out = Outputter::stdoutput().borrow_mut(); out.begin_buffering(); // Query for kitty keyboard protocol support. out.write_command(QueryKittyKeyboardProgressiveEnhancements); // Query for cursor position reporting support. self.request_cursor_position(&mut out, None); - // Query for synchronized output support. - out.write_command(QuerySynchronizedOutput); out.write_command(QueryXtversion); + query_capabilities_via_dcs(out.by_ref(), self.parser.vars()); out.write_command(QueryPrimaryDeviceAttribute); out.end_buffering(); } @@ -2529,7 +2525,7 @@ fn handle_char_event(&mut self, injected_event: Option) -> ControlFlo self.save_screen_state(); } ImplicitEvent::PrimaryDeviceAttribute => { - let mut wait_guard = self.blocking_wait(); + let wait_guard = self.blocking_wait(); let Some(wait) = &*wait_guard else { // Rogue reply. return ControlFlow::Continue(()); @@ -2540,26 +2536,14 @@ fn handle_char_event(&mut self, injected_event: Option) -> ControlFlo }; match stage { Queried::NotYet => panic!(), - Queried::Once => { + Queried::Yes => { if KITTY_KEYBOARD_SUPPORTED.load(Ordering::Relaxed) == Capability::Unknown as _ { KITTY_KEYBOARD_SUPPORTED .store(Capability::NotSupported as _, Ordering::Release); } - if SYNCHRONIZED_OUTPUT_SUPPORTED.load() { - let mut out = Outputter::stdoutput().borrow_mut(); - out.begin_buffering(); - query_capabilities_via_dcs(out.by_ref()); - out.write_command(QueryPrimaryDeviceAttribute); - out.end_buffering(); - *wait_guard = Some(BlockingWait::Startup(Queried::Twice)); - drop(wait_guard); - self.save_screen_state(); - return ControlFlow::Continue(()); - } } - Queried::Twice => (), } unblock_input(wait_guard); } @@ -2589,12 +2573,18 @@ fn send_xtgettcap_query(out: &mut impl Output, cap: &'static str) { out.write_command(QueryXtgettcap(cap)); } -fn query_capabilities_via_dcs(out: &mut impl Output) { - out.write_command(DecsetSynchronizedUpdate); // begin synchronized update +fn query_capabilities_via_dcs(out: &mut impl Output, vars: &dyn Environment) { + if vars.get_unless_empty(L!("STY")).is_some() + || vars.get_unless_empty(L!("TERM")).is_some_and(|term| { + let term = &term.as_list()[0]; + term == "screen" || term == "screen-256color" + }) + { + return; + } out.write_command(DecsetAlternateScreenBuffer); // enable alternative screen buffer send_xtgettcap_query(out, SCROLL_FORWARD_TERMINFO_CODE); out.write_command(DecrstAlternateScreenBuffer); // disable alternative screen buffer - out.write_command(DecrstSynchronizedUpdate); // end synchronized update } impl<'a> Reader<'a> { diff --git a/src/terminal.rs b/src/terminal.rs index 744e3bb86..3ac2c9d09 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -87,10 +87,6 @@ pub(crate) enum TerminalCommand<'a> { DecsetAlternateScreenBuffer, DecrstAlternateScreenBuffer, - DecsetSynchronizedUpdate, - DecrstSynchronizedUpdate, - - QuerySynchronizedOutput, // Keyboard protocols KittyKeyboardProgressiveEnhancementsEnable, @@ -176,9 +172,6 @@ fn write(out: &mut impl Output, sequence: &'static [u8]) -> bool { QueryXtgettcap(cap) => query_xtgettcap(self, cap), DecsetAlternateScreenBuffer => write(self, b"\x1b[?1049h"), DecrstAlternateScreenBuffer => write(self, b"\x1b[?1049l"), - DecsetSynchronizedUpdate => write(self, b"\x1b[?2026h"), - DecrstSynchronizedUpdate => write(self, b"\x1b[?2026l"), - QuerySynchronizedOutput => write(self, b"\x1b[?2026$p"), KittyKeyboardProgressiveEnhancementsEnable => write(self, b"\x1b[=5u"), KittyKeyboardProgressiveEnhancementsDisable => write(self, b"\x1b[=0u"), QueryKittyKeyboardProgressiveEnhancements => query_kitty_progressive_enhancements(self), @@ -237,8 +230,6 @@ pub(crate) enum Capability { pub(crate) static SCROLL_FORWARD_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false); pub(crate) static SCROLL_FORWARD_TERMINFO_CODE: &str = "indn"; -pub(crate) static SYNCHRONIZED_OUTPUT_SUPPORTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false); - pub(crate) fn use_terminfo() -> bool { !future_feature_flags::test(FeatureFlag::ignore_terminfo) && TERM.lock().unwrap().is_some() }