From bb0d4bdd322e76c2fad6d92fa99dc722da74ff37 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Tue, 29 Jul 2025 04:24:49 -0400 Subject: [PATCH] refactor: refactor the data table increment position code (#1762) * refactor: refactor the data table increment position code * some comments * changelog --- CHANGELOG.md | 1 + src/canvas/components/data_table.rs | 101 ++++++++++++++++++---------- 2 files changed, 68 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b67f7d59..ef57e644 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ That said, these are more guidelines rather than hardset rules, though the proje - [#1683](https://github.com/ClementTsang/bottom/pull/1683): Fix graph lines potentially showing up behind legends. - [#1701](https://github.com/ClementTsang/bottom/pull/1701): Fix process kill dialog occasionally causing panics. - [#1755](https://github.com/ClementTsang/bottom/pull/1755): Fix missing stats/incorrect mount name for certain entries in the disk widget. +- [#1759](https://github.com/ClementTsang/bottom/pull/1759): Fix increment for data tables if the change is greater than the number of entries left. ### Changes diff --git a/src/canvas/components/data_table.rs b/src/canvas/components/data_table.rs index 00559309..c2751886 100644 --- a/src/canvas/components/data_table.rs +++ b/src/canvas/components/data_table.rs @@ -96,46 +96,35 @@ impl, H: ColumnHeader, S: SortType, C: DataTableColumn Option { - let max_index = self.data.len(); - let current_index = self.state.current_index; + let num_entries = self.data.len(); - if change == 0 - || (change > 0 && current_index == max_index) - || (change < 0 && current_index == 0) - { + if num_entries == 0 { return None; } - let csp: Result = self.state.current_index.try_into(); - if let Ok(csp) = csp { - let proposed = csp + change; + let Ok(current_index): Result = self.state.current_index.try_into() else { + return None; + }; - let proposed: Result = if proposed.is_negative() { - Ok(0) - } else { - proposed.try_into() - }; + // We do this to clamp the proposed index to 0 if the change is greater + // than the number of entries left from the current index. This gives + // a more intuitive behaviour when using things like page up/down. + let proposed = current_index + change; - if let Ok(proposed) = proposed { - let proposed = if proposed >= max_index { - max_index - 1 - } else { - proposed - }; + // We check num_entries > 0 above. + self.state.current_index = proposed.clamp(0, (num_entries - 1) as i64) as usize; - self.state.current_index = proposed; - self.state.scroll_direction = if change < 0 { - ScrollDirection::Up - } else { - ScrollDirection::Down - }; + self.state.scroll_direction = if change < 0 { + ScrollDirection::Up + } else { + ScrollDirection::Down + }; - return Some(self.state.current_index); - } - } - - None + Some(self.state.current_index) } /// Updates the scroll position to a selected index. @@ -194,8 +183,7 @@ mod test { } } - #[test] - fn test_data_table_operations() { + fn create_test_table() -> DataTable { let columns = [Column::hard("a", 10), Column::hard("b", 10)]; let props = DataTableProps { title: Some("test".into()), @@ -207,7 +195,12 @@ mod test { }; let styling = DataTableStyling::default(); - let mut table = DataTable::new(columns, props, styling); + DataTable::new(columns, props, styling) + } + + #[test] + fn test_scrolling() { + let mut table = create_test_table(); table.set_data((0..=4).map(|index| TestType { index }).collect::>()); table.scroll_to_last(); @@ -217,6 +210,12 @@ mod test { table.scroll_to_first(); assert_eq!(table.current_index(), 0); assert_eq!(table.state.scroll_direction, ScrollDirection::Up); + } + + #[test] + fn test_set_position() { + let mut table = create_test_table(); + table.set_data((0..=4).map(|index| TestType { index }).collect::>()); table.set_position(4); assert_eq!(table.current_index(), 4); @@ -226,6 +225,16 @@ mod test { assert_eq!(table.current_index(), 4); assert_eq!(table.state.scroll_direction, ScrollDirection::Down); assert_eq!(table.current_item(), Some(&TestType { index: 4 })); + } + + #[test] + fn test_increment_position() { + let mut table = create_test_table(); + table.set_data((0..=4).map(|index| TestType { index }).collect::>()); + + table.set_position(4); + assert_eq!(table.current_index(), 4); + assert_eq!(table.state.scroll_direction, ScrollDirection::Down); table.increment_position(-1); assert_eq!(table.current_index(), 3); @@ -257,6 +266,30 @@ mod test { assert_eq!(table.state.scroll_direction, ScrollDirection::Down); assert_eq!(table.current_item(), Some(&TestType { index: 4 })); + // Make sure that overscrolling up causes clamping. + table.increment_position(-10); + assert_eq!(table.current_index(), 0); + assert_eq!(table.state.scroll_direction, ScrollDirection::Up); + assert_eq!(table.current_item(), Some(&TestType { index: 0 })); + + // Make sure that overscrolling down causes clamping. + table.increment_position(100); + assert_eq!(table.current_index(), 4); + assert_eq!(table.state.scroll_direction, ScrollDirection::Down); + assert_eq!(table.current_item(), Some(&TestType { index: 4 })); + } + + /// A test to ensure that scroll offsets are correctly handled when we "lose" rows. + #[test] + fn test_lose_data() { + let mut table = create_test_table(); + table.set_data((0..=4).map(|index| TestType { index }).collect::>()); + + table.set_position(4); + assert_eq!(table.current_index(), 4); + assert_eq!(table.state.scroll_direction, ScrollDirection::Down); + assert_eq!(table.current_item(), Some(&TestType { index: 4 })); + table.set_data((0..=2).map(|index| TestType { index }).collect::>()); assert_eq!(table.current_index(), 2); assert_eq!(table.state.scroll_direction, ScrollDirection::Down);