Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement table row selection and hover highlighting #3347

Merged
merged 16 commits into from
Jan 6, 2024

Conversation

laurooyen
Copy link
Contributor

@laurooyen laurooyen commented Sep 16, 2023

Additions and Changes

  • Add TableBuilder::sense() and StripBuilder::sense() to enable detecting clicks or drags on table and strip cells.
  • Add TableRow::select() which takes a boolean that sets the highlight state for all cells added after a call to it. This allows highlighting an entire row or specific cells.
  • Add TableRow::response() which returns the union of the Response of all cells added to the row up to that point. This makes it easy to detect interactions with an entire row. See below for an alternative design.
  • Add TableRow::index() and TableRow::col_index() helpers.
  • Remove explicit row_index from callback passed to TableBody::rows() and TableBody::heterogeneous_rows(), possible due to the above. This is a breaking change but makes the callback compatible with TableBody::row().
  • Update Table example to demonstrate all of the above.

Design Decisions

An alternative design to TableRow::response() would be to return the row response from TableBodys row(), rows() and heterogeneous_rows() functions. row() could just return the response. rows() and heterogeneous_rows() could return a tuple of the hovered row index and that rows response. I feel like this might be the cleaner soluction if only returning the hovered rows response isn't too limiting.

I didn't implement TableBuilder::select_rows() as described here because it requires an immutable borrow of the selection state for the lifetime of the TableBuilder. This makes updating the selection state from within the body unnecessarily complicated. Additionally the current design allows for selecting specific cells, though that could be possible by modifying TableBuilder::select_rows() to provide row and column indices like below.

pub fn select_cells(is_selected: impl Fn(usize, usize) -> bool) -> Self

Hover Highlighting

EDIT: Thanks to @samitbasu we now have hover highlighting too.

This is not implemented yet. Ideally we'd have an api that allows to choose between highlighting the hovered cell, column or row. Should cells containing interactive widgets, be highlighted when hovering over the widget or only when hovering over the cell itself? I'd like to implement that before this gets merged though.

Feedback is more than welcome. I'd be happy to make any changes necessary to get this merged.

@jazzay
Copy link

jazzay commented Oct 9, 2023

@emilk I am wondering if we could have this merged sooner than later without waiting for the hover. There have been multiple attempts at table row selection over the past year or so, and I am also now looking at such a capability for my own app. Rather than me implementing yet another solution, I could use this.

I get the sense that hover is trickier to sort out, so perhaps that can come in later. What blocks application development is lack of row selection interactions. Hover while nice, is not critical in my opinion.

In the meantime @laurooyen would you be able to update your branch to include the latest 0.23 release?

@laurooyen
Copy link
Contributor Author

I'll update my branch in the comming days.

I agree that actually being able to select rows is more important than the hovering effect. If this gets merged without it, I'd be willing to do a follow up pull-request for it. In fact I had it working previously, the difficult part is deciding on the api, or none at all and enabling it automatically when sense is interactive.

@jazzay
Copy link

jazzay commented Oct 9, 2023

Thanks @laurooyen. No rush, I have since merged your changes across to my own branch w 0.23 as base.

@samitbasu
Copy link
Contributor

Hi @laurooyen , I added hover and highlighting support to your branch. Here's a quick vid.
Screen Recording 2023-12-01 at 11.55.59 AM.webm

I'm not sure if you like the way I did it, so I'll open a PR against your repo in case you want to change something.

@laurooyen
Copy link
Contributor Author

One final problem I noticed is that a row is not hover highlighted when hovering an interactive widget within that row. This is visible in the video above when the mouse hovers the Click me checkbox.

The interactive widgets response intercepts the hover from the rows response. A solution could be to check if the row is hovered by checking if it contains the cursor position. However that causes rows to be hover highlighted when another window is on top of it as well. Not sure what the solution would be.

@samitbasu
Copy link
Contributor

@laurooyen - I didn't notice that before. However, now that I see it, it does make sense that it behaves the way it does (to me anyway). When you hover on the clickable widget, the row is not highlighted, but the row is also not selected when you click the widget. So if highlighting the row means the row will become selected when clicked, it behaves intuitively.

@laurooyen
Copy link
Contributor Author

@samitbasu - I see where you're comming from, however I do believe the behaviour I described is more common. For example the outliner in both Unreal Engine and Blender behave this way. Since this isn't a deal breaker though I'll leave it as is and mark this ready for review.

@laurooyen laurooyen marked this pull request as ready for review December 2, 2023 20:20
@laurooyen laurooyen changed the title Implement table row selection Implement table row selection and hover highlighting Dec 2, 2023
@samitbasu
Copy link
Contributor

@laurooyen - do you want to add @vvv and myself as reviewers? We can review the PR. Also, should we consider just breaking the table widget out into it's own crate? Or is it better to leave it here?

@jazzay
Copy link

jazzay commented Jan 5, 2024

Would be great to see this merged in soon. Thanks for all the efforts @laurooyen @samitbasu !
My opinion is it should be fine to leave the table in the already separate egui_extras crate. Table is a key widget for many apps. But up to @emilk.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! ⭐⭐⭐

I just have a few nits that I'll push fixes for

crates/egui_extras/src/table.rs Outdated Show resolved Hide resolved
crates/egui_extras/src/table.rs Outdated Show resolved Hide resolved
crates/egui_extras/src/table.rs Outdated Show resolved Hide resolved
crates/egui_extras/src/table.rs Outdated Show resolved Hide resolved
crates/egui_extras/src/table.rs Outdated Show resolved Hide resolved
crates/egui_extras/src/layout.rs Outdated Show resolved Hide resolved
@emilk emilk added feature New feature or request egui_plot Related to egui_plot labels Jan 6, 2024
@emilk emilk merged commit 5a6d1cb into emilk:master Jan 6, 2024
19 checks passed
@laurooyen laurooyen deleted the table-row-selection branch January 6, 2024 19:25
@emilk emilk added egui_extras and removed egui_plot Related to egui_plot labels Jan 7, 2024
@enomado
Copy link
Contributor

enomado commented Mar 17, 2024

So, how exactly I can configure row hovering effect, leaving ability to interact with row contents?

@enomado
Copy link
Contributor

enomado commented Mar 17, 2024

As for me, there must be only API for row styling and pointer accessing in table.

Event processing, list of selected rows should be in user level.

Row selection could be implemented just by checkbox at first glance...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui_extras feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sense param for tables
5 participants