From c339a81644fd7d9ced9d06902876a02c9f6058ae Mon Sep 17 00:00:00 2001 From: Aram Drevekenin Date: Wed, 12 Jul 2023 20:30:41 +0200 Subject: [PATCH] fix(rendering): occasional glitches while resizing (#2621) --- zellij-server/src/os_input_output.rs | 29 +++++++++ zellij-server/src/pty_writer.rs | 44 +++++++++++++- zellij-server/src/screen.rs | 5 ++ zellij-server/src/tab/mod.rs | 59 +++++++++++-------- .../src/tab/unit/tab_integration_tests.rs | 1 + ...end_cli_toggle_active_tab_sync_action.snap | 4 +- ...ests__send_cli_write_action_to_screen.snap | 4 +- ...send_cli_write_chars_action_to_screen.snap | 4 +- zellij-utils/src/errors.rs | 3 + 9 files changed, 120 insertions(+), 33 deletions(-) diff --git a/zellij-server/src/os_input_output.rs b/zellij-server/src/os_input_output.rs index 18e8f92db9..f4cbf9b4a3 100644 --- a/zellij-server/src/os_input_output.rs +++ b/zellij-server/src/os_input_output.rs @@ -10,6 +10,7 @@ use nix::{ }, unistd, }; + use signal_hook::consts::*; use sysinfo::{ProcessExt, ProcessRefreshKind, System, SystemExt}; use zellij_utils::{ @@ -840,6 +841,34 @@ pub fn get_server_os_input() -> Result { }) } +use crate::pty_writer::PtyWriteInstruction; +use crate::thread_bus::ThreadSenders; + +pub struct ResizeCache { + senders: ThreadSenders, +} + +impl ResizeCache { + pub fn new(senders: ThreadSenders) -> Self { + senders + .send_to_pty_writer(PtyWriteInstruction::StartCachingResizes) + .unwrap_or_else(|e| { + log::error!("Failed to cache resizes: {}", e); + }); + ResizeCache { senders } + } +} + +impl Drop for ResizeCache { + fn drop(&mut self) { + self.senders + .send_to_pty_writer(PtyWriteInstruction::ApplyCachedResizes) + .unwrap_or_else(|e| { + log::error!("Failed to apply cached resizes: {}", e); + }); + } +} + /// Process id's for forked terminals #[derive(Debug)] pub struct ChildId { diff --git a/zellij-server/src/pty_writer.rs b/zellij-server/src/pty_writer.rs index 9c3b6d49ae..017fed39af 100644 --- a/zellij-server/src/pty_writer.rs +++ b/zellij-server/src/pty_writer.rs @@ -2,9 +2,16 @@ use zellij_utils::errors::{prelude::*, ContextType, PtyWriteContext}; use crate::thread_bus::Bus; +// we separate these instruction to a different thread because some programs get deadlocked if +// you write into their STDIN while reading from their STDOUT (I'm looking at you, vim) +// while the same has not been observed to happen with resizes, it could conceivably happen and we have this +// here anyway, so #[derive(Debug, Clone, Eq, PartialEq)] pub enum PtyWriteInstruction { Write(Vec, u32), + ResizePty(u32, u16, u16, Option, Option), // terminal_id, columns, rows, pixel width, pixel height + StartCachingResizes, + ApplyCachedResizes, Exit, } @@ -12,6 +19,9 @@ impl From<&PtyWriteInstruction> for PtyWriteContext { fn from(tty_write_instruction: &PtyWriteInstruction) -> Self { match *tty_write_instruction { PtyWriteInstruction::Write(..) => PtyWriteContext::Write, + PtyWriteInstruction::ResizePty(..) => PtyWriteContext::ResizePty, + PtyWriteInstruction::ApplyCachedResizes => PtyWriteContext::ApplyCachedResizes, + PtyWriteInstruction::StartCachingResizes => PtyWriteContext::StartCachingResizes, PtyWriteInstruction::Exit => PtyWriteContext::Exit, } } @@ -23,7 +33,7 @@ pub(crate) fn pty_writer_main(bus: Bus) -> Result<()> { loop { let (event, mut err_ctx) = bus.recv().with_context(err_context)?; err_ctx.add_call(ContextType::PtyWrite((&event).into())); - let os_input = bus + let mut os_input = bus .os_input .clone() .context("no OS input API found") @@ -39,6 +49,38 @@ pub(crate) fn pty_writer_main(bus: Bus) -> Result<()> { .with_context(err_context) .non_fatal(); }, + PtyWriteInstruction::ResizePty( + terminal_id, + columns, + rows, + width_in_pixels, + height_in_pixels, + ) => { + os_input + .set_terminal_size_using_terminal_id( + terminal_id, + columns, + rows, + width_in_pixels, + height_in_pixels, + ) + .with_context(err_context) + .non_fatal(); + }, + PtyWriteInstruction::StartCachingResizes => { + // we do this because there are some logic traps inside the screen/tab/layout code + // the cause multiple resizes to be sent to the pty - while the last one is always + // the correct one, many programs and shells debounce those (I guess due to the + // trauma of dealing with GUI resizes of the controlling terminal window), and this + // then causes glitches and missing redraws + // so we do this to play nice and always only send the last resize instruction to + // each pane + // the logic for this happens in the main Screen event loop + os_input.cache_resizes(); + }, + PtyWriteInstruction::ApplyCachedResizes => { + os_input.apply_cached_resizes(); + }, PtyWriteInstruction::Exit => { return Ok(()); }, diff --git a/zellij-server/src/screen.rs b/zellij-server/src/screen.rs index c7a5dd89f7..61d2b391fa 100644 --- a/zellij-server/src/screen.rs +++ b/zellij-server/src/screen.rs @@ -20,6 +20,7 @@ use zellij_utils::{ position::Position, }; +use crate::os_input_output::ResizeCache; use crate::panes::alacritty_functions::xparse_color; use crate::panes::terminal_character::AnsiCode; @@ -1590,6 +1591,7 @@ pub(crate) fn screen_thread_main( config_options.copy_on_select.unwrap_or(true), ); + let thread_senders = bus.senders.clone(); let mut screen = Screen::new( bus, &client_attributes, @@ -1618,6 +1620,9 @@ pub(crate) fn screen_thread_main( .recv() .context("failed to receive event on channel")?; err_ctx.add_call(ContextType::Screen((&event).into())); + // here we start caching resizes, so that we'll send them in bulk at the end of each event + // when this cache is Dropped, for more information, see the comments in PtyWriter + let _resize_cache = ResizeCache::new(thread_senders.clone()); match event { ScreenInstruction::PtyBytes(pid, vte_bytes) => { diff --git a/zellij-server/src/tab/mod.rs b/zellij-server/src/tab/mod.rs index 2f15c680df..8e1f548248 100644 --- a/zellij-server/src/tab/mod.rs +++ b/zellij-server/src/tab/mod.rs @@ -59,13 +59,17 @@ use zellij_utils::{ macro_rules! resize_pty { ($pane:expr, $os_input:expr, $senders:expr) => {{ match $pane.pid() { - PaneId::Terminal(ref pid) => $os_input.set_terminal_size_using_terminal_id( - *pid, - $pane.get_content_columns() as u16, - $pane.get_content_rows() as u16, - None, - None, - ), + PaneId::Terminal(ref pid) => { + $senders + .send_to_pty_writer(PtyWriteInstruction::ResizePty( + *pid, + $pane.get_content_columns() as u16, + $pane.get_content_rows() as u16, + None, + None, + )) + .with_context(err_context); + }, PaneId::Plugin(ref pid) => { let err_context = || format!("failed to resize plugin {pid}"); $senders @@ -93,13 +97,19 @@ macro_rules! resize_pty { } }; match $pane.pid() { - PaneId::Terminal(ref pid) => $os_input.set_terminal_size_using_terminal_id( - *pid, - $pane.get_content_columns() as u16, - $pane.get_content_rows() as u16, - width_in_pixels, - height_in_pixels, - ), + PaneId::Terminal(ref pid) => { + use crate::PtyWriteInstruction; + let err_context = || format!("Failed to send resize pty instruction"); + $senders + .send_to_pty_writer(PtyWriteInstruction::ResizePty( + *pid, + $pane.get_content_columns() as u16, + $pane.get_content_rows() as u16, + width_in_pixels, + height_in_pixels, + )) + .with_context(err_context) + }, PaneId::Plugin(ref pid) => { let err_context = || format!("failed to resize plugin {pid}"); $senders @@ -760,16 +770,15 @@ impl Tab { Ok(()) } pub fn previous_swap_layout(&mut self, client_id: Option) -> Result<()> { - // warning, here we cache resizes rather than sending them to the pty, we do that in - // apply_cached_resizes below - beware when bailing on this function early! - self.os_api.cache_resizes(); let search_backwards = true; if self.floating_panes.panes_are_visible() { self.relayout_floating_panes(client_id, search_backwards, true)?; } else { self.relayout_tiled_panes(client_id, search_backwards, true, false)?; } - self.os_api.apply_cached_resizes(); + self.senders + .send_to_pty_writer(PtyWriteInstruction::ApplyCachedResizes) + .with_context(|| format!("failed to update plugins with mode info"))?; Ok(()) } pub fn next_swap_layout( @@ -777,16 +786,15 @@ impl Tab { client_id: Option, refocus_pane: bool, ) -> Result<()> { - // warning, here we cache resizes rather than sending them to the pty, we do that in - // apply_cached_resizes below - beware when bailing on this function early! - self.os_api.cache_resizes(); let search_backwards = false; if self.floating_panes.panes_are_visible() { self.relayout_floating_panes(client_id, search_backwards, refocus_pane)?; } else { self.relayout_tiled_panes(client_id, search_backwards, refocus_pane, false)?; } - self.os_api.apply_cached_resizes(); + self.senders + .send_to_pty_writer(PtyWriteInstruction::ApplyCachedResizes) + .with_context(|| format!("failed to update plugins with mode info"))?; Ok(()) } pub fn apply_buffered_instructions(&mut self) -> Result<()> { @@ -1826,9 +1834,6 @@ impl Tab { selectable_tiled_panes.count() > 0 } pub fn resize_whole_tab(&mut self, new_screen_size: Size) -> Result<()> { - // warning, here we cache resizes rather than sending them to the pty, we do that in - // apply_cached_resizes below - beware when bailing on this function early! - self.os_api.cache_resizes(); let err_context = || format!("failed to resize whole tab (index {})", self.index); self.floating_panes.resize(new_screen_size); // we need to do this explicitly because floating_panes.resize does not do this @@ -1848,7 +1853,9 @@ impl Tab { let _ = self.relayout_tiled_panes(None, false, false, true); } self.should_clear_display_before_rendering = true; - let _ = self.os_api.apply_cached_resizes(); + self.senders + .send_to_pty_writer(PtyWriteInstruction::ApplyCachedResizes) + .with_context(|| format!("failed to update plugins with mode info"))?; Ok(()) } pub fn resize(&mut self, client_id: ClientId, strategy: ResizeStrategy) -> Result<()> { diff --git a/zellij-server/src/tab/unit/tab_integration_tests.rs b/zellij-server/src/tab/unit/tab_integration_tests.rs index c870c3a0b7..5a04d9c145 100644 --- a/zellij-server/src/tab/unit/tab_integration_tests.rs +++ b/zellij-server/src/tab/unit/tab_integration_tests.rs @@ -175,6 +175,7 @@ impl MockPtyInstructionBus { .unwrap() .push(String::from_utf8_lossy(&msg).to_string()), PtyWriteInstruction::Exit => break, + _ => {}, } } }) diff --git a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_toggle_active_tab_sync_action.snap b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_toggle_active_tab_sync_action.snap index 8597c63091..1c2440eb85 100644 --- a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_toggle_active_tab_sync_action.snap +++ b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_toggle_active_tab_sync_action.snap @@ -1,6 +1,6 @@ --- source: zellij-server/src/./unit/screen_tests.rs -assertion_line: 1487 +assertion_line: 1825 expression: "format!(\"{:?}\", * received_pty_instructions.lock().unwrap())" --- -[Write([102, 111, 111], 0), Write([102, 111, 111], 1), Exit] +[StartCachingResizes, ApplyCachedResizes, StartCachingResizes, ResizePty(0, 59, 18, None, None), ResizePty(1, 58, 18, None, None), ResizePty(0, 59, 18, None, None), ResizePty(1, 58, 18, None, None), ResizePty(0, 59, 18, None, None), ResizePty(1, 58, 18, None, None), ApplyCachedResizes, StartCachingResizes, ApplyCachedResizes, StartCachingResizes, ApplyCachedResizes, StartCachingResizes, Write([102, 111, 111], 0), Write([102, 111, 111], 1), ApplyCachedResizes, Exit] diff --git a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_action_to_screen.snap b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_action_to_screen.snap index 32ae2a500d..aad2ad25c5 100644 --- a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_action_to_screen.snap +++ b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_action_to_screen.snap @@ -1,6 +1,6 @@ --- source: zellij-server/src/./unit/screen_tests.rs -assertion_line: 879 +assertion_line: 1065 expression: "format!(\"{:?}\", * received_pty_instructions.lock().unwrap())" --- -[Write([102, 111, 111], 0), Exit] +[StartCachingResizes, ApplyCachedResizes, StartCachingResizes, ResizePty(0, 119, 18, None, None), ResizePty(0, 119, 18, None, None), ResizePty(0, 119, 18, None, None), ApplyCachedResizes, StartCachingResizes, ApplyCachedResizes, StartCachingResizes, Write([102, 111, 111], 0), ApplyCachedResizes, Exit] diff --git a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_chars_action_to_screen.snap b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_chars_action_to_screen.snap index 90ade0ee52..ca6c9635e5 100644 --- a/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_chars_action_to_screen.snap +++ b/zellij-server/src/unit/snapshots/zellij_server__screen__screen_tests__send_cli_write_chars_action_to_screen.snap @@ -1,6 +1,6 @@ --- source: zellij-server/src/./unit/screen_tests.rs -assertion_line: 846 +assertion_line: 1039 expression: "format!(\"{:?}\", * received_pty_instructions.lock().unwrap())" --- -[Write([105, 110, 112, 117, 116, 32, 102, 114, 111, 109, 32, 116, 104, 101, 32, 99, 108, 105], 0), Exit] +[StartCachingResizes, ApplyCachedResizes, StartCachingResizes, ResizePty(0, 119, 18, None, None), ResizePty(0, 119, 18, None, None), ResizePty(0, 119, 18, None, None), ApplyCachedResizes, StartCachingResizes, ApplyCachedResizes, StartCachingResizes, Write([105, 110, 112, 117, 116, 32, 102, 114, 111, 109, 32, 116, 104, 101, 32, 99, 108, 105], 0), ApplyCachedResizes, Exit] diff --git a/zellij-utils/src/errors.rs b/zellij-utils/src/errors.rs index be857bf2f6..6c4a60dc67 100644 --- a/zellij-utils/src/errors.rs +++ b/zellij-utils/src/errors.rs @@ -415,6 +415,9 @@ pub enum ServerContext { #[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)] pub enum PtyWriteContext { Write, + ResizePty, + StartCachingResizes, + ApplyCachedResizes, Exit, }