bug: Fix mouse hitboxes (#459)

Fixes the mouse hitbox checks overextending by 1. Also reverts the bandaid fix done for #458.

8 files changed, 194 insertions, 171 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 917ffe49..92e6af53 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,8 +65,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - [#425](https://github.com/ClementTsang/bottom/pull/425): Fixed a bug allowing grouped mode in tree mode if already in grouped mode.
 
-- [#458](https://github.com/ClementTsang/bottom/pull/458): Fix basic mode having really broken click bounding boxes.
-
 ## [0.5.7] - 2021-01-30
 
 ## Bug Fixes
diff --git a/src/app.rs b/src/app.rs
index 27385ed5..365d923a 100644
--- a/src/app.rs
+++ b/src/app.rs
@@ -2809,7 +2809,7 @@ impl App {
         // Pretty dead simple - iterate through the widget map and go to the widget where the click
         // is within.
 
-        // TODO: [REFACTOR] might want to refactor this, it's ugly as sin.
+        // TODO: [REFACTOR] might want to refactor this, it's really ugly.
         // TODO: [REFACTOR] Might wanna refactor ALL state things in general, currently everything
         // is grouped up as an app state.  We should separate stuff like event state and gui state and etc.
 
@@ -2826,7 +2826,8 @@ impl App {
                 Some((right_brc_x, right_brc_y)),
             ) = (bt.left_tlc, bt.left_brc, bt.right_tlc, bt.right_brc)
             {
-                if (x >= left_tlc_x && y >= left_tlc_y) && (x <= left_brc_x && y <= left_brc_y) {
+                if (x >= left_tlc_x && y >= left_tlc_y) && (x < left_brc_x && y < left_brc_y) {
+                    // Case for the left "button" in the simple arrow.
                     if let Some(new_widget) =
                         self.widget_map.get(&(bt.currently_displayed_widget_id))
                     {
@@ -2846,8 +2847,9 @@ impl App {
                         return;
                     }
                 } else if (x >= right_tlc_x && y >= right_tlc_y)
-                    && (x <= right_brc_x && y <= right_brc_y)
+                    && (x < right_brc_x && y < right_brc_y)
                 {
+                    // Case for the right "button" in the simple arrow.
                     if let Some(new_widget) =
                         self.widget_map.get(&(bt.currently_displayed_widget_id))
                     {
@@ -2905,7 +2907,7 @@ impl App {
             if let (Some((tlc_x, tlc_y)), Some((brc_x, brc_y))) =
                 (widget.top_left_corner, widget.bottom_right_corner)
             {
-                if (x >= tlc_x && y >= tlc_y) && (x <= brc_x && y <= brc_y) {
+                if (x >= tlc_x && y >= tlc_y) && (x < brc_x && y < brc_y) {
                     if let Some(new_widget) = self.widget_map.get(&new_widget_id) {
                         self.current_widget = new_widget.clone();
 
@@ -2939,179 +2941,187 @@ impl App {
         }
 
         // Now handle click propagation down to widget.
-        if let Some((_tlc_x, tlc_y)) = &self.current_widget.top_left_corner {
-            match &self.current_widget.widget_type {
-                BottomWidgetType::Proc
-                | BottomWidgetType::ProcSort
-                | BottomWidgetType::CpuLegend
-                | BottomWidgetType::Temp
-                | BottomWidgetType::Disk => {
-                    // Get our index...
-                    let clicked_entry = y - *tlc_y;
-                    // + 1 so we start at 0.
-                    let border_offset = if self.is_drawing_border() { 1 } else { 0 };
-                    let header_gap_offset = 1 + if self.is_drawing_gap(&self.current_widget) {
-                        self.app_config_fields.table_gap
-                    } else {
-                        0
-                    };
-                    let offset = border_offset + header_gap_offset;
-                    if clicked_entry >= offset {
-                        let offset_clicked_entry = clicked_entry - offset;
-                        match &self.current_widget.widget_type {
-                            BottomWidgetType::Proc => {
-                                if let Some(proc_widget_state) = self
-                                    .proc_state
-                                    .get_widget_state(self.current_widget.widget_id)
-                                {
-                                    if let Some(visual_index) =
-                                        proc_widget_state.scroll_state.table_state.selected()
+        if let (Some((_tlc_x, tlc_y)), Some((_brc_x, brc_y))) = (
+            &self.current_widget.top_left_corner,
+            &self.current_widget.bottom_right_corner,
+        ) {
+            let border_offset = if self.is_drawing_border() { 1 } else { 0 };
+
+            // This check ensures the click isn't actually just clicking on the bottom border.
+            if y < (brc_y - border_offset) {
+                match &self.current_widget.widget_type {
+                    BottomWidgetType::Proc
+                    | BottomWidgetType::ProcSort
+                    | BottomWidgetType::CpuLegend
+                    | BottomWidgetType::Temp
+                    | BottomWidgetType::Disk => {
+                        // Get our index...
+                        let clicked_entry = y - *tlc_y;
+                        // + 1 so we start at 0.
+                        let header_gap_offset = 1 + if self.is_drawing_gap(&self.current_widget) {
+                            self.app_config_fields.table_gap
+                        } else {
+                            0
+                        };
+                        let offset = border_offset + header_gap_offset;
+                        if clicked_entry >= offset {
+                            let offset_clicked_entry = clicked_entry - offset;
+                            match &self.current_widget.widget_type {
+                                BottomWidgetType::Proc => {
+                                    if let Some(proc_widget_state) = self
+                                        .proc_state
+                                        .get_widget_state(self.current_widget.widget_id)
                                     {
-                                        // If in tree mode, also check to see if this click is on
-                                        // the same entry as the already selected one - if it is,
-                                        // then we minimize.
-
-                                        let previous_scroll_position =
-                                            proc_widget_state.scroll_state.current_scroll_position;
-                                        let is_tree_mode = proc_widget_state.is_tree_mode;
-
-                                        let new_position = self.increment_process_position(
-                                            offset_clicked_entry as i64 - visual_index as i64,
-                                        );
-
-                                        if is_tree_mode {
-                                            if let Some(new_position) = new_position {
-                                                if previous_scroll_position == new_position {
-                                                    self.toggle_collapsing_process_branch();
+                                        if let Some(visual_index) =
+                                            proc_widget_state.scroll_state.table_state.selected()
+                                        {
+                                            // If in tree mode, also check to see if this click is on
+                                            // the same entry as the already selected one - if it is,
+                                            // then we minimize.
+
+                                            let previous_scroll_position = proc_widget_state
+                                                .scroll_state
+                                                .current_scroll_position;
+                                            let is_tree_mode = proc_widget_state.is_tree_mode;
+
+                                            let new_position = self.increment_process_position(
+                                                offset_clicked_entry as i64 - visual_index as i64,
+                                            );
+
+                                            if is_tree_mode {
+                                                if let Some(new_position) = new_position {
+                                                    if previous_scroll_position == new_position {
+                                                        self.toggle_collapsing_process_branch();
+                                                    }
                                                 }
                                             }
                                         }
                                     }
                                 }
-                            }
-                            BottomWidgetType::ProcSort => {
-                                if let Some(proc_widget_state) = self
-                                    .proc_state
-                                    .get_widget_state(self.current_widget.widget_id - 2)
-                                {
-                                    if let Some(visual_index) =
-                                        proc_widget_state.columns.column_state.selected()
+                                BottomWidgetType::ProcSort => {
+                                    if let Some(proc_widget_state) = self
+                                        .proc_state
+                                        .get_widget_state(self.current_widget.widget_id - 2)
                                     {
-                                        self.increment_process_sort_position(
-                                            offset_clicked_entry as i64 - visual_index as i64,
-                                        );
+                                        if let Some(visual_index) =
+                                            proc_widget_state.columns.column_state.selected()
+                                        {
+                                            self.increment_process_sort_position(
+                                                offset_clicked_entry as i64 - visual_index as i64,
+                                            );
+                                        }
                                     }
                                 }
-                            }
-                            BottomWidgetType::CpuLegend => {
-                                if let Some(cpu_widget_state) = self
-                                    .cpu_state
-                                    .get_widget_state(self.current_widget.widget_id - 1)
-                                {
-                                    if let Some(visual_index) =
-                                        cpu_widget_state.scroll_state.table_state.selected()
+                                BottomWidgetType::CpuLegend => {
+                                    if let Some(cpu_widget_state) = self
+                                        .cpu_state
+                                        .get_widget_state(self.current_widget.widget_id - 1)
                                     {
-                                        self.increment_cpu_legend_position(
-                                            offset_clicked_entry as i64 - visual_index as i64,
-                                        );
+                                        if let Some(visual_index) =
+                                            cpu_widget_state.scroll_state.table_state.selected()
+                                        {
+                                            self.increment_cpu_legend_position(
+                                                offset_clicked_entry as i64 - visual_index as i64,
+                                            );
+                                        }
                                     }
                                 }
-                            }
-                            BottomWidgetType::Temp => {
-                                if let Some(temp_widget_state) = self
-                                    .temp_state
-                                    .get_widget_state(self.current_widget.widget_id)
-                                {
-                                    if let Some(visual_index) =
-                                        temp_widget_state.scroll_state.table_state.selected()
+                                BottomWidgetType::Temp => {
+                                    if let Some(temp_widget_state) = self
+                                        .temp_state
+                                        .get_widget_state(self.current_widget.widget_id)
                                     {
-                                        self.increment_temp_position(
-                                            offset_clicked_entry as i64 - visual_index as i64,
-                                        );
+                                        if let Some(visual_index) =
+                                            temp_widget_state.scroll_state.table_state.selected()
+                                        {
+                                            self.increment_temp_position(
+                                                offset_clicked_entry as i64 - visual_index as i64,
+                                            );
+                                        }
                                     }
                                 }
-                            }
-                            BottomWidgetType::Disk => {
-                                if let Some(disk_widget_state) = self
-                                    .disk_state
-                                    .get_widget_state(self.current_widget.widget_id)
-                                {
-                                    if let Some(visual_index) =
-                                        disk_widget_state.scroll_state.table_state.selected()
+                                BottomWidgetType::Disk => {
+                                    if let Some(disk_widget_state) = self
+                                        .disk_state
+                                        .get_widget_state(self.current_widget.widget_id)
                                     {
-                                        self.increment_disk_position(
-                                            offset_clicked_entry as i64 - visual_index as i64,
-                                        );
+                                        if let Some(visual_index) =
+                                            disk_widget_state.scroll_state.table_state.selected()
+                                        {
+                                            self.increment_disk_position(
+                                                offset_clicked_entry as i64 - visual_index as i64,
+                                            );
+                                        }
                                     }
                                 }
+                                _ => {}
                             }
-                            _ => {}
-                        }
-                    } else {
-                        // We might have clicked on a header!  Check if we only exceeded the table + border offset, and
-                        // it's implied we exceeded the gap offset.
-                        if clicked_entry == border_offset {
-                            #[allow(clippy::single_match)]
-                            match &self.current_widget.widget_type {
-                                BottomWidgetType::Proc => {
-                                    if let Some(proc_widget_state) = self
-                                        .proc_state
-                                        .get_mut_widget_state(self.current_widget.widget_id)
-                                    {
-                                        // Let's now check if it's a column header.
-                                        if let (Some(y_loc), Some(x_locs)) = (
-                                            &proc_widget_state.columns.column_header_y_loc,
-                                            &proc_widget_state.columns.column_header_x_locs,
-                                        ) {
-                                            // debug!("x, y: {}, {}", x, y);
-                                            // debug!("y_loc: {}", y_loc);
-                                            // debug!("x_locs: {:?}", x_locs);
-
-                                            if y == *y_loc {
-                                                for (itx, (x_left, x_right)) in
-                                                    x_locs.iter().enumerate()
-                                                {
-                                                    if x >= *x_left && x <= *x_right {
-                                                        // Found our column!
-                                                        proc_widget_state
+                        } else {
+                            // We might have clicked on a header!  Check if we only exceeded the table + border offset, and
+                            // it's implied we exceeded the gap offset.
+                            if clicked_entry == border_offset {
+                                #[allow(clippy::single_match)]
+                                match &self.current_widget.widget_type {
+                                    BottomWidgetType::Proc => {
+                                        if let Some(proc_widget_state) = self
+                                            .proc_state
+                                            .get_mut_widget_state(self.current_widget.widget_id)
+                                        {
+                                            // Let's now check if it's a column header.
+                                            if let (Some(y_loc), Some(x_locs)) = (
+                                                &proc_widget_state.columns.column_header_y_loc,
+                                                &proc_widget_state.columns.column_header_x_locs,
+                                            ) {
+                                                // debug!("x, y: {}, {}", x, y);
+                                                // debug!("y_loc: {}", y_loc);
+                                                // debug!("x_locs: {:?}", x_locs);
+
+                                                if y == *y_loc {
+                                                    for (itx, (x_left, x_right)) in
+                                                        x_locs.iter().enumerate()
+                                                    {
+                                                        if x >= *x_left && x <= *x_right {
+                                                            // Found our column!
+                                                            proc_widget_state
                                                             .columns
                                                             .set_to_sorted_index_from_visual_index(
                                                                 itx,
                                                             );
-                                                        proc_widget_state
-                                                            .update_sorting_with_columns();
-                                                        self.proc_state.force_update =
-                                                            Some(self.current_widget.widget_id);
-                                                        break;
+                                                            proc_widget_state
+                                                                .update_sorting_with_columns();
+                                                            self.proc_state.force_update =
+                                                                Some(self.current_widget.widget_id);
+                                                            break;
+                                                        }
                                                     }
                                                 }
                                             }
                                         }
                                     }
+                                    _ => {}
                                 }
-                                _ => {}
                             }
                         }
                     }
-                }
-                BottomWidgetType::Battery => {
-                    if let Some(battery_widget_state) = self
-                        .battery_state
-                        .get_mut_widget_state(self.current_widget.widget_id)
-                    {
-                        if let Some(tab_spacing) = &battery_widget_state.tab_click_locs {
-                            for (itx, ((tlc_x, tlc_y), (brc_x, brc_y))) in
-                                tab_spacing.iter().enumerate()
-                            {
-                                if (x >= *tlc_x && y >= *tlc_y) && (x <= *brc_x && y <= *brc_y) {
-                                    battery_widget_state.currently_selected_battery_index = itx;
-                                    break;
+                    BottomWidgetType::Battery => {
+                        if let Some(battery_widget_state) = self
+                            .battery_state
+                            .get_mut_widget_state(self.current_widget.widget_id)
+                        {
+                            if let Some(tab_spacing) = &battery_widget_state.tab_click_locs {
+                                for (itx, ((tlc_x, tlc_y), (brc_x, brc_y))) in
+                                    tab_spacing.iter().enumerate()
+                                {
+                                    if (x >= *tlc_x && y >= *tlc_y) && (x < *brc_x && y < *brc_y) {
+                                        battery_widget_state.currently_selected_battery_index = itx;
+                                        break;
+                                    }
                                 }
                             }
                         }
                     }
+                    _ => {}
                 }
-                _ => {}
             }
         }
     }
diff --git a/src/app/states.rs b/src/app/states.rs
index c8af1737..c596f038 100644
--- a/src/app/states.rs
+++ b/src/app/states.rs
@@ -61,7 +61,7 @@ impl Default for KillSignal {
 pub struct AppDeleteDialogState {
     pub is_showing_dd: bool,
     pub selected_signal: KillSignal,
-    // tl x, tl y, br x, br y
+    /// tl x, tl y, br x, br y, index/signal
     pub button_positions: Vec<(u16, u16, u16, u16, usize)>,
     pub keyboard_signal_select: usize,
     pub last_number_press: Option<Instant>,
diff --git a/src/canvas/dialogs/dd_dialog.rs b/src/canvas/dialogs/dd_dialog.rs
index 0415aa2d..ac9c6910 100644
--- a/src/canvas/dialogs/dd_dialog.rs
+++ b/src/canvas/dialogs/dd_dialog.rs
@@ -108,20 +108,31 @@ impl KillDialog for Painter {
             );
 
             if app_state.should_get_widget_bounds() {
+                // This is kinda weird, but the gist is:
+                // - We have three sections; we put our mouse bounding box for the "yes" button at the very right edge
+                //   of the left section and 3 characters back.  We then give it a buffer size of 1 on the x-coordinate.
+                // - Same for the "no" button, except it is the right section and we do it from the start of the right
+                //   section.
+                //
+                // Lastly, note that mouse detection for the dd buttons assume correct widths.  As such, we correct
+                // them here and check with >= and <= mouse bound checks, as opposed to how we do it elsewhere with
+                // >= and <.  See https://github.com/ClementTsang/bottom/pull/459 for details.
                 app_state.delete_dialog_state.button_positions = vec![
+                    // Yes
                     (
-                        button_layout[2].x,
-                        button_layout[2].y,
-                        button_layout[2].x + button_layout[2].width,
-                        button_layout[2].y + button_layout[2].height,
-                        0,
-                    ),
-                    (
-                        button_layout[0].x,
+                        button_layout[0].x + button_layout[0].width - 4,
                         button_layout[0].y,
                         button_layout[0].x + button_layout[0].width,
-                        button_layout[0].y + button_layout[0].height,
-                        1,
+                        button_layout[0].y,
+                        if cfg!(target_os = "windows") { 1 } else { 15 },
+                    ),
+                    // No
+                    (
+                        button_layout[2].x - 1,
+                        button_layout[2].y,
+                        button_layout[2].x + 2,
+                        button_layout[2].y,
+                        0,
                     ),
                 ];
             }
diff --git a/src/canvas/widgets/basic_table_arrows.rs b/src/canvas/widgets/basic_table_arrows.rs
index e7f769a8..6ab51124 100644
--- a/src/canvas/widgets/basic_table_arrows.rs
+++ b/src/canvas/widgets/basic_table_arrows.rs
@@ -136,18 +136,28 @@ impl BasicTableArrows for Painter {
             );
 
             if app_state.should_get_widget_bounds() {
+                // Some explanations for future readers:
+                // - The "height" as of writing of this entire widget is 2.  If it's 1, it occasionally doesn't draw.
+                // - As such, the buttons are only on the lower part of this 2-high widget.
+                // - So, we want to only check at one location, the `draw_loc.y + 1`, and that's it.
+                // - But why is it "+2" then?  Well, it's because I have a REALLY ugly hack
+                //   for mouse button checking, since most button checks are of the form `(draw_loc.y + draw_loc.height)`,
+                //   and the same for the x and width.  Unfortunately, if you check using >= and <=, the outer bound is
+                //   actually too large - so, we assume all of them are one too big and check via < (see
+                //   https://github.com/ClementTsang/bottom/pull/459 for details).
+                // - So in other words, to make it simple, we keep this to a standard and overshoot by one here.
                 if let Some(basic_table) = &mut app_state.basic_table_widget_state {
                     basic_table.left_tlc =
-                        Some((margined_draw_loc[0].x - 1, margined_draw_loc[0].y + 1));
+                        Some((margined_draw_loc[0].x, margined_draw_loc[0].y + 1));
                     basic_table.left_brc = Some((
-                        margined_draw_loc[0].x + margined_draw_loc[0].width - 1,
-                        margined_draw_loc[0].y + 1,
+                        margined_draw_loc[0].x + margined_draw_loc[0].width,
+                        margined_draw_loc[0].y + 2,
                     ));
                     basic_table.right_tlc =
-                        Some((margined_draw_loc[2].x - 1, margined_draw_loc[2].y + 1));
+                        Some((margined_draw_loc[2].x, margined_draw_loc[2].y + 1));
                     basic_table.right_brc = Some((
-                        margined_draw_loc[2].x + margined_draw_loc[2].width - 1,
-                        margined_draw_loc[2].y + 1,
+                        margined_draw_loc[2].x + margined_draw_loc[2].width,
+                        margined_draw_loc[2].y + 2,
                     ));
                 }
             }
diff --git a/src/canvas/widgets/cpu_basic.rs b/src/canvas/widgets/cpu_basic.rs
index e88bbf03..5beb3f73 100644
--- a/src/canvas/widgets/cpu_basic.rs
+++ b/src/canvas/widgets/cpu_basic.rs
@@ -198,10 +198,8 @@ impl CpuBasicWidget for Painter {
             // Update draw loc in widget map
             if let Some(widget) = app_state.widget_map.get_mut(&widget_id) {
                 widget.top_left_corner = Some((draw_loc.x, draw_loc.y));
-                widget.bottom_right_corner = Some((
-                    draw_loc.x + draw_loc.width - 1,
-                    draw_loc.y + draw_loc.height - 1,
-                ));
+                widget.bottom_right_corner =
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
             }
         }
     }
diff --git a/src/canvas/widgets/mem_basic.rs b/src/canvas/widgets/mem_basic.rs
index b252c5d3..3bb40ad7 100644
--- a/src/canvas/widgets/mem_basic.rs
+++ b/src/canvas/widgets/mem_basic.rs
@@ -122,10 +122,8 @@ impl MemBasicWidget for Painter {
         if app_state.should_get_widget_bounds() {
             if let Some(widget) = app_state.widget_map.get_mut(&widget_id) {
                 widget.top_left_corner = Some((draw_loc.x, draw_loc.y));
-                widget.bottom_right_corner = Some((
-                    draw_loc.x + draw_loc.width - 1,
-                    draw_loc.y + draw_loc.height - 1,
-                ));
+                widget.bottom_right_corner =
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
             }
         }
     }
diff --git a/src/canvas/widgets/network_basic.rs b/src/canvas/widgets/network_basic.rs
index 08aa1b6b..7f0e393e 100644
--- a/src/canvas/widgets/network_basic.rs
+++ b/src/canvas/widgets/network_basic.rs
@@ -70,10 +70,8 @@ impl NetworkBasicWidget for Painter {
         if app_state.should_get_widget_bounds() {
             if let Some(widget) = app_state.widget_map.get_mut(&widget_id) {
                 widget.top_left_corner = Some((draw_loc.x, draw_loc.y));
-                widget.bottom_right_corner = Some((
-                    draw_loc.x + draw_loc.width - 1,
-                    draw_loc.y + draw_loc.height - 1,
-                ));
+                widget.bottom_right_corner =
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
             }
         }
     }