Enhance panel display and scrollbar alignment#14
Merged
Merged
Conversation
Reviewer's GuideRefines panel and viewer layout/scrollbar behavior, introduces a user-toggleable and persisted “show permissions” option affecting long listing and status bar, updates date formatting to a shorter YY-MM-DD format, and adjusts tests/config/menu wiring accordingly. Sequence diagram for toggling permissions display in panelsequenceDiagram
actor User
participant Menu as MenuUI
participant Input as InputHandler
participant App as AppState
participant Panel as PanelState
participant UI as PanelRenderer
User->>Menu: Select Show_permissions
Menu->>Input: MenuAction TogglePermissions
Input->>App: execute_panel_action(TogglePermissions)
App->>Panel: active_panel_mut()
App->>Panel: toggle show_permissions
App->>App: set status_message
App-->>Input: Some(None)
User->>UI: Next render tick
UI->>App: read active PanelState
UI->>Panel: render_panel(..., show_permissions)
UI->>Panel: render_status_bar(..., show_permissions)
Panel-->>User: Panel list and status bar with permissions visibility updated
Class diagram for panel state, persisted panel, and menu actionsclassDiagram
class PanelState {
+SortMode sort_mode
+ListingMode listing_mode
+bool show_hidden
+bool show_permissions
+Option_String filter
+usize selected_count
+u64 selected_size
}
class PersistedPanel {
+PathBuf cwd
+SortMode sort_mode
+String filter
+bool show_hidden
+bool show_permissions
}
class AppState {
+PanelState left_panel
+PanelState right_panel
+ActivePanel active_panel
+Option_String status_message
+PanelState active_panel_mut()
}
class MenuAction {
<<enumeration>>
ToggleHiddenFiles
TogglePermissions
ToggleLayoutMode
TogglePanelHidden
ResetPanelFilter
SaveSetup
DirectoryHotlist
SaveCurrentPathToHotlist
}
class PanelsModule {
+render_panel(f, area, panel, is_active)
+render_status_bar(f, area, panel)
+format_entry_line(entry, width, show_permissions) String
}
class ConfigModule {
+panel_to_persisted(panel) PersistedPanel
+apply_panel(panel, persisted)
}
class MenuActionsModule {
+execute_panel_action(action, state) Option_Option_KeyEvent
}
AppState "1" o-- "1" PanelState : active_panel
AppState "1" o-- "1" PanelState : other_panel
ConfigModule ..> PanelState : use
ConfigModule ..> PersistedPanel : create_apply
MenuActionsModule ..> AppState : mutate
MenuActionsModule ..> MenuAction : consume
PanelsModule ..> PanelState : read
PanelsModule ..> FileEntry : format
PersistedPanel --> PanelState : applied_to
MenuAction --> MenuActionsModule : handled_by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
format_entry_line, the suffix selection logic now builds and measures three separate strings on every call; consider refactoring to reuse computed widths/strings or branch earlier onshow_permissionsto avoid unnecessary allocations. - In
render_status_bar, thefull_info/metabranches duplicate the formatting of name/size/owner/group and recompute permissions whenshow_permissionsis true; extracting a small helper to build these segments once would simplify the code and reduce repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `format_entry_line`, the suffix selection logic now builds and measures three separate strings on every call; consider refactoring to reuse computed widths/strings or branch earlier on `show_permissions` to avoid unnecessary allocations.
- In `render_status_bar`, the `full_info`/`meta` branches duplicate the formatting of name/size/owner/group and recompute permissions when `show_permissions` is true; extracting a small helper to build these segments once would simplify the code and reduce repetition.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a "Show permissions" toggle for file panels, enabling users to view file mode bits in long listing mode and the status bar. The changes involve updates to configuration persistence, menu actions, and UI rendering logic. The feedback recommends maintaining a consistent date format throughout the application, optimizing performance by minimizing redundant string allocations in the rendering loop, and addressing a visual inconsistency introduced by changing the viewer's border style.
Comment on lines
+54
to
+57
| "{:02}-{:02}-{:02} {:02}:{:02}", | ||
| local.day(), | ||
| local.month(), | ||
| local.year() % 100, |
There was a problem hiding this comment.
The date format has been changed from ISO-like YYYY-MM-DD to DD-MM-YY. This introduces an inconsistency with FileEntry::display_modified in src/app/types.rs (line 682), which still uses %Y-%m-%d. It is recommended to maintain a consistent date format across the application, preferably ISO 8601 for clarity and to avoid regional ambiguity.
Comment on lines
+178
to
+180
| let full_with_perms = format!(" {size_str} {date_str} {perms_str}"); | ||
| let full_no_perms = format!(" {size_str} {date_str}"); | ||
| let nd = format!(" {size_str}"); |
There was a problem hiding this comment.
These format! calls are executed for every visible file entry in the panel on every frame render. This results in a high number of redundant string allocations. It would be more efficient to calculate the visual widths of the individual components (size_str, date_str, perms_str) first and only perform the format! call for the suffix that actually fits the available width.
let size_width = UnicodeWidthStr::width(size_str.as_str());
let date_width = UnicodeWidthStr::width(date_str.as_str());
let perms_width = UnicodeWidthStr::width(perms_str.as_str());
if show_permissions && 2 + size_width + 1 + date_width + 1 + perms_width <= width {
let full = format!(" {size_str} {date_str} {perms_str}");
let w = size_width + date_width + perms_width + 3;
(full, w)
} else if 2 + size_width + 1 + date_width <= width {
let full = format!(" {size_str} {date_str}");
let w = size_width + date_width + 2;
(full, w)
} else if 2 + size_width <= width {
let full = format!(" {size_str}");
let w = size_width + 1;
(full, w)
} else {
(String::new(), 0)
}
Comment on lines
+352
to
+363
| let full_info = if panel.show_permissions { | ||
| let perms_str = format_permissions(entry.mode_bits()); | ||
| format!( | ||
| "{} | {} | {} | {} | {}", | ||
| entry.name, size_str, perms_str, entry.owner, entry.group, | ||
| ) | ||
| } else { | ||
| format!( | ||
| "{} | {} | {} | {}", | ||
| entry.name, size_str, entry.owner, entry.group, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Similar to the file entry formatting, this logic performs redundant format! calls and string allocations in the status bar rendering. Calculating widths before formatting would improve performance.
| pub fn render_viewer(f: &mut Frame, area: Rect, state: &ViewerState) { | ||
| let block = Block::default() | ||
| .borders(Borders::ALL) | ||
| .borders(Borders::TOP | Borders::BOTTOM) |
There was a problem hiding this comment.
Changing the viewer borders to TOP | BOTTOM only creates a visual inconsistency with the main file panels, which use Borders::ALL. If the intention was to maximize horizontal space, consider if this inconsistency is acceptable for the overall UI design.
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
1 participant
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.
Summary by Sourcery
Adjust panel and viewer layout to improve scrollbar alignment and add configurability for displaying file permissions.
New Features:
Enhancements:
Tests: