Skip to content

Commit

Permalink
bug: fix bug causing click bounds to fail
Browse files Browse the repository at this point in the history
There were three bugs:

1. The click bounds calculation was incorrect. I did the silly mistake
   of checking for <= bounds for the bottom and right sections of a
   Rect when checking if the mouse intersected - this is WRONG.

   For example, let's say you want to calculate if an x value of 5 falls
   between something that starts at 0 and is 5 long.  It shouldn't,
   right?  Because it draws from 0 to 4?  But if you just did <=
   Rect.right(), you would get a hit - because it just does (start +
   width), so you get 5, and 5 <= 5!

   So, easy fix, change all far bounds checks to <.

2. The second bug is a mistake where I accidentally did not include
   bounds sets for my memory and net widgets. Instead, they set their
   bounds to the underlying graph representation, which is WRONG, since
   that bound gets updated on draw, and gets set to a slightly smaller
   rect due to borders!

3. A slightly sneakier one. This broke my bounds checks for the CPU
   widget - and it would have broken my process widget too.

   The problem lies in the concept of widgets that handle multiple
   "sub"-blocks internally, and how I was doing click detection
   internally - I would check if the bounds of the internal Components
   were hit.  Say, the CPU, I would check if the internal graph was hit,
   then if the internal table was hit.

   But wait! I said in point 2 that a graph gets its borders updated on
   draw to something slightly smaller, due to borders!  And there's the
   problem - it affected tables too.  I was setting the bounds of
   components to that of the *internal* representation - without borders
   - but my click detection *needed* borders included!

   Solution?  Add another trait function to check bordered bounds, and
   make the default implementation just check the existing bounds. For
   cases like internal Components that may need it, I add a separate
   implementation.

   I also switched over all border bounds checks for Widgets to that,
   since it's a bit more consistent.
  • Loading branch information
