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(widgets): implement Widget for Widget refs #833

Merged
merged 6 commits into from
Jan 24, 2024
Merged

Conversation

joshka
Copy link
Member

@joshka joshka commented Jan 17, 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().

// 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

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (736605e) 92.3% compared to head (4dc61c2) 92.3%.

❗ Current head 4dc61c2 differs from pull request most recent head 2269c5d. Consider uploading reports for the commit 2269c5d to get more accurate results

Files Patch % Lines
src/text/line.rs 44.4% 5 Missing ⚠️
src/text/span.rs 37.5% 5 Missing ⚠️
src/text/text.rs 44.4% 5 Missing ⚠️
src/widgets/list.rs 75.0% 4 Missing ⚠️
src/widgets/table/table.rs 66.6% 4 Missing ⚠️
src/widgets/canvas.rs 88.8% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #833     +/-   ##
=======================================
- Coverage   92.3%   92.3%   -0.1%     
=======================================
  Files         61      61             
  Lines      16096   16141     +45     
=======================================
+ Hits       14872   14903     +31     
- Misses      1224    1238     +14     

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

@joshka joshka marked this pull request as draft January 17, 2024 01:22
@joshka joshka marked this pull request as ready for review January 17, 2024 14:32
@joshka
Copy link
Member Author

joshka commented Jan 17, 2024

Not all the examples have been updated, but this is about a good enough size to review as it stands.

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

As stated on discord I like this way of introducing render by reference. Approved!

@joshka
Copy link
Member Author

joshka commented Jan 18, 2024

Fun fact that I discovered earlier tonight.

RefWidget and MutWidget aren't actually necessary as the same effect can be had by implementing Widget on &T.

impl Widget for Block<'_> {
    fn render(self, area: Rect, buf: &mut Buffer) {
        (&self).render(area, buf);
    }
}

impl Widget for &Block<'_> {
    fn render(self, area: Rect, buf: &mut Buffer) {
        if area.is_empty() {
            return;
        }
        self.render_borders(area, buf);
        self.render_titles(area, buf);
    }
}

This allows you to do all the things you'd need to against a reference to the widget - you can store it, render a reference to it, etc.

let block = Block::default().title("Block").borders(Borders::ALL);

// does not compile
terminal.draw(|frame| {
    frame.render_widget(block, frame.size());
    let _area = block.inner(Rect::default());
})?;

// compiles
terminal.draw(|frame| {
    frame.render_widget(&block, frame.size());
    let _area = block.inner(Rect::default());
})?;

The upshot of this is that instead of adding two traits, we should instead implement Widget on &T more often.
The latest commit makes this change and removes the RefWidget and MutWidget traits.

@kdheepak
Copy link
Collaborator

And instead of MutWidget we continue using the stateful widget strategy? I actually like that a lot.

@orhun
Copy link
Member

orhun commented Jan 18, 2024

Seconding kd here, I think having impl &T is much cleaner solution. And yeah, nice find!

@joshka
Copy link
Member Author

joshka commented Jan 18, 2024

And instead of MutWidget we continue using the stateful widget strategy? I actually like that a lot.

impl Widget on &mut SomeWidget also works for situations where it makes sense to just consider the widget as the state instead of having external state.

What this does suggest is that we could do with some really clear docs on these scenarios.

@Valentin271
Copy link
Member

+1 for implementing on &T and &mut T.
Also +1 for keeping the StatefulWidget pattern.

What this does suggest is that we could do with some really clear docs on these scenarios.

Yep if we keep StatefuleWidget and &mut T this is essential.

@joshka
Copy link
Member Author

joshka commented Jan 18, 2024

StatefulWdget shouldn't go anywere - it's useful to have the state and widget separate - mainly it simplifies the application state. Often state values won't have any lifetimes, as they're just storing indexes, etc. This simplification is helpful to newer users (as it avoids the pain of adding lifetimes everywhere).

@joshka joshka changed the title feat(widgets): add RefWidget and MutWidget traits feat(widgets): implement Widget for Widget refs Jan 20, 2024
@joshka joshka force-pushed the refactor-widget branch 4 times, most recently from eef2054 to 45146ce Compare January 20, 2024 02:53
@joshka
Copy link
Member Author

joshka commented Jan 20, 2024

I rebased this on main, squashed to remove the RefWidget implmentation, and updated the commit message and PR comment to fit as a good changelog message.

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

Looks good, only the doc is a mandatory change for me.

src/widgets.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
tests/widgets_calendar.rs Show resolved Hide resolved
@Valentin271 Valentin271 added this to the v0.26.0 milestone Jan 22, 2024
@joshka
Copy link
Member Author

joshka commented Jan 24, 2024

I've pulled in the changes from main and finished off all the widgets except BarChart.

src/prelude.rs Show resolved Hide resolved
@joshka joshka requested a review from Valentin271 January 24, 2024 15:25
src/widgets/barchart.rs Show resolved Hide resolved
src/widgets.rs Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

All good to me. We'll finally have render by ref! I still can't believe it was this simple.

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 joshka merged commit 815757f into main Jan 24, 2024
32 checks passed
@joshka joshka deleted the refactor-widget branch January 24, 2024 18:34
zjp-CN added a commit to zjp-CN/term-rustdoc that referenced this pull request Feb 3, 2024
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.

4 participants