Add segmentation for separator dividers#60
Add segmentation for separator dividers#60max-legrand wants to merge 3 commits intorockorager:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for working on this! The feature is useful - tmux-style segmented borders that highlight the active pane path are a nice UX improvement.
However, after reviewing the implementation, I think it's heavier than it needs to be for what it accomplishes. Here are some concerns and suggestions:
1. Unrelated behavior change in focus navigation
The PR changes move_focus to use get_preferred_leaf() instead of directional get_first_leaf()/get_last_leaf():
-- Before:
if forward then
target_leaf = get_first_leaf(sibling_node)
else
target_leaf = get_last_leaf(sibling_node)
end
-- After:
local target_leaf = get_preferred_leaf(sibling_node)This means left/right/up/down focus moves no longer respect direction - they always go to the last-focused leaf in that subtree. This is a real behavior change unrelated to border rendering and could surprise users.
Suggestion: Either keep the old behavior for navigation, or split this into a separate PR with clear justification.
2. New widget type may be unnecessary
Adding SegmentedSeparator as a new widget kind requires changes in many places (destroy, layout, paint, hit regions, etc). Consider extending the existing Separator instead:
pub const Separator = struct {
axis: Axis,
style: vaxis.Style = .{},
border: Box.Border = .single,
segments: ?[]const SeparatorSegment = null, // optional
};In Lua:
prise.Separator({
axis = sep_axis,
border = config.borders.style,
segments = segments, -- optional, when needed
style = { fg = default_color },
})This keeps the concept simple: "it's just a separator, sometimes with segments."
3. Over-engineered segment API
The current API supports both ratios AND absolute positions:
---@field ratio_start? number Start position as ratio (0.0-1.0)
---@field ratio_end? number End position as ratio (0.0-1.0)
---@field start? number Absolute start position
---@field end? number Absolute end positionBut only ratios are actually used. This adds complexity (the is_ratio bool, 1000x scaling, etc). Suggest dropping absolute support until actually needed.
4. Asymmetric handling of splits
create_divider_segments has:
if left_is_col_split then
...
elseif right_is_col_split then
...
endIf both sides are splits, only the left side is considered. Suggest explicitly handling "both sides split" by falling back to simple separator coloring:
if left_is_col_split and right_is_col_split then
return {}
end5. Missing tests
No tests for:
create_divider_segmentsbehavior for various layouts- Serialization of
last_focused_child_idx - Edge cases in segmentation logic
Overall I think this could be simplified to ~200-300 lines by:
- Extending
Separatorinstead of new widget type - Only supporting ratio-based segments
- Decoupling focus navigation changes
- Using simpler
contains_focusedheuristic for v1
Happy to discuss further!
|
(Note that this was an AI generated review...need to fix that this posts as me) |
rockorager
left a comment
There was a problem hiding this comment.
See previous review comment
Add segmentation to better highlight only the active pane
Screenshots