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

Select a range of rows in a DataTable #3821

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

davetapley
Copy link
Contributor

@davetapley davetapley commented Dec 6, 2023

@davetapley davetapley changed the title Simplify DataTable._should_highlight Select a range of rows in a DataTable Dec 8, 2023
@davetapley davetapley marked this pull request as ready for review December 19, 2023 02:35
@davetapley davetapley marked this pull request as draft January 5, 2024 18:54
@rodrigogiraoserrao
Copy link
Contributor

(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
@davetapley davetapley marked this pull request as ready for review January 8, 2024 16:43
@davetapley
Copy link
Contributor Author

Thanks @rodrigogiraoserrao, your fix also fixed it for this 🙏🏻

@sstoops
Copy link

sstoops commented Jan 10, 2024

Awesome work! I'm eager to see some sort of native multi-selection abilities within the DataTable widget. The contiguous requirement of this PR is, unfortunately, a non-starter for my particular needs.

I have a working version that I built for an internal tool at my company, but I must warn you, I wrote this about a day after I started using Textual. So I already know it's poorly implemented in many ways, but it works for our use-case and it's been in use for about a month now.

I'd like to take a closer look at your implementation and think through my own to see how I could "meet in the middle".

class MultiSelectDataTable(DataTable):
    _row_selected = Text("[X]")
    _row_unselected = Text("[ ]")

    selected: var[list[RowKey]] = var([])

    BINDINGS = [
        ("c", "clear()", "Clear selection"),
        ("a", "select_all()", "Select all"),
    ]

    def action_clear(self) -> None:
        """Clear the list of selected rows."""
        self.selected = []

    def action_select_all(self) -> None:
        """Select all rows."""
        self.selected = list(self.rows.keys())

    def on_key(self, event: events.Key) -> None:
        row_key, _ = self.coordinate_to_cell_key(self.cursor_coordinate)
        if event.key == "enter":
            # We catch the "selection" of a row here instead of the RowSelected
            # event because the latter will be fired upon a single mouse click of a
            # row, which isn't what we want.
            event.stop()
            self.post_message(FilterTable.Submitted(self.selected if self.selected else [row_key]))
        elif event.key == "space":
            event.stop()
            self.toggle_row_selection(row_key)

    @on(DataTable.RowLabelSelected)
    def row_label_selected(self, event: DataTable.RowLabelSelected) -> None:
        """Toggle the selection of a row when the label is clicked."""
        self.toggle_row_selection(event.row_key)

    def toggle_row_selection(self, row_key: RowKey) -> None:
        """Toggle the selection of a row."""
        row = self.rows[row_key]
        if row.label == self._row_unselected:
            self.selected = self.selected + [row_key]
        else:
            # Rewrite the value to trigger reactivity
            self.selected = [x for x in self.selected if x != row_key]

    def watch_selected(self, value: list[RowKey]) -> None:
        """Change the selection label of the selected rows."""
        # Clear the render caches to get the updated label to render
        self._clear_caches()
        for row_key, row in self.rows.items():
            if row_key in value:
                row.label = self._row_selected
            else:
                row.label = self._row_unselected
        # Refresh the table to redraw the selection boxes
        self.refresh()
Screenshot 2024-01-09 at 4 45 17 PM

Comment on lines -1311 to -1314
should_highlight = self._should_highlight
cursor_type = self.cursor_type
cursor_location = self.cursor_coordinate
hover_location = self.hover_coordinate
Copy link
Contributor

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.

Copy link
Contributor Author

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".

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

Successfully merging this pull request may close these issues.

None yet

3 participants