refactor: refactor the data table increment position code (#1762)

* refactor: refactor the data table increment position code

* some comments

* changelog

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
 

src/canvas/components/data_table.rs

@@ -96,46 +96,35 @@ impl<DataType: DataToCell<H>, H: ColumnHeader, S: SortType, C: DataTableColumn<H
     /// Increments the scroll position if possible by a positive/negative
     /// offset. If there is a valid change, this function will also return
     /// the new position wrapped in an [`Option`].
+    ///
+    /// Note that despite the name, this handles both incrementing (positive
+    /// change) and decrementing (negative change).
     pub fn increment_position(&mut self, change: i64) -> Option<usize> {
-        let max_index = self.data.len();
+        let num_entries = self.data.len();
-        let current_index = self.state.current_index;
 
-        if change == 0
+        if num_entries == 0 {
-            || (change > 0 && current_index == max_index)
-            || (change < 0 && current_index == 0)
-        {
             return None;
         }
 
-        let csp: Result<i64, _> = self.state.current_index.try_into();
+        let Ok(current_index): Result<i64, _> = self.state.current_index.try_into() else {
-        if let Ok(csp) = csp {
+            return None;
-            let proposed = csp + change;
-
-            let proposed: Result<usize, _> = if proposed.is_negative() {
-                Ok(0)
-            } else {
-                proposed.try_into()
         };
 
-            if let Ok(proposed) = proposed {
+        // We do this to clamp the proposed index to 0 if the change is greater
-                let proposed = if proposed >= max_index {
+        // than the number of entries left from the current index. This gives
-                    max_index - 1
+        // a more intuitive behaviour when using things like page up/down.
-                } else {
+        let proposed = current_index + change;
-                    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
         };
 
-                return Some(self.state.current_index);
+        Some(self.state.current_index)
-            }
-        }
-
-        None
     }
 
     /// Updates the scroll position to a selected index.
@@ -194,8 +183,7 @@ mod test {
         }
     }
 
-    #[test]
+    fn create_test_table() -> DataTable<TestType, &'static str> {
-    fn test_data_table_operations() {
         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::<Vec<_>>());
 
         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::<Vec<_>>());
 
         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::<Vec<_>>());
+
+        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::<Vec<_>>());
+
+        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::<Vec<_>>());
         assert_eq!(table.current_index(), 2);
         assert_eq!(table.state.scroll_direction, ScrollDirection::Down);