bug: fix choosing an out of list bounds selecting the last entry (#717)

This is a simple bug fix that changes the behaviour of a scroll select
(and column select) to only update if the updated position is _within_
the bounds of the list (0 to the max index, inclusive). Prior to this,
all the implementations but the disk implementation would just bound the
change. This was both inconsistent with the disk scroll state, but also
jarring since this meant a user could click on seemingly empty space but
it would somehow click on the very last entry.

This change also unifies the scroll calculation function between all the
scroll select functions. Ideally we get rid of the intermediary
functions but that might require more refactoring than I want for this
fairly simple bug fix.

The column select scroll calculation was also changed to fit this
behaviour, but it does not use the same logic as the other scroll
states. What could be done in the future is a generic implementation for
direction (or maybe just "increment vs. decrement") to share it all.

CHANGELOG.md

@@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [#711](https://github.com/ClementTsang/bottom/pull/711): Fix building in Rust beta 1.61 due to `as_ref()` calls
   causing type inference issues.
 
+  - [#717](https://github.com/ClementTsang/bottom/pull/717): Fix clicking on empty space in tables selecting the very last entry of a list in some cases.
+
 ## Changes
 
 - [#690](https://github.com/ClementTsang/bottom/pull/690): Adds some colour to `-h`/`--help` as part of updating to clap 3.0.

src/app.rs

@@ -1,6 +1,7 @@
 use std::{
     cmp::{max, min},
     collections::HashMap,
+    convert::TryInto,
     path::PathBuf,
     time::Instant,
 };
@@ -2370,7 +2371,7 @@ impl App {
                 }
                 BottomWidgetType::ProcSort => self.change_process_sort_position(amount),
                 BottomWidgetType::Temp => self.change_temp_position(amount),
-                BottomWidgetType::Disk => self.increment_disk_position(amount),
+                BottomWidgetType::Disk => self.change_disk_position(amount),
                 BottomWidgetType::CpuLegend => self.change_cpu_legend_position(amount),
                 _ => {}
             }
@@ -2384,12 +2385,11 @@ impl App {
         {
             let current_posn = proc_widget_state.columns.current_scroll_position;
             let num_columns = proc_widget_state.columns.get_enabled_columns_len();
+            let prop: core::result::Result<usize, _> =
+                (current_posn as i64 + num_to_change_by).try_into();
 
-            if current_posn as i64 + num_to_change_by < 0 {
-                proc_widget_state.columns.current_scroll_position = 0;
-            } else if current_posn as i64 + num_to_change_by >= num_columns as i64 {
-                proc_widget_state.columns.current_scroll_position = num_columns.saturating_sub(1);
-            } else {
+            if let Ok(prop) = prop {
+                if prop < num_columns {
                     proc_widget_state.columns.current_scroll_position =
                         (current_posn as i64 + num_to_change_by) as usize;
                 }
@@ -2401,6 +2401,7 @@ impl App {
                 }
             }
         }
+    }
 
     fn change_cpu_legend_position(&mut self, num_to_change_by: i64) {
         if let Some(cpu_widget_state) = self
@@ -2408,23 +2409,9 @@ impl App {
             .widget_states
             .get_mut(&(self.current_widget.widget_id - 1))
         {
-            let current_posn = cpu_widget_state.scroll_state.current_scroll_position;
-
-            let cap = self.canvas_data.cpu_data.len();
-            if current_posn as i64 + num_to_change_by < 0 {
-                cpu_widget_state.scroll_state.current_scroll_position = 0;
-            } else if current_posn as i64 + num_to_change_by >= cap as i64 {
-                cpu_widget_state.scroll_state.current_scroll_position = cap.saturating_sub(1);
-            } else {
-                cpu_widget_state.scroll_state.current_scroll_position =
-                    (current_posn as i64 + num_to_change_by) as usize;
-            }
-
-            if num_to_change_by < 0 {
-                cpu_widget_state.scroll_state.scroll_direction = ScrollDirection::Up;
-            } else {
-                cpu_widget_state.scroll_state.scroll_direction = ScrollDirection::Down;
-            }
+            cpu_widget_state
+                .scroll_state
+                .update_position(num_to_change_by, self.canvas_data.cpu_data.len());
         }
     }
 
@@ -2434,36 +2421,21 @@ impl App {
             .proc_state
             .get_mut_widget_state(self.current_widget.widget_id)
         {
-            let current_posn = proc_widget_state.scroll_state.current_scroll_position;
             if let Some(finalized_process_data) = self
                 .canvas_data
                 .finalized_process_data_map
                 .get(&self.current_widget.widget_id)
             {
-                if current_posn as i64 + num_to_change_by < 0 {
-                    proc_widget_state.scroll_state.current_scroll_position = 0;
-                } else if current_posn as i64 + num_to_change_by
-                    >= finalized_process_data.len() as i64
-                {
-                    proc_widget_state.scroll_state.current_scroll_position =
-                        finalized_process_data.len().saturating_sub(1);
+                proc_widget_state
+                    .scroll_state
+                    .update_position(num_to_change_by, finalized_process_data.len())
             } else {
-                    proc_widget_state.scroll_state.current_scroll_position =
-                        (current_posn as i64 + num_to_change_by) as usize;
-                }
-            }
-
-            if num_to_change_by < 0 {
-                proc_widget_state.scroll_state.scroll_direction = ScrollDirection::Up;
-            } else {
-                proc_widget_state.scroll_state.scroll_direction = ScrollDirection::Down;
-            }
-
-            return Some(proc_widget_state.scroll_state.current_scroll_position);
-        }
-
                 None
             }
+        } else {
+            None
+        }
+    }
 
     fn change_temp_position(&mut self, num_to_change_by: i64) {
         if let Some(temp_widget_state) = self
@@ -2471,48 +2443,21 @@ impl App {
             .widget_states
             .get_mut(&self.current_widget.widget_id)
         {
-            let current_posn = temp_widget_state.scroll_state.current_scroll_position;
-
-            if current_posn as i64 + num_to_change_by < 0 {
-                temp_widget_state.scroll_state.current_scroll_position = 0;
-            } else if current_posn as i64 + num_to_change_by
-                >= self.canvas_data.temp_sensor_data.len() as i64
-            {
-                temp_widget_state.scroll_state.current_scroll_position =
-                    self.canvas_data.temp_sensor_data.len().saturating_sub(1);
-            } else {
-                temp_widget_state.scroll_state.current_scroll_position =
-                    (current_posn as i64 + num_to_change_by) as usize;
-            }
-
-            if num_to_change_by < 0 {
-                temp_widget_state.scroll_state.scroll_direction = ScrollDirection::Up;
-            } else {
-                temp_widget_state.scroll_state.scroll_direction = ScrollDirection::Down;
-            }
+            temp_widget_state
+                .scroll_state
+                .update_position(num_to_change_by, self.canvas_data.temp_sensor_data.len());
         }
     }
 
-    fn increment_disk_position(&mut self, num_to_change_by: i64) {
+    fn change_disk_position(&mut self, num_to_change_by: i64) {
         if let Some(disk_widget_state) = self
             .disk_state
             .widget_states
             .get_mut(&self.current_widget.widget_id)
         {
-            let current_posn = disk_widget_state.scroll_state.current_scroll_position;
-
-            if current_posn as i64 + num_to_change_by >= 0
-                && current_posn as i64 + num_to_change_by < self.canvas_data.disk_data.len() as i64
-            {
-                disk_widget_state.scroll_state.current_scroll_position =
-                    (current_posn as i64 + num_to_change_by) as usize;
-            }
-
-            if num_to_change_by < 0 {
-                disk_widget_state.scroll_state.scroll_direction = ScrollDirection::Up;
-            } else {
-                disk_widget_state.scroll_state.scroll_direction = ScrollDirection::Down;
-            }
+            disk_widget_state
+                .scroll_state
+                .update_position(num_to_change_by, self.canvas_data.disk_data.len());
         }
     }
 
@@ -3076,7 +3021,7 @@ impl App {
                                         if let Some(visual_index) =
                                             disk_widget_state.scroll_state.table_state.selected()
                                         {
-                                            self.increment_disk_position(
+                                            self.change_disk_position(
                                                 offset_clicked_entry as i64 - visual_index as i64,
                                             );
                                         }

src/app/states.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, time::Instant};
+use std::{collections::HashMap, convert::TryInto, time::Instant};
 
 use unicode_segmentation::GraphemeCursor;
 
@@ -35,11 +35,39 @@ pub enum CursorDirection {
 #[derive(Default)]
 pub struct AppScrollWidgetState {
     pub current_scroll_position: usize,
-    pub previous_scroll_position: usize,
+    pub scroll_bar: usize,
     pub scroll_direction: ScrollDirection,
     pub table_state: TableState,
 }
 
+impl AppScrollWidgetState {
+    /// Updates the position if possible, and if there is a valid change, returns the new position.
+    pub fn update_position(&mut self, change: i64, num_entries: usize) -> Option<usize> {
+        if change == 0 {
+            return None;
+        }
+
+        let csp: Result<i64, _> = self.current_scroll_position.try_into();
+        if let Ok(csp) = csp {
+            let proposed: Result<usize, _> = (csp + change).try_into();
+            if let Ok(proposed) = proposed {
+                if proposed < num_entries {
+                    self.current_scroll_position = proposed;
+                    if change < 0 {
+                        self.scroll_direction = ScrollDirection::Up;
+                    } else {
+                        self.scroll_direction = ScrollDirection::Down;
+                    }
+
+                    return Some(self.current_scroll_position);
+                }
+            }
+        }
+
+        None
+    }
+}
+
 #[derive(PartialEq)]
 pub enum KillSignal {
     Cancel,
@@ -635,7 +663,7 @@ impl ProcWidgetState {
                 self.process_search_state.search_state.error_message = Some(err.to_string());
             }
         }
-        self.scroll_state.previous_scroll_position = 0;
+        self.scroll_state.scroll_bar = 0;
         self.scroll_state.current_scroll_position = 0;
     }
 
@@ -942,3 +970,62 @@ pub struct ConfigCategory {
 pub struct ConfigOption {
     pub set_function: Box<dyn Fn() -> anyhow::Result<()>>,
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_scroll_update_position() {
+        let mut scroll = AppScrollWidgetState {
+            current_scroll_position: 5,
+            scroll_bar: 0,
+            scroll_direction: ScrollDirection::Down,
+            table_state: Default::default(),
+        };
+
+        // Update by 0. Should not change.
+        assert_eq!(scroll.update_position(0, 15), None);
+        assert_eq!(scroll.current_scroll_position, 5);
+
+        // Update by 5. Should increment to index 10.
+        assert_eq!(scroll.update_position(5, 15), Some(10));
+        assert_eq!(scroll.current_scroll_position, 10);
+
+        // Update by 5. Should not change.
+        assert_eq!(scroll.update_position(5, 15), None);
+        assert_eq!(scroll.current_scroll_position, 10);
+
+        // Update by 4. Should increment to index 14 (supposed max).
+        assert_eq!(scroll.update_position(4, 15), Some(14));
+        assert_eq!(scroll.current_scroll_position, 14);
+
+        // Update by 1. Should do nothing.
+        assert_eq!(scroll.update_position(1, 15), None);
+        assert_eq!(scroll.current_scroll_position, 14);
+
+        // Update by -15. Should do nothing.
+        assert_eq!(scroll.update_position(-15, 15), None);
+        assert_eq!(scroll.current_scroll_position, 14);
+
+        // Update by -14. Should land on position 0.
+        assert_eq!(scroll.update_position(-14, 15), Some(0));
+        assert_eq!(scroll.current_scroll_position, 0);
+
+        // Update by -1. Should do nothing.
+        assert_eq!(scroll.update_position(-1, 15), None);
+        assert_eq!(scroll.current_scroll_position, 0);
+
+        // Update by 0. Should do nothing.
+        assert_eq!(scroll.update_position(0, 15), None);
+        assert_eq!(scroll.current_scroll_position, 0);
+
+        // Update by 15. Should do nothing.
+        assert_eq!(scroll.update_position(15, 15), None);
+        assert_eq!(scroll.current_scroll_position, 0);
+
+        // Update by 15 but with a larger bound. Should increment to 15.
+        assert_eq!(scroll.update_position(15, 16), Some(15));
+        assert_eq!(scroll.current_scroll_position, 15);
+    }
+}

src/canvas/widgets/cpu_graph.rs

@@ -401,7 +401,7 @@ impl CpuGraphWidget for Painter {
                     (draw_loc.height + (1 - table_gap)).saturating_sub(self.table_height_offset),
                 ),
                 &cpu_widget_state.scroll_state.scroll_direction,
-                &mut cpu_widget_state.scroll_state.previous_scroll_position,
+                &mut cpu_widget_state.scroll_state.scroll_bar,
                 cpu_widget_state.scroll_state.current_scroll_position,
                 app_state.is_force_redraw,
             );

src/canvas/widgets/disk_table.rs

@@ -51,7 +51,7 @@ impl DiskTableWidget for Painter {
                     (draw_loc.height + (1 - table_gap)).saturating_sub(self.table_height_offset),
                 ),
                 &disk_widget_state.scroll_state.scroll_direction,
-                &mut disk_widget_state.scroll_state.previous_scroll_position,
+                &mut disk_widget_state.scroll_state.scroll_bar,
                 disk_widget_state.scroll_state.current_scroll_position,
                 app_state.is_force_redraw,
             );

src/canvas/widgets/process_table.rs

@@ -301,7 +301,7 @@ impl ProcessTableWidget for Painter {
                             .saturating_sub(self.table_height_offset),
                     ),
                     &proc_widget_state.scroll_state.scroll_direction,
-                    &mut proc_widget_state.scroll_state.previous_scroll_position,
+                    &mut proc_widget_state.scroll_state.scroll_bar,
                     proc_widget_state.scroll_state.current_scroll_position,
                     app_state.is_force_redraw,
                 );

src/canvas/widgets/temp_table.rs

@@ -51,7 +51,7 @@ impl TempTableWidget for Painter {
                     (draw_loc.height + (1 - table_gap)).saturating_sub(self.table_height_offset),
                 ),
                 &temp_widget_state.scroll_state.scroll_direction,
-                &mut temp_widget_state.scroll_state.previous_scroll_position,
+                &mut temp_widget_state.scroll_state.scroll_bar,
                 temp_widget_state.scroll_state.current_scroll_position,
                 app_state.is_force_redraw,
             );

src/lib.rs

@@ -451,7 +451,7 @@ fn update_final_process_list(app: &mut App, widget_id: u64) {
             {
                 proc_widget_state.scroll_state.current_scroll_position =
                     finalized_process_data.len().saturating_sub(1);
-                proc_widget_state.scroll_state.previous_scroll_position = 0;
+                proc_widget_state.scroll_state.scroll_bar = 0;
                 proc_widget_state.scroll_state.scroll_direction = app::ScrollDirection::Down;
             }