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(line): add Line::raw constructor #511

Merged
merged 2 commits into from Sep 22, 2023

Conversation

orhun
Copy link
Sponsor Member

@orhun orhun commented Sep 17, 2023

closes #508

There is already `Span::raw` and `Text::raw` methods
and this commit simply adds `Line::raw` method for symmetry.
@orhun
Copy link
Sponsor Member Author

orhun commented Sep 17, 2023

Question: should we break newlines into different spans? e.g. test\ntest will result in vec![Span, Span] etc.

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Merging #511 (9d4db84) into main (0c7d547) will increase coverage by 0.13%.
Report is 8 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   89.93%   90.06%   +0.13%     
==========================================
  Files          41       40       -1     
  Lines       11334    11267      -67     
==========================================
- Hits        10193    10148      -45     
+ Misses       1141     1119      -22     
Files Changed Coverage Δ
src/text/line.rs 96.75% <100.00%> (+0.20%) ⬆️

... and 9 files with indirect coverage changes

@joshka
Copy link
Member

joshka commented Sep 17, 2023

There are a few ways we could handle newlines in Line:

  1. reject them completely (panic and consider adding some try_ methods)
  2. remove them during creation and create a single Span that has a single Cow::Owned constructed from the input with the lines removed.
  3. remove them and create multiple Spans that each hold Cow::Borrowed slices.
  4. ignore them and handle it on render (I don't like this approach as it means every thing that takes Line now has to understand that it might have newlines).

Option 3 makes most sense to me, but Option 1 is also possible. Option 2 allocates new memory, but is probably a reasonable approach and "performant enough" except for massive strings. Option 4 is a pretty terrible one.

@kdheepak
Copy link
Collaborator

I think I have some naming + semantic questions about what we want Line to be?

From Wiki

In computing, a line is a unit of organization for text files. A line consists of a sequence of zero or more characters, usually displayed within a single horizontal sequence.
The term comes directly from physical printing, where a line of text is a horizontal row of characters.

Line currently uses Vec<Spans> as the implementation. But it is only during rendering that we decide where the "end of a line" should be. Right? If someone wants to store this content in a Paragraph:

This is a sentence that
wraps to the next line.
This is another sentence that doesn't wrap to the next line

Do they make 3 lines to convert them to a Text object?
Do they make 2 lines with the first line having a \n in it? Should the first line split it into 2 spans? What if they want This to be bold but the rest not? Then the first line would have 3 Spans because Span is the lowest unit of styling.

@orhun
Copy link
Sponsor Member Author

orhun commented Sep 19, 2023

I think @joshka's 3rd option makes the most sense. If everyone agrees then I can update the PR.

@joshka
Copy link
Member

joshka commented Sep 19, 2023

Line currently uses Vec<Spans> as the implementation. But it is only during rendering that we decide where the "end of a line" should be. Right? If someone wants to store this content in a Paragraph:

I think it's reasonable that Line would have a fn wrapped(params) -> impl Iterator<Type = Line<'a>> method that helps make the distinction more obvious.

Do they make 3 lines to convert them to a Text object?
Do they make 2 lines with the first line having a \n in it? Should the first line split it into 2 spans? What if they want This to be bold but the rest not? Then the first line would have 3 Spans because Span is the lowest unit of styling.

3 lines Text::raw(content: T: Into<Cow<'a, str>>) method will handle Text::raw("Line1\nLine2\nLine3") by creating 3 lines each with a single span:

ratatui/src/text/text.rs

Lines 45 to 57 in 5498a88

pub fn raw<T>(content: T) -> Text<'a>
where
T: Into<Cow<'a, str>>,
{
let lines: Vec<_> = match content.into() {
Cow::Borrowed("") => vec![Line::from("")],
Cow::Borrowed(s) => s.lines().map(Line::from).collect(),
Cow::Owned(s) if s.is_empty() => vec![Line::from("")],
Cow::Owned(s) => s.lines().map(|l| Line::from(l.to_owned())).collect(),
};
Text::from(lines)
}

So at the least for consistency, Line should ever have a newline character. This also avoids having every widget that takes a line parameter have to work manually handle it (e.g. what should buffer.cell_mut().symbol = "\n" do? I'm sure there are other arguments as well.

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Please add newline handling ("a\nb" becomes Line::from(vec![Span::raw("a"), Span::raw("b")]))

@orhun
Copy link
Sponsor Member Author

orhun commented Sep 22, 2023

Updated in 9d4db84

@orhun orhun requested a review from joshka September 22, 2023 11:17
@joshka joshka merged commit d67fa2c into ratatui-org:main Sep 22, 2023
33 checks passed
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.

Add Line::raw for symmetry with Span::raw and Text::raw
3 participants