fix(panes): show visual error when failing to resize panes (#2036)

* fix(panes): show visual error when failing to resize pane vertically/horizontally

* fix(resize): retry pane resize on rounding errors

* fix(resize): proper error when resizing other panes into fixed panes

* style(fmt): rustfmt
This commit is contained in:
Aram Drevekenin 2022-12-19 12:48:43 +01:00 committed by GitHub
parent d1f50150f6
commit 1b5f3c52a4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 428 additions and 74 deletions

View file

@ -148,18 +148,14 @@ impl InputHandler {
}
},
Ok((InputInstruction::StartedParsing, _error_context)) => {
log::info!("sending done loading");
self.send_client_instructions
.send(ClientInstruction::StartedParsingStdinQuery)
.unwrap();
log::info!("done sent done loading");
},
Ok((InputInstruction::DoneParsing, _error_context)) => {
log::info!("sending done loading");
self.send_client_instructions
.send(ClientInstruction::DoneParsingStdinQuery)
.unwrap();
log::info!("done sent done loading");
},
Err(err) => panic!("Encountered read error: {:?}", err),
}

View file

@ -10,7 +10,7 @@ use crate::thread_bus::Bus;
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub enum BackgroundJob {
DisplayPaneError(PaneId, String),
DisplayPaneError(Vec<PaneId>, String),
Exit,
}
@ -34,7 +34,7 @@ pub(crate) fn background_jobs_main(bus: Bus<BackgroundJob>) -> Result<()> {
err_ctx.add_call(ContextType::BackgroundJob((&event).into()));
let job = event.clone();
match event {
BackgroundJob::DisplayPaneError(pane_id, text) => {
BackgroundJob::DisplayPaneError(pane_ids, text) => {
if job_already_running(job, &mut running_jobs) {
continue;
}
@ -42,11 +42,14 @@ pub(crate) fn background_jobs_main(bus: Bus<BackgroundJob>) -> Result<()> {
let senders = bus.senders.clone();
async move {
let _ = senders.send_to_screen(
ScreenInstruction::AddRedPaneFrameColorOverride(pane_id, Some(text)),
ScreenInstruction::AddRedPaneFrameColorOverride(
pane_ids.clone(),
Some(text),
),
);
task::sleep(std::time::Duration::from_millis(FLASH_DURATION_MS)).await;
let _ = senders.send_to_screen(
ScreenInstruction::ClearPaneFrameColorOverride(pane_id),
ScreenInstruction::ClearPaneFrameColorOverride(pane_ids),
);
}
});

View file

@ -16,7 +16,7 @@ use zellij_utils::{
channels::SenderWithContext,
data::{Event, InputMode, Mouse, Palette, PaletteColor, Style},
errors::prelude::*,
pane_size::{Dimension, PaneGeom},
pane_size::PaneGeom,
shared::make_terminal_title,
vte,
};
@ -341,28 +341,28 @@ impl Pane for PluginPane {
}
fn reduce_height(&mut self, percent: f64) {
if let Some(p) = self.geom.rows.as_percent() {
self.geom.rows = Dimension::percent(p - percent);
self.geom.rows.set_percent(p - percent);
self.resize_grids();
self.set_should_render(true);
}
}
fn increase_height(&mut self, percent: f64) {
if let Some(p) = self.geom.rows.as_percent() {
self.geom.rows = Dimension::percent(p + percent);
self.geom.rows.set_percent(p + percent);
self.resize_grids();
self.set_should_render(true);
}
}
fn reduce_width(&mut self, percent: f64) {
if let Some(p) = self.geom.cols.as_percent() {
self.geom.cols = Dimension::percent(p - percent);
self.geom.cols.set_percent(p - percent);
self.resize_grids();
self.set_should_render(true);
}
}
fn increase_width(&mut self, percent: f64) {
if let Some(p) = self.geom.cols.as_percent() {
self.geom.cols = Dimension::percent(p + percent);
self.geom.cols.set_percent(p + percent);
self.resize_grids();
self.set_should_render(true);
}

View file

@ -18,8 +18,8 @@ use zellij_utils::pane_size::Offset;
use zellij_utils::{
data::{InputMode, Palette, PaletteColor, Style},
errors::prelude::*,
pane_size::PaneGeom,
pane_size::SizeInPixels,
pane_size::{Dimension, PaneGeom},
position::Position,
shared::make_terminal_title,
vte,
@ -446,25 +446,25 @@ impl Pane for TerminalPane {
}
fn reduce_height(&mut self, percent: f64) {
if let Some(p) = self.geom.rows.as_percent() {
self.geom.rows = Dimension::percent(p - percent);
self.geom.rows.set_percent(p - percent);
self.set_should_render(true);
}
}
fn increase_height(&mut self, percent: f64) {
if let Some(p) = self.geom.rows.as_percent() {
self.geom.rows = Dimension::percent(p + percent);
self.geom.rows.set_percent(p + percent);
self.set_should_render(true);
}
}
fn reduce_width(&mut self, percent: f64) {
if let Some(p) = self.geom.cols.as_percent() {
self.geom.cols = Dimension::percent(p - percent);
self.geom.cols.set_percent(p - percent);
self.set_should_render(true);
}
}
fn increase_width(&mut self, percent: f64) {
if let Some(p) = self.geom.cols.as_percent() {
self.geom.cols = Dimension::percent(p + percent);
self.geom.cols.set_percent(p + percent);
self.set_should_render(true);
}
}

View file

@ -577,9 +577,38 @@ impl TiledPanes {
*self.viewport.borrow(),
);
pane_grid
match pane_grid
.change_pane_size(&active_pane_id, strategy, (RESIZE_PERCENT, RESIZE_PERCENT))
.with_context(err_context)?;
.with_context(err_context)
{
Ok(_) => {},
Err(err) => match err.downcast_ref::<ZellijError>() {
Some(ZellijError::PaneSizeUnchanged) => {
// try once more with double the resize percent, but let's keep it at that
match pane_grid
.change_pane_size(
&active_pane_id,
strategy,
(RESIZE_PERCENT * 2.0, RESIZE_PERCENT * 2.0),
)
.with_context(err_context)
{
Ok(_) => {},
Err(err) => match err.downcast_ref::<ZellijError>() {
Some(ZellijError::PaneSizeUnchanged) => {
Err::<(), _>(err).non_fatal()
},
_ => {
return Err(err);
},
},
}
},
_ => {
return Err(err);
},
},
}
for pane in self.panes.values_mut() {
resize_pty!(pane, self.os_api, self.senders).unwrap();

View file

@ -53,7 +53,7 @@ impl<'a> PaneResizer<'a> {
let spans = self
.discretize_spans(grid, space)
.map_err(|err| anyhow!("{}", err))?;
self.apply_spans(spans);
self.apply_spans(spans)?;
Ok(())
}
@ -127,10 +127,13 @@ impl<'a> PaneResizer<'a> {
Ok(grid.into_iter().flatten().collect())
}
fn apply_spans(&mut self, spans: Vec<Span>) {
fn apply_spans(&mut self, spans: Vec<Span>) -> Result<()> {
let err_context = || format!("Failed to apply spans");
let mut panes = self.panes.borrow_mut();
let mut geoms_changed = false;
for span in spans {
let pane = panes.get_mut(&span.pid).unwrap();
let current_geom = pane.position_and_size();
let new_geom = match span.direction {
SplitDirection::Horizontal => PaneGeom {
x: span.pos,
@ -143,12 +146,26 @@ impl<'a> PaneResizer<'a> {
..pane.current_geom()
},
};
if new_geom.rows.as_usize() != current_geom.rows.as_usize()
|| new_geom.cols.as_usize() != current_geom.cols.as_usize()
{
geoms_changed = true;
}
if pane.geom_override().is_some() {
pane.set_geom_override(new_geom);
} else {
pane.set_geom(new_geom);
}
}
if geoms_changed {
Ok(())
} else {
// probably a rounding issue - this might be considered an error depending on who
// called us - if it's an explicit resize operation, it's clearly an error (the user
// wanted to resize and doesn't care about percentage rounding), if it's resizing the
// terminal window as a whole, it might not be
Err(ZellijError::PaneSizeUnchanged).with_context(err_context)
}
}
// FIXME: Functions like this should have unit tests!

View file

@ -88,6 +88,56 @@ impl<'a> TiledPaneGrid<'a> {
}
.is_fixed())
}
fn neighbor_pane_ids(&self, pane_id: &PaneId, direction: Direction) -> Result<Vec<PaneId>> {
let err_context = || format!("Failed to get neighboring panes");
// Shorthand
use Direction as Dir;
let mut neighbor_terminals = self
.pane_ids_directly_next_to(pane_id, &direction)
.with_context(err_context)?;
let neighbor_terminal_borders: HashSet<_> = if direction.is_horizontal() {
neighbor_terminals
.iter()
.map(|t| self.panes.borrow().get(t).unwrap().y())
.collect()
} else {
neighbor_terminals
.iter()
.map(|t| self.panes.borrow().get(t).unwrap().x())
.collect()
};
// Only return those neighbors that are aligned and between pane borders
let (some_direction, other_direction) = match direction {
Dir::Left | Dir::Right => (Dir::Up, Dir::Down),
Dir::Down | Dir::Up => (Dir::Left, Dir::Right),
};
let (some_borders, _some_terminals) = self
.contiguous_panes_with_alignment(
pane_id,
&neighbor_terminal_borders,
&direction,
&some_direction,
)
.with_context(err_context)?;
let (other_borders, _other_terminals) = self
.contiguous_panes_with_alignment(
pane_id,
&neighbor_terminal_borders,
&direction,
&other_direction,
)
.with_context(err_context)?;
neighbor_terminals.retain(|t| {
if direction.is_horizontal() {
self.pane_is_between_horizontal_borders(t, some_borders, other_borders)
} else {
self.pane_is_between_vertical_borders(t, some_borders, other_borders)
}
});
Ok(neighbor_terminals)
}
// Check if panes in the desired direction can be resized. Returns the maximum resize that's
// possible (at most `change_by`).
@ -100,12 +150,39 @@ impl<'a> TiledPaneGrid<'a> {
let err_context = || format!("failed to determine if pane {pane_id:?} can {strategy}");
if let Some(direction) = strategy.direction {
let mut pane_ids = self
.pane_ids_directly_next_to(pane_id, &direction)
if !self
.pane_is_flexible(direction.into(), pane_id)
.unwrap_or(false)
{
let pane_ids = match pane_id {
PaneId::Terminal(id) => vec![(*id, true)],
PaneId::Plugin(id) => vec![(*id, false)],
};
return Err(ZellijError::CantResizeFixedPanes { pane_ids })
.with_context(err_context);
}
let pane_ids = self
.neighbor_pane_ids(pane_id, direction)
.with_context(err_context)?;
pane_ids.retain(|id| self.pane_is_flexible(direction.into(), id).unwrap_or(false));
let fixed_panes: Vec<PaneId> = pane_ids
.iter()
.filter(|p| !self.pane_is_flexible(direction.into(), p).unwrap_or(false))
.copied()
.collect();
if !fixed_panes.is_empty() {
let mut pane_ids = vec![];
for fixed_pane in fixed_panes {
match fixed_pane {
PaneId::Terminal(id) => pane_ids.push((id, true)),
PaneId::Plugin(id) => pane_ids.push((id, false)),
};
}
return Err(ZellijError::CantResizeFixedPanes { pane_ids })
.with_context(err_context);
}
if pane_ids.is_empty() {
// TODO: proper error
return Ok(false);
}
@ -160,37 +237,56 @@ impl<'a> TiledPaneGrid<'a> {
change_by: (f64, f64),
) -> Result<bool> {
let err_context = || format!("failed to {strategy} by {change_by:?} for pane {pane_id:?}");
// Shorthand
use Direction as Dir;
let mut fixed_panes_blocking_resize = vec![];
// Default behavior is to only increase pane size, unless the direction being resized to is
// a boundary. In this case, decrease size from the other side (invert strategy)!
let strategy = if strategy.resize_increase() // Only invert when increasing
let can_invert_strategy_if_needed = strategy.resize_increase() // Only invert when increasing
&& strategy.invert_on_boundaries // Only invert if configured to do so
&& strategy.direction.is_some() // Only invert if there's a direction
&& strategy
.direction
.and_then(|direction| {
// Only invert if there are no neighbor IDs in the given direction
let mut neighbors = self.pane_ids_directly_next_to(pane_id, &direction)
.unwrap_or_default();
neighbors.retain(|pane_id| self.pane_is_flexible(direction.into(), pane_id).unwrap_or(false));
neighbors.is_empty().then_some(true)
})
.unwrap_or(false)
{
strategy.invert()
} else {
*strategy
};
&& strategy.direction.is_some(); // Only invert if there's a direction
if !self
let can_change_pane_size_in_main_direction = self
.can_change_pane_size(pane_id, &strategy, change_by)
.with_context(err_context)?
.unwrap_or_else(|err| {
if let Some(ZellijError::CantResizeFixedPanes { pane_ids }) =
err.downcast_ref::<ZellijError>()
{
// Resize not possible, quit
return Ok(false);
fixed_panes_blocking_resize.append(&mut pane_ids.clone());
}
false
});
let can_change_pane_size_in_inverted_direction = if can_invert_strategy_if_needed {
let strategy = strategy.invert();
self.can_change_pane_size(pane_id, &strategy, change_by)
.unwrap_or_else(|err| {
if let Some(ZellijError::CantResizeFixedPanes { pane_ids }) =
err.downcast_ref::<ZellijError>()
{
fixed_panes_blocking_resize.append(&mut pane_ids.clone());
}
false
})
} else {
false
};
if strategy.direction.is_some()
&& !can_change_pane_size_in_main_direction
&& !can_change_pane_size_in_inverted_direction
{
// we can't resize in any direction, not playing the blame game, but I'm looking at
// you: fixed_panes_blocking_resize
return Err(ZellijError::CantResizeFixedPanes {
pane_ids: fixed_panes_blocking_resize,
})
.with_context(err_context);
}
let strategy = if can_change_pane_size_in_main_direction {
*strategy
} else {
strategy.invert()
};
if let Some(direction) = strategy.direction {
let mut neighbor_terminals = self
@ -407,7 +503,7 @@ impl<'a> TiledPaneGrid<'a> {
};
if self
.change_pane_size(pane_id, &new_strategy, change_by)
.with_context(err_context)?
.unwrap_or(false)
{
return Ok(true);
}

View file

@ -224,8 +224,8 @@ pub enum ScreenInstruction {
SearchToggleCaseSensitivity(ClientId),
SearchToggleWholeWord(ClientId),
SearchToggleWrap(ClientId),
AddRedPaneFrameColorOverride(PaneId, Option<String>), // Option<String> => optional error text
ClearPaneFrameColorOverride(PaneId),
AddRedPaneFrameColorOverride(Vec<PaneId>, Option<String>), // Option<String> => optional error text
ClearPaneFrameColorOverride(Vec<PaneId>),
}
impl From<&ScreenInstruction> for ScreenContext {
@ -2120,24 +2120,28 @@ pub(crate) fn screen_thread_main(
screen.render()?;
screen.unblock_input()?;
},
ScreenInstruction::AddRedPaneFrameColorOverride(pane_id, error_text) => {
ScreenInstruction::AddRedPaneFrameColorOverride(pane_ids, error_text) => {
let all_tabs = screen.get_tabs_mut();
for pane_id in pane_ids {
for tab in all_tabs.values_mut() {
if tab.has_pane_with_pid(&pane_id) {
tab.add_red_pane_frame_color_override(pane_id, error_text);
tab.add_red_pane_frame_color_override(pane_id, error_text.clone());
break;
}
}
}
screen.render()?;
},
ScreenInstruction::ClearPaneFrameColorOverride(pane_id) => {
ScreenInstruction::ClearPaneFrameColorOverride(pane_ids) => {
let all_tabs = screen.get_tabs_mut();
for pane_id in pane_ids {
for tab in all_tabs.values_mut() {
if tab.has_pane_with_pid(&pane_id) {
tab.clear_pane_frame_color_override(pane_id);
break;
}
}
}
screen.render()?;
},
}

View file

@ -1048,7 +1048,7 @@ impl Tab {
if let Some(active_pane_id) = self.tiled_panes.get_active_pane_id(client_id) {
self.senders
.send_to_background_jobs(BackgroundJob::DisplayPaneError(
active_pane_id,
vec![active_pane_id],
"TOO SMALL!".into(),
))
.with_context(err_context)?;
@ -1102,7 +1102,7 @@ impl Tab {
if let Some(active_pane_id) = self.tiled_panes.get_active_pane_id(client_id) {
self.senders
.send_to_background_jobs(BackgroundJob::DisplayPaneError(
active_pane_id,
vec![active_pane_id],
"TOO SMALL!".into(),
))
.with_context(err_context)?;
@ -1630,6 +1630,7 @@ impl Tab {
self.should_clear_display_before_rendering = true;
}
pub fn resize(&mut self, client_id: ClientId, strategy: ResizeStrategy) -> Result<()> {
let err_context = || format!("unable to resize pane");
if self.floating_panes.panes_are_visible() {
let successfully_resized = self
.floating_panes
@ -1639,9 +1640,28 @@ impl Tab {
self.set_force_render(); // we force render here to make sure the panes under the floating pane render and don't leave "garbage" in case of a decrease
}
} else {
self.tiled_panes
.resize_active_pane(client_id, &strategy)
.unwrap();
match self.tiled_panes.resize_active_pane(client_id, &strategy) {
Ok(_) => {},
Err(err) => match err.downcast_ref::<ZellijError>() {
Some(ZellijError::CantResizeFixedPanes { pane_ids }) => {
let mut pane_ids_to_error = vec![];
for (id, is_terminal) in pane_ids {
if *is_terminal {
pane_ids_to_error.push(PaneId::Terminal(*id));
} else {
pane_ids_to_error.push(PaneId::Plugin(*id));
};
}
self.senders
.send_to_background_jobs(BackgroundJob::DisplayPaneError(
pane_ids_to_error,
"FIXED!".into(),
))
.with_context(err_context)?;
},
_ => Err::<(), _>(err).fatal(),
},
}
}
Ok(())
}

View file

@ -231,14 +231,7 @@ fn create_new_tab_with_layout(size: Size, layout: PaneLayout) -> Tab {
for i in 0..layout.extract_run_instructions().len() {
new_terminal_ids.push((i as u32, None));
}
tab.apply_layout(
layout,
// vec![(1, None), (2, None)],
new_terminal_ids,
HashMap::new(),
index,
client_id,
)
tab.apply_layout(layout, new_terminal_ids, HashMap::new(), index, client_id)
.unwrap();
tab
}
@ -5932,6 +5925,144 @@ pub fn cannot_resize_down_when_pane_below_is_at_minimum_height() {
);
}
#[test]
pub fn cannot_resize_down_when_pane_has_fixed_rows() {
// ┌───────────┐ ┌───────────┐
// │███████████│ │███████████│
// ├───────────┤ ==resize=down==> ├───────────┤
// │ │ │ │
// └───────────┘ └───────────┘
// █ == focused pane
let size = Size {
cols: 121,
rows: 20,
};
let mut initial_layout = PaneLayout::default();
initial_layout.children_split_direction = SplitDirection::Horizontal;
let mut fixed_child = PaneLayout::default();
fixed_child.split_size = Some(SplitSize::Fixed(10));
initial_layout.children = vec![fixed_child, PaneLayout::default()];
let mut tab = create_new_tab_with_layout(size, initial_layout);
tab_resize_down(&mut tab, 1);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(0))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 1 height stayed the same"
);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(1))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 2 height stayed the same"
);
}
#[test]
pub fn cannot_resize_down_when_pane_below_has_fixed_rows() {
// ┌───────────┐ ┌───────────┐
// │███████████│ │███████████│
// ├───────────┤ ==resize=down==> ├───────────┤
// │ │ │ │
// └───────────┘ └───────────┘
// █ == focused pane
let size = Size {
cols: 121,
rows: 20,
};
let mut initial_layout = PaneLayout::default();
initial_layout.children_split_direction = SplitDirection::Horizontal;
let mut fixed_child = PaneLayout::default();
fixed_child.split_size = Some(SplitSize::Fixed(10));
initial_layout.children = vec![PaneLayout::default(), fixed_child];
let mut tab = create_new_tab_with_layout(size, initial_layout);
tab_resize_down(&mut tab, 1);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(0))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 1 height stayed the same"
);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(1))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 2 height stayed the same"
);
}
#[test]
pub fn cannot_resize_up_when_pane_below_has_fixed_rows() {
// ┌───────────┐ ┌───────────┐
// │███████████│ │███████████│
// ├───────────┤ ==resize=down==> ├───────────┤
// │ │ │ │
// └───────────┘ └───────────┘
// █ == focused pane
let size = Size {
cols: 121,
rows: 20,
};
let mut initial_layout = PaneLayout::default();
initial_layout.children_split_direction = SplitDirection::Horizontal;
let mut fixed_child = PaneLayout::default();
fixed_child.split_size = Some(SplitSize::Fixed(10));
initial_layout.children = vec![PaneLayout::default(), fixed_child];
let mut tab = create_new_tab_with_layout(size, initial_layout);
tab_resize_up(&mut tab, 1);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(0))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 1 height stayed the same"
);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(1))
.unwrap()
.position_and_size()
.rows
.as_usize(),
10,
"pane 2 height stayed the same"
);
}
#[test]
pub fn resize_left_with_pane_to_the_left() {
// ┌─────┬─────┐ ┌───┬───────┐
@ -11217,6 +11348,52 @@ pub fn cannot_resize_right_when_pane_to_the_left_is_at_minimum_width() {
);
}
#[test]
pub fn cannot_resize_right_when_pane_has_fixed_columns() {
// ┌──┬──┐ ┌──┬──┐
// │██│ │ │██│ │
// │██│ │ ==resize=right==> │██│ │
// │██│ │ │██│ │
// └──┴──┘ └──┴──┘
// █ == focused pane
let size = Size {
cols: 121,
rows: 20,
};
let mut initial_layout = PaneLayout::default();
initial_layout.children_split_direction = SplitDirection::Vertical;
let mut fixed_child = PaneLayout::default();
fixed_child.split_size = Some(SplitSize::Fixed(60));
initial_layout.children = vec![fixed_child, PaneLayout::default()];
let mut tab = create_new_tab_with_layout(size, initial_layout);
tab_resize_down(&mut tab, 1);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(0))
.unwrap()
.position_and_size()
.cols
.as_usize(),
60,
"pane 1 height stayed the same"
);
assert_eq!(
tab.tiled_panes
.panes
.get(&PaneId::Terminal(1))
.unwrap()
.position_and_size()
.cols
.as_usize(),
61,
"pane 2 height stayed the same"
);
}
#[test]
pub fn resize_up_with_pane_above() {
// ┌───────────┐ ┌───────────┐

View file

@ -495,6 +495,14 @@ open an issue on GitHub:
source: anyhow::Error,
},
// this is a temporary hack until we're able to merge custom errors from within the various
// crates themselves without having to move their payload types here
#[error("Cannot resize fixed panes")]
CantResizeFixedPanes { pane_ids: Vec<(u32, bool)> }, // bool: 0 => terminal_pane, 1 =>
// plugin_pane
#[error("Pane size remains unchanged")]
PaneSizeUnchanged,
#[error("an error occured")]
GenericError { source: anyhow::Error },
}

View file

@ -79,6 +79,10 @@ impl Dimension {
}
}
pub fn set_percent(&mut self, percent: f64) {
self.constraint = Constraint::Percent(percent);
}
pub fn set_inner(&mut self, inner: usize) {
self.inner = inner;
}