diff --git a/local/recipes/system/redbear-power/source/src/app.rs b/local/recipes/system/redbear-power/source/src/app.rs index 2b554d2b46..5474d28b8d 100644 --- a/local/recipes/system/redbear-power/source/src/app.rs +++ b/local/recipes/system/redbear-power/source/src/app.rs @@ -161,6 +161,10 @@ pub meminfo: crate::meminfo::MemInfo, /// Cursor index into the visible (post-filter) process list. /// Distinct from `table_state` which tracks the Per-CPU tab. pub process_cursor: usize, + /// Last CPU id clicked by the mouse. A second click on the + /// same row toggles expand (mirrors htop). `None` until + /// the first click. + pub last_clicked_cpu: Option, pub pid_detail: Option, pub refresh_counter: u32, pub status_msg: String, @@ -330,6 +334,7 @@ impl App { process_tree: false, folded: std::collections::BTreeSet::new(), io_history: std::collections::BTreeMap::new(), + last_clicked_cpu: None, cpu_history: std::collections::BTreeMap::new(), rss_history: std::collections::BTreeMap::new(), process_cursor: 0, @@ -993,6 +998,29 @@ mod tests { assert_eq!(app.selected_pid(), None); } + #[test] + fn selected_pid_respects_filter_to_find_correct_row() { + // v1.37 fix for the v1.36 right-click bug: when a + // filter is active, the cursor indexes the post-filter + // visible list. selected_pid() must return the PID of + // the visible-row-at-cursor, not the unfiltered row at + // the same index. With 3 procs (pids 100, 101, 102), + // set process_filter to "proc1" (matches only pid 101), + // set cursor=0, and selected_pid should return 101 + // (not 100). + let mut app = make_app_with_processes(3); + app.process_filter = "proc1".to_string(); + app.process_cursor = 0; + assert_eq!(app.selected_pid(), Some(101)); + // Now set cursor beyond the visible count; should be + // clamped (visible_processes is post-filter; the cursor + // bounds check in move_process_selection is what keeps + // the cursor in range). selected_pid at out-of-range + // returns None. + app.process_cursor = 99; + assert_eq!(app.selected_pid(), None); + } + #[test] fn update_io_history_reaps_exited_pids() { let mut app = make_app_with_processes(3); @@ -1034,14 +1062,10 @@ mod tests { // (e.g. multi-threaded proc that contributes 2+ samples // per refresh — not the current design but exercised by // direct test), the max within the window normalizes. - // Drive via direct call to commit_normalized. + // Drive via direct call to commit_normalized_visible. let mut app = App::new(); let samples = vec![50.0, 100.0]; // max=100 - crate::app::tests::commit_normalized_visible( - &mut app.cpu_history, - 100, - &samples, - ); + App::commit_normalized_visible(&mut app.cpu_history, 100, &samples); let hist = &app.cpu_history[&100]; // 50/100*255 = 127.5 -> rounds to 128 // 100/100*255 = 255 @@ -1049,6 +1073,59 @@ mod tests { assert_eq!(hist[1], 255); } + #[test] + fn update_io_history_then_sparkline_render_is_nonzero() { + // v1.37 integration test: would have caught the v1.32 + // type-confusion bug. The previous design wrote integer- + // as-u64 to the history cells, which the renderer read + // as f64 bits (subnormal ~6e-324 -> 0). This test seeds + // a real rate, runs update, and asserts the renderer + // produces a NON-SPACE char (proving the type is u8 + // and the max is non-zero). + let mut app = make_app_with_processes(1); + // Set a non-zero rate. The first refresh produces a + // single-sample window; max=the value; normalized=255. + app.processes.processes[0].io_read_kb = Some(1024); + app.processes.processes[0].io_write_kb = Some(2048); + app.processes.processes[0].io_read_rate_kbs = Some(100.0); + app.processes.processes[0].io_write_rate_kbs = Some(200.0); + app.update_io_history(); + let pid = 100; + let hist = app + .io_history + .get(&pid) + .expect("io_history should have an entry for pid 100"); + assert!(!hist.is_empty(), "io_history should have at least one sample"); + // Every cell must be a true u8 in 0..=255. + for &b in hist { + // u8 max is 255 by type, but a subnormal conversion + // would yield 0. The bug had all cells = 0; the + // fix has at least one non-zero cell. + // We assert all cells are non-zero (this PID has + // a known rate). + assert!(b > 0, "io_history cell is 0; would render as blank bar (v1.32 bug)"); + } + } + + #[test] + fn update_io_history_no_rate_yields_zero_history() { + // Sanity check: a PID with no rate (io_read_kb/io_write_kb + // = None) should NOT have an entry in io_history. v1.32 + // created entries for CPU and RSS but skipped IO when + // the rate was None. + let mut app = make_app_with_processes(2); + // pid 100 has no rate; pid 101 has a rate. + app.processes.processes[1].io_read_kb = Some(1024); + app.processes.processes[1].io_write_kb = Some(2048); + app.processes.processes[1].io_read_rate_kbs = Some(100.0); + app.processes.processes[1].io_write_rate_kbs = Some(200.0); + app.update_io_history(); + assert!(!app.io_history.contains_key(&100), + "pid 100 has no rate; io_history should skip it"); + assert!(app.io_history.contains_key(&101), + "pid 101 has a rate; io_history should include it"); + } + #[test] fn update_io_history_handles_all_zero() { // All-zero values: max is 0, the algorithm should leave @@ -1145,4 +1222,23 @@ mod tests { app.move_to_edge(false); assert_eq!(app.process_cursor, 0); } + + #[test] + fn page_selection_process_jumps_by_8_rows() { + // PageDown/PageUp for the Process tab should move the + // cursor by ROWS_PER_PAGE (8) rows at a time. v1.37 + // regression test (v1.35 added the keypress wiring; the + // existing test coverage was for `move_selection`, not + // the page dispatcher). + let mut app = make_app_with_processes(20); + app.process_cursor = 0; + app.page_selection(1); // PageDown +1 page + assert_eq!(app.process_cursor, 8); + app.page_selection(1); // another PageDown + assert_eq!(app.process_cursor, 16); + app.page_selection(1); // would land at 24, clamped to 19 + assert_eq!(app.process_cursor, 19); + app.page_selection(-1); // PageUp -1 page + assert_eq!(app.process_cursor, 11); + } } \ No newline at end of file diff --git a/local/recipes/system/redbear-power/source/src/main.rs b/local/recipes/system/redbear-power/source/src/main.rs index 15017bafdb..b1cf2506ca 100644 --- a/local/recipes/system/redbear-power/source/src/main.rs +++ b/local/recipes/system/redbear-power/source/src/main.rs @@ -149,7 +149,15 @@ fn handle_mouse(me: MouseEvent, header: &Rect, table: &Rect, controls: &Rect, ap if let Some(id) = id { if let Some(idx) = app.cpus.iter().position(|c| c.id == id) { app.table_state.select(Some(idx)); - app.expanded_cpu = None; + // Re-click on the same row toggles expand + // (mirrors htop). A click on a different + // row just selects that row. + if app.last_clicked_cpu == Some(id) { + app.toggle_expand(); + } else { + app.expanded_cpu = None; + } + app.last_clicked_cpu = Some(id); } } } else if hit_test(*header, x, y) { @@ -167,11 +175,11 @@ fn handle_mouse(me: MouseEvent, header: &Rect, table: &Rect, controls: &Rect, ap // Process tab uses the full body_area as its panel. // Click in the body maps to visible_index. The // Process tab header is at body.y; rows start at - // body.y + 2 (line 0 is the title; line 1 is blank; + // body.y + 3 (line 0 is the title; line 1 is blank; // line 2 is the column header; rows start at line 3). let body = Rect::new(table.x, table.y, table.width, table.height); if hit_test(body, x, y) { - let row = y.saturating_sub(table.y + 2) as usize; + let row = y.saturating_sub(table.y + 3) as usize; app.process_cursor = row; } } @@ -185,7 +193,7 @@ fn handle_mouse(me: MouseEvent, header: &Rect, table: &Rect, controls: &Rect, ap // Right-click on a process row opens the PID detail. let body = Rect::new(table.x, table.y, table.width, table.height); if hit_test(body, x, y) { - let row = y.saturating_sub(table.y + 2) as usize; + let row = y.saturating_sub(table.y + 3) as usize; app.process_cursor = row; if let Some(pid) = app.selected_pid() { app.pid_detail = Some(crate::pid_detail::PidDetail::read(pid)); diff --git a/local/recipes/system/redbear-power/source/src/render.rs b/local/recipes/system/redbear-power/source/src/render.rs index 98af961ba8..d03526eede 100644 --- a/local/recipes/system/redbear-power/source/src/render.rs +++ b/local/recipes/system/redbear-power/source/src/render.rs @@ -1023,6 +1023,16 @@ pub fn render_process_panel<'a>(app: &'a App, focused: bool) -> Paragraph<'a> { /// /// O(N) per call, O(N^2) worst case for the full render. Fine for /// the truncated top-50 list. +#[cfg(test)] +pub(crate) fn tree_prefix_for_test( + pid: u32, + ppid: u32, + all: &[crate::process::ProcessInfo], + folded: &std::collections::BTreeSet, +) -> String { + tree_prefix(pid, ppid, all, folded) +} + fn tree_prefix( pid: u32, ppid: u32, @@ -1067,43 +1077,37 @@ fn tree_prefix( let next_ppid = all.get(my_index + 1).map(|n| n.ppid); let is_last = next_ppid.map_or(true, |np| np != ppid); - // For each ancestor level, decide if the bar continues: - // the bar at depth `d` (counting from 0 for the row itself's - // parent) continues iff there is a next row whose ancestor - // chain at that depth is the same ancestor. Equivalently: for - // ancestor `a` at chain index `d`, look at the next row in - // the list. If the next row's ancestor at depth `d` is - // `a` (or the next row's ppid is `a`), then the bar at depth - // `d` continues. + // For each ancestor level, decide if the bar continues. The + // bar at depth d (counting from 0 for the row's immediate + // parent) continues iff the NEXT row in the list is still a + // descendant of `ancestor` — either directly (next row's + // ppid == ancestor) or through a deeper chain. The v1.34 + // implementation walked the next row's chain only `0..=d` + // steps which missed the case where the next row is a deeper + // descendant of `ancestor`. v1.37 walks the full chain. let mut bar = String::new(); - for (d, &ancestor) in ancestors.iter().enumerate() { + for &ancestor in ancestors.iter() { let next_row = all.get(my_index + 1); - // Is the ancestor still "active" at this depth for the - // NEXT row? An ancestor is active for the next row iff: - // - the next row's ppid chain has `ancestor` at depth `d`, - // OR - // - the next row's ppid IS `ancestor` (the next row is - // a child of the ancestor, so the ancestor is the - // immediate parent of the next row). let ancestor_active_in_next = next_row.map_or(false, |n| { if n.ppid == ancestor { return true; } - // Walk the next row's ancestor chain and see if - // `ancestor` appears at the same depth. - let mut next_cur = n.pid; - for next_d in 0..=d { - let np = match by_pid.get(&next_cur) { + // Walk the next row's full ancestor chain looking + // for `ancestor`. Walk is bounded at 64 hops for + // cycle safety. + let mut cur = n.pid; + for _ in 0..64 { + let np = match by_pid.get(&cur) { Some(np) => *np, None => return false, }; - if next_d == d { - return np.ppid == ancestor; - } - if np.ppid == 0 || !by_pid.contains_key(&np.ppid) { + if np.ppid == 0 { return false; } - next_cur = np.ppid; + if np.ppid == ancestor { + return true; + } + cur = np.ppid; } false }); @@ -1637,4 +1641,128 @@ pub fn render_once(app: &App) -> io::Result<()> { .expect("draw"); print!("{}", buffer_to_string(terminal.backend().buffer())); Ok(()) -} \ No newline at end of file +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::process::ProcessInfo; + + fn make_proc(pid: u32, ppid: u32) -> ProcessInfo { + ProcessInfo { + pid, + ppid, + comm: format!("p{pid}"), + ..Default::default() + } + } + + fn empty_folded() -> std::collections::BTreeSet { + std::collections::BTreeSet::new() + } + + #[test] + fn tree_prefix_root_has_no_ancestor_bar() { + // Single root, no children -> no connector, no bar. + let all = vec![make_proc(1, 0)]; + let s = tree_prefix_for_test(1, 0, &all, &empty_folded()); + // No ancestors -> empty prefix. + assert!(s.is_empty() || s == " ", "root prefix is empty or fold marker"); + } + + #[test] + fn tree_prefix_child_uses_connector() { + // 1 -> 2: child of 1. Last child of 1 in this list. + // Leaf (no children) so fold marker is " ". + let all = vec![make_proc(1, 0), make_proc(2, 1)]; + let s = tree_prefix_for_test(2, 1, &all, &empty_folded()); + // Depth 1, no continuation (2 is the last in the list). + // Connector is "└─ " (last child), bar is empty. + assert_eq!(s, " └─ ", "leaf child of single child: expected ' └─ ' (bar=empty, connector=last, fold=leaf), got '{s}'"); + } + + #[test] + fn tree_prefix_two_level_continues_bar() { + // 1 -> {2 -> 3, 4}. pid=2 is NOT the last child of 1 + // (pid=4 follows), so its bar at depth 0 should + // continue: "│ └─ " (the bar is at depth 0; depth 1 + // is the row itself's connector). pid=2 has a child + // (pid=3) so the fold marker is "▼ " (expanded). + let all = vec![ + make_proc(1, 0), + make_proc(2, 1), + make_proc(3, 2), + make_proc(4, 1), + ]; + let s = tree_prefix_for_test(2, 1, &all, &empty_folded()); + // bar="│ " + connector="└─ " + fold="▼ " + assert_eq!(s, "│ └─ ▼ ", "got '{s}'"); + } + + #[test] + fn tree_prefix_three_level_continues_bar_at_each_depth() { + // 1 -> {2 -> {3, 4}, 5} + // pid=3 is depth 2 (parent=2, grandparent=1, great-grandparent=root). + // Its bar at depth 0 (above pid=3) is for ancestor 2. + // is 2 still active in the next row? Next row is pid=4, + // ppid=2. So yes -> "│ ". + // Its bar at depth 1 (above pid=3) is for ancestor 1. + // is 1 still active in the next row? Next row is pid=4, + // ppid=2; walk: 4's ppid=2, 2's ppid=1. So 1 is in + // the chain -> "│ ". + let all = vec![ + make_proc(1, 0), + make_proc(2, 1), + make_proc(3, 2), + make_proc(4, 2), + make_proc(5, 1), + ]; + let s = tree_prefix_for_test(3, 2, &all, &empty_folded()); + // Expected: "│ │ ├─ " (2 levels of "│ ", then the + // connector for the row itself). + assert!(s.starts_with("│ │ ├─ "), "expected double-continue bar, got '{s}'"); + } + + #[test] + fn tree_prefix_bar_ends_when_no_more_siblings() { + // 1 -> 2 (the only child). pid=2's bar at depth 0 is + // for ancestor 1. Is 1 active in the next row? No + // (there's no next row). So bar is " " and the + // connector is "└─ " (last child). Leaf so fold + // marker is " ". + let all = vec![make_proc(1, 0), make_proc(2, 1)]; + let s = tree_prefix_for_test(2, 1, &all, &empty_folded()); + // bar=" " + connector="└─ " + fold=" " + assert_eq!(s, " └─ ", "single-child tree leaf: got '{s}'"); + } + + #[test] + fn tree_prefix_fold_marker_shown_when_has_children() { + // 1 -> 2. Pid 1 has a child, so the fold marker is "▼ " + // (expanded). The full prefix for pid 1 (depth 0, root) + // is just "▼ " (no connector, fold marker). + let all = vec![make_proc(1, 0), make_proc(2, 1)]; + let s = tree_prefix_for_test(1, 0, &all, &empty_folded()); + assert!(s.starts_with("▼ "), "expanded root should show ▼, got '{s}'"); + } + + #[test] + fn tree_prefix_fold_marker_collapsed() { + // 1 -> 2, with pid 1 in the folded set. + let all = vec![make_proc(1, 0), make_proc(2, 1)]; + let mut folded = std::collections::BTreeSet::new(); + folded.insert(1); + let s = tree_prefix_for_test(1, 0, &all, &folded); + assert!(s.starts_with("▶ "), "folded root should show ▶, got '{s}'"); + } + + #[test] + fn tree_prefix_leaf_has_no_fold_marker() { + // 1 -> 2. Pid 2 has no children, so no fold marker. + let all = vec![make_proc(1, 0), make_proc(2, 1)]; + let s = tree_prefix_for_test(2, 1, &all, &empty_folded()); + // No "▶" or "▼" should appear. + assert!(!s.contains('▶'), "leaf should not show ▶, got '{s}'"); + assert!(!s.contains('▼'), "leaf should not show ▼, got '{s}'"); + } +}