From 2347edeed6b733f21ae58e7f44081894b115d208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Tue, 6 Oct 2026 16:21:39 -0400 Subject: [PATCH] =?UTF-8?q?Panics=20=C2=B7=20P03=20Layout:=20panes=20and?= =?UTF-8?q?=20splits=20meet=20stale=20ids,=20lagging=20sizes=20and=20mid-d?= =?UTF-8?q?rag=20changes=20without=20a=20panic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/layout/grid.rs | 7 ++- src/layout/panes.rs | 56 ++++++++++++++---- src/layout/split.rs | 43 +++++++++----- src/layout/tests.rs | 136 ++++++++++++++++++++++++++++++++++++++++++-- tasks/panics.md | 3 +- 5 files changed, 214 insertions(+), 31 deletions(-) diff --git a/src/layout/grid.rs b/src/layout/grid.rs index cbcdab0..8f0d8ee 100644 --- a/src/layout/grid.rs +++ b/src/layout/grid.rs @@ -22,7 +22,7 @@ fn shortest(heights: &[Pixels]) -> usize { heights .iter() .enumerate() - .min_by(|a, b| a.1.partial_cmp(b.1).expect("heights are finite")) + .min_by(|a, b| f32::from(*a.1).total_cmp(&f32::from(*b.1))) .map(|(ix, _)| ix) .expect("at least one column") } @@ -182,6 +182,11 @@ mod tests { use super::{columns_for, fit, shortest}; + #[test] + fn a_nan_height_never_stops_the_shortest_column() { + assert_eq!(shortest(&[px(f32::NAN), px(4.0), px(2.0)]), 2); + } + #[test] fn what_fits_sits_centered() { assert_eq!(fit(px(100.0), px(3.0), px(2.0)), (20, px(1.0))); diff --git a/src/layout/panes.rs b/src/layout/panes.rs index e941dee..86abaf9 100644 --- a/src/layout/panes.rs +++ b/src/layout/panes.rs @@ -118,16 +118,23 @@ impl PaneLayout { true } - fn set_sizes(&mut self, path: &[usize], shares: &[f32]) { + /// Sets the split at `path`; false if the tree no longer has it. + fn set_sizes(&mut self, path: &[usize], shares: &[f32]) -> bool { let PaneLayout::Split { sizes, children, .. } = self else { - panic!("pane layout path points past a leaf"); + return false; }; match path.split_first() { - None => *sizes = shares.to_vec(), - Some((ix, rest)) => children[*ix].set_sizes(rest, shares), + None if shares.len() == sizes.len() => { + *sizes = shares.to_vec(); + true + } + None => false, + Some((ix, rest)) => children + .get_mut(*ix) + .is_some_and(|child| child.set_sizes(rest, shares)), } } } @@ -162,21 +169,30 @@ impl PaneGroup { out } - /// Opens a new pane beside `pane` and focuses it. - pub fn split(&mut self, pane: PaneId, axis: Axis, cx: &mut Context) -> PaneId { + /// Opens a new pane beside `pane` and focuses it; `None` if `pane` is gone. + pub fn split(&mut self, pane: PaneId, axis: Axis, cx: &mut Context) -> Option { let new = self.next; - assert!(self.root.split(pane, axis, new), "no pane {pane} to split"); + if !self.root.split(pane, axis, new) { + log::error!("pane group: no pane {pane} to split"); + return None; + } self.next = new.checked_add(1).expect("pane ids exhausted"); self.focused = new; log::info!("pane group: split {pane} -> {new} ({axis:?})"); cx.notify(); - new + Some(new) } - /// Panics on the last pane; its close button is disabled. + /// Closes `pane`; the last pane, or one already gone, stays as it is. pub fn close(&mut self, pane: PaneId, cx: &mut Context) { - assert!(self.panes().len() > 1, "cannot close the last pane"); - assert!(self.root.close(pane), "no pane {pane} to close"); + if self.panes().len() < 2 { + log::error!("pane group: pane {pane} is the last; it stays"); + return; + } + if !self.root.close(pane) { + log::error!("pane group: no pane {pane} to close"); + return; + } if self.focused == pane { self.focused = self.panes()[0]; } @@ -185,6 +201,10 @@ impl PaneGroup { } pub fn focus(&mut self, pane: PaneId, cx: &mut Context) { + if !self.panes().contains(&pane) { + log::error!("pane group: no pane {pane} to focus"); + return; + } self.focused = pane; cx.notify(); } @@ -225,7 +245,9 @@ impl PaneGroup { let mut split = SplitPane::new(gpui::SharedString::from(key), *axis, min) .sizes(sizes) .on_resize(cx.listener(move |group: &mut Self, shares: &[f32], _, _| { - group.root.set_sizes(&at, shares) + if !group.root.set_sizes(&at, shares) { + log::error!("pane group: no split at {at:?} for {shares:?}"); + } })); for (ix, child) in children.iter().enumerate() { let mut child_path = path.clone(); @@ -412,6 +434,16 @@ mod tests { assert_eq!(sizes(&kept), [0.25, 0.75]); } + #[test] + fn a_stale_split_path_sets_nothing() { + let mut root = PaneLayout::Pane(1); + root.split(1, Axis::Horizontal, 2); + assert!(!root.set_sizes(&[0], &[0.5, 0.5]), "a leaf"); + assert!(!root.set_sizes(&[4], &[0.5, 0.5]), "past the children"); + assert!(!root.set_sizes(&[], &[1.0]), "a share too few"); + assert!(root.set_sizes(&[], &[0.3, 0.7])); + } + #[test] fn close_folds_single_children() { let mut root = PaneLayout::Pane(1); diff --git a/src/layout/split.rs b/src/layout/split.rs index 24d3cbc..917a137 100644 --- a/src/layout/split.rs +++ b/src/layout/split.rs @@ -137,20 +137,30 @@ impl RenderOnce for SplitPane { fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement { let count = self.panes.len(); assert!(count >= 2, "split pane {:?} needs two panes", self.id); - let weights = self.sizes.unwrap_or_else(|| vec![1.0; count]); - assert_eq!( - weights.len(), - count, - "split pane {:?}: one size per pane", - self.id - ); - let total = check_weights(&weights) - .unwrap_or_else(|error| panic!("split pane {:?}: {error:#}", self.id)); - let normalized: Vec = weights.iter().map(|weight| weight / total).collect(); - let fractions = use_seeded(self.id.clone(), normalized, window, cx); + let even = vec![1.0 / count as f32; count]; + let normalized = match self.sizes.clone() { + None => even, + Some(weights) if weights.len() != count => { + log::error!( + "split pane {:?}: {} sizes for {count} panes; even shares", + self.id, + weights.len() + ); + even + } + Some(weights) => match check_weights(&weights) { + Ok(total) => weights.iter().map(|weight| weight / total).collect(), + Err(error) => { + log::error!("split pane {:?}: {error:#}; even shares", self.id); + even + } + }, + }; + let seed = (self.sizes, normalized); + let fractions = use_seeded(self.id.clone(), seed, window, cx); let owner = fractions.entity_id(); let on_resize = self.on_resize; - let shares = fractions.read(cx).value.clone(); + let shares = fractions.read(cx).value.1.clone(); let (axis, min) = (self.axis, self.min); let mut root = div().id(self.id).size_full().flex().on_drag_move( move |event: &DragMoveEvent, window, cx| { @@ -158,6 +168,13 @@ impl RenderOnce for SplitPane { if drag.owner != owner { return; } + if drag.start.len() != count { + log::error!( + "split pane: a drag over {} panes ends at {count}", + drag.start.len() + ); + return; + } let (along, extent) = match axis { Axis::Horizontal => (event.event.position.x, event.bounds.size.width), Axis::Vertical => (event.event.position.y, event.bounds.size.height), @@ -179,7 +196,7 @@ impl RenderOnce for SplitPane { report(&next, window, cx); } fractions.update(cx, |shares, cx| { - shares.value = next; + shares.value.1 = next; cx.notify(); }); }, diff --git a/src/layout/tests.rs b/src/layout/tests.rs index 0edfcb3..10410dd 100644 --- a/src/layout/tests.rs +++ b/src/layout/tests.rs @@ -1,12 +1,12 @@ use std::time::Duration; use gpui::{ - AnyElement, App, Bounds, Context, InteractiveElement, IntoElement, Modifiers, MouseButton, - ParentElement, Pixels, Point, Render, Styled, TestAppContext, VisualTestContext, Window, div, - point, px, + AnyElement, App, AppContext, Axis, Bounds, Context, InteractiveElement, IntoElement, Modifiers, + MouseButton, ParentElement, Pixels, Point, Render, Styled, TestAppContext, VisualTestContext, + Window, div, point, px, }; -use super::Drawer; +use super::{Drawer, PaneGroup, SplitPane}; use crate::theme::Theme; type Build = Box AnyElement>; @@ -126,3 +126,131 @@ fn a_released_drawer_glides_back(cx: &mut TestAppContext) { "the panel glides back from {lift:?} below rest" ); } + +#[gpui::test] +fn a_pane_group_leaves_a_gone_or_last_pane_as_it_is(cx: &mut TestAppContext) { + let group = cx.new(|_| PaneGroup::new(|_, _, _| div().into_any_element())); + group.update(cx, |group, cx| { + assert_eq!(group.split(9, Axis::Horizontal, cx), None); + group.close(1, cx); + assert_eq!(group.panes(), [1], "the last pane stays"); + let second = group.split(1, Axis::Horizontal, cx).expect("pane 1 splits"); + group.close(second, cx); + group.close(second, cx); + group.focus(second, cx); + assert_eq!((group.panes(), group.focused()), (vec![1], 1)); + }); +} + +/// Three panes given two sizes. +struct Lagging; + +impl Render for Lagging { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let pane = |name: &'static str| div().debug_selector(move || name.into()).size_full(); + div().w(px(300.0)).h(px(100.0)).child( + SplitPane::new("lagging", Axis::Horizontal, px(10.0)) + .sizes(&[1.0, 3.0]) + .pane(pane("first")) + .pane(pane("second")) + .pane(pane("third")), + ) + } +} + +#[gpui::test] +fn sizes_that_lag_their_panes_split_evenly(cx: &mut TestAppContext) { + cx.update(Theme::init); + let (_, cx) = cx.add_window_view(|_, _| Lagging); + cx.run_until_parked(); + let widths = + ["first", "second", "third"].map(|name| cx.debug_bounds(name).expect(name).size.width); + assert_eq!(widths[0], widths[2]); + assert_eq!(widths[1], widths[2]); +} + +/// Two panes that may grow a third, with sizes that may turn invalid. +struct Changing { + grown: bool, + invalid: bool, +} + +impl Render for Changing { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let pane = |name: &'static str| div().debug_selector(move || name.into()).size_full(); + let sizes = if self.invalid { [0.0, 0.0] } else { [1.0, 1.0] }; + let mut split = SplitPane::new("changing", Axis::Horizontal, px(10.0)) + .sizes(&sizes) + .pane(pane("first")) + .pane(pane("second")); + if self.grown { + split = split.pane(pane("third")); + } + div().w(px(300.0)).h(px(100.0)).child(split) + } +} + +fn drag_divider(cx: &mut VisualTestContext) -> Point { + cx.run_until_parked(); + let first = cx.debug_bounds("first").expect("first"); + let second = cx.debug_bounds("second").expect("second"); + let grab = point((first.right() + second.left()) / 2.0, px(50.0)); + cx.simulate_mouse_move(grab, None, Modifiers::none()); + cx.simulate_mouse_down(grab, MouseButton::Left, Modifiers::none()); + for step in [8.0, 8.0, 16.0] { + cx.simulate_mouse_move( + grab + point(px(step), px(0.0)), + MouseButton::Left, + Modifiers::none(), + ); + cx.run_until_parked(); + } + let moved = cx.debug_bounds("first").expect("first").size.width; + assert!(moved > first.size.width, "the drag moved the divider"); + grab +} + +#[gpui::test] +fn a_pane_added_mid_drag_drops_the_drag(cx: &mut TestAppContext) { + cx.update(Theme::init); + let (host, cx) = cx.add_window_view(|_, _| Changing { + grown: false, + invalid: false, + }); + let grab = drag_divider(cx); + host.update(cx, |host, cx| { + host.grown = true; + cx.notify(); + }); + cx.run_until_parked(); + cx.simulate_mouse_move( + grab + point(px(24.0), px(0.0)), + MouseButton::Left, + Modifiers::none(), + ); + cx.run_until_parked(); + assert!(cx.debug_bounds("third").is_some()); +} + +#[gpui::test] +fn invalid_sizes_after_a_drag_split_evenly(cx: &mut TestAppContext) { + cx.update(Theme::init); + let (host, cx) = cx.add_window_view(|_, _| Changing { + grown: false, + invalid: false, + }); + let grab = drag_divider(cx); + cx.simulate_mouse_up( + grab + point(px(16.0), px(0.0)), + MouseButton::Left, + Modifiers::none(), + ); + host.update(cx, |host, cx| { + host.invalid = true; + cx.notify(); + }); + cx.run_until_parked(); + let first = cx.debug_bounds("first").expect("first").size.width; + let second = cx.debug_bounds("second").expect("second").size.width; + assert_eq!(first, second); +} diff --git a/tasks/panics.md b/tasks/panics.md index 020b087..ffcd1d1 100644 --- a/tasks/panics.md +++ b/tasks/panics.md @@ -17,7 +17,7 @@ Each item reads every site in its folders, fixes what the rules name, updates ca - [x] P00 Policy in AGENTS.md - [x] P01 Primitives — `src/primitives`, `src/lib.rs`, `src/assets.rs` - [x] P02 Typography — `src/typography` -- [ ] P03 Layout — `src/layout` +- [x] P03 Layout — `src/layout` - [ ] P04 Window & Shell — `src/shell` - [ ] P05 Buttons & Actions — `src/buttons` - [ ] P06 Forms — `src/forms` @@ -67,3 +67,4 @@ One line per item: what changed, what stayed and why, review rounds. - P00: AGENTS.md holds the three rules; this list follows the PRD's chapters. Review: 2 rounds (round 1 found the runtime clause missing). - P01: `init` returns `anyhow::Result`; a missing app icon logs once and shows `Icon::broken`. Kept: `layer.rs` expects (gpui's element order), `checked_ratio` (one value, the caller's). Review: 2 rounds. - P02: `Link::new` takes its handler; `system_zone` logs once and reads as jiff's unknown zone, which `datetime` marks UTC; an oversized highlight query logs and marks nothing; a cut line's paint fault logs. Kept: caller contracts (speed, digits, step, keystroke, heading level, pattern) and the exponent parse. Review: 3 rounds. +- P03: `PaneGroup` leaves a gone or last pane and logs (`split` returns `Option`); a stale split path sets nothing; `SplitPane` splits evenly when sizes lag or fail, seeds by the owner's raw sizes, and drops a drag over another pane count; masonry sorts with `total_cmp`. Kept: dock ids (panels never leave), one-value contracts, pane-id overflow, workspace JSON. Review: 2 rounds.