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(widget): add render_ref method to Widget trait #16

Closed
wants to merge 1 commit into from

Conversation

orhun
Copy link
Sponsor Member

@orhun orhun commented Feb 12, 2023

Upstream: #528

Description

This PR adds Terminal::render_widget_ref() and (Stateful)Widget::render_ref() functions that take Widgets by reference instead of taking ownership. This allows to call render on a Trait object with unknown size with is useful where the exact object implementing Widget is unknown or depends on some kind of condition. I decided not to change the render_widget() and render() signatures as to keep backwards compatibility but sadly all custom widgets are still broken because they are now required to implement render_ref() instead. This is just a proof of concept PR and I would appreciate any help on getting it up to the standards since this is my first time contributing to a big rust project. I made this to fix issue #526 I posted yesterday and I'm already using this PR in a project of mine.

Checklist

@orhun
Copy link
Sponsor Member Author

orhun commented Feb 12, 2023

harrypunk said: Super helpful feature. I'm looking for a way of rendering a dynamic widget.

@orhun
Copy link
Sponsor Member Author

orhun commented Feb 12, 2023

kroonhorstdino said: Would definitely like to see this feature merged! Would make my life so much easier, right now you have to work around so much limitations in that regard.

@mindoodoo mindoodoo added the enhancement New feature or request label Feb 14, 2023
@joshka
Copy link
Member

joshka commented Apr 12, 2023

I ran into a similar issue with the same root cause (rendering consumes the widget, which makes this call sometimes annoying - particularly as a rust newb). I'd be wary of this implementation however as it breaks all custom widgets, unless I'm mistaken?

@sophacles
Copy link
Collaborator

Widget is a trait. Why not have a RefWidget trait also? This would allow backwards compatability with the existing widgets, and allow for rendering by ref.

I think though, a bigger thing that's a bit confusing at first about the way ratatui is designed is that widgets aren't supposed to be a place to store state - (I'm not sure of the exact right wording here but heres a try) They are basically the builder pattern for all the arguments to render. Instead of having draw_paragraph(text: T, block: Option<Block>, style: Option<Style>, wrap: Option<Wrap>, scroll: Option<(u16,u16)>, alignment: Option<Alignment>), there's the paragraph widget that effectively does the same thing (that is the widget is created on the stack just like function args would be, and by having the render method consume the struct, the compiler/linker can do some nice optimization.

A similar thing is true for several of the small structs and enums that the the widgets consume for arguments (e.g. Block or Style or Wrap. When they are hardcoded (e.g Paragraph.wrap(Wrap{...}), the compiler can optimize that away.

When writing the code it can feel expensive... "why am i calling this all the time?" and similar questions come up. One of the places this gets a bit hairy is that Text, Spans, and some of the widgets (like List) all alloc Vec, and then consume the vec - this can be expensive, particularly if the text or list changes often. I wonder if it would make sense to look at reducing those allocs instead somehow - e.g. List can be generic on IntoIterator<Item=&ListItem>, and Text heavy widgets could take Rc<Text> or evenAsRef<Text>, and that way they don't have to consume the allocated Vecs and Strings each time.

joshka pushed a commit to joshka/ratatui that referenced this pull request Apr 29, 2023
@joshka joshka added the imported from tui Imported from tui-rs PRs / issues label May 5, 2023
@joshka joshka changed the title Add a render method for the Widget trait that takes by reference instead feat(widget): add render_ref method to Widget trait May 7, 2023
@joshka joshka marked this pull request as draft May 20, 2023 03:49
@joshka
Copy link
Member

joshka commented May 20, 2023

I'm going to suggest that this one be deferred until we resolve the answers to:

@orhun
Copy link
Sponsor Member Author

orhun commented Dec 13, 2023

This issue was closed due to it becoming stale. Feel free to reopen and work on this.

1 similar comment
@joshka
Copy link
Member

joshka commented Dec 13, 2023

This issue was closed due to it becoming stale. Feel free to reopen and work on this.

@joshka joshka closed this Dec 13, 2023
joshka added a commit that referenced this pull request Jan 20, 2024
Many widgets can be rendered without changing their state.

This commit implements The `Widget` trait for various references to
widgets and changes their implementations to be immutable.

This allows us to render widgets without consuming them by passing a ref
to the widget when calling `Frame::render_widget()`.

```rust
// this might be stored in a struct
let paragraph = Paragraph::new("Hello world!");

let [left, right] = area.split(&Layout::horizontal([20, 20]));
frame.render_widget(&paragraph, left);
frame.render_widget(&paragraph, right); // we can reuse the widget
```

- Clear
- Block
- Tabs
- Sparkline
- Paragraph
- Gauge
- Calendar

Other widgets will be implemented in follow up commits.

Fixes: #164
Replaces PRs: #122 and
  #16
Enables: #132
Validated as a viable working solution by: #836
joshka added a commit that referenced this pull request Jan 24, 2024
Many widgets can be rendered without changing their state.

This commit implements The `Widget` trait for various references to
widgets and changes their implementations to be immutable.

This allows us to render widgets without consuming them by passing a ref
to the widget when calling `Frame::render_widget()`.

```rust
// this might be stored in a struct
let paragraph = Paragraph::new("Hello world!");

let [left, right] = area.split(&Layout::horizontal([20, 20]));
frame.render_widget(&paragraph, left);
frame.render_widget(&paragraph, right); // we can reuse the widget
```

- Clear
- Block
- Tabs
- Sparkline
- Paragraph
- Gauge
- Calendar

Other widgets will be implemented in follow up commits.

Fixes: #164
Replaces PRs: #122 and
  #16
Enables: #132
Validated as a viable working solution by: #836
joshka added a commit that referenced this pull request Jan 24, 2024
Many widgets can be rendered without changing their state.

This commit implements The `Widget` trait for references to
widgets and changes their implementations to be immutable.

This allows us to render widgets without consuming them by passing a ref
to the widget when calling `Frame::render_widget()`.

```rust
// this might be stored in a struct
let paragraph = Paragraph::new("Hello world!");

let [left, right] = area.split(&Layout::horizontal([20, 20]));
frame.render_widget(&paragraph, left);
frame.render_widget(&paragraph, right); // we can reuse the widget
```

Implemented for all widgets except BarChart (which has an implementation
that modifies the internal state and requires a rewrite to fix.

Other widgets will be implemented in follow up commits.

Fixes: #164
Replaces PRs: #122 and
#16
Enables: #132
Validated as a viable working solution by:
#836
@Valentin271 Valentin271 deleted the 528/feature/render-by-ref branch January 26, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request imported from tui Imported from tui-rs PRs / issues
Projects
Status: Done
Status: Feature
Development

Successfully merging this pull request may close these issues.

None yet

5 participants