Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/app/api/panes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -871,10 +871,6 @@ impl App {
self.recover_failed_pane_move(recovery_context, moved);
return encode_error(id, "pane_move_failed", "target tab disappeared");
};
let previous_target_focus = self.state.workspaces[target_ws_idx].tabs
[target_tab_idx]
.layout
.focused();
let direction = split_direction_to_layout(split);
let moved_pane_id = match self.state.workspaces[target_ws_idx]
.insert_moved_pane_into_tab(
Expand All @@ -883,6 +879,7 @@ impl App {
moved,
direction,
ratio,
focus,
) {
Ok(pane_id) => pane_id,
Err(moved) => {
Expand All @@ -894,11 +891,6 @@ impl App {
);
}
};
if !focus {
self.state.workspaces[target_ws_idx].tabs[target_tab_idx]
.layout
.focus_pane(previous_target_focus);
}
(target_ws_idx, target_tab_idx, moved_pane_id)
}
ResolvedPaneMoveDestination::NewTab {
Expand Down
232 changes: 221 additions & 11 deletions src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ pub enum Node {
pub struct TileLayout {
root: Node,
focus: PaneId,
/// Pane focused before `focus`, used by `close_focused`. Only a real focus
/// move writes it; tree edits go through the target-taking primitives
/// (`split_pane`, `close_pane`, unfocused `insert_pane_near`) so internal
/// focus excursions never corrupt it.
prev_focus: Option<PaneId>,
}

impl TileLayout {
Expand All @@ -95,11 +100,20 @@ impl TileLayout {
Self {
root: Node::Pane(root_id),
focus: root_id,
prev_focus: None,
},
root_id,
)
}

/// Move focus, recording the pane being left. No-op when focus is unchanged.
fn set_focus(&mut self, id: PaneId) {
if id != self.focus {
self.prev_focus = Some(self.focus);
self.focus = id;
}
}

pub fn focused(&self) -> PaneId {
self.focus
}
Expand All @@ -122,29 +136,52 @@ impl TileLayout {
result
}

/// Split the focused pane. Returns the new pane's id.
/// Split the focused pane. Returns the new pane's id. Production splits
/// flow through `Tab` so a failed runtime spawn can roll back; this remains
/// as the user-split shape for tests.
#[cfg(test)]
pub fn split_focused(&mut self, direction: Direction) -> PaneId {
self.split_focused_with_ratio(direction, 0.5)
}

/// Split the focused pane with a custom first-child ratio.
#[cfg(test)]
pub fn split_focused_with_ratio(&mut self, direction: Direction, ratio: f32) -> PaneId {
let new_id = self
.split_pane(self.focus, direction, ratio)
.expect("focused pane is in the layout");
self.set_focus(new_id);
new_id
}

/// Split `target` without moving focus. Returns the new pane's id, or None
/// when `target` is not in the layout.
pub fn split_pane(
&mut self,
target: PaneId,
direction: Direction,
ratio: f32,
) -> Option<PaneId> {
if !self.pane_ids().contains(&target) {
return None;
}
let new_id = PaneId::alloc();
let placeholder = PaneId::from_raw(0);
let old = std::mem::replace(&mut self.root, Node::Pane(placeholder));
self.root = split_at(old, self.focus, direction, new_id, valid_split_ratio(ratio));
self.focus = new_id;
new_id
self.root = split_at(old, target, direction, new_id, valid_split_ratio(ratio));
Some(new_id)
}

/// Insert an existing pane id next to a target pane without allocating a new
/// pane or spawning a terminal runtime.
/// pane or spawning a terminal runtime. When `focus` is false, focus and its
/// history are left untouched.
pub fn insert_pane_near(
&mut self,
target: PaneId,
moved: PaneId,
direction: Direction,
ratio: f32,
focus: bool,
) -> bool {
if target == moved {
return false;
Expand All @@ -157,37 +194,66 @@ impl TileLayout {
let placeholder = PaneId::from_raw(0);
let old = std::mem::replace(&mut self.root, Node::Pane(placeholder));
self.root = split_at(old, target, direction, moved, valid_split_ratio(ratio));
self.focus = moved;
if focus {
self.set_focus(moved);
}
true
}

/// Close the focused pane. Returns false if it's the last pane.
/// Close the focused pane, returning focus to the pane it came from when
/// that pane is still open. Returns false if it's the last pane.
pub fn close_focused(&mut self) -> bool {
if self.pane_count() <= 1 {
return false;
}
let target = self.focus;
let ids = self.pane_ids();
let pos = ids.iter().position(|id| *id == target).unwrap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle a stale focus without panicking.

Line 211 panics if focus is absent from the layout tree. Line 341 restores focus without validating that root contains it, so stale saved state can crash pane closing. Handle the missing position before calculating the fallback, or normalize focus in from_saved.

Proposed fix
-        let pos = ids.iter().position(|id| *id == target).unwrap();
+        let Some(pos) = ids.iter().position(|id| *id == target) else {
+            return false;
+        };

As per coding guidelines, “Do not use unwrap() in production Rust code.”

Also applies to: 338-343

Source: Coding guidelines

let new_focus = if pos + 1 < ids.len() {
let ordered = if pos + 1 < ids.len() {
ids[pos + 1]
} else {
ids[pos - 1]
};
let new_focus = match self.prev_focus {
Some(prev) if prev != target && ids.contains(&prev) => prev,
_ => ordered,
};
let placeholder = PaneId::from_raw(0);
let old = std::mem::replace(&mut self.root, Node::Pane(placeholder));
if let Some(new_root) = remove_pane(old, target) {
self.root = new_root;
self.focus = new_focus;
self.prev_focus = None;
Comment thread
greptile-apps[bot] marked this conversation as resolved.
true
} else {
false
}
}

/// Close any pane. Focus and its history are left alone unless the closed
/// pane is the focused one.
pub fn close_pane(&mut self, id: PaneId) -> bool {
if self.focus == id {
return self.close_focused();
}
if self.pane_count() <= 1 || !self.pane_ids().contains(&id) {
return false;
}
let placeholder = PaneId::from_raw(0);
let old = std::mem::replace(&mut self.root, Node::Pane(placeholder));
let Some(new_root) = remove_pane(old, id) else {
return false;
};
self.root = new_root;
if self.prev_focus == Some(id) {
self.prev_focus = None;
}
true
}

pub fn focus_pane(&mut self, id: PaneId) {
if self.pane_ids().contains(&id) {
self.focus = id;
self.set_focus(id);
}
}

Expand Down Expand Up @@ -270,7 +336,11 @@ impl TileLayout {
/// Reconstruct a layout from a saved tree.
/// Reconstruct a layout from a saved tree.
pub fn from_saved(root: Node, focus: PaneId) -> Self {
Self { root, focus }
Self {
root,
focus,
prev_focus: None,
}
}
}

Expand Down Expand Up @@ -746,7 +816,7 @@ mod tests {
let (mut layout, root) = TileLayout::new();
let moved = pane(99);

assert!(layout.insert_pane_near(root, moved, Direction::Horizontal, 0.25));
assert!(layout.insert_pane_near(root, moved, Direction::Horizontal, 0.25, true));

assert_eq!(layout.pane_count(), 2);
assert_eq!(layout.pane_ids(), vec![root, moved]);
Expand Down Expand Up @@ -954,4 +1024,144 @@ mod tests {
Some(pane(3))
);
}

#[test]
fn close_focused_returns_to_the_pane_focus_came_from() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

assert!(layout.close_focused());

assert_eq!(layout.focused(), pane(2));
}

#[test]
fn close_focused_returns_to_the_pane_that_opened_a_split() {
// Allocated ids only: sample_layout() uses from_raw and shares the id
// space with the allocator.
let (mut layout, first) = TileLayout::new();
let second = layout.split_focused(Direction::Horizontal);
let third = layout.split_focused(Direction::Vertical);
assert_eq!(layout.pane_ids().len(), 3);

layout.focus_pane(first);
let opened = layout.split_focused(Direction::Horizontal);
assert_eq!(layout.focused(), opened);

assert!(layout.close_focused());

assert_eq!(layout.focused(), first);
assert!(layout.pane_ids().contains(&second));
assert!(layout.pane_ids().contains(&third));
}

#[test]
fn closing_a_background_pane_keeps_the_focused_pane_history() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

assert!(layout.close_pane(pane(1)));
assert_eq!(layout.focused(), pane(4));

assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(2));
}

#[test]
fn closing_the_remembered_pane_drops_the_focus_history() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

assert!(layout.close_pane(pane(2)));

assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(3));
}

