chore(core): add tui layout manager #30947
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
View your CI Pipeline Execution ↗ for commit 7ce07ff.
☁️ Nx Cloud last updated this comment at |
JamesHenry
force-pushed
the
tui-layout-refactor
branch
from
b21cf59 to
3d184f1
Compare
| // Cached frame area used for layout calculations, only updated on terminal resize | ||
| frame_area: Option<Rect>, | ||
| // Cached result of layout manager's calculate_layout, only updated when necessary (e.g. terminal resize, task list visibility change etc) | ||
| layout_areas: Option<LayoutAreas>, |
There was a problem hiding this comment.
future: Sort of questioning why this has to be an Option instead of.. just always being a Rect that gets replaced on resize. Why can't we instantiate the app with an area?
| // Iterate over the pinned tasks and assign them to the terminal panes (up to the maximum of 2), focusing the first one as well | ||
| let pinned_tasks = self.pinned_tasks.clone(); | ||
| for (idx, task) in pinned_tasks.iter().enumerate() { | ||
| if idx < 2 { |
There was a problem hiding this comment.
nit: don't we limit the support for the number of pinned tasks by nature of there being no way to set a 3rd pinned task? Or are there cases where we have more than 2 pinned tasks?
| .lock() | ||
| .unwrap() | ||
| .select_task(task.clone()); | ||
| self.dispatch_action(Action::SortTasks); |
There was a problem hiding this comment.
This is a weird action to me.. seems more like the action should be "select task" and sort tasks should be a side effect.
| // If the task is continuous, ensure the pty instances get resized appropriately | ||
| if let Some(task) = self.tasks.iter_mut().find(|t| t.id == task_id) { | ||
| if task.continuous.unwrap_or(false) { | ||
| let _ = self.debounce_pty_resize(); |
There was a problem hiding this comment.
future: any idea why this is done?
| fn register_action_handler(&mut self, tx: UnboundedSender<Action>) -> Result<()> { | ||
| let _ = tx; // to appease clippy |
There was a problem hiding this comment.
You can appease clippy by naming it _ in the signature.
| /// | ||
| /// * `Result<()>` - An Ok result or an error. | ||
| fn init(&mut self, area: Size) -> Result<()> { | ||
| let _ = area; // to appease clippy |
| } | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
Or you can bring this back
| _ => None, | ||
| }; | ||
| Ok(r) | ||
| Ok(action) |
There was a problem hiding this comment.
Is this the main way of sending out events from components? They also seem to have Action Senders? We ideally only have 1 way.
| @@ -9,6 +9,9 @@ use ratatui::{ | |||
| }, | |||
| }; | |||
| use std::any::Any; | |||
| use tokio::sync::mpsc::UnboundedSender; | |||
There was a problem hiding this comment.
Do the crossbeam senders work? Why use the tokio ones? We mostly use the crossbeam senders elsewhere in the codebase.
| } | ||
|
|
||
| /// Gets the current task count. | ||
| pub fn get_task_count(&self) -> usize { |
There was a problem hiding this comment.
future: This should not be responsible for getting the task count
|
|
||
| /// Sets the task count. | ||
| pub fn set_task_count(&mut self, count: usize) { | ||
| self.task_count = count; |
There was a problem hiding this comment.
future: If we want to change the layout based on the task count, we can do it via the other methods.
| } | ||
|
|
||
| match self.task_list_visibility { | ||
| TaskListVisibility::Hidden => self.calculate_layout_hidden_task_list(area), |
There was a problem hiding this comment.
| TaskListVisibility::Hidden => self.calculate_layout_hidden_task_list(area), | |
| TaskListVisibility::Hidden => self.calculate_layout_without_task_list(area), |
|
|
||
| match self.task_list_visibility { | ||
| TaskListVisibility::Hidden => self.calculate_layout_hidden_task_list(area), | ||
| TaskListVisibility::Visible => self.calculate_layout_visible_task_list(area), |
There was a problem hiding this comment.
| TaskListVisibility::Visible => self.calculate_layout_visible_task_list(area), | |
| TaskListVisibility::Visible => self.calculate_layout_with_task_list(area), |
| PaneArrangement::Double => { | ||
| // Split the area into two equal parts | ||
| let chunks = Layout::default() | ||
| .direction(Direction::Horizontal) |
There was a problem hiding this comment.
future: We can split it horizontally or vertically right?
| let use_vertical = match self.mode { | ||
| LayoutMode::Auto => { | ||
| self.is_vertical_layout_preferred(area.width, area.height, self.task_count) | ||
| } | ||
| LayoutMode::Vertical => true, | ||
| LayoutMode::Horizontal => false, | ||
| }; |
There was a problem hiding this comment.
future: We should have a helper function for this.
| let terminal_pane_area = chunks[2]; // Skip the padding area (chunks[1]) | ||
|
|
||
| let terminal_panes = match self.pane_arrangement { | ||
| PaneArrangement::None => Vec::new(), |
There was a problem hiding this comment.
This should not get hit because it's checked up above.
| } | ||
|
|
||
| /// Creates a LayoutConfig from the current internal state. | ||
| pub fn create_config(&self) -> LayoutConfig { |
There was a problem hiding this comment.
There's something between pub and private.. pub(mod) I think? Or I'm hallucinating
| let aspect_ratio = terminal_width as f32 / terminal_height as f32; | ||
|
|
||
| // If very wide and not very tall, prefer horizontal | ||
| if aspect_ratio > 2.0 && terminal_height < self.min_vertical_height { | ||
| return false; | ||
| } | ||
|
|
||
| // If very tall and not very wide, prefer vertical | ||
| if aspect_ratio < 1.0 && terminal_width < self.min_horizontal_width { | ||
| return true; | ||
| } | ||
|
|
||
| // Otherwise, prefer horizontal for wider terminals, vertical for taller ones | ||
| aspect_ratio < 1.5 |
There was a problem hiding this comment.
I think this gets simpler if we look at it this way:
There are constraints which force vertical or horizontal if there's not enough space.
If the terminal area is within those constraints then an aspect ratio of 1.5 is basically where we draw the line between preferring vertical vs horizontal.
No description provided.