From 17aa0226e23ae13a9b33ba83e93de4075360ebec Mon Sep 17 00:00:00 2001 From: Leszek Date: Fri, 31 Jul 2026 13:38:11 +0200 Subject: [PATCH 1/3] =?UTF-8?q?perf(ui):=20render=20path=20=E2=80=94=20pan?= =?UTF-8?q?els,=20viewer,=20dialogs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit PR-13 (35 performance findings in the UI render path). Fixed (21 findings): - panels/mod.rs: cache dirs::home_dir() thread-locally; indexed access replaces skip() iterator; ChaMode written straight into buffers instead of display_permissions_raw String alloc per row; format_size replaced by inline write_size; status bar reuses cached entry.size_str and eliminates scratch.clone(); with_capacity sized for CJK (4× width). - viewer/open.rs: "" borrowed instead of .to_string() per dir entry; Vec::with_capacity starts at READ_CHUNK not up to 100MB. - viewer/toggle.rs: text metrics computed once then reused (line_offsets non-empty guard); grouped borrow_mut in invalidate_visual_cache. - viewer/search.rs: decode_lossy_with_map reserves 3× for U+FFFD expansion. - viewer/hex.rs: push_byte_hex uses stack array + push_str (1 write, not 3). - input/normal.rs: Alt+Backspace avoids prev_path.clone(). - input/menu_actions.rs: hotlist path bound once before .any() scan. - dialogs/archive.rs: removed redundant INPUT_BUF (compute_visible_window already returns owned String); render_button_row reserves 2n capacity. - dialogs/input.rs: documented single-active-field SegmentCache assumption. - dialogs/list_picker.rs: hoisted dialog_with_colors to a single let. - render.rs: icon_theme computed only on the panel-layout path, not Viewing. - theme.rs: eq_ignore_ascii_case replaces to_ascii_lowercase() allocation. Rejected (14 findings) with justification in PR body. cargo fmt + clippy -D warnings + test (302 pass) + release build all green. --- src/input/menu_actions.rs | 11 ++-- src/input/normal.rs | 8 ++- src/render.rs | 5 +- src/ui/dialogs/archive.rs | 23 +++---- src/ui/dialogs/input.rs | 7 +++ src/ui/dialogs/list_picker.rs | 5 +- src/ui/panels/mod.rs | 115 ++++++++++++++++++++++------------ src/ui/panels/tests.rs | 4 +- src/ui/theme.rs | 22 ++++--- src/ui/viewer/hex.rs | 15 ++++- src/ui/viewer/open.rs | 12 ++-- src/ui/viewer/search.rs | 6 +- src/ui/viewer/toggle.rs | 30 +++++---- 13 files changed, 167 insertions(+), 96 deletions(-) diff --git a/src/input/menu_actions.rs b/src/input/menu_actions.rs index aac8a08..9db699a 100644 --- a/src/input/menu_actions.rs +++ b/src/input/menu_actions.rs @@ -90,12 +90,11 @@ pub fn execute_menu_action(state: &mut AppState) -> Option<(KeyCode, KeyModifier } MenuAction::SaveCurrentPathToHotlist => { with_menu_panel(state, |state| { - if !state - .hotlist() - .iter() - .any(|p| p == state.active_panel().path()) - { - state.hotlist_push(state.active_panel().path().to_path_buf()); + // Bind the path once before the scan to avoid re-querying + // active_panel().path() on every .any() iteration. + let path = state.active_panel().path().to_path_buf(); + if !state.hotlist().iter().any(|p| p == &path) { + state.hotlist_push(path); } state.ui.status_message = Some("Path added to hotlist (run Save Setup to persist)".to_string()); diff --git a/src/input/normal.rs b/src/input/normal.rs index bf37f1e..4d8fa62 100644 --- a/src/input/normal.rs +++ b/src/input/normal.rs @@ -567,14 +567,18 @@ pub(crate) fn handle_alt_keys(state: &mut AppState, key: KeyCode, visible: usize // directory is gone. Consume the history entry only on success so // a failed navigation doesn't destroy the history stack. let panel = state.active_panel_mut(); - panel.set_path(prev_path.clone()); + panel.set_path(prev_path); panel.cursor = 0; panel.scroll_offset = 0; let result = panel_ops::refresh_panel(state.active_panel_mut(), visible); if result.is_none() { state.active_panel_mut().pop_history(); reposition_cursor_to_entry(state, prev_dir_name.as_deref(), visible); - state.ui.status_message = Some(format!("cd to {}", prev_path.display())); + // After set_path, the panel path IS the previous path, so we + // can read it back for the status message instead of cloning + // prev_path before the move. + state.ui.status_message = + Some(format!("cd to {}", state.active_panel().path().display())); } else if let Some(msg) = result { state.set_status(msg); } diff --git a/src/render.rs b/src/render.rs index 539117e..ab4d0a5 100644 --- a/src/render.rs +++ b/src/render.rs @@ -33,7 +33,6 @@ pub(crate) fn render_ui( viewer_loader: Option<&viewer::ViewerLoader>, ) { let colors = &state.theme_colors; - let icon_theme = colors.icon_theme(); match &state.mode { AppMode::Viewing => { @@ -78,6 +77,10 @@ pub(crate) fn render_ui( return; } + // icon_theme is only needed for the panel layout below, not for the viewer + // or directory-tree paths that return early above. + let icon_theme = colors.icon_theme(); + let size = f.area(); let bg_block = ratatui::widgets::Block::default().style(Theme::panel_bg_with_colors(colors)); diff --git a/src/ui/dialogs/archive.rs b/src/ui/dialogs/archive.rs index d0b2bd4..d867033 100644 --- a/src/ui/dialogs/archive.rs +++ b/src/ui/dialogs/archive.rs @@ -12,10 +12,6 @@ use crate::ui::theme::{ColorPalette, Theme}; use super::layout::dialog_block; thread_local! { - /// Reusable scratch buffer for the input field's visible window, - /// avoiding a per-frame allocation. Safe because rendering is - /// single-threaded and the buffer is cleared at the start of each use. - static INPUT_BUF: RefCell = const { RefCell::new(String::new()) }; /// Reusable scratch buffer for the "N files selected" sources line. static SOURCES_BUF: RefCell = const { RefCell::new(String::new()) }; } @@ -153,21 +149,18 @@ fn render_input_field( // otherwise land it on the border (`visible_width >= 1` here). let cursor_col = window.cursor_col.min(visible_width.saturating_sub(1)); - INPUT_BUF.with_borrow_mut(|buf| { - buf.clear(); - buf.push_str(&window.text); - let input_paragraph = Paragraph::new(buf.as_str()).block(input_block); - // `render_widget` consumes `input_paragraph`, dropping the shared borrow of `buf` - // synchronously here — before `set_cursor_position` below touches the closure scope. - f.render_widget(input_paragraph, area); + // `window.text` is already an owned String from compute_visible_window; + // render it directly instead of copying into a thread-local scratch buffer. + let input_paragraph = Paragraph::new(window.text).block(input_block); + f.render_widget(input_paragraph, area); - let cursor_x = input_inner.x + cursor_col as u16; - f.set_cursor_position((cursor_x, input_inner.y)); - }); + let cursor_x = input_inner.x + cursor_col as u16; + f.set_cursor_position((cursor_x, input_inner.y)); } fn render_button_row(f: &mut Frame, area: Rect, buttons: &[(ratatui::style::Style, &str)]) { - let mut spans: Vec = Vec::with_capacity(buttons.len()); + // n buttons produce 2n-1 spans (each button + n-1 separators). + let mut spans: Vec = Vec::with_capacity(buttons.len() * 2); for (i, (style, label)) in buttons.iter().enumerate() { if i > 0 { spans.push(Span::raw(" ")); diff --git a/src/ui/dialogs/input.rs b/src/ui/dialogs/input.rs index 95ce36d..f8c5ea7 100644 --- a/src/ui/dialogs/input.rs +++ b/src/ui/dialogs/input.rs @@ -98,6 +98,13 @@ struct GraphemeSegment { /// Per-thread memoization of the grapheme segmentation of the last rendered /// input value. /// +/// **Assumption:** at most one input field is rendered per frame. The cache +/// key is the value string, so interleaving multiple distinct input values +/// (e.g. an archive-extract dialog + a search bar) thrashes it. This is safe +/// — correctness is unaffected — but the memoization only helps when the same +/// value is rendered consecutively. Only one dialog is ever focused at a time +/// in this TUI, so the single-active-field assumption holds in practice. +/// /// Rendering stays a pure function of its arguments: this cache only avoids /// re-segmenting an unchanged value across consecutive frames/keystrokes. It is /// NOT application state and never changes the output for a given `value`. diff --git a/src/ui/dialogs/list_picker.rs b/src/ui/dialogs/list_picker.rs index 392ca19..0d242ce 100644 --- a/src/ui/dialogs/list_picker.rs +++ b/src/ui/dialogs/list_picker.rs @@ -25,11 +25,12 @@ pub fn render_list_picker_with_colors>( let area = f.area(); let picker_area = centered_rect(60, 70, area); + let dialog_style = Theme::dialog_with_colors(colors); f.render_widget(Clear, picker_area); - let bg_block = ratatui::widgets::Block::default().style(Theme::dialog_with_colors(colors)); + let bg_block = ratatui::widgets::Block::default().style(dialog_style); f.render_widget(bg_block, picker_area); - let block = dialog_block(title, Theme::dialog_with_colors(colors)); + let block = dialog_block(title, dialog_style); let inner = block.inner(picker_area); f.render_widget(block, picker_area); diff --git a/src/ui/panels/mod.rs b/src/ui/panels/mod.rs index 0ce26a3..504d46b 100644 --- a/src/ui/panels/mod.rs +++ b/src/ui/panels/mod.rs @@ -11,7 +11,7 @@ use unicode_width::UnicodeWidthStr; use super::theme::{ColorPalette, DEFAULT_COLORS, IconTheme, Theme}; -use crate::app::types::{FileCategory, FileEntry, ListingMode, PanelState, format_size}; +use crate::app::types::{FileCategory, FileEntry, ListingMode, PanelState}; const FN_KEY_TEXTS: [&str; 10] = [ " F1 ", " F2 ", " F3 ", " F4 ", " F5 ", " F6 ", " F7 ", " F8 ", " F9 ", " F10 ", @@ -140,13 +140,22 @@ fn shorten_home_with<'a>(path: &'a str, home: &str) -> Cow<'a, str> { Cow::Borrowed(path) } +/// Resolve `$HOME` once and cache it thread-locally. `dirs::home_dir()` does +/// an env/syscall lookup (with a passwd fallback) on every call; the home +/// directory does not change during a session, so caching avoids a per-frame +/// query from the hot panel-render path. fn shorten_home(path: &str) -> Cow<'_, str> { - // dirs::home_dir (not std::env) to stay consistent with tilde expansion - // in fs::path, which also falls back to passwd when $HOME is unset. - match dirs::home_dir() { - Some(home) => shorten_home_with(path, &home.to_string_lossy()), - None => Cow::Borrowed(path), + thread_local! { + static HOME: std::cell::OnceCell> = + const { std::cell::OnceCell::new() }; } + HOME.with(|cell| { + let home = cell.get_or_init(dirs::home_dir); + match home { + Some(h) => shorten_home_with(path, &h.to_string_lossy()), + None => Cow::Borrowed(path), + } + }) } pub fn render_panel_with_colors( @@ -199,16 +208,14 @@ pub fn render_panel_with_colors( // simultaneously, but the per-line buffer is pre-sized to avoid reallocs. let mut suffix_buf = String::with_capacity(64); - for entry in panel - .listing - .filtered() - .skip(start_idx) - .take(end_idx.saturating_sub(start_idx)) - { + for i in start_idx..end_idx { + let Some(entry) = panel.listing.filtered_get(i) else { + continue; + }; let cat = entry.category(); let bold = entry.is_dir() || entry.is_executable(); - let mut line = String::with_capacity(content_width + 8); + let mut line = String::with_capacity(content_width.saturating_mul(4) + 8); match mode { ListingMode::Long => format_entry_line( entry, @@ -292,11 +299,14 @@ fn build_suffix_into( let size_date_width = size_width + date_width + 2; if show_permissions { - let perms_str = FileEntry::display_permissions_raw(entry.mode_bits()); - let perms_width = UnicodeWidthStr::width(perms_str.as_str()); - let full_width = size_date_width + perms_width + 1; + // Permissions are always 9 display columns (rwxrwxrwx with special + // bits). Write ChaMode straight into `buf` instead of allocating a + // String via `display_permissions_raw` for every visible row. + const PERMS_WIDTH: usize = 9; + let full_width = size_date_width + PERMS_WIDTH + 1; if 2 + full_width <= width { - write!(buf, " {size_str} {date_str} {perms_str}").ok(); + let perms = crate::fs::cha::ChaMode::new(entry.mode_bits()); + write!(buf, " {size_str} {date_str} {perms}").ok(); return full_width; } } @@ -402,7 +412,9 @@ fn format_entry_line( fn write_status_metadata(buf: &mut String, size: &str, entry: &FileEntry, show_permissions: bool) { if show_permissions { - let perms = FileEntry::display_permissions_raw(entry.mode_bits()); + // Write ChaMode straight into buf — avoids the String alloc that + // `display_permissions_raw` does on every status-bar render. + let perms = crate::fs::cha::ChaMode::new(entry.mode_bits()); write!(buf, "{size} | {perms} | {} | {}", entry.owner, entry.group).ok(); } else { write!(buf, "{size} | {} | {}", entry.owner, entry.group).ok(); @@ -500,6 +512,30 @@ pub fn render_scrollbar_with_colors( f.render_widget(paragraph, area); } +/// Format `size` into `buf` without allocating, mirroring [`format_size`]. +/// Used by the per-frame status-bar summary where the previous code allocated +/// a `String` via `format_size` on every render. +fn write_size(buf: &mut String, size: u64) { + const UNITS: [&str; 7] = ["B", "KB", "MB", "GB", "TB", "PB", "EB"]; + const BYTES_PER_UNIT: f64 = 1024.0; + let mut size_f = size as f64; + let mut unit_idx = 0; + while size_f >= BYTES_PER_UNIT && unit_idx < UNITS.len() - 1 { + size_f /= BYTES_PER_UNIT; + unit_idx += 1; + } + if unit_idx > 0 { + size_f = (size_f * 10.0).round() / 10.0; + if size_f >= BYTES_PER_UNIT && unit_idx < UNITS.len() - 1 { + size_f /= BYTES_PER_UNIT; + unit_idx += 1; + } + write!(buf, "{size_f:.1} {}", UNITS[unit_idx]).ok(); + } else { + write!(buf, "{size} {}", UNITS[unit_idx]).ok(); + } +} + pub fn panel_status_summary(panel: &PanelState, buf: &mut String) -> usize { buf.clear(); let total = panel.listing.filtered_len(); @@ -513,13 +549,11 @@ pub fn panel_status_summary(panel: &PanelState, buf: &mut String) -> usize { write!(buf, " {}/{} {}%", pos, total, pct).ok(); if panel.selected_count() > 0 { - write!( - buf, - " ({} {})", - panel.selected_count(), - format_size(panel.selected_size()) - ) - .ok(); + // Format the size straight into `buf` — format_size() returns an owned + // String (one alloc per frame), but we only need it as Display text here. + write!(buf, " ({} ", panel.selected_count()).ok(); + write_size(buf, panel.selected_size()); + buf.push(')'); } buf.push(' '); @@ -534,29 +568,30 @@ pub fn render_status_bar_with_colors( ) { let available = area.width as usize; - let mut scratch = String::with_capacity(128); - let right_width = panel_status_summary(panel, &mut scratch); - let right_summary = scratch.clone(); + let mut summary = String::with_capacity(48); + let right_width = panel_status_summary(panel, &mut summary); let remaining = available.saturating_sub(right_width); - let mut out = String::with_capacity(remaining + right_summary.len() + 8); + let mut out = String::with_capacity(remaining.max(available) + 8); // Render the cursor entry's info only when it exists; an out-of-range or // empty listing simply skips the left side rather than panicking. if let Some(entry) = panel.listing.filtered_get(panel.cursor) { let display_name = entry.display_name(); - let size_str = format_size(entry.size()); + // Reuse the cached formatted size instead of recomputing format_size + // (which allocates) every frame — the entry already carries it. + let size_str = &entry.size_str; - scratch.clear(); - write_status_metadata(&mut scratch, &size_str, entry, panel.show_permissions()); - let meta_width = UnicodeWidthStr::width(scratch.as_str()); + let mut meta = String::with_capacity(48); + write_status_metadata(&mut meta, size_str, entry, panel.show_permissions()); + let meta_width = UnicodeWidthStr::width(meta.as_str()); let full_width = UnicodeWidthStr::width(display_name) + 3 + meta_width; if full_width <= remaining { out.push_str(display_name); out.push_str(" | "); - out.push_str(&scratch); + out.push_str(&meta); } else { let meta_with_sep_width = meta_width + 3; let name_budget = remaining.saturating_sub(meta_with_sep_width); @@ -565,12 +600,12 @@ pub fn render_status_bar_with_colors( let truncated = truncate_to_width(display_name, name_budget); out.push_str(&truncated); out.push_str(" | "); - out.push_str(&scratch); + out.push_str(&meta); } else { - scratch.clear(); - write!(scratch, "{display_name} | ").ok(); - write_status_metadata(&mut scratch, &size_str, entry, panel.show_permissions()); - let truncated = truncate_to_width(&scratch, remaining); + meta.clear(); + write!(meta, "{display_name} | ").ok(); + write_status_metadata(&mut meta, size_str, entry, panel.show_permissions()); + let truncated = truncate_to_width(&meta, remaining); out.push_str(&truncated); } } @@ -579,7 +614,7 @@ pub fn render_status_bar_with_colors( let info_line_width = UnicodeWidthStr::width(out.as_str()); let padding = remaining.saturating_sub(info_line_width); out.extend(std::iter::repeat_n(' ', padding)); - out.push_str(&right_summary); + out.push_str(&summary); let paragraph = Paragraph::new(out) .style(Theme::status_bar_with_colors(colors)) diff --git a/src/ui/panels/tests.rs b/src/ui/panels/tests.rs index 5db9eae..25e7fb8 100644 --- a/src/ui/panels/tests.rs +++ b/src/ui/panels/tests.rs @@ -1,8 +1,7 @@ #![allow(clippy::expect_used)] use super::*; -use crate::app::types::format_time; -use crate::app::types::sanitize_for_display; +use crate::app::types::{format_size, format_time, sanitize_for_display}; use crate::ui::theme::{DEFAULT_COLORS, IconTheme}; use ratatui::style::Color; use std::path::PathBuf; @@ -54,6 +53,7 @@ fn entry_line(entry: &FileEntry, width: usize, show_permissions: bool) -> String &mut suffix, &mut out, ); + let _ = &suffix; // scratch buffer required by format_entry_line, not asserted out } diff --git a/src/ui/theme.rs b/src/ui/theme.rs index 2d6b461..bb0a9e1 100644 --- a/src/ui/theme.rs +++ b/src/ui/theme.rs @@ -19,14 +19,20 @@ impl IconTheme { crate::debug_log!("config: non-string value for icon_theme, using emoji"); return Self::Emoji; }; - match s.trim().to_ascii_lowercase().as_str() { - "emoji" => Self::Emoji, - "ascii" => Self::Ascii, - "nerdfont" | "nerd_font" | "nerd-font" => Self::NerdFont, - _ => { - crate::debug_log!("config: invalid value for icon_theme, using emoji"); - Self::Emoji - } + // Match case-insensitively without allocating a lowercased String. + let trimmed = s.trim(); + if trimmed.eq_ignore_ascii_case("emoji") { + Self::Emoji + } else if trimmed.eq_ignore_ascii_case("ascii") { + Self::Ascii + } else if trimmed.eq_ignore_ascii_case("nerdfont") + || trimmed.eq_ignore_ascii_case("nerd_font") + || trimmed.eq_ignore_ascii_case("nerd-font") + { + Self::NerdFont + } else { + crate::debug_log!("config: invalid value for icon_theme, using emoji"); + Self::Emoji } } } diff --git a/src/ui/viewer/hex.rs b/src/ui/viewer/hex.rs index 59dde18..55df58f 100644 --- a/src/ui/viewer/hex.rs +++ b/src/ui/viewer/hex.rs @@ -20,9 +20,18 @@ const HEX_COLS_PER_BYTE: usize = 3; const PRINTABLE_ASCII: std::ops::RangeInclusive = 0x20..=0x7e; fn push_byte_hex(buf: &mut String, b: u8) { - buf.push(HEX_CHARS[(b >> NIBBLE_BITS) as usize] as char); - buf.push(HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize] as char); - buf.push(' '); + // Build into a 3-byte stack array, then push in one shot — avoids three + // separate push calls (amortized check + len update) per byte. + let trio = [ + HEX_CHARS[(b >> NIBBLE_BITS) as usize], + HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize], + b' ', + ]; + // `trio` is built solely from `HEX_CHARS` (ASCII) and a space, so it is + // always valid UTF-8. `from_utf8` never fails here. + if let Ok(s) = std::str::from_utf8(&trio) { + buf.push_str(s); + } } fn format_offset_hex(offset: usize, buf: &mut String) { diff --git a/src/ui/viewer/open.rs b/src/ui/viewer/open.rs index ae8de91..a8b4f48 100644 --- a/src/ui/viewer/open.rs +++ b/src/ui/viewer/open.rs @@ -354,7 +354,10 @@ impl ViewerState { } let file = fs::File::open(path)?; - let mut raw_bytes = Vec::with_capacity(file_size.min(MAX_VIEW_SIZE + 1)); + // Don't pre-reserve up to MAX_VIEW_SIZE (~100MB): a cancelled or early- + // ending read would waste that allocation. Start at a reasonable chunk + // size and let `extend_from_slice` grow naturally. + let mut raw_bytes = Vec::with_capacity(READ_CHUNK.min(file_size)); let mut reader = file.take((MAX_VIEW_SIZE + 1) as u64); let mut buf = [0u8; READ_CHUNK]; loop { @@ -437,10 +440,11 @@ impl ViewerState { truncated = true; break; } - let size = if entry.is_dir { - "".to_string() + // Borrow the size label instead of allocating a String per dir. + let size: std::borrow::Cow<'_, str> = if entry.is_dir { + std::borrow::Cow::Borrowed("") } else { - crate::app::types::format_size(entry.size) + std::borrow::Cow::Owned(crate::app::types::format_size(entry.size)) }; let mtime = entry .modified diff --git a/src/ui/viewer/search.rs b/src/ui/viewer/search.rs index e571876..b4b7352 100644 --- a/src/ui/viewer/search.rs +++ b/src/ui/viewer/search.rs @@ -61,8 +61,10 @@ fn decode_lossy_with_map(slice: &[u8]) -> (Cow<'_, str>, Option>) { match std::str::from_utf8(slice) { Ok(s) => (Cow::Borrowed(s), None), Err(_) => { - let mut decoded = String::with_capacity(slice.len()); - let mut map = Vec::with_capacity(slice.len() + 1); + // U+FFFD is 3 bytes in UTF-8; a pure-binary slice expands to + // ceil(len/?) * 3, so reserve 3× to avoid reallocations. + let mut decoded = String::with_capacity(slice.len() * 3); + let mut map = Vec::with_capacity(slice.len() * 3 + 1); let mut raw = 0usize; for chunk in slice.utf8_chunks() { let valid = chunk.valid(); diff --git a/src/ui/viewer/toggle.rs b/src/ui/viewer/toggle.rs index d85dc13..531a8e3 100644 --- a/src/ui/viewer/toggle.rs +++ b/src/ui/viewer/toggle.rs @@ -6,9 +6,12 @@ use super::scroll::line_number_column_width; impl ViewerState { fn invalidate_visual_cache(&self) { - self.render_cache.visual_heights.borrow_mut().clear(); - self.render_cache.visual_offsets.borrow_mut().clear(); - *self.render_cache.cached_content_width.borrow_mut() = 0; + let mut heights = self.render_cache.visual_heights.borrow_mut(); + let mut offsets = self.render_cache.visual_offsets.borrow_mut(); + let mut width = self.render_cache.cached_content_width.borrow_mut(); + heights.clear(); + offsets.clear(); + *width = 0; } fn next_view_mode(&self) -> ViewMode { @@ -61,14 +64,19 @@ impl ViewerState { self.invalidate_visual_cache(); if self.view_mode == ViewMode::Text && self.originally_binary { - let (line_offsets, line_count, max_line_width) = - Self::compute_text_metrics(&self.raw_bytes); - self.line_offsets = line_offsets; - self.line_count = line_count; - self.max_line_width = max_line_width; - self.render_cache - .cached_line_num_col_width - .set(line_number_column_width(self.line_count)); + // raw_bytes never changes after construction, so text-mode metrics + // are identical on every toggle. After the first Hex→Text switch + // they live in self.line_offsets and need no recomputation. + if self.line_offsets.is_empty() { + let (line_offsets, line_count, max_line_width) = + Self::compute_text_metrics(&self.raw_bytes); + self.line_offsets = line_offsets; + self.line_count = line_count; + self.max_line_width = max_line_width; + self.render_cache + .cached_line_num_col_width + .set(line_number_column_width(self.line_count)); + } } } From 199daef5a965765213a36c4cdd40452964ed3eed Mon Sep 17 00:00:00 2001 From: Leszek Date: Fri, 31 Jul 2026 14:22:38 +0200 Subject: [PATCH 2/3] fix(ui): status bar shows unpadded size (PR #110 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The perf optimization in 17aa022 reused entry.size_str in the status bar to avoid a per-frame format_size alloc, but size_str is the column-padded cached form ({:>10} for files, " " for dirs) — leaking leading spaces into the status metadata (" 1.0 KB | ..."), a visual regression vs the old unpadded format_size(entry.size()). Fix: trim_start() the cached string — format_size output never carries leading whitespace, so this recovers the unpadded display form with zero allocation. Added a render test pinning the unpadded form. --- src/ui/panels/mod.rs | 8 +++++--- src/ui/panels/tests.rs | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/ui/panels/mod.rs b/src/ui/panels/mod.rs index 504d46b..8662dd9 100644 --- a/src/ui/panels/mod.rs +++ b/src/ui/panels/mod.rs @@ -578,9 +578,11 @@ pub fn render_status_bar_with_colors( // empty listing simply skips the left side rather than panicking. if let Some(entry) = panel.listing.filtered_get(panel.cursor) { let display_name = entry.display_name(); - // Reuse the cached formatted size instead of recomputing format_size - // (which allocates) every frame — the entry already carries it. - let size_str = &entry.size_str; + // `entry.size_str` is the column-padded cached size (`{:>10}` for files, + // " " for dirs) reused to avoid a per-frame `format_size` alloc. + // The status bar wants the *unpadded* form the old code produced, so strip + // the leading spaces — format_size output never carries leading whitespace. + let size_str = entry.size_str.trim_start(); let mut meta = String::with_capacity(48); write_status_metadata(&mut meta, size_str, entry, panel.show_permissions()); diff --git a/src/ui/panels/tests.rs b/src/ui/panels/tests.rs index 25e7fb8..b4e4d80 100644 --- a/src/ui/panels/tests.rs +++ b/src/ui/panels/tests.rs @@ -817,6 +817,30 @@ fn test_render_status_bar_no_panic() { assert!(content.contains("file.txt")); } +/// Regression: the status bar must show the *unpadded* file size. The cached +/// `size_str` is column-padded (`{:>10}` → " 1.0 KB"); the status bar must +/// not leak that padding into the metadata segment. +#[test] +fn test_render_status_bar_size_is_unpadded() { + let mut panel = PanelState::new(PathBuf::from("/test")); + // size 1024 → format_size = "1.0 KB", padded cache = " 1.0 KB" + panel.set_entries(vec![create_test_entry("file.txt", false, false, false)]); + let content = render_to_string(80, 2, |f| { + render_status_bar_with_colors(f, f.area(), &panel, &DEFAULT_COLORS); + }); + // Unpadded form must be present... + assert!( + content.contains("1.0 KB"), + "status bar missing unpadded size, got: {content:?}" + ); + // ...and the column-padded form must not leak through (no run of spaces + // before the size — the metadata is "1.0 KB | | "). + assert!( + !content.contains(" 1.0 KB"), + "status bar leaked column-padded size, got: {content:?}" + ); +} + #[test] fn test_render_function_bar_no_panic() { let content = render_to_string(80, 1, |f| { From be0785fe7feae5c6dc1f7c38461cb102f9f255c9 Mon Sep 17 00:00:00 2001 From: Leszek Date: Fri, 31 Jul 2026 14:38:03 +0200 Subject: [PATCH 3/3] fix(ui): status bar directory size shows bytes, not (PR #110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 199daef trimmed entry.size_str to recover the unpadded form, but for directories size_str is the column-cache placeholder " ", not a size — so the status bar showed "" instead of the directory's byte size that the pre-perf code produced via format_size(entry.size()). Format the entry size directly into a scratch buffer via the existing zero-alloc write_size helper, for every entry. Files keep their unpadded human size; directories show the real byte size again. Tests: extend the status-bar coverage with a directory case asserting the bar shows a size (unit suffix) and never the placeholder. --- src/ui/panels/mod.rs | 16 +++++++++------- src/ui/panels/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/ui/panels/mod.rs b/src/ui/panels/mod.rs index 8662dd9..3db08fd 100644 --- a/src/ui/panels/mod.rs +++ b/src/ui/panels/mod.rs @@ -578,14 +578,16 @@ pub fn render_status_bar_with_colors( // empty listing simply skips the left side rather than panicking. if let Some(entry) = panel.listing.filtered_get(panel.cursor) { let display_name = entry.display_name(); - // `entry.size_str` is the column-padded cached size (`{:>10}` for files, - // " " for dirs) reused to avoid a per-frame `format_size` alloc. - // The status bar wants the *unpadded* form the old code produced, so strip - // the leading spaces — format_size output never carries leading whitespace. - let size_str = entry.size_str.trim_start(); + // `entry.size_str` is the column-padded cache — for directories it is + // " ", not a size. The status bar wants the real byte size for + // every entry (as the pre-perf code did via `format_size`), so format the + // entry's size directly into `size_buf` with the zero-alloc helper rather + // than reusing the column cache. + let mut size_buf = String::with_capacity(8); + write_size(&mut size_buf, entry.size()); let mut meta = String::with_capacity(48); - write_status_metadata(&mut meta, size_str, entry, panel.show_permissions()); + write_status_metadata(&mut meta, &size_buf, entry, panel.show_permissions()); let meta_width = UnicodeWidthStr::width(meta.as_str()); let full_width = UnicodeWidthStr::width(display_name) + 3 + meta_width; @@ -606,7 +608,7 @@ pub fn render_status_bar_with_colors( } else { meta.clear(); write!(meta, "{display_name} | ").ok(); - write_status_metadata(&mut meta, size_str, entry, panel.show_permissions()); + write_status_metadata(&mut meta, &size_buf, entry, panel.show_permissions()); let truncated = truncate_to_width(&meta, remaining); out.push_str(&truncated); } diff --git a/src/ui/panels/tests.rs b/src/ui/panels/tests.rs index b4e4d80..a8e9957 100644 --- a/src/ui/panels/tests.rs +++ b/src/ui/panels/tests.rs @@ -841,6 +841,31 @@ fn test_render_status_bar_size_is_unpadded() { ); } +/// Regression: a directory must show its real byte size in the status bar, not +/// the column-cache placeholder "". The pre-perf code used +/// `format_size(entry.size())`; the column cache `size_str` is " " +/// for directories, which is not a size. +#[test] +fn test_render_status_bar_directory_shows_size_not_dir_label() { + let mut panel = PanelState::new(PathBuf::from("/test")); + // Directory with a size (e.g. directory entry block size); column cache = + // " ", but size() = 1024 → "1.0 KB". + panel.set_entries(vec![entry_with("docs", true, false, false, 1024)]); + let content = render_to_string(80, 2, |f| { + render_status_bar_with_colors(f, f.area(), &panel, &DEFAULT_COLORS); + }); + // Must not show the column-cache placeholder... + assert!( + !content.contains(""), + "status bar showed for a directory, got: {content:?}" + ); + // ...and must show a real size (contains a unit suffix, e.g. "B"/"KB"). + assert!( + content.contains(" KB") || content.contains(" MB") || content.contains(" B"), + "status bar missing directory size for dir, got: {content:?}" + ); +} + #[test] fn test_render_function_bar_no_panic() { let content = render_to_string(80, 1, |f| {