diff options
| author | user <user@clank> | 2026-08-11 19:35:03 +0200 |
|---|---|---|
| committer | user <user@clank> | 2026-08-11 19:35:03 +0200 |
| commit | 29eadcc1f06bfc5b06ea8a56aa1a18da318a049f (patch) | |
| tree | 63cf440239d4fcc07f93b8a1791b2eff370330a9 /zellij-server | |
| parent | sl0p: keep expanding possible after the pane count changes (diff) | |
| download | sl0ppij-29eadcc1f06bfc5b06ea8a56aa1a18da318a049f.tar.gz sl0ppij-29eadcc1f06bfc5b06ea8a56aa1a18da318a049f.tar.xz sl0ppij-29eadcc1f06bfc5b06ea8a56aa1a18da318a049f.zip | |
sl0p: resolve responsive layouts from current state, not a remembered position
Every bug this feature has had came from the same place: it remembered the swap
layout position that was active before collapsing, and replayed it to expand.
Remembered positions stop applying when the pane count changes, are only stable
while nothing is inserted, and a failed apply left the state claiming a
transition that never happened.
Both directions now resolve fresh. Collapsing picks the configured layout, or a
synthesized stack if that one is missing OR cannot currently apply -- a
`stacked` layout with min_panes=4 and three panes open used to be found, fail
its constraint, and give up. Expanding picks BASE when it still applies, since
that reproduces the user's actual layout, and otherwise a synthesized grid.
Both chains end in an unconstrained layout, so neither can come up empty.
The only remaining state is one bool, flipped only after a layout was applied.
Also stops consulting is_tiled_damaged. It is meant to mean "the user
rearranged panes by hand" but is equally set when a relayout finds nothing
applicable, which happens on the first pane added to a tab with no usable swap
layout and then never clears -- it disabled this feature in four distinct
situations. It protected little regardless: expanding applies BASE or a grid,
so a manual arrangement does not survive a round trip either way.
Synthesized layouts are created once and reused; the test asserts the layout
list does not grow across repeated collapse/expand cycles, since they are now
appended lazily in both directions.
Diffstat (limited to 'zellij-server')
| -rw-r--r-- | zellij-server/src/tab/mod.rs | 233 | ||||
| -rw-r--r-- | zellij-server/src/tab/swap_layouts.rs | 38 | ||||
| -rw-r--r-- | zellij-server/src/tab/unit/tab_integration_tests.rs | 104 |
3 files changed, 213 insertions, 162 deletions
diff --git a/zellij-server/src/tab/mod.rs b/zellij-server/src/tab/mod.rs index 5719ba8c..65b272f8 100644 --- a/zellij-server/src/tab/mod.rs +++ b/zellij-server/src/tab/mod.rs @@ -162,6 +162,11 @@ const MAX_PENDING_VTE_EVENTS: usize = 7000; type HoldForCommand = Option<RunCommand>; pub type SuppressedPanes = HashMap<PaneId, (bool, Box<dyn Pane>)>; // bool => is scrollback editor +/// Internal names for the layouts the responsive collapse synthesizes. Prefixed +/// so they cannot collide with a name from a user's swap file. +const RESPONSIVE_COLLAPSE_LAYOUT_NAME: &str = "sl0p:responsive-collapse"; +const RESPONSIVE_EXPAND_LAYOUT_NAME: &str = "sl0p:responsive-expand"; + pub type StackListId = usize; #[derive(Debug, Clone)] @@ -377,9 +382,10 @@ pub(crate) struct Tab { /// the live layout acts as the preview. Undone when the drag moves to a /// different target. drag_preview_swapped_with: HashMap<ClientId, PaneId>, - /// Swap layout position to restore when the terminal grows back, set while - /// the tab is collapsed into the responsive layout. None = not collapsed. - responsive_restore_position: Option<usize>, + /// Whether the tab is currently collapsed by the responsive layout. The + /// only state the feature keeps: everything else is resolved from the panes + /// as they are, and this is flipped only after a layout was applied. + responsive_collapsed: bool, responsive_min_cols: usize, responsive_layout_name: String, /// Cap on picker rows; see stack_list_window. 0 = uncapped. @@ -1202,7 +1208,7 @@ impl Tab { frame_clicks: HashMap::new(), tiled_pane_being_dragged: HashMap::new(), drag_preview_swapped_with: HashMap::new(), - responsive_restore_position: None, + responsive_collapsed: false, responsive_min_cols: 0, responsive_layout_name: String::from("stacked"), stack_list_max_rows: 0, @@ -5469,148 +5475,119 @@ impl Tab { let selectable_tiled_panes = self.tiled_panes.get_panes().filter(|(_, p)| p.selectable()); selectable_tiled_panes.count() > 0 } - /// Collapse into the configured swap layout when the terminal is too narrow, - /// and restore the previous one when it grows back. + /// Collapse into a narrow layout when the terminal is too narrow, and spread + /// back out when it grows again. /// - /// Hysteresis: collapsing happens below `responsive_min_cols`, but restoring - /// only above `responsive_min_cols + RESPONSIVE_HYSTERESIS_COLS`. Without a - /// gap, dragging a terminal edge that lands exactly on the threshold flips + /// Hysteresis: collapsing happens below `responsive_min_cols`, expanding only + /// above `responsive_min_cols + RESPONSIVE_HYSTERESIS_COLS`. Without a dead + /// band, dragging a terminal edge that lands exactly on the threshold flips /// the layout on every motion event, and each flip resizes every PTY. /// - /// Does nothing once the user has manually rearranged panes - /// (`is_tiled_damaged`), on the same principle as auto_layout: an explicit - /// arrangement outranks an automatic one. - fn apply_responsive_layout( - &mut self, - new_size: Size, - was_damaged_before_resize: bool, - ) -> Result<()> { + /// BOTH directions resolve their target from the panes as they are now, + /// rather than replaying a remembered one. Remembering a position was the + /// source of every bug this feature has had: the remembered layout stops + /// applying when the pane count changes, positions are only stable if + /// nothing is ever inserted, and a failed apply left the state claiming a + /// transition that never happened. The only state kept now is one bit -- + /// which side of the threshold we are on -- and even that is flipped only + /// after a layout has actually been applied. + fn apply_responsive_layout(&mut self, new_size: Size) -> Result<()> { const RESPONSIVE_HYSTERESIS_COLS: usize = 10; - // internal, so it cannot collide with a name from a user's swap file - const RESPONSIVE_EXPAND_LAYOUT_NAME: &str = "sl0p:responsive-expand"; if self.responsive_min_cols == 0 { return Ok(()); } - // NB: deliberately the flag as sampled at the TOP of resize_whole_tab, - // not the live `self.swap_layouts.is_tiled_damaged()`. - // - // By the time we run, resize_whole_tab's own auto_layout pass has - // already set the flag as bookkeeping and then cleared it only if it - // found a layout that fits. On a terminal too narrow for any of the wide - // layouts it can find none, which leaves the flag true even though the - // user never rearranged anything -- and that is exactly the case this - // feature exists to handle. Sampling before the resize keeps the flag - // meaning "the user rearranged panes by hand" and nothing else. - // - // The restore_position exemption matters for the same reason: once we - // have collapsed, a later resize that damages the layout must not strand - // the user in the narrow layout forever, so restore stays reachable. - // Only meaningful when the tab actually has user-defined swap layouts; - // see SwapLayouts::has_user_defined_tiled_layouts for why the flag says - // nothing about user intent otherwise. - let damage_means_user_intent = self.swap_layouts.has_user_defined_tiled_layouts(); - if damage_means_user_intent - && was_damaged_before_resize - && self.responsive_restore_position.is_none() - { - return Ok(()); - } - let too_narrow = new_size.cols < self.responsive_min_cols; - let wide_again = - new_size.cols >= self.responsive_min_cols + RESPONSIVE_HYSTERESIS_COLS; - - let mut is_restore = false; - let mut target_position = match (self.responsive_restore_position, too_narrow, wide_again) { - // collapse: remember where we were, go to the responsive layout - (None, true, _) => { - let position = match self - .swap_layouts - .position_of_tiled_layout_named(&self.responsive_layout_name) - { - Some(position) => position, - None => { - // No layout by that name, which is the normal case: a - // custom layout file only gets swap layouts from a - // hand-written sibling `<layout>.swap.kdl`. Synthesize - // the one thing the collapse needs -- every pane - // flattened into a single stack, in layout order -- - // rather than making the feature depend on that file. - // - // Logged once per tab, because a synthesized layout is - // not what a user who DID write a swap file expects, and - // a silent fallback would hide the name not matching. - if !self.responsive_missing_layout_warned { - self.responsive_missing_layout_warned = true; - log::info!( - "no swap layout named {:?} in this tab, so the responsive collapse \ - synthesized one that flattens every pane into a single stack. \ - Define `swap_tiled_layout name={:?}` in a `<layout>.swap.kdl` \ - beside your layout file to control this yourself.", - self.responsive_layout_name, - self.responsive_layout_name - ); - } - self.swap_layouts - .append_synthesized_stack_layout(&self.responsive_layout_name) - }, - }; - self.responsive_restore_position = - Some(self.swap_layouts.current_tiled_layout_position()); - Some(position) - }, - // restore: back to whatever was active before collapsing - (Some(restore_to), _, true) => { - is_restore = true; - Some(restore_to) - }, - _ => None, + let collapse = match ( + self.responsive_collapsed, + new_size.cols < self.responsive_min_cols, + new_size.cols >= self.responsive_min_cols + RESPONSIVE_HYSTERESIS_COLS, + ) { + (false, true, _) => true, + (true, _, true) => false, + // inside the dead band, or already on the right side of it + _ => return Ok(()), }; - let Some(mut target_position) = target_position else { - return Ok(()); + let resolved = if collapse { + self.responsive_collapse_target() + } else { + self.responsive_expand_target() }; - let mut layout = self - .swap_layouts - .tiled_layout_at(target_position, &self.tiled_panes); - if layout.is_none() && is_restore { - // The layout that was active before the collapse no longer applies. - // Usually BASE: it is pinned to ExactPanes of the count it was - // written with, so adding or closing a single pane leaves nothing to - // expand into. Without a fallback the tab stays stacked for good, - // because a wide terminal is not a state that retries. - let position = self - .swap_layouts - .append_synthesized_grid_layout(RESPONSIVE_EXPAND_LAYOUT_NAME); - layout = self - .swap_layouts - .tiled_layout_at(position, &self.tiled_panes); - if layout.is_some() { - target_position = position; - } - } - let Some(layout) = layout else { - // Changed nothing, so leave the state alone: clearing the restore - // position here would mark the tab expanded while it is visibly - // still collapsed, and it would never try again. + let Some((position, layout)) = resolved else { + // Nothing applied, so change nothing -- not even the flag. Claiming + // a transition that did not happen is what previously left a tab + // stacked while believing it had expanded, with no path back. return Ok(()); }; - if is_restore { - self.responsive_restore_position = None; - } - self.swap_layouts.set_tiled_layout_position(target_position); + self.swap_layouts.set_tiled_layout_position(position); self.dissolve_stack_lists_for_classic_mutation(); self.apply_tiled_layout_candidate(&layout); self.tiled_panes.reapply_pane_frames(); self.set_should_clear_display_before_rendering(); + self.responsive_collapsed = collapse; Ok(()) } + /// Where to collapse to: the configured swap layout, else a synthesized + /// single stack. + /// + /// The synthesized one is kept under its own name rather than the + /// configured one, so a layout that exists but does not currently apply -- + /// a `stacked` layout with `min_panes=4` and three panes open, say -- falls + /// back instead of failing. + fn responsive_collapse_target(&mut self) -> Option<(usize, TiledPaneLayout)> { + if let Some(position) = self + .swap_layouts + .position_of_tiled_layout_named(&self.responsive_layout_name) + { + if let Some(layout) = self.swap_layouts.tiled_layout_at(position, &self.tiled_panes) { + return Some((position, layout)); + } + } + // Logged once per tab: a synthesized layout is not what someone who DID + // write a swap file expects, and staying silent would hide both a name + // that does not match and a constraint that never fits. + if !self.responsive_missing_layout_warned { + self.responsive_missing_layout_warned = true; + log::info!( + "no usable swap layout named {:?} in this tab, so the responsive collapse \ + synthesized one that flattens every pane into a single stack. Define \ + `swap_tiled_layout name={:?}` in a `<layout>.swap.kdl` beside your layout \ + file (and check its min_panes/max_panes) to control this yourself.", + self.responsive_layout_name, + self.responsive_layout_name + ); + } + let position = self + .swap_layouts + .append_synthesized_stack_layout(RESPONSIVE_COLLAPSE_LAYOUT_NAME); + self.swap_layouts + .tiled_layout_at(position, &self.tiled_panes) + .map(|layout| (position, layout)) + } + /// Where to expand to, resolved from the current panes. + /// + /// BASE first, because when the pane count has not changed it reproduces the + /// user's actual layout, which no synthesized grid can. It is pinned to + /// ExactPanes though, so a single added or closed pane rules it out -- hence + /// the grid, which carries no constraint and so always applies. + fn responsive_expand_target(&mut self) -> Option<(usize, TiledPaneLayout)> { + if let Some(position) = self + .swap_layouts + .position_of_tiled_layout_named(swap_layouts::BASE_LAYOUT_NAME) + { + if let Some(layout) = self.swap_layouts.tiled_layout_at(position, &self.tiled_panes) { + return Some((position, layout)); + } + } + let position = self + .swap_layouts + .append_synthesized_grid_layout(RESPONSIVE_EXPAND_LAYOUT_NAME); + self.swap_layouts + .tiled_layout_at(position, &self.tiled_panes) + .map(|layout| (position, layout)) + } pub fn resize_whole_tab(&mut self, new_screen_size: Size) -> Result<()> { let err_context = || format!("failed to resize whole tab (id {})", self.id); self.size = new_screen_size; - // Sample this BEFORE the auto_layout pass below mutates it, so the - // responsive switch can tell a genuine manual rearrangement from this - // function's own bookkeeping. See apply_responsive_layout. - let was_tiled_damaged = self.swap_layouts.is_tiled_damaged(); // If a tiled pane is fullscreen, exit fullscreen first so that *all* // tiled panes (including the currently hidden ones) participate in the // resize/relayout. We re-enter fullscreen on the same pane after the @@ -5676,10 +5653,8 @@ impl Tab { } self.resize_all_stack_list_hidden_members(); // after the geometry settles, decide whether this size warrants - // collapsing into (or restoring from) the responsive layout. - // was_tiled_damaged is sampled at the top of this function, before the - // auto_layout pass above overwrites it -- see apply_responsive_layout. - self.apply_responsive_layout(new_screen_size, was_tiled_damaged)?; + // collapsing or expanding + self.apply_responsive_layout(new_screen_size)?; Ok(()) } pub fn resize(&mut self, client_id: ClientId, strategy: ResizeStrategy) -> Result<()> { diff --git a/zellij-server/src/tab/swap_layouts.rs b/zellij-server/src/tab/swap_layouts.rs index 0fc45bb9..2739444b 100644 --- a/zellij-server/src/tab/swap_layouts.rs +++ b/zellij-server/src/tab/swap_layouts.rs @@ -9,6 +9,11 @@ use zellij_utils::{ pane_size::{PaneGeom, Size}, }; +/// Name zellij gives the tab's own layout when it inserts it as a swap layout. +/// Pinned to ExactPanes of the count it was written with, so it stops applying +/// as soon as a pane is added or closed. +pub const BASE_LAYOUT_NAME: &str = "BASE"; + #[derive(Clone, Debug, Default)] pub struct SwapLayouts { swap_tiled_layouts: Vec<SwapTiledLayout>, @@ -17,8 +22,6 @@ pub struct SwapLayouts { current_tiled_layout_position: usize, is_floating_damaged: bool, is_tiled_damaged: bool, - /// Positions of layouts synthesized for the responsive collapse. - synthesized_tiled_positions: Vec<usize>, display_area: Rc<RefCell<Size>>, // includes all panes (including eg. the status bar and tab bar in the default layout) } @@ -51,9 +54,9 @@ impl SwapLayouts { base_swap_floating_layout .insert(LayoutConstraint::ExactPanes(floating_panes_count), layout.1); self.swap_tiled_layouts - .insert(0, (base_swap_tiled_layout, Some("BASE".into()))); + .insert(0, (base_swap_tiled_layout, Some(BASE_LAYOUT_NAME.into()))); self.swap_floating_layouts - .insert(0, (base_swap_floating_layout, Some("BASE".into()))); + .insert(0, (base_swap_floating_layout, Some(BASE_LAYOUT_NAME.into()))); self.current_tiled_layout_position = 0; self.current_floating_layout_position = 0; } @@ -130,27 +133,7 @@ impl SwapLayouts { constraints.insert(constraint, layout); self.swap_tiled_layouts .push((constraints, Some(name.to_string()))); - let position = self.swap_tiled_layouts.len() - 1; - self.synthesized_tiled_positions.push(position); - position - } - /// Whether this tab has any tiled swap layout the USER defined. - /// - /// "BASE" is inserted from the tab's own layout, and the synthesized stack is - /// ours, so neither says anything about user intent. Matters because the - /// damaged flag means two different things: with user swap layouts it tracks - /// "the arrangement was changed by hand", but with none it is set on the - /// first pane added -- relayout finds nothing applicable and gives up -- and - /// then never clears, which would disable the responsive collapse entirely - /// for exactly the bare layouts it is most useful for. - pub fn has_user_defined_tiled_layouts(&self) -> bool { - self.swap_tiled_layouts - .iter() - .enumerate() - .any(|(position, (_constraints, name))| { - !self.synthesized_tiled_positions.contains(&position) - && name.as_deref() != Some("BASE") - }) + self.swap_tiled_layouts.len() - 1 } /// Position of the swap tiled layout with this name, if any. /// @@ -166,6 +149,11 @@ impl SwapLayouts { layout_name.as_deref().map(str::trim) == Some(name.trim()) }) } + /// How many tiled swap layouts exist. Used to assert that the synthesized + /// fallbacks are created once and reused, not appended on every resize. + pub fn tiled_layout_count(&self) -> usize { + self.swap_tiled_layouts.len() + } pub fn current_tiled_layout_position(&self) -> usize { self.current_tiled_layout_position } diff --git a/zellij-server/src/tab/unit/tab_integration_tests.rs b/zellij-server/src/tab/unit/tab_integration_tests.rs index 319fde2a..cfde77aa 100644 --- a/zellij-server/src/tab/unit/tab_integration_tests.rs +++ b/zellij-server/src/tab/unit/tab_integration_tests.rs @@ -16520,15 +16520,23 @@ fn sl0p_responsive_collapse_synthesizes_a_stack_when_the_layout_has_no_swap_layo widths(&tab) ); - // and the synthesis must not be repeated on every resize - tab.resize_whole_tab(Size { cols: 65, rows: 20 }).unwrap(); - tab.resize_whole_tab(narrow).unwrap(); + // Synthesized layouts must be created once and reused. They are appended + // lazily in BOTH directions now, so a bug here grows the layout list without + // bound, one entry per resize, for as long as the session lives. + let wide = Size { + cols: 121, + rows: 20, + }; + tab.resize_whole_tab(wide).unwrap(); + let after_first_cycle = tab.swap_layouts.tiled_layout_count(); + for _ in 0..5 { + tab.resize_whole_tab(narrow).unwrap(); + tab.resize_whole_tab(wide).unwrap(); + } assert_eq!( - tab.swap_layouts - .position_of_tiled_layout_named("stacked") - .is_some(), - true, - "the synthesized layout should be reusable, not rebuilt each time" + tab.swap_layouts.tiled_layout_count(), + after_first_cycle, + "synthesized layouts must be reused, not appended on every resize" ); } @@ -16624,3 +16632,83 @@ fn sl0p_responsive_expands_again_after_a_pane_is_added_while_collapsed() { widths(&tab) ); } + +#[test] +fn sl0p_responsive_collapses_even_when_the_named_layout_never_applies() { + // A swap layout can exist under the right name and still never apply: this + // one wants min_panes=4 and the tab has three. Resolving the collapse target + // fresh means falling back to a synthesized stack, where keying off the + // configured name alone would find the layout, fail its constraint, and give + // up -- looking exactly like the feature being broken. + let wide = Size { + cols: 121, + rows: 20, + }; + let client_id = 1; + let swap_layouts = r#" + layout { + swap_tiled_layout name="stacked" { + tab min_panes=4 { + pane stacked=true { children; } + } + } + } + "#; + let layout = Layout::from_kdl(swap_layouts, Some("file_name.kdl".into()), None, None).unwrap(); + let mut tab = create_new_tab_with_swap_layouts( + wide, + ModeInfo::default(), + ( + layout.swap_tiled_layouts.clone(), + layout.swap_floating_layouts.clone(), + ), + None, + true, + true, + ); + tab.update_responsive_layout(80, String::from("stacked")); + for pid in [2, 3] { + tab.new_pane( + PaneId::Terminal(pid), + None, + None, + false, + true, + NewPanePlacement::default(), + Some(client_id), + None, + ) + .unwrap(); + } + + let widths = |tab: &Tab| -> Vec<usize> { + let mut v: Vec<usize> = tab + .tiled_panes + .panes + .values() + .map(|p| p.position_and_size().cols.as_usize()) + .collect(); + v.sort_unstable(); + v + }; + let all_full_width = |tab: &Tab, cols: usize| widths(tab).iter().all(|w| *w == cols); + + // precondition: three panes, so the named layout's min_panes=4 cannot match + assert_eq!(tab.tiled_panes.panes.len(), 3); + + let narrow = Size { cols: 70, rows: 20 }; + tab.resize_whole_tab(narrow).unwrap(); + assert!( + all_full_width(&tab, narrow.cols), + "should fall back to a synthesized stack when the named layout cannot apply, \ + got widths {:?}", + widths(&tab) + ); + + tab.resize_whole_tab(wide).unwrap(); + assert!( + !all_full_width(&tab, wide.cols), + "and still expand again afterwards, got widths {:?}", + widths(&tab) + ); +} |
