Closed orhun closed 1 week ago
I also looked into using something more generic (like Num
trait from num-traits
) but felt like an overkill at the end.
This doesn't seem like something that would be expected idiomatically in rust.
String::from(num)
doesn't existCow<str>::from(num)
also doesn'tI'd expect to generally use format!
for this rather than adding conversions from random things to strings. (And the ratatui-macros have some macros that call into format - line!
, text!
, span!
if you want something shorter)
Perhaps this could be implemented as traits: ToSpan
/ ToLine
/ ToText
if we're going for something more aligned with existing rust idioms, and then those would be implemented on num etc.
What about creating a Span from every Display implementation?
What about creating a Span from every Display implementation?
I like the idea - I'd be concerned about whether this is too magical for newer users. Any ideas about whether there could be other downsides of this?
This doesn't seem like something that would be expected idiomatically in rust.
Yeah, I see your point & makes sense. I'm happy to take the ToLine
approach here.
What about creating a Span from every Display implementation?
Can you elaborate this? I didn't quite get what this would look like.
Can you elaborate this? I didn't quite get what this would look like.
perhaps either:
impl<T:Display> From<T> for Span { ... }
// or
impl <T: Display> ToSpan for T { ... }
I suspect the first approach might clash with other existing / future generic implementations, so it's something to be careful with. It's a reasonable idea that's worth exploring, but I'd want to see how it pans out in real use cases to evaluate whether it really works well.
Maybe a more explicit Span::from_display<T: Display>(input: T)
might be helpful for readable and understandable code.
Maybe a more explicit
Span::from_display<T: Display>(input: T)
might be helpful for readable and understandable code.
The ToSpan trait idea is more in line with ToString in std.
https://doc.rust-lang.org/std/string/struct.String.html#impl-ToString-for-T
impl<T: fmt::Display + ?Sized> ToString for T { ... }
So (copying wording etc. from std too):
/// A trait for converting a value to a [`Span`].
///
/// This trait is automatically implemented for any type that implements the [`Display`] trait. As
/// such, `ToSpan` shouln't be implemented directly: [`Display`] should be implemented instead, and
/// you get the `ToSpan` implementation for free.
///
/// [`Display`]: std::fmt::Display
pub trait ToSpan {
fn to_span(&self) -> Span<'_>;
}
/// # Panics
///
/// In this implementation, the `to_span` method panics if the `Display` implementation returns an
/// error. This indicates an incorrect `Display` implementation since `fmt::Write for String` never
/// returns an error itself.
impl<T: fmt::Display> ToSpan for T {
fn to_span(&self) -> Span<'_> {
Span::raw(self.to_string())
}
}
#[test]
fn test_num() {
assert_eq!(42.to_span(), Span::raw("42"));
}
... and ToLine / ToText follow pretty nicely.
That looks good! Implemented in bdb2096c7af75360293f86e13546ed1a631eb382
However, this is not exactly what I wanted to see:
-let text = Text::from(42);
+let text = Text::from(42.to_span());
To take the value directly I'm guessing we need impl<T: Display> for Text
but that conflicts with the other implementations.
However, this is not exactly what I wanted to see:
-let text = Text::from(42); +let text = Text::from(42.to_span());
What about adding ToLine
and ToText
?
let text = 42.to_text();
I like the .to_span()
idea but I don't think we should do .to_line()
and .to_text()
because it is non intuitive that
42.to_line()
becomes [Span::new(42.to_string())]
and 42.to_text()
becomes [ [Span::new(42.to_string())] ]
I'd prefer upstreaming the macros from ratatui-macros
after adding a .to_span()
method. Then users can write:
span!(42)
// or
line![42]
// or
text![42]
I like the
.to_span()
idea but I don't think we should do.to_line()
and.to_text()
because it is non intuitive that* `42.to_line()` becomes `[Span::new(42.to_string())]` and * `42.to_text()` becomes `[ [Span::new(42.to_string())] ]`
They would create a Line and a Text, not arrays. I think these extra traits seem to add some consistency. In particular, if you know that the Display implementation of something is multi-line, then you should expect to be able to convert that to text, not to a span / line.
I'd prefer upstreaming the macros from
ratatui-macros
after adding a.to_span()
method. Then users can write:span!(42) // or line![42] // or text![42]
This is not necessarily an alternative - wouldn't it make sense to do both?
to_span
and to_text
seem reasonable to me. to_line
seems pointless, as a Line consists of multiple Spans with different styling. But there can only be a single style assigned here (the raw / unset one).
The only difference of to_text
and to_span
would be the handling of new lines.
to_line
seems pointless, as a Line consists of multiple Spans with different styling.
As a direct conversion perhaps not, but as an intermediate value where a line is composed, or just as a more obviously explicit call where a line is expected
let mut line = 42.to_line();
line.push_span("%".red());
let mut text = Text::from("asdf");
text.push_line(42.to_line());
I definitely agree that ToSpan and ToText have strong reasons to belong. ToLine is a weaker reason, but still useful (and for consistency this makes sense). There's no obvious downside to this.
Handling newlines should be the same as for Line - they get swallowed afaik (noting that there might be some ways to get newline characters in there).
If there is no to_line then the latter might be written as:
text.push_line(42.to_span());
// which does something completely different (but not obviously so) to:
text.push_span(42.to_span());
let mut line = 42.to_line(); line.push_span("%".red());
When the length is already known it's also more performant to write it differently:
let line = Line::from([42.to_span(), "%".red()];
to_line
would only be a convenience thing, but a single span should always work as a line, so to_line
would be pointless.
If there is no to_line then the latter might be written as: […]
As long as the concept of Span, Line, and Text are clear, this isn't confusing. push_line
accepts Into<Line>
so push_line(42.to_span())
should work as expected.
When the concept isn't clear, Into
is introducing confusing behavior. So these three thingies should be explained well enough, then I don't see an issue here.
let mut line = 42.to_line(); line.push_span("%".red());
When the length is already known it's also more performant to write it differently:
let line = Line::from([42.to_span(), "%".red()];
to_line
would only be a convenience thing, but a single span should always work as a line, soto_line
would be pointless.If there is no to_line then the latter might be written as: […]
As long as the concept of Span, Line, and Text are clear, this isn't confusing.
push_line
acceptsInto<Line>
sopush_line(42.to_span())
should work as expected.When the concept isn't clear,
Into
is introducing confusing behavior. So these three thingies should be explained well enough, then I don't see an issue here.
Ed, you're straw-manning the argument. The examples are intentionally simplified. Try strong-manning instead and see if you're able to find some examples that do make sense for ToLine to exist. Particularly try taking on the new rust user perspective.
They would create a Line and a Text, not arrays.
My mental model is that we have Span
, const Line = Vec<Span>
, const Text = Vec<Line>
.
In particular, if you know that the Display implementation of something is multi-line, then you should expect to be able to convert that to text, not to a span / line.
Having something that can convert multiple lines separated by \n
in the display output has me convinced of .to_text()
. I'm indifferent towards .to_line()
. Maybe just for the sake on consistency it should exist?
One way to approach deciding these kinds of API questions is to go find larger examples, in code that is already out there today, that would be improved if it could use the new API. It is difficult to assess whether small examples are good ideas in isolation, and easier if, say, a 10-20 line code sample is improved in some way.
Thinking about this more, I think there's a bit of a problem with using the Display for any of this - what if you'd want to put styling info directly on the type you're dealing with instead of external to the type. Let's take an example like:
struct MarkdownHeading {
content: String,
...
}
impl fmt::Display for MarkdownHeading {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", content)
}
}
// but what if I want this to be always styled
impl ToLine for MarkdownHeading {
fn to_line(&self) -> Line {
content.blue().bold().into()
}
}
The same could be said for Span and Text easily - if we implement ToXXX for Display, then we prevent the ability for types to implement their own conversion to Span/Line/Text through these methods. Perhaps these traits might be better named ToRawSpan, ToRawLine, ToRawText?
My mental model is that we have
Span
,const Line = Vec<Span>
,const Text = Vec<Line>
.
Ah, I see. Mine is that these are all just String
s, but with formatting and alignment. The internal representation should be treated as an implementation detail.
Where I really do like this idea is that it provides a more natural way to compose text/line oriented things (like the markdown example, or writing pretty printed JSON). If you have a container of things which implement trait MyCustomThing: ToLine
, then you can iterate over that for rendering nicely. In https://github.com/joshka/ratatui-json-tutorial/ I was trying to work out ways to convert the json tree into a tree of widgets, but got stuck thinking about how to keep track of where each element widget starts and finishes on a line. In this situation, switching to treating them as being convertible to individual lines / spans seems a logical step.
Thinking about this more, I think there's a bit of a problem with using the Display for any of this - what if you'd want to put styling info directly on the type you're dealing with instead of external to the type.
It is probably better to talk about doing this for the std::string::ToString
trait instead of std::fmt::Display
trait, since at least in that case it is clear there are zero available formatting options.
But I agree with you about the bit of a problem.
I usually think it is a good practice to define a trait/protocol/interface next to its consumer. That is the case with std::fmt::Display
: it is defined in std::fmt
for the purpose of std::fmt::format!
.
The benefit of this approach is that the two can be designed together. For example, std::fmt::Display
implementations are given a Formatter, which has all sorts of stuff related to formatting numbers, debug strings, etc., in support formatting options set on the format string. By omitting the format string, and just calling Display
, flexibility is lost. At the same time, stuff related to rendering into terminals is absent (colors, underline, blink, cursor position, etc.).
It might be convenient to take advantage of a type's std::fmt::Display
trait for rendering in Ratatui (or other UIs), but that is perhaps done in an opt-in way. A pretty clear, obvious way is to require a call to format!
, which has the advantage of allowing the above-mentioned std::fmt
options to be set on the Formatter
passed to Display::fmt
.
...and if talking about supporting the ToString
trait, how about just having application code call thing.to_string()
instead?
Where I really do like this idea is that it provides a more natural way to compose text/line oriented things (like the markdown example, or writing pretty printed JSON). If you have a container of things which implement
trait MyCustomThing: ToLine
, then you can iterate over that for rendering nicely. In https://github.com/joshka/ratatui-json-tutorial/ I was trying to work out ways to convert the json tree into a tree of widgets, but got stuck thinking about how to keep track of where each element widget starts and finishes on a line. In this situation, switching to treating them as being convertible to individual lines / spans seems a logical step.
Analogous to std::fmt
that would be a trait under the ratatui::
module for Ratatui's purposes. Makes a lot of sense to me.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.4%. Comparing base (
38bb196
) to head (67718c5
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I personally like the idea of having ToXyz
implementations for Line
, Span
and Text
. I implemented that in my latest commit along with some tests.
I'm happy to rename it to ToRawXyz
, but I think having this in the first place is a good start.
what if you'd want to put styling info directly on the type you're dealing with instead of external to the type.
Can't we export the trait and let the users implement it for their own types? Also, we should think about how common that use-case is too.
what if you'd want to put styling info directly on the type you're dealing with instead of external to the type.
Can't we export the trait and let the users implement it for their own types? Also, we should think about how common that use-case is too.
See https://github.com/rust-lang/rfcs/blob/master/text/0565-show-string-guidelines.md#user-facing-fmtdisplay, which talks about how it is expected that application code will use a different type (most likely a newtype) for each variation of Display
behavior they want.
what if you'd want to put styling info directly on the type you're dealing with instead of external to the type.
Can't we export the trait and let the users implement it for their own types? Also, we should think about how common that use-case is too.
See https://github.com/rust-lang/rfcs/blob/master/text/0565-show-string-guidelines.md#user-facing-fmtdisplay, which talks about how it is expected that application code will use a different type (most likely a newtype) for each variation of
Display
behavior they want.
I think what you're saying is if the widget implements display, and that's not what we want to be rendered, then it's easy enough to do MyWidget::for_whatever().to_text()
etc., so we shouldn't worry too much about the blanket Display implementation being a blocker.
@orhun we can export the trait, but my point was that you can't implement it twice on the one type, and implementing it as a blanket implementation for every type that implements display would be the first implementation in some cases. I'd guess this is probably rare enough that it shouldn't matter too much though (and the workaround above is fine).
Addressed the review comments and rebased on main. Now the only issue is:
warning: method `to_line` is never used
warning: method `to_span` is never used
warning: method `to_text` is never used
Should we come up places to use this new mechanism somewhere in the code or just slap dead_code
on it? What do you think @ratatui-org/maintainers
Now the only issue is:
warning: method `to_line` is never used warning: method `to_span` is never used warning: method `to_text` is never used
Should we come up places to use this new mechanism somewhere in the code or just slap
dead_code
on it? What do you think @ratatui-org/maintainers
Nothing pub
should warn about being unused. A pub
thing counts as being (possibly) used by the users of the crate.
It took me a while to figure this out, but it turns out that the new traits were not actually pub outside Ratatui. Try the patch below (which causes new warnings about no doc comment for the trait methods):
diff --git a/src/text.rs b/src/text.rs
index 6262ca6..92aad3a 100644
--- a/src/text.rs
+++ b/src/text.rs
@@ -49,13 +49,16 @@ pub use grapheme::StyledGrapheme;
mod line;
pub use line::Line;
+pub use line::ToLine;
mod masked;
pub use masked::Masked;
mod span;
pub use span::Span;
+pub use span::ToSpan;
#[allow(clippy::module_inception)]
mod text;
pub use text::Text;
+pub use text::ToText;
Thanks for the pointers guys, it should be good to go now!
Now you can create
Line
andText
from numbers like so:(I was doing little testing for my TUI app and saw that this isn't supported - then I was like WHA and decided to make it happen :tm:)