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

feat: Add selection of multiple marks for table widget #1104

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

Conversation

kdheepak
Copy link
Collaborator

@kdheepak kdheepak commented May 13, 2024

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

• Cell Cell     // e.g. unmarked and selected row
⦾ Cell Cell    // e.g. marked row
  Cell Cell    // e.g. unmarked row
  Cell Cell    // e.g. unmarked row
⦾ Cell Cell    // e.g. marked row
  Cell Cell    // e.g. unmarked row    

If we move the cursor down 1, the marked row and the currently selected row overlap:

  Cell Cell     // e.g. unmarked row
⦿ Cell Cell    // e.g. marked and selected row
  Cell Cell    // e.g. unmarked row
  Cell Cell    // e.g. unmarked row
⦾ Cell Cell    // e.g. marked row
  Cell Cell    // e.g. unmarked row    

The index for all marked rows can be retrieved from TableState::marked(). This PR adds other methods to the TableState struct to manipulate the marks.

@kdheepak
Copy link
Collaborator Author

kdheepak commented May 13, 2024

Edit: first two points are resolved in 31de358

To keep this proof of concept PR small, I didn't address the following features:

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

@kdheepak kdheepak marked this pull request as draft May 13, 2024 05:39
@joshka
Copy link
Member

joshka commented May 13, 2024

  • TableMode::SingleSelection and TableMode::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.

@kdheepak
Copy link
Collaborator Author

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.

@kdheepak kdheepak marked this pull request as ready for review May 13, 2024 07:59
@EdJoPaTo
Copy link
Member

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.

@joshka
Copy link
Member

joshka commented May 13, 2024

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?

@EdJoPaTo
Copy link
Member

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?

@kdheepak
Copy link
Collaborator Author

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:

image

@kdheepak kdheepak changed the title feat: Add multi-selection for table feat: Add selection of multiple marks for table widget May 13, 2024
Copy link

codecov bot commented May 14, 2024

Codecov Report

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 ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1104    +/-   ##
======================================
  Coverage   94.3%   94.4%            
======================================
  Files         61      61            
  Lines      14768   15131   +363     
======================================
+ Hits       13932   14285   +353     
- Misses       836     846    +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshka
Copy link
Member

joshka commented May 14, 2024

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.

@kdheepak
Copy link
Collaborator Author

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.

@joshka
Copy link
Member

joshka commented May 14, 2024

We unfortunately already have TableState::select, TableState::selected and TableState::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.)?

@kdheepak
Copy link
Collaborator Author

kdheepak commented May 14, 2024

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.

src/widgets/table/table.rs Outdated Show resolved Hide resolved
Comment on lines +536 to +552
/// Set the symbol to be displayed in front of the unmarked row
///
/// This is a fluent setter method which must be chained or used as it consumes self
///
/// # Examples
///
/// ```rust
/// # use ratatui::{prelude::*, widgets::*};
/// # let rows = [Row::new(vec!["Cell1", "Cell2"])];
/// # let widths = [Constraint::Length(5), Constraint::Length(5)];
/// let table = Table::new(rows, widths).unmark_symbol(" ");
/// ```
#[must_use = "method moves the value of self and returns the modified value"]
pub fn unmark_symbol<T: Into<Text<'a>>>(mut self, unmark_symbol: T) -> Self {
self.unmark_symbol = unmark_symbol.into();
self
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a better name for this? nomark_symbol? blank_symbol?

@joshka
Copy link
Member

joshka commented May 19, 2024

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.

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)

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