feat: use CoW for List and Table contents #1900
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1900 +/- ##
=======================================
+ Coverage 91.5% 91.7% +0.1%
=======================================
Files 90 93 +3
Lines 17217 17632 +415
=======================================
+ Hits 15759 16174 +415
Misses 1458 1458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshka
left a comment
There was a problem hiding this comment.
For the macros, it could be worth putting these in the ratatui-macros project which has some stuff for table rows already. WDYT?
https://docs.rs/ratatui-macros/latest/ratatui_macros/#row
I think in general, this idea seems fine. The main question I'd have are about how this affects existing code. I'm assuming that it should continue to work fine, but there's lots of changes to constructor type code which I'm unsure whether it was due to necessity or preference. Leaving a few clear tests that show the existing call patterns being undisturbed would generally allay that concern.
ratatui-widgets/src/list.rs
Outdated
| /// use ratatui::Frame; | ||
| /// use ratatui::layout::Rect; | ||
| /// use ratatui::style::{Style, Stylize}; | ||
| /// use ratatui::widgets::{Block, List, items}; | ||
| /// | ||
| /// # fn ui(frame: &mut Frame) { | ||
| /// # let area = Rect::default(); | ||
| /// let items = items!["Item 1", "Item 2", "Item 3"]; | ||
| /// let list = List::from(&items) | ||
| /// .block(Block::bordered().title("List")) | ||
| /// .highlight_style(Style::new().reversed()) | ||
| /// .highlight_symbol(">>") | ||
| /// .repeat_highlight_symbol(true); | ||
| /// | ||
| /// frame.render_widget(list, area); | ||
| /// # } |
There was a problem hiding this comment.
This example could be a bit shorter, it doesn't really have to show how the items are used. Mainly it should show how the macro is used. So it should highlight any things that are interesting about how this facilitates Cow semantics (or not) and how it handles conversions to list item.
It's sort of similar to how the vec! macro works, should there be some syntax that accepts ; n?
There was a problem hiding this comment.
Since ListItem does not implement copy, the trivial implementation does not work
ratatui-widgets/src/table.rs
Outdated
| ([] as [$crate::table::Row<'_>; 0]) | ||
| ); | ||
| ($([$($x:expr),+ $(,)?]),+ $(,)?) => ( | ||
| // TODO: avoid to_vec() |
There was a problem hiding this comment.
to avoid the allocation in .to_vec()
6c15ad1 to
6d28745
Compare
|
Thanks for the review. |
Use a Copy-on-Write type for the contents of tables and lists so their content can be cached and only recomputed on actual changes. This avoids unnecessary computations for the rendering in case of unchanged data. Related to ratatui#1004.
There was a problem hiding this comment.
As Josh pointed out earlier, what do you think about moving this to ratatui-macros? I think there it would be more appropriate.
There was a problem hiding this comment.
When trying to extract the macro I get the following errors:
error[E0271]: type mismatch resolving `<[ListItem<'_>] as ToOwned>::Owned == Vec<ListItem<'_>>`
--> ratatui-widgets/src/list.rs:743:40
|
743 | let cow: Cow<[ListItem<'_>]> = Cow::Owned(items);
| ^^^^^^^^^^^^^^^^^ expected `ratatui::widgets::ListItem<'_>`, found `item::ListItem<'_>`
|
note: the crate `ratatui_widgets` is compiled multiple times, possibly with different configurations
--> ratatui-widgets/src/list/item.rs:73:1
|
73 | pub struct ListItem<'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^ this is the found type `item::ListItem`
|
::: /var/home/user/Coding/workspaces/ratatui/ratatui-widgets/src/list/item.rs:73:1
|
73 | pub struct ListItem<'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^ this is the expected type `ratatui::widgets::ListItem`
|
::: ratatui-widgets/src/list.rs:523:9
|
523 | use ratatui::macros::list_items;
| ------- one version of crate `ratatui_widgets` used here, as a dependency of crate `ratatui_macros`
= help: you can use `cargo tree` to explore your dependency tree
There was a problem hiding this comment.
That looks odd. Are you sure you are using ratatui-widgets from the workspace?
Use a Copy-on-Write type for the contents of tables and lists so their content can be cached and only recomputed on actual changes. This avoids unnecessary computations for the rendering in case of unchanged data.
Related to #1004.
Contains first commit of #1791, with
'lendand'datalifetimes combined into'a.