From 453142775c96752730d7caf85532e616e2aaa719 Mon Sep 17 00:00:00 2001 From: har7an <99636919+har7an@users.noreply.github.com> Date: Tue, 8 Nov 2022 10:56:23 +0000 Subject: [PATCH] errors: Remove `log::error` in server (#1881) * server/wasm_vm: Replace `log::error!` with better error logging by means of `non_fatal`. This preserves the original error and allows adding context information on top. Also makes error formatting more uniform across the application. * server/tab: Replace `log::error!` with better error logging by means of `non_fatal`. This preserves the original error and allows adding context information on top. Also makes error formatting more uniform across the application. * server/route: Replace `log::error!` and propagate the error to the caller instead. * server/pty: Replace `log::error!` with better error logging by means of `non_fatal`. This preserves the original error and allows adding context information on top. Also makes error formatting more uniform across the application. Also add per-instruction error context to make it clear what we tried to accomplish when an error occured. * server/panes/tiled_panes: Merge dependencies and sort them into a better order. * server/panes/tiled_panes: Replace `log::error!` with better error logging by means of `non_fatal`. This preserves the original error and allows adding context information on top. Also makes error formatting more uniform across the application. * server/os_input_output: Merge depndencies and sort them into a better order. * server/logging_pipe: Replace `log::error!` with better error logging by means of `non_fatal`. This preserves the original error and allows adding context information on top. Also makes error formatting more uniform across the application. * server/os_io: Remove uses of `log::error` * changelog: Add PR #1881 * server/os_io: Gracefully handle failing resize for terminals IDs that don't exist, instead of propagating the error to the user. * server/lib: Remove leftover log message * server/pty: Log error cause rather than providing a hard-coded error reason which is plain wrong in this context. * server/screen: Remove calls to `log::error!` and change `get_active_tab(_mut)?` to return a `Result` instead of an `Option`. This already makes many places in the code obsolete where previously "failed to get active tab..." was logged manually. Rather than logging, use the `anyhow::Error`s we have, along with all their context information, and log these instead. --- CHANGELOG.md | 1 + zellij-server/src/lib.rs | 5 - zellij-server/src/logging_pipe.rs | 37 +-- zellij-server/src/os_input_output.rs | 82 ++--- zellij-server/src/panes/tiled_panes/mod.rs | 56 ++-- zellij-server/src/pty.rs | 53 ++-- zellij-server/src/route.rs | 5 +- zellij-server/src/screen.rs | 336 ++++++++++++--------- zellij-server/src/tab/mod.rs | 17 +- zellij-server/src/wasm_vm.rs | 4 +- 10 files changed, 343 insertions(+), 253 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cede18f11..bed8647c76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) * fix: allow cli actions to be run outside of a tty environment (https://github.com/zellij-org/zellij/pull/1905) * Terminal compatibility: send focus in/out events to terminal panes (https://github.com/zellij-org/zellij/pull/1908) * fix: various bugs with no-frames and floating panes (https://github.com/zellij-org/zellij/pull/1909) +* debugging: Improve error logging in server (https://github.com/zellij-org/zellij/pull/1881) ## [0.32.0] - 2022-10-25 diff --git a/zellij-server/src/lib.rs b/zellij-server/src/lib.rs index dc28a9aea1..07e8d0515f 100644 --- a/zellij-server/src/lib.rs +++ b/zellij-server/src/lib.rs @@ -573,11 +573,6 @@ pub fn start_server(mut os_input: Box, socket_path: PathBuf) { }, ServerInstruction::ActiveClients(client_id) => { let client_ids = session_state.read().unwrap().client_ids(); - log::error!( - "Sending client_ids {:?} to client {}", - client_ids, - client_id - ); send_to_client!( client_id, os_input, diff --git a/zellij-server/src/logging_pipe.rs b/zellij-server/src/logging_pipe.rs index 085a672fd8..d54de9b095 100644 --- a/zellij-server/src/logging_pipe.rs +++ b/zellij-server/src/logging_pipe.rs @@ -5,7 +5,7 @@ use std::{ use log::{debug, error}; use wasmer_wasi::{WasiFile, WasiFsError}; -use zellij_utils::serde; +use zellij_utils::{errors::prelude::*, serde}; use chrono::prelude::*; use serde::{Deserialize, Serialize}; @@ -75,27 +75,30 @@ impl Write for LoggingPipe { fn flush(&mut self) -> std::io::Result<()> { self.buffer.make_contiguous(); - if let Ok(converted_buffer) = std::str::from_utf8(self.buffer.as_slices().0) { - if converted_buffer.contains('\n') { - let mut consumed_bytes = 0; - let mut split_converted_buffer = converted_buffer.split('\n').peekable(); - - while let Some(msg) = split_converted_buffer.next() { - if split_converted_buffer.peek().is_none() { - // Log last chunk iff the last char is endline. Otherwise do not do it. - if converted_buffer.ends_with('\n') && !msg.is_empty() { + match std::str::from_utf8(self.buffer.as_slices().0) { + Ok(converted_buffer) => { + if converted_buffer.contains('\n') { + let mut consumed_bytes = 0; + let mut split_converted_buffer = converted_buffer.split('\n').peekable(); + + while let Some(msg) = split_converted_buffer.next() { + if split_converted_buffer.peek().is_none() { + // Log last chunk iff the last char is endline. Otherwise do not do it. + if converted_buffer.ends_with('\n') && !msg.is_empty() { + self.log_message(msg); + consumed_bytes += msg.len() + 1; + } + } else { self.log_message(msg); consumed_bytes += msg.len() + 1; } - } else { - self.log_message(msg); - consumed_bytes += msg.len() + 1; } + drop(self.buffer.drain(..consumed_bytes)); } - drop(self.buffer.drain(..consumed_bytes)); - } - } else { - error!("Buffer conversion didn't work. This is unexpected"); + }, + Err(e) => Err::<(), _>(e) + .context("failed to flush logging pipe buffer") + .non_fatal(), } Ok(()) diff --git a/zellij-server/src/os_input_output.rs b/zellij-server/src/os_input_output.rs index 322cfb9fda..24a2ace217 100644 --- a/zellij-server/src/os_input_output.rs +++ b/zellij-server/src/os_input_output.rs @@ -1,45 +1,44 @@ -use std::collections::{BTreeMap, HashMap, HashSet}; -use std::{fs::File, io::Write}; +use crate::{panes::PaneId, ClientId}; -use crate::panes::PaneId; -use zellij_utils::tempfile::tempfile; - -use std::env; -use std::os::unix::io::RawFd; -use std::os::unix::process::CommandExt; -use std::path::PathBuf; -use std::process::{Child, Command}; -use std::sync::{Arc, Mutex}; - -use zellij_utils::errors::prelude::*; -use zellij_utils::{async_std, interprocess, libc, nix, signal_hook}; - -use async_std::fs::File as AsyncFile; -use async_std::os::unix::io::FromRawFd; +use async_std::{fs::File as AsyncFile, io::ReadExt, os::unix::io::FromRawFd}; use interprocess::local_socket::LocalSocketStream; - -use sysinfo::{ProcessExt, ProcessRefreshKind, System, SystemExt}; - -use nix::pty::{openpty, OpenptyResult, Winsize}; -use nix::sys::signal::{kill, Signal}; -use nix::sys::termios; - -use nix::unistd; +use nix::{ + pty::{openpty, OpenptyResult, Winsize}, + sys::{ + signal::{kill, Signal}, + termios, + }, + unistd, +}; use signal_hook::consts::*; +use sysinfo::{ProcessExt, ProcessRefreshKind, System, SystemExt}; use zellij_utils::{ + async_std, data::Palette, + errors::prelude::*, input::command::{RunCommand, TerminalAction}, + interprocess, ipc::{ClientToServerMsg, IpcReceiverWithContext, IpcSenderWithContext, ServerToClientMsg}, + libc, nix, shared::default_palette, + signal_hook, + tempfile::tempfile, }; -use async_std::io::ReadExt; -pub use async_trait::async_trait; +use std::{ + collections::{BTreeMap, HashMap, HashSet}, + env, + fs::File, + io::Write, + os::unix::{io::RawFd, process::CommandExt}, + path::PathBuf, + process::{Child, Command}, + sync::{Arc, Mutex}, +}; +pub use async_trait::async_trait; pub use nix::unistd::Pid; -use crate::ClientId; - fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) { // TODO: do this with the nix ioctl use libc::ioctl; @@ -164,10 +163,11 @@ fn handle_openpty( command.current_dir(current_dir); } else { // TODO: propagate this to the user - log::error!( - "Failed to set CWD for new pane. {} does not exist or is not a folder", + return Err(anyhow!( + "Failed to set CWD for new pane. '{}' does not exist or is not a folder", current_dir.display() - ); + )) + .context("failed to open PTY"); } } command @@ -417,16 +417,18 @@ pub trait ServerOsApi: Send + Sync { impl ServerOsApi for ServerOsInputOutput { fn set_terminal_size_using_terminal_id(&self, id: u32, cols: u16, rows: u16) -> Result<()> { + let err_context = || { + format!( + "failed to set terminal id {} to size ({}, {})", + id, rows, cols + ) + }; + match self .terminal_id_to_raw_fd .lock() .to_anyhow() - .with_context(|| { - format!( - "failed to set terminal id {} to size ({}, {})", - id, rows, cols - ) - })? + .with_context(err_context)? .get(&id) { Some(Some(fd)) => { @@ -435,7 +437,9 @@ impl ServerOsApi for ServerOsInputOutput { } }, _ => { - log::error!("Failed to find terminal fd for id: {id}, so cannot resize terminal"); + Err::<(), _>(anyhow!("failed to find terminal fd for id {id}")) + .with_context(err_context) + .non_fatal(); }, } Ok(()) diff --git a/zellij-server/src/panes/tiled_panes/mod.rs b/zellij-server/src/panes/tiled_panes/mod.rs index 3bbd872a22..52b287955e 100644 --- a/zellij-server/src/panes/tiled_panes/mod.rs +++ b/zellij-server/src/panes/tiled_panes/mod.rs @@ -1,29 +1,29 @@ mod pane_resizer; mod tiled_pane_grid; -use crate::tab::{Pane, MIN_TERMINAL_HEIGHT, MIN_TERMINAL_WIDTH}; -use tiled_pane_grid::{split, TiledPaneGrid}; - use crate::{ os_input_output::ServerOsApi, output::Output, panes::{ActivePanes, PaneId}, - ui::boundaries::Boundaries, - ui::pane_contents_and_ui::PaneContentsAndUi, + tab::{Pane, MIN_TERMINAL_HEIGHT, MIN_TERMINAL_WIDTH}, + ui::{boundaries::Boundaries, pane_contents_and_ui::PaneContentsAndUi}, ClientId, }; -use std::cell::RefCell; -use std::collections::{BTreeMap, HashMap, HashSet}; -use std::rc::Rc; -use std::time::Instant; -use zellij_utils::errors::prelude::*; +use tiled_pane_grid::{split, TiledPaneGrid}; use zellij_utils::{ data::{ModeInfo, Style}, - input::command::RunCommand, - input::layout::SplitDirection, + errors::prelude::*, + input::{command::RunCommand, layout::SplitDirection}, pane_size::{Offset, PaneGeom, Size, SizeInPixels, Viewport}, }; +use std::{ + cell::RefCell, + collections::{BTreeMap, HashMap, HashSet}, + rc::Rc, + time::Instant, +}; + macro_rules! resize_pty { ($pane:expr, $os_input:expr) => { if let PaneId::Terminal(ref pid) = $pane.pid() { @@ -226,17 +226,18 @@ impl TiledPanes { *self.display_area.borrow(), *self.viewport.borrow(), ); - let result = match direction { + match direction { SplitDirection::Horizontal => { pane_grid.layout(direction, (*self.display_area.borrow()).cols) }, SplitDirection::Vertical => { pane_grid.layout(direction, (*self.display_area.borrow()).rows) }, - }; - if let Err(e) = &result { - log::error!("{:?} relayout of the tab failed: {}", direction, e); } + .or_else(|e| Err(anyError::msg(e))) + .with_context(|| format!("{:?} relayout of tab failed", direction)) + .non_fatal(); + self.set_pane_frames(self.draw_pane_frames); } pub fn set_pane_frames(&mut self, draw_pane_frames: bool) { @@ -492,16 +493,23 @@ impl TiledPanes { display_area.cols = cols; }, Err(e) => { - log::error!("Failed to horizontally resize the tab: {:?}", e); + Err::<(), _>(anyError::msg(e)) + .context("failed to resize tab horizontally") + .non_fatal(); + }, + }; + match pane_grid.layout(SplitDirection::Vertical, rows) { + Ok(_) => { + let row_difference = rows as isize - display_area.rows as isize; + viewport.rows = (viewport.rows as isize + row_difference) as usize; + display_area.rows = rows; + }, + Err(e) => { + Err::<(), _>(anyError::msg(e)) + .context("failed to resize tab vertically") + .non_fatal(); }, }; - if pane_grid.layout(SplitDirection::Vertical, rows).is_ok() { - let row_difference = rows as isize - display_area.rows as isize; - viewport.rows = (viewport.rows as isize + row_difference) as usize; - display_area.rows = rows; - } else { - log::error!("Failed to vertically resize the tab!!!"); - } } self.set_pane_frames(self.draw_pane_frames); } diff --git a/zellij-server/src/pty.rs b/zellij-server/src/pty.rs index da8fd20dc4..3f081bf75e 100644 --- a/zellij-server/src/pty.rs +++ b/zellij-server/src/pty.rs @@ -87,7 +87,6 @@ pub(crate) struct Pty { } pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { - let err_context = || "failed in pty thread main".to_string(); loop { let (event, mut err_ctx) = pty.bus.recv().expect("failed to receive event on channel"); err_ctx.add_call(ContextType::Pty((&event).into())); @@ -98,6 +97,9 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { name, client_or_tab_index, ) => { + let err_context = + || format!("failed to spawn terminal for {:?}", client_or_tab_index); + let (hold_on_close, run_command, pane_title) = match &terminal_action { Some(TerminalAction::RunCommand(run_command)) => ( run_command.hold_on_close, @@ -146,7 +148,7 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { .with_context(err_context)?; } } else { - log::error!("Failed to spawn terminal: command not found"); + log::error!("Failed to spawn terminal: {:?}", err); pty.close_pane(PaneId::Terminal(*terminal_id)) .with_context(err_context)?; } @@ -156,6 +158,9 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { } }, PtyInstruction::OpenInPlaceEditor(temp_file, line_number, client_id) => { + let err_context = + || format!("failed to open in-place editor for client {}", client_id); + match pty.spawn_terminal( Some(TerminalAction::OpenFile(temp_file, line_number)), ClientOrTabIndex::ClientId(client_id), @@ -170,11 +175,14 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { .with_context(err_context)?; }, Err(e) => { - log::error!("Failed to open editor: {}", e); + Err::<(), _>(e).with_context(err_context).non_fatal(); }, } }, PtyInstruction::SpawnTerminalVertically(terminal_action, name, client_id) => { + let err_context = + || format!("failed to spawn terminal vertically for client {client_id}"); + let (hold_on_close, run_command, pane_title) = match &terminal_action { Some(TerminalAction::RunCommand(run_command)) => ( run_command.hold_on_close, @@ -242,6 +250,9 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { } }, PtyInstruction::SpawnTerminalHorizontally(terminal_action, name, client_id) => { + let err_context = + || format!("failed to spawn terminal horizontally for client {client_id}"); + let (hold_on_close, run_command, pane_title) = match &terminal_action { Some(TerminalAction::RunCommand(run_command)) => ( run_command.hold_on_close, @@ -315,9 +326,13 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { pty.bus .senders .send_to_screen(ScreenInstruction::GoToTab(tab_index, Some(client_id))) - .with_context(err_context)?; + .with_context(|| { + format!("failed to move client {} to tab {}", client_id, tab_index) + })?; }, PtyInstruction::NewTab(terminal_action, tab_layout, tab_name, client_id) => { + let err_context = || format!("failed to open new tab for client {}", client_id); + pty.spawn_terminals_for_layout( tab_layout.unwrap_or_else(|| layout.new_tab()), terminal_action.clone(), @@ -341,20 +356,26 @@ pub(crate) fn pty_thread_main(mut pty: Pty, layout: Box) -> Result<()> { } }, PtyInstruction::ClosePane(id) => { - pty.close_pane(id).with_context(err_context)?; - pty.bus - .senders - .send_to_server(ServerInstruction::UnblockInputThread) - .with_context(err_context)?; + pty.close_pane(id) + .and_then(|_| { + pty.bus + .senders + .send_to_server(ServerInstruction::UnblockInputThread) + }) + .with_context(|| format!("failed to close pane {:?}", id))?; }, PtyInstruction::CloseTab(ids) => { - pty.close_tab(ids).with_context(err_context)?; - pty.bus - .senders - .send_to_server(ServerInstruction::UnblockInputThread) - .with_context(err_context)?; + pty.close_tab(ids) + .and_then(|_| { + pty.bus + .senders + .send_to_server(ServerInstruction::UnblockInputThread) + }) + .context("failed to close tabs")?; }, PtyInstruction::ReRunCommandInPane(pane_id, run_command) => { + let err_context = || format!("failed to rerun command in pane {:?}", pane_id); + match pty .rerun_command_in_pane(pane_id, run_command.clone()) .with_context(err_context) @@ -585,9 +606,7 @@ impl Pty { // stripped later )); }, - Err(e) => { - log::error!("Failed to spawn terminal: {}", e); - }, + Err(e) => Err::<(), _>(e).with_context(err_context).non_fatal(), } } else { match self diff --git a/zellij-server/src/route.rs b/zellij-server/src/route.rs index 0d2da7b7d1..0c84171282 100644 --- a/zellij-server/src/route.rs +++ b/zellij-server/src/route.rs @@ -795,14 +795,15 @@ pub(crate) fn route_thread_main( } }, None => { - log::error!("Received empty message from client"); let _ = os_input.send_to_client( client_id, ServerToClientMsg::Exit(ExitReason::Error( "Received empty message".to_string(), )), ); - break; + + return Err(anyhow!("received empty message from client")) + .with_context(err_context); }, } } diff --git a/zellij-server/src/screen.rs b/zellij-server/src/screen.rs index 3d6d28514e..01a4b33f06 100644 --- a/zellij-server/src/screen.rs +++ b/zellij-server/src/screen.rs @@ -46,41 +46,41 @@ use zellij_utils::{ /// argument is optional and not needed when the closure returns `()` macro_rules! active_tab { ($screen:ident, $client_id:ident, $closure:expr) => { - if let Some(active_tab) = $screen.get_active_tab_mut($client_id) { - // This could be made more ergonomic by declaring the type of 'active_tab' in the - // closure, known as "Type Ascription". Then we could hint the type here and forego the - // "&mut Tab" in all the closures below... - // See: https://github.com/rust-lang/rust/issues/23416 - $closure(active_tab); - } else { - log::error!("Active tab not found for client id: {:?}", $client_id); - } + match $screen.get_active_tab_mut($client_id) { + Ok(active_tab) => { + // This could be made more ergonomic by declaring the type of 'active_tab' in the + // closure, known as "Type Ascription". Then we could hint the type here and forego the + // "&mut Tab" in all the closures below... + // See: https://github.com/rust-lang/rust/issues/23416 + $closure(active_tab); + }, + Err(err) => Err::<(), _>(err).non_fatal(), + }; }; // Same as above, but with an added `?` for when the close returns a `Result` type. ($screen:ident, $client_id:ident, $closure:expr, ?) => { - if let Some(active_tab) = $screen.get_active_tab_mut($client_id) { + match $screen.get_active_tab_mut($client_id) { + Ok(active_tab) => { $closure(active_tab)?; - } else { - log::error!("Active tab not found for client id: {:?}", $client_id); - } + }, + Err(err) => Err::<(), _>(err).non_fatal(), + }; }; } macro_rules! active_tab_and_connected_client_id { ($screen:ident, $client_id:ident, $closure:expr) => { match $screen.get_active_tab_mut($client_id) { - Some(active_tab) => { + Ok(active_tab) => { $closure(active_tab, $client_id); }, - None => { + Err(_) => { if let Some(client_id) = $screen.get_first_client_id() { match $screen.get_active_tab_mut(client_id) { - Some(active_tab) => { + Ok(active_tab) => { $closure(active_tab, client_id); }, - None => { - log::error!("Active tab not found for client id: {:?}", $client_id); - }, + Err(err) => Err::<(), _>(err).non_fatal(), } } else { log::error!("No client ids in screen found"); @@ -91,18 +91,16 @@ macro_rules! active_tab_and_connected_client_id { // Same as above, but with an added `?` for when the closure returns a `Result` type. ($screen:ident, $client_id:ident, $closure:expr, ?) => { match $screen.get_active_tab_mut($client_id) { - Some(active_tab) => { + Ok(active_tab) => { $closure(active_tab, $client_id)?; }, - None => { + Err(_) => { if let Some(client_id) = $screen.get_first_client_id() { match $screen.get_active_tab_mut(client_id) { - Some(active_tab) => { + Ok(active_tab) => { $closure(active_tab, client_id)?; }, - None => { - log::error!("Active tab not found for client id: {:?}", $client_id); - }, + Err(err) => Err::<(), _>(err).non_fatal(), } } else { log::error!("No client ids in screen found"); @@ -440,10 +438,13 @@ impl Screen { let err_context = || "failed to move clients from closed tab".to_string(); if self.tabs.is_empty() { - log::error!( + Err::<(), _>(anyhow!( "No tabs left, cannot move clients: {:?} from closed tab", client_ids_and_mode_infos - ); + )) + .with_context(err_context) + .non_fatal(); + return Ok(()); } let first_tab_index = *self @@ -530,44 +531,48 @@ impl Screen { }; if let Some(new_tab) = self.tabs.values().find(|t| t.position == new_tab_pos) { - if let Some(current_tab) = self.get_active_tab(client_id) { - // If new active tab is same as the current one, do nothing. - if current_tab.position == new_tab_pos { - return Ok(()); - } + //if let Some(current_tab) = self.get_active_tab(client_id) { + match self.get_active_tab(client_id) { + Ok(current_tab) => { + // If new active tab is same as the current one, do nothing. + if current_tab.position == new_tab_pos { + return Ok(()); + } - let current_tab_index = current_tab.index; - let new_tab_index = new_tab.index; - if self.session_is_mirrored { - self.move_clients_between_tabs(current_tab_index, new_tab_index, None) + let current_tab_index = current_tab.index; + let new_tab_index = new_tab.index; + if self.session_is_mirrored { + self.move_clients_between_tabs(current_tab_index, new_tab_index, None) + .with_context(err_context)?; + let all_connected_clients: Vec = + self.connected_clients.borrow().iter().copied().collect(); + for client_id in all_connected_clients { + self.update_client_tab_focus(client_id, new_tab_index); + } + } else { + self.move_clients_between_tabs( + current_tab_index, + new_tab_index, + Some(vec![client_id]), + ) .with_context(err_context)?; - let all_connected_clients: Vec = - self.connected_clients.borrow().iter().copied().collect(); - for client_id in all_connected_clients { self.update_client_tab_focus(client_id, new_tab_index); } - } else { - self.move_clients_between_tabs( - current_tab_index, - new_tab_index, - Some(vec![client_id]), - ) - .with_context(err_context)?; - self.update_client_tab_focus(client_id, new_tab_index); - } - if let Some(current_tab) = self.get_indexed_tab_mut(current_tab_index) { - if current_tab.has_no_connected_clients() { - current_tab.visible(false).with_context(err_context)?; + if let Some(current_tab) = self.get_indexed_tab_mut(current_tab_index) { + if current_tab.has_no_connected_clients() { + current_tab.visible(false).with_context(err_context)?; + } + } else { + Err::<(), _>(anyhow!("Tab index {:?} not found", current_tab_index)) + .with_context(err_context) + .non_fatal(); } - } else { - log::error!("Tab index: {:?} not found", current_tab_index); - } - self.update_tabs().with_context(err_context)?; - return self.render().with_context(err_context); - } else { - log::error!("Active tab not found for client id: {client_id:?}"); + self.update_tabs().with_context(err_context)?; + return self.render().with_context(err_context); + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), } } Ok(()) @@ -575,45 +580,51 @@ impl Screen { /// Sets this [`Screen`]'s active [`Tab`] to the next tab. pub fn switch_tab_next(&mut self, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || format!("failed to switch to next tab for client {client_id}"); + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; + if let Some(client_id) = client_id { - if let Some(active_tab) = self.get_active_tab(client_id) { - let active_tab_pos = active_tab.position; - let new_tab_pos = (active_tab_pos + 1) % self.tabs.len(); - return self.switch_active_tab(new_tab_pos, client_id); - } else { - log::error!("Active tab not found for client_id: {:?}", client_id); + match self.get_active_tab(client_id) { + Ok(active_tab) => { + let active_tab_pos = active_tab.position; + let new_tab_pos = (active_tab_pos + 1) % self.tabs.len(); + return self.switch_active_tab(new_tab_pos, client_id); + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), } - log::error!("Active tab not found for client id: {client_id:?}"); } Ok(()) } /// Sets this [`Screen`]'s active [`Tab`] to the previous tab. pub fn switch_tab_prev(&mut self, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || format!("failed to switch to previous tab for client {client_id}"); + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; - if let Some(client_id) = client_id { - if let Some(active_tab) = self.get_active_tab(client_id) { - let active_tab_pos = active_tab.position; - let new_tab_pos = if active_tab_pos == 0 { - self.tabs.len() - 1 - } else { - active_tab_pos - 1 - }; - return self.switch_active_tab(new_tab_pos, client_id); - } else { - log::error!("Active tab not found for client_id: {:?}", client_id); + if let Some(client_id) = client_id { + match self.get_active_tab(client_id) { + Ok(active_tab) => { + let active_tab_pos = active_tab.position; + let new_tab_pos = if active_tab_pos == 0 { + self.tabs.len() - 1 + } else { + active_tab_pos - 1 + }; + + return self.switch_active_tab(new_tab_pos, client_id); + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), } - log::error!("Active tab not found for client id: {client_id:?}"); } Ok(()) } @@ -663,11 +674,13 @@ impl Screen { // Closes the client_id's focused tab pub fn close_tab(&mut self, client_id: ClientId) -> Result<()> { let err_context = || format!("failed to close tab for client {client_id:?}"); - let client_id = if self.get_active_tab(client_id).is_some() { + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; + match client_id { Some(client_id) => { let active_tab_index = *self @@ -767,10 +780,13 @@ impl Screen { } /// Returns an immutable reference to this [`Screen`]'s active [`Tab`]. - pub fn get_active_tab(&self, client_id: ClientId) -> Option<&Tab> { + pub fn get_active_tab(&self, client_id: ClientId) -> Result<&Tab> { match self.active_tab_indices.get(&client_id) { - Some(tab) => self.tabs.get(tab), - None => None, + Some(tab) => self + .tabs + .get(tab) + .ok_or_else(|| anyhow!("active tab {} does not exist", tab)), + None => Err(anyhow!("active tab not found for client {:?}", client_id)), } } @@ -797,10 +813,13 @@ impl Screen { } /// Returns a mutable reference to this [`Screen`]'s active [`Tab`]. - pub fn get_active_tab_mut(&mut self, client_id: ClientId) -> Option<&mut Tab> { + pub fn get_active_tab_mut(&mut self, client_id: ClientId) -> Result<&mut Tab> { match self.active_tab_indices.get(&client_id) { - Some(tab) => self.tabs.get_mut(tab), - None => None, + Some(tab) => self + .tabs + .get_mut(tab) + .ok_or_else(|| anyhow!("active tab {} does not exist", tab)), + None => Err(anyhow!("active tab not found for client {:?}", client_id)), } } @@ -822,14 +841,16 @@ impl Screen { new_ids: Vec<(u32, HoldForCommand)>, client_id: ClientId, ) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || format!("failed to create new tab for client {client_id:?}",); + + let client_id = if self.get_active_tab(client_id).is_ok() { client_id } else if let Some(first_client_id) = self.get_first_client_id() { first_client_id } else { client_id }; - let err_context = || format!("failed to create new tab for client {client_id:?}",); + let tab_index = self.get_new_tab_index(); let position = self.tabs.len(); let mut tab = Tab::new( @@ -859,7 +880,7 @@ impl Screen { tab.apply_layout(layout, new_ids, tab_index, client_id) .with_context(err_context)?; if self.session_is_mirrored { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { + if let Ok(active_tab) = self.get_active_tab_mut(client_id) { let client_mode_infos_in_source_tab = active_tab.drain_connected_clients(None); tab.add_multiple_clients(client_mode_infos_in_source_tab) .with_context(err_context)?; @@ -872,7 +893,7 @@ impl Screen { for client_id in all_connected_clients { self.update_client_tab_focus(client_id, tab_index); } - } else if let Some(active_tab) = self.get_active_tab_mut(client_id) { + } else if let Ok(active_tab) = self.get_active_tab_mut(client_id) { let client_mode_info_in_source_tab = active_tab.drain_connected_clients(Some(vec![client_id])); tab.add_multiple_clients(client_mode_info_in_source_tab) @@ -985,58 +1006,68 @@ impl Screen { } pub fn update_active_tab_name(&mut self, buf: Vec, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = + || format!("failed to update active tabs name for client id: {client_id:?}"); + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; + match client_id { Some(client_id) => { let s = str::from_utf8(&buf) - .with_context(|| format!("failed to construct tab name from buf: {buf:?}"))?; - if let Some(active_tab) = self.get_active_tab_mut(client_id) { - match s { - "\0" => { - active_tab.name = String::new(); - }, - "\u{007F}" | "\u{0008}" => { - // delete and backspace keys - active_tab.name.pop(); - }, - c => { - // It only allows printable unicode - if buf.iter().all(|u| matches!(u, 0x20..=0x7E)) { - active_tab.name.push_str(c); - } - }, - } - self.update_tabs() - .context("failed to update active tabs name for client id: {client_id:?}") - } else { - log::error!("Active tab not found for client id: {client_id:?}"); - Ok(()) + .with_context(|| format!("failed to construct tab name from buf: {buf:?}")) + .with_context(err_context)?; + match self.get_active_tab_mut(client_id) { + Ok(active_tab) => { + match s { + "\0" => { + active_tab.name = String::new(); + }, + "\u{007F}" | "\u{0008}" => { + // delete and backspace keys + active_tab.name.pop(); + }, + c => { + // It only allows printable unicode + if buf.iter().all(|u| matches!(u, 0x20..=0x7E)) { + active_tab.name.push_str(c); + } + }, + } + self.update_tabs().with_context(err_context) + }, + Err(err) => { + Err::<(), _>(err).with_context(err_context).non_fatal(); + Ok(()) + }, } }, None => Ok(()), } } pub fn undo_active_rename_tab(&mut self, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || format!("failed to undo active tab rename for client {}", client_id); + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; match client_id { Some(client_id) => { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { - if active_tab.name != active_tab.prev_name { - active_tab.name = active_tab.prev_name.clone(); - self.update_tabs() - .context("failed to undo renaming of active tab")?; - } - } else { - log::error!("Active tab not found for client id: {client_id:?}"); - } + match self.get_active_tab_mut(client_id) { + Ok(active_tab) => { + if active_tab.name != active_tab.prev_name { + active_tab.name = active_tab.prev_name.clone(); + self.update_tabs() + .context("failed to undo renaming of active tab")?; + } + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), + }; Ok(()) }, None => Ok(()), @@ -1068,7 +1099,7 @@ impl Screen { if previous_mode == InputMode::Scroll && (mode_info.mode == InputMode::Normal || mode_info.mode == InputMode::Locked) { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { + if let Ok(active_tab) = self.get_active_tab_mut(client_id) { active_tab .clear_active_terminal_scroll(client_id) .with_context(err_context)?; @@ -1076,13 +1107,13 @@ impl Screen { } if mode_info.mode == InputMode::RenameTab { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { + if let Ok(active_tab) = self.get_active_tab_mut(client_id) { active_tab.prev_name = active_tab.name.clone(); } } if mode_info.mode == InputMode::RenamePane { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { + if let Ok(active_tab) = self.get_active_tab_mut(client_id) { if let Some(active_pane) = active_tab.get_active_pane_or_floating_pane_mut(client_id) { @@ -1119,38 +1150,55 @@ impl Screen { Ok(()) } pub fn move_focus_left_or_previous_tab(&mut self, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || { + format!( + "failed to move focus left or to previous tab for client {}", + client_id + ) + }; + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; if let Some(client_id) = client_id { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { - if !active_tab.move_focus_left(client_id) { - self.switch_tab_prev(client_id) - .context("failed to move focus left")?; - } - } else { - log::error!("Active tab not found for client id: {:?}", client_id); - } + match self.get_active_tab_mut(client_id) { + Ok(active_tab) => { + if !active_tab.move_focus_left(client_id) { + self.switch_tab_prev(client_id) + .context("failed to move focus left")?; + } + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), + }; } Ok(()) } pub fn move_focus_right_or_next_tab(&mut self, client_id: ClientId) -> Result<()> { - let client_id = if self.get_active_tab(client_id).is_some() { + let err_context = || { + format!( + "failed to move focus right or to next tab for client {}", + client_id + ) + }; + + let client_id = if self.get_active_tab(client_id).is_ok() { Some(client_id) } else { self.get_first_client_id() }; + if let Some(client_id) = client_id { - if let Some(active_tab) = self.get_active_tab_mut(client_id) { - if !active_tab.move_focus_right(client_id) { - self.switch_tab_next(client_id) - .context("failed to move focus right")?; - } - } else { - log::error!("Active tab not found for client id: {:?}", client_id); - } + match self.get_active_tab_mut(client_id) { + Ok(active_tab) => { + if !active_tab.move_focus_right(client_id) { + self.switch_tab_next(client_id) + .context("failed to move focus right")?; + } + }, + Err(err) => Err::<(), _>(err).with_context(err_context).non_fatal(), + }; } Ok(()) } diff --git a/zellij-server/src/tab/mod.rs b/zellij-server/src/tab/mod.rs index 9546f2201d..618c2b35d9 100644 --- a/zellij-server/src/tab/mod.rs +++ b/zellij-server/src/tab/mod.rs @@ -490,10 +490,13 @@ impl Tab { "failed to apply layout {layout:#?} in tab {tab_index} for client id {client_id}" ) }; + if self.tiled_panes.has_panes() { - log::error!( + Err::<(), _>(anyhow!( "Applying a layout to a tab with existing panes - this is not yet supported!" - ); + )) + .with_context(err_context) + .non_fatal(); } let (viewport_cols, viewport_rows) = { let viewport = self.viewport.borrow(); @@ -620,7 +623,9 @@ impl Tab { .send_to_pty(PtyInstruction::ClosePane(PaneId::Terminal(unused_pid))) .with_context(err_context)?; } - log::error!("{}", e); // TODO: propagate this to the user + Err::<(), _>(anyError::msg(e)) + .with_context(err_context) + .non_fatal(); // TODO: propagate this to the user Ok(()) }, } @@ -934,7 +939,11 @@ impl Tab { .with_context(err_context)?; }, None => { - log::error!("Could not find editor pane to replace - is no pane focused?") + Err::<(), _>(anyhow!( + "Could not find editor pane to replace - is no pane focused?" + )) + .with_context(err_context) + .non_fatal(); }, } }, diff --git a/zellij-server/src/wasm_vm.rs b/zellij-server/src/wasm_vm.rs index 4fea6adb0a..a7bb81cfc0 100644 --- a/zellij-server/src/wasm_vm.rs +++ b/zellij-server/src/wasm_vm.rs @@ -149,7 +149,9 @@ pub(crate) fn wasm_thread_main( let plugin_global_data_dir = plugin_dir.join("data"); #[cfg(not(feature = "disable_automatic_asset_installation"))] - fs::create_dir_all(&plugin_global_data_dir).unwrap_or_else(|e| log::error!("{:?}", e)); + fs::create_dir_all(&plugin_global_data_dir) + .context("failed to create plugin asset directory") + .non_fatal(); loop { let (event, mut err_ctx) = bus.recv().expect("failed to receive event on channel");