#[test]
fn close_focused_uses_tree_order_without_focus_history() {
let mut layout = sample_layout();

assert!(layout.close_focused());

assert_eq!(layout.focused(), pane(3));
}

#[test]
fn close_focused_does_not_reuse_history_after_it_is_consumed() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(2));

assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(3));
}

#[test]
fn resize_does_not_disturb_the_close_focus_target() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));
layout.resize_pane(pane(1), NavDirection::Right, 0.05, Rect::new(0, 0, 100, 40));

assert!(layout.close_focused());

assert_eq!(layout.focused(), pane(2));
}

#[test]
fn split_pane_leaves_focus_and_history_untouched() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

let new_id = layout
.split_pane(pane(1), Direction::Horizontal, 0.5)
.expect("target exists");

assert!(layout.pane_ids().contains(&new_id));
assert_eq!(layout.focused(), pane(4));
assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(2));
}

#[test]
fn split_pane_missing_target_changes_nothing() {
let mut layout = sample_layout();
let ids = layout.pane_ids();

assert_eq!(
layout.split_pane(pane(99), Direction::Horizontal, 0.5),
None
);

assert_eq!(layout.pane_ids(), ids);
}

#[test]
fn insert_pane_near_unfocused_keeps_focus_and_history() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

assert!(layout.insert_pane_near(pane(1), pane(9), Direction::Horizontal, 0.5, false));

assert_eq!(layout.focused(), pane(4));
assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(2));
}

#[test]
fn failed_split_rollback_preserves_focus_history() {
let mut layout = sample_layout();
layout.focus_pane(pane(4));

let new_id = layout
.split_pane(layout.focused(), Direction::Horizontal, 0.5)
.expect("target exists");
assert!(layout.close_pane(new_id));

assert_eq!(layout.focused(), pane(4));
assert!(layout.close_focused());
assert_eq!(layout.focused(), pane(2));
}
}
Loading
Loading