-
Notifications
You must be signed in to change notification settings - Fork 307
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 WidgetExt trait for debugging widgets #1065
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1065 /- ##
=======================================
- Coverage 89.4% 89.2% -0.3%
=======================================
Files 61 63 2
Lines 15464 15615 151
=======================================
Hits 13833 13936 103
- Misses 1631 1679 48 ☔ View full report in Codecov by Sentry. |
Importing the `WidgetExt` trait allows users to easily get a string representation of a widget with ANSI escape sequences for the terminal. This is useful for debugging and testing widgets. ```rust use ratatui::{prelude::*, widgets::widget_ext::WidgetExt}; fn main() { let greeting = Text::from(vec![ Line::styled("Hello", Color::Blue), Line::styled("World ", Color::Green), ]); println!("{}", greeting.to_ansi_string(5, 2)); } ``` Fixes: #1045
Made AnsiStringBuffer less private so this works too: use ratatui::{prelude::*, widgets::ansi_string_buffer::AnsiStringBuffer};
fn main() {
let mut buf = AnsiStringBuffer::new(5, 2);
buf.render_ref(&Line::styled("Hello", Color::Blue), Rect::new(0, 0, 5, 1));
buf.render_ref(&Line::styled("World", Color::Green), Rect::new(0, 1, 5, 1));
println!("{buf}");
} |
Things I'm not 100% sure of with this (and the rationale for this being "unstable"):
|
feature = "widget-ext", | ||
issue = "https://github.com/ratatui-org/ratatui/issues/1045" | ||
)] | ||
pub trait WidgetExt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your comment about the naming. In the off chance we want to add more extensions in the future, this should be named ToAnsiStringWidgetExt
or something to that effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either this trait should allow for more things on the same trait (in the future), then the name is fine, or it should be specific to this.
I also dislike the Ext ending here.
AnsiStringPrintable
would be a trait name that represents what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Ext is probably the right suffix for this pattern if it's not a more generic trait. https://rust-lang.github.io/rfcs/0445-extension-trait-conventions.html. It's used in tokio / futures streams (StreamExt), tracing-subscriber (a bunch of times https://docs.rs/tracing-subscriber/latest/tracing_subscriber/?search=ext) and various other crates use similar naming. I mostly wasn't sure if adding something before Ext made sense.
The proposed convention is, first of all, to (1) prefer adding default methods to existing traits or (2) prefer generically useful traits to extension traits whenever feasible.
Adding default methods doesn't seem like the right idea as converting to ANSI doesn't seem like it's something everyone would need on a widget, and it's something I'd prefer not to confuse newer users with if possible (hence adding as an extension trait / generically useful trait seems right). Adding a default method would however allow each widget to define their own ANSI representation (though I can't think of a useful need for this).
I'm not sure that given that the method needs a certain size to render, whether this trait has a generic usefulness. I don't see that there's really any need for this trait other than printing out widgets (I could be wrong there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other objects not being Widgets could implement that trait too. Buffer for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking Ext
was the right suffix since I had seen it in several other places, but maybe it is more commonly used when extending external traits or standard library traits? I like AnsiStringPrintable
too.
If it has just one method .to_ansi_string()
, why not trait ToAnsiString {}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other objects not being Widgets could implement that trait too. Buffer for example.
The bit I struggle with is working out what the purpose of passing in the width and height would be if implemented on Buffer? Would trait RenderToAnsiString { fn render_to_ansi_string() }
make sense?
@@ -21,6 21,7 @@ | |||
//! - [`Tabs`]: displays a tab bar and allows selection. | |||
//! | |||
//! [`Canvas`]: crate::widgets::canvas::Canvas | |||
pub mod ansi_string_buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to mark this as unstable and the widget module and re-export the trait / struct, but this needs sagebind/stability#11 (enable unstable attribute on use statements).
Importing the
WidgetExt
trait allows users to easily get a stringrepresentation of a widget with ANSI escape sequences for the terminal.
This is useful for debugging and testing widgets.
Fixes: #1045