diff --git a/CHANGELOG.md b/CHANGELOG.md index a3b745a3..4ecba5c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) * fix(grid): reap sixel images on clear (https://github.com/zellij-org/zellij/pull/3982) * fix(panes): properly render stacked panes when pane frames are disabled (https://github.com/zellij-org/zellij/pull/4035) * fix(panes): break ties by last focus time when focusing panes on screen edge (https://github.com/zellij-org/zellij/pull/4037) +* fix(serialization): properly serialize and handle multiple stacks in layouts (https://github.com/zellij-org/zellij/pull/4041) * fix(multiplayer): some issues in conjunction with stacked panes and multiple users (https://github.com/zellij-org/zellij/pull/4038) ## [0.41.2] - 2024-11-19 diff --git a/zellij-server/src/tab/mod.rs b/zellij-server/src/tab/mod.rs index 26fcb81b..e1fb5a8b 100644 --- a/zellij-server/src/tab/mod.rs +++ b/zellij-server/src/tab/mod.rs @@ -737,7 +737,7 @@ impl Tab { ) -> Result<()> { self.swap_layouts .set_base_layout((layout.clone(), floating_panes_layout.clone())); - if let Ok(should_show_floating_panes) = LayoutApplier::new( + match LayoutApplier::new( &self.viewport, &self.senders, &self.sixel_image_store, @@ -766,17 +766,25 @@ impl Tab { new_plugin_ids, client_id, ) { - #[allow(clippy::if_same_then_else)] - if should_show_floating_panes && !self.floating_panes.panes_are_visible() { - self.toggle_floating_panes(Some(client_id), None) - .non_fatal(); - } else if !should_show_floating_panes && self.floating_panes.panes_are_visible() { - self.toggle_floating_panes(Some(client_id), None) - .non_fatal(); - } - self.tiled_panes.reapply_pane_frames(); - self.is_pending = false; - self.apply_buffered_instructions().non_fatal(); + Ok(should_show_floating_panes) => { + if should_show_floating_panes && !self.floating_panes.panes_are_visible() { + self.toggle_floating_panes(Some(client_id), None) + .non_fatal(); + } else if !should_show_floating_panes && self.floating_panes.panes_are_visible() { + self.toggle_floating_panes(Some(client_id), None) + .non_fatal(); + } + self.tiled_panes.reapply_pane_frames(); + self.is_pending = false; + self.apply_buffered_instructions().non_fatal(); + }, + Err(e) => { + // TODO: this should only happen due to an erroneous layout created by user + // configuration that was somehow not caught in our KDL layout parser + // we should still be able to properly recover from this with a useful error + // message though + log::error!("Failed to apply layout: {}", e); + }, } Ok(()) } diff --git a/zellij-utils/src/input/layout.rs b/zellij-utils/src/input/layout.rs index 8e3dbd53..4738a23f 100644 --- a/zellij-utils/src/input/layout.rs +++ b/zellij-utils/src/input/layout.rs @@ -914,15 +914,25 @@ impl TiledPaneLayout { layout_to_split.focus_deepest_pane(); } + let mut stack_id = 0; split_space( space, &layout_to_split, space, ignore_percent_split_sizes, - 0, + &mut stack_id, + )? + }, + None => { + let mut stack_id = 0; + split_space( + space, + self, + space, + ignore_percent_split_sizes, + &mut stack_id, )? }, - None => split_space(space, self, space, ignore_percent_split_sizes, 0)?, }; for (_pane_layout, pane_geom) in layouts.iter() { if !pane_geom.is_at_least_minimum_size() { @@ -938,10 +948,17 @@ impl TiledPaneLayout { if self.children.is_empty() { run_instructions.push(self.run.clone()); } + let mut run_instructions_of_children = vec![]; for child in &self.children { let mut child_run_instructions = child.extract_run_instructions(); - run_instructions.append(&mut child_run_instructions); + // add the only first child to run_instructions only adding the others after all the + // childfree panes have been added so that the returned vec will be sorted breadth-first + if !child_run_instructions.is_empty() { + run_instructions.push(child_run_instructions.remove(0)); + } + run_instructions_of_children.append(&mut child_run_instructions); } + run_instructions.append(&mut run_instructions_of_children); let mut successfully_ignored = 0; for instruction_to_ignore in &self.run_instructions_to_ignore { if let Some(position) = run_instructions @@ -1617,7 +1634,7 @@ fn split_space( layout: &TiledPaneLayout, total_space_to_split: &PaneGeom, ignore_percent_split_sizes: bool, - mut next_stack_id: usize, + next_stack_id: &mut usize, ) -> Result, &'static str> { let sizes: Vec> = if layout.children_are_stacked { let index_of_expanded_pane = layout.children.iter().position(|p| p.is_expanded_in_stack); @@ -1631,8 +1648,9 @@ fn split_space( } else if let Some(last_size) = sizes.last_mut() { *last_size = None; } - if sizes.len() > space_to_split.rows.as_usize().saturating_sub(4) { - // 4 is MIN_TERMINAL_HEIGHT + if sizes.len() > space_to_split.rows.as_usize().saturating_sub(3) { + // 4 is MIN_TERMINAL_HEIGHT, minus 1 because sizes also includes the expanded pane in + // the stack return Err("Not enough room for stacked panes in this layout"); } sizes @@ -1687,8 +1705,8 @@ fn split_space( } }); let stacked = if layout.children_are_stacked { - let stack_id = next_stack_id; - next_stack_id += 1; + let stack_id = *next_stack_id; + *next_stack_id += 1; Some(stack_id) } else { None diff --git a/zellij-utils/src/session_serialization.rs b/zellij-utils/src/session_serialization.rs index 9ad23a5c..cfa3d997 100644 --- a/zellij-utils/src/session_serialization.rs +++ b/zellij-utils/src/session_serialization.rs @@ -1,5 +1,5 @@ use kdl::{KdlDocument, KdlEntry, KdlNode, KdlValue}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::path::PathBuf; use crate::{ @@ -630,6 +630,8 @@ fn serialize_multiple_tabs( ); if let Some(serialized) = serialized { serialized_tabs.push(serialized); + } else { + return Err("Failed to serialize session state"); } } Ok(serialized_tabs) @@ -665,6 +667,44 @@ fn serialize_floating_pane( floating_pane_node } +fn stack_layout_from_manifest( + geoms: &Vec, + split_size: Option, +) -> Option { + let mut children_stacks: HashMap> = HashMap::new(); + for p in geoms { + if let Some(stack_id) = p.geom.stacked { + children_stacks + .entry(stack_id) + .or_insert_with(Default::default) + .push(p.clone()); + } + } + let mut stack_nodes = vec![]; + for (_stack_id, stacked_panes) in children_stacks.into_iter() { + stack_nodes.push(TiledPaneLayout { + split_size, + children: stacked_panes + .iter() + .map(|p| tiled_pane_layout_from_manifest(Some(p), None)) + .collect(), + children_are_stacked: true, + ..Default::default() + }) + } + if stack_nodes.len() == 1 { + // if there's only one stack, we return it without a wrapper + stack_nodes.iter().next().cloned() + } else { + // here there is more than one stack, so we wrap it in a logical container node + Some(TiledPaneLayout { + split_size, + children: stack_nodes, + ..Default::default() + }) + } +} + fn tiled_pane_layout_from_manifest( manifest: Option<&PaneLayoutManifest>, split_size: Option, @@ -709,10 +749,16 @@ fn get_tiled_panes_layout_from_panegeoms( let (children_split_direction, splits) = match get_splits(&geoms) { Some(x) => x, None => { - return Some(tiled_pane_layout_from_manifest( - geoms.iter().next(), - split_size, - )) + if geoms.len() > 1 { + // this can only happen if all geoms belong to one or more stacks + // since stack splits are discounted in the get_splits method + return stack_layout_from_manifest(geoms, split_size); + } else { + return Some(tiled_pane_layout_from_manifest( + geoms.iter().next(), + split_size, + )); + } }, }; let mut children = Vec::new(); @@ -742,7 +788,17 @@ fn get_tiled_panes_layout_from_panegeoms( }, } } + + if let Some(SplitSize::Fixed(fixed_size)) = split_size { + if fixed_size == 1 && !new_geoms.is_empty() { + // invalid state, likely an off-by-one error somewhere, we do not serialize + log::error!("invalid state, not serializing"); + return None; + } + } + let new_split_sizes = get_split_sizes(&new_constraints); + for (subgeoms, subsplit_size) in new_geoms.iter().zip(new_split_sizes) { match get_tiled_panes_layout_from_panegeoms(&subgeoms, subsplit_size) { Some(child) => { @@ -892,11 +948,40 @@ fn get_row_splits( let mut splits = Vec::new(); let mut sorted_geoms = geoms.clone(); sorted_geoms.sort_by_key(|g| g.geom.y); - for y in sorted_geoms.iter().map(|g| g.geom.y) { + + // here we make sure the various panes in all the stacks aren't counted as splits, since + // stacked panes must always stay togethyer - we group them into one "geom" for the purposes + // of figuring out their splits + let mut stack_geoms: HashMap> = HashMap::new(); + let mut all_geoms = vec![]; + for pane_layout_manifest in sorted_geoms.drain(..) { + if let Some(stack_id) = pane_layout_manifest.geom.stacked { + stack_geoms + .entry(stack_id) + .or_insert_with(Default::default) + .push(pane_layout_manifest) + } else { + all_geoms.push(pane_layout_manifest); + } + } + for (_stack_id, mut geoms_in_stack) in stack_geoms.into_iter() { + let mut geom_of_whole_stack = geoms_in_stack.remove(0); + if let Some(last_geom) = geoms_in_stack.last() { + geom_of_whole_stack + .geom + .rows + .set_inner(last_geom.geom.y + last_geom.geom.rows.as_usize()) + } + all_geoms.push(geom_of_whole_stack); + } + + all_geoms.sort_by_key(|g| g.geom.y); + + for y in all_geoms.iter().map(|g| g.geom.y) { if splits.contains(&y) { continue; } - if sorted_geoms + if all_geoms .iter() .filter(|g| g.geom.y == y) .map(|g| g.geom.cols.as_usize()) @@ -1641,6 +1726,286 @@ mod tests { assert_snapshot!(kdl.0); } #[test] + fn can_serialize_tab_with_multiple_stacked_panes_in_the_same_node() { + let tab_layout_manifest = TabLayoutManifest { + tiled_panes: vec![ + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 0, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 1, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 11, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 12, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: None, + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 22, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 23, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 33, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + ], + ..Default::default() + }; + let global_layout_manifest = GlobalLayoutManifest { + tabs: vec![("Tab with \"stacked panes\"".to_owned(), tab_layout_manifest)], + ..Default::default() + }; + let kdl = serialize_session_layout(global_layout_manifest).unwrap(); + assert_snapshot!(kdl.0); + } + #[test] + fn can_serialize_tab_with_multiple_stacks_next_to_eachother() { + let tab_layout_manifest = TabLayoutManifest { + tiled_panes: vec![ + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 0, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 1, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 11, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(0), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 12, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: None, + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 22, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 23, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 0, + y: 33, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(1), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 0, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(2), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 1, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(2), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 11, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(2), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 12, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: None, + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 22, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(3), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 23, + rows: Dimension::fixed(10), + cols: Dimension::fixed(10), + stacked: Some(3), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + PaneLayoutManifest { + geom: PaneGeom { + x: 10, + y: 33, + rows: Dimension::fixed(1), + cols: Dimension::fixed(10), + stacked: Some(3), + is_pinned: false, + logical_position: None, + }, + ..Default::default() + }, + ], + ..Default::default() + }; + let global_layout_manifest = GlobalLayoutManifest { + tabs: vec![("Tab with \"stacked panes\"".to_owned(), tab_layout_manifest)], + ..Default::default() + }; + let kdl = serialize_session_layout(global_layout_manifest).unwrap(); + assert_snapshot!(kdl.0); + } + #[test] fn can_serialize_multiple_tabs() { let tab_1_layout_manifest = TabLayoutManifest { tiled_panes: vec![PaneLayoutManifest { diff --git a/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacked_panes_in_the_same_node.snap b/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacked_panes_in_the_same_node.snap new file mode 100644 index 00000000..701c9baa --- /dev/null +++ b/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacked_panes_in_the_same_node.snap @@ -0,0 +1,21 @@ +--- +source: zellij-utils/src/session_serialization.rs +assertion_line: 1899 +expression: kdl.0 +--- +layout { + tab name="Tab with \"stacked panes\"" { + pane size=12 stacked=true { + pane + pane expanded=true + pane + } + pane size=10 + pane size=12 stacked=true { + pane + pane expanded=true + pane + } + } +} + diff --git a/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacks_next_to_eachother.snap b/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacks_next_to_eachother.snap new file mode 100644 index 00000000..883da9d9 --- /dev/null +++ b/zellij-utils/src/snapshots/zellij_utils__session_serialization__tests__can_serialize_tab_with_multiple_stacks_next_to_eachother.snap @@ -0,0 +1,38 @@ +--- +source: zellij-utils/src/session_serialization.rs +assertion_line: 2081 +expression: kdl.0 +--- +layout { + tab name="Tab with \"stacked panes\"" { + pane size=12 split_direction="vertical" { + pane size=10 stacked=true { + pane + pane expanded=true + pane + } + pane size=10 stacked=true { + pane + pane expanded=true + pane + } + } + pane size=10 split_direction="vertical" { + pane size=10 + pane size=10 + } + pane size=12 split_direction="vertical" { + pane size=10 stacked=true { + pane + pane expanded=true + pane + } + pane size=10 stacked=true { + pane + pane expanded=true + pane + } + } + } +} +