-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
@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? |
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. |
Thanks @laurooyen. No rush, I have since merged your changes across to my own branch w 0.23 as base. |
Hi @laurooyen , I added hover and highlighting support to your branch. Here's a quick vid. 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. |
Highlight/hover support.
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. |
@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. |
@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 - 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? |
Would be great to see this merged in soon. Thanks for all the efforts @laurooyen @samitbasu ! |
There was a problem hiding this 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
So, how exactly I can configure row hovering effect, leaving ability to interact with row contents? |
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... |
Additions and Changes
TableBuilder::sense()
andStripBuilder::sense()
to enable detecting clicks or drags on table and strip cells.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.TableRow::response()
which returns the union of theResponse
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.TableRow::index()
andTableRow::col_index()
helpers.row_index
from callback passed toTableBody::rows()
andTableBody::heterogeneous_rows()
, possible due to the above. This is a breaking change but makes the callback compatible withTableBody::row()
.Design Decisions
An alternative design to
TableRow::response()
would be to return the row response fromTableBody
srow()
,rows()
andheterogeneous_rows()
functions.row()
could just return the response.rows()
andheterogeneous_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 theTableBuilder
. 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 modifyingTableBuilder::select_rows()
to provide row and column indices like below.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.