bug: Fix mouse hitboxes (#459)

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

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

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,7 +2941,14 @@ impl App {
         }
 
         // Now handle click propagation down to widget.
-        if let Some((_tlc_x, tlc_y)) = &self.current_widget.top_left_corner {
+        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
@@ -2949,7 +2958,6 @@ impl App {
                         // 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 {
@@ -2971,8 +2979,9 @@ impl App {
                                             // the same entry as the already selected one - if it is,
                                             // then we minimize.
 
-                                        let previous_scroll_position =
+                                            let previous_scroll_position = proc_widget_state
-                                            proc_widget_state.scroll_state.current_scroll_position;
+                                                .scroll_state
+                                                .current_scroll_position;
                                             let is_tree_mode = proc_widget_state.is_tree_mode;
 
                                             let new_position = self.increment_process_position(
@@ -3103,7 +3112,7 @@ impl App {
                                 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) {
+                                    if (x >= *tlc_x && y >= *tlc_y) && (x < *brc_x && y < *brc_y) {
                                         battery_widget_state.currently_selected_battery_index = itx;
                                         break;
                                     }
@@ -3115,6 +3124,7 @@ impl App {
                 }
             }
         }
+    }
 
     fn is_drawing_border(&self) -> bool {
         self.is_expanded || !self.app_config_fields.use_basic_mode

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>,

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[0].x + button_layout[0].width - 4,
-                        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].y,
                         button_layout[0].x + button_layout[0].width,
-                        button_layout[0].y + button_layout[0].height,
+                        button_layout[0].y,
-                        1,
+                        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,
                     ),
                 ];
             }

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].x + margined_draw_loc[0].width,
-                        margined_draw_loc[0].y + 1,
+                        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].x + margined_draw_loc[2].width,
-                        margined_draw_loc[2].y + 1,
+                        margined_draw_loc[2].y + 2,
                     ));
                 }
             }

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((
+                widget.bottom_right_corner =
-                    draw_loc.x + draw_loc.width - 1,
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
-                    draw_loc.y + draw_loc.height - 1,
-                ));
             }
         }
     }

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((
+                widget.bottom_right_corner =
-                    draw_loc.x + draw_loc.width - 1,
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
-                    draw_loc.y + draw_loc.height - 1,
-                ));
             }
         }
     }

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((
+                widget.bottom_right_corner =
-                    draw_loc.x + draw_loc.width - 1,
+                    Some((draw_loc.x + draw_loc.width, draw_loc.y + draw_loc.height));
-                    draw_loc.y + draw_loc.height - 1,
-                ));
             }
         }
     }