Skip to content

Comments

Add last focus behaviour for navigation between panes#77

Open
max-legrand wants to merge 3 commits intorockorager:mainfrom
max-legrand:last-focus
Open

Add last focus behaviour for navigation between panes#77
max-legrand wants to merge 3 commits intorockorager:mainfrom
max-legrand:last-focus

Conversation

@max-legrand
Copy link
Contributor

Added last_focus behavior for navigation (split out from #60)

https://ampcode.com/threads/T-019b95f7-cb4d-70d9-9bf1-a6785cf358ee

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Review Summary

The remember_focus navigation feature is well-designed overall with good documentation and proper serialization support. Found two issues to address:

  1. Bug (line 2185): Duplicate call to update_pty_focus after set_focus_and_track which already calls it internally.

  2. Consistency issue (line 1399): move_focus manually updates focus state and only tracks the immediate parent split, while set_focus_and_track properly walks the entire ancestor path. This could cause last_focused_child_idx to be stale in grandparent splits when using keyboard navigation vs. mouse clicks.

The rest of the implementation looks solid - the type definitions, get_preferred_leaf, find_node_in_tree, and serialization changes are all correct.


View full review context

src/lua/tiling.lua:1399: Inconsistent focus tracking

This manually updates state.focused_id and only tracks the immediate parent split (lines 1404-1407), while elsewhere set_focus_and_track is used to properly track all ancestor splits in the path. This could lead to inconsistent last_focused_child_idx state in deeply nested splits.

Consider using set_focus_and_track(target_leaf.id) here instead of the manual approach, then just call prise.request_frame() after.

-- Use set_focus_and_track to register the new pane and track parent splits
set_focus_and_track(new_pane.id)
state.pending_split = nil
update_pty_focus(old_focused_id, state.focused_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Duplicate update_pty_focus call

set_focus_and_track already calls update_pty_focus(old_id, state.focused_id) internally (line 846). This second call is redundant and could cause unnecessary PTY operations.

Suggested change
update_pty_focus(old_focused_id, state.focused_id)
set_focus_and_track(new_pane.id)
state.pending_split = nil
prise.request_frame()
prise.save() -- Auto-save on pane added
return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant