Open
Conversation
Contributor
There was a problem hiding this comment.
Layout System Review
Overall this is a solid implementation of the layout feature. The code follows TigerStyle conventions with good assertions at API boundaries, and the security-conscious approach in expand_path() (avoiding shell commands for directory checks) is appreciated.
Issues Found
- Line 1492: Missing
.closeexistence check before callingpty:close()on floating panes (trivial fix provided) - Line 3701:
available_heightcan go negative on small screens, causing unexpected loop behavior (trivial fix provided) - Line 2868: Magic number
15for visible height calculation doesn't matchitems_start_y = 8inbuild_layout_picker()- could cause scrolling inconsistencies
Minor Observations
- The
finalize_layoutfunction is ~80 lines, slightly over the TigerStyle 70-line guideline, but the logic is cohesive and well-structured - Good error recovery pattern: building new state in memory before destroying old state
Full review context: https://ampcode.com/threads/T-019bafe4-7a36-732c-aba8-2d453c444ad1
Contributor
Author
|
fixed all three reported issues including the refactoring of finalize_layout |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for predefined layouts. It enables specifying layouts with tabs, panes and floating pane support.
supports
depends on: #86 since we are using prise.spawn to spawn a layout with cmd
example layouts config with video.
layouts.mp4
work and config design started with https://ampcode.com/threads/T-019bae2e-2593-7412-8b5e-d760915f9aac