fix(serialization): stacked panes fixes (#4041)

* fix(serialization): properly handle multiple stacked panes in the same logical node

* fix(layouts): properly handle stacked panes in new layouts

* fix(layouts): properly serialize stacked panes into layouts

* style(fmt): rustfmt

* docs(changelog): update pr

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

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)]
+            Ok(should_show_floating_panes) => {
-            if should_show_floating_panes && !self.floating_panes.panes_are_visible() {
+                if should_show_floating_panes && !self.floating_panes.panes_are_visible() {
-                self.toggle_floating_panes(Some(client_id), None)
+                    self.toggle_floating_panes(Some(client_id), None)
-                    .non_fatal();
+                        .non_fatal();
-            } else if !should_show_floating_panes && self.floating_panes.panes_are_visible() {
+                } else if !should_show_floating_panes && self.floating_panes.panes_are_visible() {
-                self.toggle_floating_panes(Some(client_id), None)
+                    self.toggle_floating_panes(Some(client_id), None)
-                    .non_fatal();
+                        .non_fatal();
-            }
+                }
-            self.tiled_panes.reapply_pane_frames();
+                self.tiled_panes.reapply_pane_frames();
-            self.is_pending = false;
+                self.is_pending = false;
-            self.apply_buffered_instructions().non_fatal();
+                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(())
     }

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<Vec<(TiledPaneLayout, PaneGeom)>, &'static str> {
     let sizes: Vec<Option<SplitSize>> = 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) {
+        if sizes.len() > space_to_split.rows.as_usize().saturating_sub(3) {
-            // 4 is MIN_TERMINAL_HEIGHT
+            // 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;
+        let stack_id = *next_stack_id;
-        next_stack_id += 1;
+        *next_stack_id += 1;
         Some(stack_id)
     } else {
         None

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<PaneLayoutManifest>,
+    split_size: Option<SplitSize>,
+) -> Option<TiledPaneLayout> {
+    let mut children_stacks: HashMap<usize, Vec<PaneLayoutManifest>> = 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<SplitSize>,
@@ -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(
+            if geoms.len() > 1 {
-                geoms.iter().next(),
+                // this can only happen if all geoms belong to one or more stacks
-                split_size,
+                // 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<usize, Vec<PaneLayoutManifest>> = 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 {

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

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