-
Notifications
You must be signed in to change notification settings - Fork 788
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
Select a range of rows in a DataTable #3821
base: main
Are you sure you want to change the base?
Conversation
ff8a4e0
to
c1e3b72
Compare
(FYI 3965 has been merged.) |
* Use self.cursor_type instead of passing it in * Let _should_highlight get cursor/hover coordinate from self * Pass Coordinate and unbox when needed * Calculate cursor and hover when needed Simplfies changes from: * cf6b069 * dfba992 Ahead of work for: Textualize#3606
Only a range (not a set of individual rows) can be selected. Textualize#3606
d97cff1
to
048b628
Compare
Thanks @rodrigogiraoserrao, your fix also fixed it for this 🙏🏻 |
should_highlight = self._should_highlight | ||
cursor_type = self.cursor_type | ||
cursor_location = self.cursor_coordinate | ||
hover_location = self.hover_coordinate |
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.
I'm going to be straight with you: this PR will need to be reviewed by Darren and/or Will but changes like this decrease the probability that it gets merged.
I see you deleted these assignments from here and then changed things like _render_cell
to check the hover or cursor type there.
This is a small optimisation to reduce the number of attribute lookups: here, we're checking the cursor type once.
If you do this inside _render_cell
, you'll access that attribute once for each new row.
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.
Okay thanks for the heads up.
I isolated this refactor to 4354760, so it should be reasonably easy to test performance before/after. I'm curious how much of a different it will make.
To be sure: I think I'm doing the same number of comparisons, it's just e.g. self.cursor_type == "row"
instead of type_of_cursor == "row"
.
Implements: