fix(rendering): occasional glitches while resizing (zellij-org#2621)

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<ServerOsInputOutput, nix::Error> {
     })
 }
 
+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 {

zellij-server/src/pty_writer.rs

@@ -2,16 +2,26 @@ 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<u8>, u32),
+    ResizePty(u32, u16, u16, Option<u16>, Option<u16>), // terminal_id, columns, rows, pixel width, pixel height
+    StartCachingResizes,
+    ApplyCachedResizes,
     Exit,
 }
 
 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<PtyWriteInstruction>) -> 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<PtyWriteInstruction>) -> 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(());
             },

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) => {

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,33 +770,31 @@ impl Tab {
         Ok(())
     }
     pub fn previous_swap_layout(&mut self, client_id: Option<ClientId>) -> 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(
         &mut self,
         client_id: Option<ClientId>,
         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<()> {

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,
+                        _ => {},
                     }
                 }
             })

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]

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]

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]

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,
 }