ClementTsang committed Aug 30, 2021
1 parent 48c572d commit 3fa5060
Show file tree
Hide file tree
Showing 18 changed files with 204 additions and 69 deletions.
6 changes: 3 additions & 3 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,11 +353,11 @@ impl AppState {
}
} else {
for (id, widget) in self.widget_lookup_map.iter_mut() {
if widget.does_intersect_mouse(&event) {
let is_id_selected = self.selected_widget == *id;
if widget.does_border_intersect_mouse(&event) {
let was_id_already_selected = self.selected_widget == *id;
self.selected_widget = *id;

if is_id_selected {
if was_id_already_selected {
let result = widget.handle_mouse_event(event);
return self.convert_widget_event_result(result);
} else {
Expand Down
3 changes: 3 additions & 0 deletions src/app/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const MAX_TIMEOUT: Duration = Duration::from_millis(400);

/// These are "signals" that are sent along with an [`WidgetEventResult`] to signify a potential additional action
/// that the caller must do, along with the "core" result of either drawing or redrawing.
#[derive(Debug)]
pub enum ReturnSignal {
/// A signal returned when some process widget was told to try to kill a process (or group of processes).
///
Expand All @@ -18,6 +19,7 @@ pub enum ReturnSignal {
}

/// The results of handling an event by the [`AppState`].
#[derive(Debug)]
pub enum EventResult {
/// Kill the program.
Quit,
Expand All @@ -29,6 +31,7 @@ pub enum EventResult {

/// The results of a widget handling some event, like a mouse or key event,
/// signifying what the program should then do next.
#[derive(Debug)]
pub enum WidgetEventResult {
/// Kill the program.
Quit,
Expand Down
5 changes: 4 additions & 1 deletion src/app/layout_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1001,13 +1001,16 @@ pub struct ColLayout {
}

/// A [`LayoutNode`] represents a single node in the overall widget hierarchy. Each node is one of:
/// - [`LayoutNode::Row`] (a a non-leaf that distributes its children horizontally)
/// - [`LayoutNode::Row`] (a non-leaf that distributes its children horizontally)
/// - [`LayoutNode::Col`] (a non-leaf node that distributes its children vertically)
/// - [`LayoutNode::Widget`] (a leaf node that contains the ID of the widget it is associated with)
#[derive(PartialEq, Eq)]
pub enum LayoutNode {
/// A non-leaf that distributes its children horizontally
Row(RowLayout),
/// A non-leaf node that distributes its children vertically
Col(ColLayout),
/// A leaf node that contains the ID of the widget it is associated with
Widget,
}

Expand Down
33 changes: 27 additions & 6 deletions src/app/widgets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use tui::{backend::Backend, layout::Rect, widgets::TableState, Frame};

use crate::{
app::{
event::{WidgetEventResult, SelectionAction},
event::{SelectionAction, WidgetEventResult},
layout_manager::BottomWidgetType,
},
canvas::Painter,
Expand Down Expand Up @@ -63,15 +63,36 @@ pub trait Component {
/// coordinates.
fn bounds(&self) -> Rect;

/// Updates a [`Component`]s bounding box to `new_bounds`.
/// Updates a [`Component`]'s bounding box to `new_bounds`.
fn set_bounds(&mut self, new_bounds: Rect);

/// Returns whether a [`MouseEvent`] intersects a [`Component`].
fn does_intersect_mouse(&self, event: &MouseEvent) -> bool {
/// Returns a [`Component`]'s bounding box, *including the border*. Defaults to just returning the normal bounds.
/// Note that these are defined in *global*, *absolute* coordinates.
fn border_bounds(&self) -> Rect {
self.bounds()
}

/// Updates a [`Component`]'s bounding box to `new_bounds`. Defaults to just setting the normal bounds.
fn set_border_bounds(&mut self, new_bounds: Rect) {
self.set_bounds(new_bounds);
}

/// Returns whether a [`MouseEvent`] intersects a [`Component`]'s bounds.
fn does_bounds_intersect_mouse(&self, event: &MouseEvent) -> bool {
let x = event.column;
let y = event.row;
let bounds = self.bounds();

x >= bounds.left() && x < bounds.right() && y >= bounds.top() && y < bounds.bottom()
}

/// Returns whether a [`MouseEvent`] intersects a [`Component`]'s bounds, including any borders, if there are.
fn does_border_intersect_mouse(&self, event: &MouseEvent) -> bool {
let x = event.column;
let y = event.row;
let rect = self.bounds();
x >= rect.left() && x <= rect.right() && y >= rect.top() && y <= rect.bottom()
let bounds = self.border_bounds();

x >= bounds.left() && x < bounds.right() && y >= bounds.top() && y < bounds.bottom()
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/app/widgets/base/scrollable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ impl Component for Scrollable {
fn handle_mouse_event(&mut self, event: MouseEvent) -> WidgetEventResult {
match event.kind {
MouseEventKind::Down(MouseButton::Left) => {
if self.does_intersect_mouse(&event) {
if self.does_bounds_intersect_mouse(&event) {
// This requires a bit of fancy calculation. The main trick is remembering that
// we are using a *visual* index here - not what is the actual index! Luckily, we keep track of that
// inside our linked copy of TableState!
Expand Down
66 changes: 55 additions & 11 deletions src/app/widgets/base/sort_text_table.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::borrow::Cow;

use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind};
use tui::{backend::Backend, layout::Rect, widgets::Block};
use tui::{backend::Backend, layout::Rect, widgets::Block, Frame};

use crate::{
app::{
Expand Down Expand Up @@ -60,6 +60,9 @@ pub enum SortStatus {

/// A trait for sortable columns.
pub trait SortableColumn {
/// Returns the original name of the column.
fn original_name(&self) -> &Cow<'static, str>;

/// Returns the shortcut for the column, if it exists.
fn shortcut(&self) -> &Option<(KeyEvent, String)>;

Expand All @@ -73,12 +76,18 @@ pub trait SortableColumn {
/// Sets the sorting status.
fn set_sorting_status(&mut self, sorting_status: SortStatus);

// ----- The following are required since SortableColumn implements TableColumn -----

/// Returns the displayed name on the column when drawing.
fn display_name(&self) -> Cow<'static, str>;

/// Returns the desired width of the column when drawing.
fn get_desired_width(&self) -> &DesiredColumnWidth;

/// Returns the x bounds of a column. The y is assumed to be 0, relative to the table..
fn get_x_bounds(&self) -> Option<(u16, u16)>;

/// Sets the x bounds of a column.
fn set_x_bounds(&mut self, x_bounds: Option<(u16, u16)>);
}

Expand Down Expand Up @@ -106,8 +115,11 @@ where
/// A [`SimpleSortableColumn`] represents some column in a [`SortableTextTable`].
#[derive(Debug)]
pub struct SimpleSortableColumn {
original_name: Cow<'static, str>,
pub shortcut: Option<(KeyEvent, String)>,
pub default_descending: bool,
x_bounds: Option<(u16, u16)>,

pub internal: SimpleColumn,

/// Whether this column is currently selected for sorting, and which direction.
Expand All @@ -117,12 +129,15 @@ pub struct SimpleSortableColumn {
impl SimpleSortableColumn {
/// Creates a new [`SimpleSortableColumn`].
fn new(
full_name: Cow<'static, str>, shortcut: Option<(KeyEvent, String)>,
default_descending: bool, desired_width: DesiredColumnWidth,
original_name: Cow<'static, str>, full_name: Cow<'static, str>,
shortcut: Option<(KeyEvent, String)>, default_descending: bool,
desired_width: DesiredColumnWidth,
) -> Self {
Self {
original_name,
shortcut,
default_descending,
x_bounds: None,
internal: SimpleColumn::new(full_name, desired_width),
sorting_status: SortStatus::NotSorting,
}
Expand All @@ -141,11 +156,12 @@ impl SimpleSortableColumn {
Some((shortcut, shortcut_name)),
)
} else {
(name, None)
(name.clone(), None)
};
let full_name_len = full_name.len();

SimpleSortableColumn::new(
name,
full_name,
shortcut,
default_descending,
Expand All @@ -165,11 +181,12 @@ impl SimpleSortableColumn {
Some((shortcut, shortcut_name)),
)
} else {
(name, None)
(name.clone(), None)
};
let full_name_len = full_name.len();

SimpleSortableColumn::new(
name,
full_name,
shortcut,
default_descending,
Expand All @@ -182,6 +199,10 @@ impl SimpleSortableColumn {
}

impl SortableColumn for SimpleSortableColumn {
fn original_name(&self) -> &Cow<'static, str> {
&self.original_name
}

fn shortcut(&self) -> &Option<(KeyEvent, String)> {
&self.shortcut
}
Expand Down Expand Up @@ -236,6 +257,9 @@ where

/// The underlying [`TextTable`].
pub table: TextTable<S>,

/// A corresponding "sort" menu.
pub sort_menu: TextTable,
}

impl<S> SortableTextTable<S>
Expand All @@ -244,9 +268,15 @@ where
{
/// Creates a new [`SortableTextTable`]. Note that `columns` cannot be empty.
pub fn new(columns: Vec<S>) -> Self {
let sort_menu_columns = columns
.iter()
.map(|column| SimpleColumn::new_hard(column.original_name().clone(), None))
.collect::<Vec<_>>();

let mut st = Self {
sort_index: 0,
table: TextTable::new(columns),
sort_menu: TextTable::new(sort_menu_columns),
};
st.set_sort_index(0);
st
Expand Down Expand Up @@ -317,15 +347,21 @@ where

/// Draws a [`tui::widgets::Table`] on screen.
///
/// Note if the number of columns don't match in the [`TextTable`] and data,
/// Note if the number of columns don't match in the [`SortableTextTable`] and data,
/// it will only create as many columns as it can grab data from both sources from.
pub fn draw_tui_table<B: Backend>(
&mut self, painter: &Painter, f: &mut tui::Frame<'_, B>, data: &TextTableData,
block: Block<'_>, block_area: Rect, show_selected_entry: bool,
&mut self, painter: &Painter, f: &mut Frame<'_, B>, data: &TextTableData, block: Block<'_>,
block_area: Rect, show_selected_entry: bool,
) {
self.table
.draw_tui_table(painter, f, data, block, block_area, show_selected_entry);
}

/// Draws a [`tui::widgets::Table`] on screen corresponding to the sort columns of this [`SortableTextTable`].
pub fn draw_sort_table<B: Backend>(
&mut self, painter: &Painter, f: &mut Frame<'_, B>, block: Block<'_>, block_area: Rect,
) {
}
}

impl<S> Component for SortableTextTable<S>
Expand All @@ -347,7 +383,7 @@ where

fn handle_mouse_event(&mut self, event: MouseEvent) -> WidgetEventResult {
if let MouseEventKind::Down(MouseButton::Left) = event.kind {
if !self.does_intersect_mouse(&event) {
if !self.does_bounds_intersect_mouse(&event) {
return WidgetEventResult::NoRedraw;
}

Expand All @@ -373,10 +409,18 @@ where
}

fn bounds(&self) -> Rect {
self.table.bounds
self.table.bounds()
}

fn set_bounds(&mut self, new_bounds: Rect) {
self.table.bounds = new_bounds;
self.table.set_bounds(new_bounds)
}

fn border_bounds(&self) -> Rect {
self.table.border_bounds()
}

fn set_border_bounds(&mut self, new_bounds: Rect) {
self.table.set_border_bounds(new_bounds)
}
}
9 changes: 9 additions & 0 deletions src/app/widgets/base/text_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub struct TextInput {
text: String,
cursor_index: usize,
bounds: Rect,
border_bounds: Rect,
}

impl TextInput {
Expand Down Expand Up @@ -92,6 +93,14 @@ impl Component for TextInput {
self.bounds = new_bounds;
}

fn border_bounds(&self) -> Rect {
self.border_bounds
}

fn set_border_bounds(&mut self, new_bounds: Rect) {
self.border_bounds = new_bounds;
}

fn handle_key_event(&mut self, event: KeyEvent) -> WidgetEventResult {
if event.modifiers.is_empty() {
match event.code {
Expand Down
16 changes: 14 additions & 2 deletions src/app/widgets/base/text_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ where
/// The bounding box of the [`TextTable`].
pub bounds: Rect, // TODO: Consider moving bounds to something else???

/// The bounds including the border, if there is one.
pub border_bounds: Rect,

/// Whether we draw columns from left-to-right.
pub left_to_right: bool,

Expand All @@ -149,6 +152,7 @@ where
cached_column_widths: CachedColumnWidths::Uncached,
show_gap: true,
bounds: Rect::default(),
border_bounds: Rect::default(),
left_to_right: true,
selectable: true,
}
Expand Down Expand Up @@ -342,7 +346,7 @@ where
}
}

/// Draws a [`Table`] on screen..
/// Draws a [`Table`] on screen corresponding to the [`TextTable`].
///
/// Note if the number of columns don't match in the [`TextTable`] and data,
/// it will only create as many columns as it can grab data from both sources from.
Expand All @@ -353,14 +357,14 @@ where
use tui::widgets::Row;

let inner_area = block.inner(block_area);

let table_gap = if !self.show_gap || inner_area.height < TABLE_GAP_HEIGHT_LIMIT {
0
} else {
1
};

self.set_num_items(data.len());
self.set_border_bounds(block_area);
self.set_bounds(inner_area);
let table_extras = 1 + table_gap;
let scrollable_height = inner_area.height.saturating_sub(table_extras);
Expand Down Expand Up @@ -466,4 +470,12 @@ where
fn set_bounds(&mut self, new_bounds: Rect) {
self.bounds = new_bounds;
}

fn border_bounds(&self) -> Rect {
self.border_bounds
}

fn set_border_bounds(&mut self, new_bounds: Rect) {
self.border_bounds = new_bounds;
}
}
Loading

0 comments on commit 3fa5060

Please sign in to comment.