ratatui-org / ratatui

Rust library that's all about cooking up terminal user interfaces (TUIs) 👨‍🍳🐀
https://ratatui.rs
MIT License
8.82k stars 263 forks source link

fix(widgets)!: rename `StatefulWidgetRef::render_ref` to `render_stateful_ref` #1184

Open joshka opened 2 weeks ago

joshka commented 2 weeks ago

This commit renames the StatefulWidgetRef::render_ref method to render_stateful_ref. This helps avoid collisions with the WidgetRef trait's render_ref method. This change is breaking and requires updating all implementations of StatefulWidgetRef.

BREAKING CHANGE: StatefulWidgetRef::render_ref has been renamed to StatefulWidgetRef::render_stateful_ref.

 trait StatefulWidgetRef {
     type State;
-    fn render_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
+    fn render_stateful_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
 }

Partially addresses https://github.com/ratatui-org/ratatui/issues/996

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.3%. Comparing base (4bfdc15) to head (330d071). Report is 6 commits behind head on main.

Files Patch % Lines
src/terminal/frame.rs 0.0% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1184 +/- ## ===================================== Coverage 94.3% 94.3% ===================================== Files 60 60 Lines 14679 14679 ===================================== Hits 13843 13843 Misses 836 836 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kdheepak commented 2 weeks ago

I think we could do render_ref_with_state() instead, but I'm fine with render_stateful_ref(). Maybe check in with other maintainers before merging?

joshka commented 2 weeks ago

I think we could do render_ref_with_state() instead, but I'm fine with render_stateful_ref(). Maybe check in with other maintainers before merging?

I thought about that a little prior to writing this. The main pro for _stateful is that it's in the name of the trait already, so it's kinda expected to be in the method name. The main pro for _with_state seems is that it's a natural name for what this does. For me this feels like a 55/45 naming decision. I have a small amount of preference for _stateful, but would be convinced if there were good examples of _with_xxx on trait implementations in std.

kdheepak commented 2 weeks ago

I have a preference for renaming these new trait names (the ones that are unstable) to Render and RenderWithState and then changing the names of the trait methods to match accordingly.

The disadvantage is that it would break from convention with the Widget and StatefulWidget traits. The advantage is that it is more consistent and clear (imo).

And I can see a world where everyone moves to using these new traits, and we deprecate the old ones and remove them eventually (a few years later?).

joshka commented 2 weeks ago

I have a preference for renaming these new trait names (the ones that are unstable) to Render and RenderWithState and then changing the names of the trait methods to match accordingly.

The disadvantage is that it would break from convention with the Widget and StatefulWidget traits. The advantage is that it is more consistent and clear (imo).

And I can see a world where everyone moves to using these new traits, and we deprecate the old ones and remove them eventually (a few years later?).

Let's move this point to a discussion on the forum.

joshka commented 2 weeks ago

Let's move this point to a discussion on the forum.

https://forum.ratatui.rs/t/naming-render-traits-methods/68

@kdheepak based on the logic in the above forum post are you happy to see this merged as is, or would you prefer to hold for a bit on this. It would be good to get it out in 0.27.0 perhaps.

EdJoPaTo commented 2 weeks ago

Probably off-topic to this PR… why move this to a different place? Personally the forum feels like a place for "how to do this" and "showoffs". Stuff that should end in the code should be discussed as issues next to the code.

To the topic of naming the trait / methods… Personally I went to ratatui::widget::TRAIT::render(widget, …) explicitly and that works well so far for me without naming conflicts. This is not ideal, but until there is a good way of handling this trait thing, good enough. Somehow this feels a bit rushed and all over the place to me currently. Discussion in multiple PR, the forum, …. This PR seems more like a draft to me currently. This might be a way to go forward. But it's nothing that should be merged for the next release as it will result in a lot of churn for sure.

kdheepak commented 2 weeks ago

I’d like to hold out till we figure out the naming, if that’s okay. Making one breaking change later (v0.28.0?) is better than making one now and potentially again one later.

joshka commented 2 weeks ago

Probably off-topic to this PR… why move this to a different place? Personally the forum feels like a place for "how to do this" and "showoffs". Stuff that should end in the code should be discussed as issues next to the code.

It is off-topic for this PR. If you ask about the rationale for this on the forum, I'll answer there... ;P