Open kdheepak opened 2 months ago
Edit: first two points are resolved in https://github.com/ratatui-org/ratatui/pull/1104/commits/31de3586f75ab3c6b643e26f6fce8025f189243b
To keep this proof of concept PR small, I didn't address the following features:
[x] ~What should happen visually when selected row overlaps with a marked row? In taskwarrior-tui
, I allow the user to choose. That requires an additional separate field mark_and_highlight_symbol
that contains the text that will be displayed when the user is selecting a marked row.~
[x] ~Should we allow users to set a symbol for unmarked rows? In taskwarrior-tui
, I allow the user to choose a unmark_symbol
as well. This allows users to make rows with ballot boxes as the mark and unmark symbols.~
☐ Unmarked Row 1
☑ Marked Row 1
☑ Marked Row 2
☐ Unmarked Row 1
[ ] Currently in this PR, if a user wants to toggle between showing all marked rows or not, they'll have to store the state of the marked rows, drain the elements, and then later set them back again at a later stage. In taskwarrior-tui
I have TableMode::SingleSelection
and TableMode::MultipleSelection
that just changes the UI without changing the state.
TableMode::SingleSelection
andTableMode::MultipleSelection
Perhaps move Selection
to the name of the enum? (I wouldn't be surprised if there is a pedantic lint for that).
I suspect this should be an enum named ratatui::widgets::table::Selection
or something similar.
I'm tempted to leave the TableSelectionMode
out of this PR for a few reasons. I think it'll be possible to add it in a future PR if the need arises.
Selection is something done normally with the cursor. Which row is selected is moved up / down by keyboard / mouse. Maybe split the naming more clearly, mark and cursor? At the position of the cursor a mark can be added/removed?
Maybe I am mainly tipped of by the PR title, haven't really looked into the implementation yet.
Generally most widgets / controls in other frameworks will have a flag that is set to enable multi select. This is not generally enabled by default. It feels a bit odd to me for that to be different here. Does that sway you?
I am also fine with cursor and selection. Currently, the selection is the cursor. Mixing these feels weird to me.
Generally most widgets / controls in other frameworks will have a flag that is set to enable multi select.
They are likely mouse based?
Maybe the name of the PR should be "multi-mark" instead? We use the terminology "selection" to mean current cursor position. In this PR, you can "mark" the current cursor selection and the state will remember the index
, and you can repeatedly "mark" multiple rows one after another.
e.g. in taskwarrior-tui
I use v
to toggle the mark on a current row. Here's an example where I have marked multiple rows:
Attention: Patch coverage is 97.36842%
with 10 lines
in your changes are missing coverage. Please review.
Project coverage is 94.4%. Comparing base (
9bd89c2
) to head (ccf9d92
).
Files | Patch % | Lines |
---|---|---|
src/widgets/table/table_state.rs | 54.5% | 10 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think (could be worng) Selection and Highlight are the two concept names I'd use for what you're calling Mark and Selection. I'm not 100% sure of this though.
We unfortunately already have TableState::select
, TableState::selected
and TableState::with_selected
, so it'd be a breaking change to do it that way.
But you are right, we use highlight_symbol
, highlight_spacing
and HighlightSpacing
in Table
.
I'm open to suggestions on how to proceed.
We unfortunately already have
TableState::select
,TableState::selected
andTableState::with_selected
, so it'd be a breaking change to do it that way.
Yeah, I think this is a difficult one to get right - I wonder if the table would be worth moving out of Ratatui to get some higher velocity / experimental breaking changes like this and some of the other things (multi-select, built-in scrolling, column / cell selection, borders, property name changes etc.)?
I see the appeal but I'm not sure I want to go down that route. I'd guess an external package containing a Table
wouldn't see much use unless we removed Table
from ratatui
.
I see the appeal but I'm not sure I want to go down that route. I'd guess an external package containing a
Table
wouldn't see much use unless we removedTable
fromratatui
.
We can feature flag the new table in or something like that. I guess it would only really cause problems for situations where another library uses the table widget as part of its rendering. That seems pretty niche however (perhaps tui-logger or similar crates might be affected).
Either way, given that the table has many things that it doesn't do, I'd really suggest that if we want to make it really nice getting it out of Ratatui and into a separate crate is the right thing to do. This would allow faster iteration, more frequent breaking changes / unstable ideas, and doing the right thing with respect to the existing downsides of the table without having to worry about existing users (e.g. rendering tables with large numbers of rows with reasonable perf)
This PR allows users to display a mark next to multiple rows. The marked rows are stored in the table.
e.g. here's a table with 2 rows marked and the first row selected (i.e. where the cursor currently is located).
If we move the cursor down 1, the marked row and the currently selected row overlap:
The index for all marked rows can be retrieved from
TableState::marked()
. This PR adds other methods to theTableState
struct to manipulate the marks.