Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Version 0.10: Spans can't contain line-breaks #339

Closed
TheLostLambda opened this issue Jul 17, 2020 · 8 comments
Closed

Version 0.10: Spans can't contain line-breaks #339

TheLostLambda opened this issue Jul 17, 2020 · 8 comments
Labels

Comments

@TheLostLambda
Copy link
Contributor

Describe the bug
I understand that multiple lines in a paragraph are intended to be represented using Text, but I seem unable to apply a span over several lines which I think is still very useful.

To Reproduce
Using the current development version from Git, I'm ultimately building the following:

[src/console/render.rs:58] vec![Spans :: from(text), Spans :: from("This is on the second line...")] = [
    Spans(
        [
            Span {
                content: "To Kill a Mockingbird\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: ITALIC,
                    sub_modifier: (empty),
                },
            },
            Span {
                content: "*noun*\n   1) outfit, equipment, attire\n\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: BOLD,
                    sub_modifier: (empty),
                },
            },
            Span {
                content: "1) obstreperous\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: (empty),
                    sub_modifier: (empty),
                },
            },
            Span {
                content: "2) invective\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: (empty),
                    sub_modifier: (empty),
                },
            },
            Span {
                content: "3) habiliment\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: (empty),
                    sub_modifier: (empty),
                },
            },
            Span {
                content: "4) furore\n",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: (empty),
                    sub_modifier: (empty),
                },
            },
        ],
    ),
    Spans(
        [
            Span {
                content: "This is on the second line...",
                style: Style {
                    fg: None,
                    bg: None,
                    add_modifier: (empty),
                    sub_modifier: (empty),
                },
            },
        ],
    ),
]

Which results in the following (note that all of my \n's were ignored):

To Kill a Mockingbird*noun*   1) outfit, equipment, attire1) obstreperous2)   
invective3) habiliment4) furore                                               
This is on the second line...      

Expected behavior

I'm really looking for what I had before the text refactoring:

To Kill a Mockingbird
*noun*
   1) outfit, equipment, attire

1) obstreperous
2) invective
3) habiliment
4) furore                                               
This is on the second line...      

Desktop:

  • OS: Linux
  • Terminal Emulator: gnome-terminal
  • Font: IBM Plex Mono
  • Crate version:
name = "tui"
version = "0.9.5"
source = "git+https://github.com/fdehau/tui-rs.git#6b52c91257f9073e4fe913f15bee0716b45dfd0d"
  • Backend: crossterm
@TheLostLambda
Copy link
Contributor Author

Hi @fdehau !

Following on from the discussion in #341, I figured I'd give more examples of my problem. One of my biggest grievances is how difficult it's become to apply a style to a multi-line string. If there is no style, I can just use Text::from(), but something like this (using my fix from #341):

fn print_question(quiz: &QuizRef) -> Vec<Span> {
    let style = Style::default().add_modifier(Modifier::BOLD);
    vec![Span::styled(format!("{}\n\n", dbg!(quiz.borrow().ask())), style)]
}

Where quiz.borrow().ask() is:

[src/console/render.rs:72] quiz.borrow().ask() = "*verb*\n   1) to soothe in temper of disposition : appease\n   2) to reduce the rigidity of : soften\n   3) to reduce in intensity : assuage, temper"

Becomes something like this (untested pseudo-code) in 0.10:

fn print_question(quiz: &QuizRef) -> Vec<Spans> {
    let style = Style::default().add_modifier(Modifier::BOLD);
    let mut result = quiz.borrow().ask().lines()
        .map(|line| Spans::from(Span::styled(line, style)))
        .collect();
    // Whoops, not even this would work:
    // result.push(Spans::from(Span::raw("\n\n"));
    result.push(Spans::from(""));
    result.push(Spans::from(""));
    result
}

I find that a lot of boilerplate and bloat for, in this case, no extra functionality. When I already have a string with line-breaks, it's become unreasonably difficult to apply a style to them.

It also makes inserting empty lines, particularly several in a row, somewhat annoying, as I now need two repetitive lines of result.push(Spans::from("")); to work around the newline stripping.

Some of this ultimately falls in again with the last PR I opened I suppose, when the user has a string with whitespaces and newlines, it's very unexpected for the library to go stripping them and changing the formatting.

I'd love to know what you think about all of this!

@TheLostLambda TheLostLambda changed the title Current Development Version: Span's can't contain line-breaks Version 0.10: Spans can't contain line-breaks Aug 9, 2020
@extrawurst
Copy link
Contributor

Same here with gitui - I am forwarding a ton of text (git diffs) that include linebreaks to be drawn by tui-rs and now to be able to upgrade to tui-rs 0.10 I need to parse the line breaks in there manually and create a ton of before unneeded allocations

@dotdash
Copy link
Contributor

dotdash commented Aug 18, 2020

(Moved here from #341, sorry for the noise)

First off, the new Text/Spans/Span approach allowed me to remove my custom multi-line List hack, thanks for that!

Ignoring the issue of nested vectors (which IMHO needs addressing as well, but I have not though of a comprehensive solution, yet), the main point for me is that things seem to be only "half way there". Converting a &str that contains newlines to Spans´ using From/Intosilently messes the result up, but directly converting it to aTexthandles the newlines just fine. But then I cannot add styling, nor can I (ergonomically) make use of that functionality if I want anything else in thatText` element.

For my use case, I guess I'd already be pretty happy with just two methods on Text, append(&str) and append_styled(&str, Style), letting me just hand over a string and optionally a style, which is then broken into Span/Spans. The implementation might be a bit tricky as you would probably need some way to track the current state with regards to whether or not an append call starts a new Spans or just adds to the current one. I'm not sure what's the best approach there.

One note on allocations: about 95% of my Spans only contain a single span, which makes me feel like that's a valid case to optimize for, i.e. avoiding the Vec in the Spans for this case. Maybe by having an enum, maybe by representing Text as a single Vec<Span> to begin with, and storing line breaks in term of offsets into that Vec.

@dotdash
Copy link
Contributor

dotdash commented Aug 18, 2020

FWIW, I just scrambled together this extension trait, which isn't quite ideal, but makes life a lot easier for me.

pub trait TextExt<'a> {
    fn append<T: Into<Cow<'a, str>>>(&mut self, s: T) -> &mut Self;
    fn append_styled<T: Into<Cow<'a, str>>>(&mut self, s: T, style: Style) -> &mut Self;
}

impl<'a> TextExt<'a> for Text<'a> {
    fn append<T: Into<Cow<'a, str>>>(&mut self, s: T) -> &mut Self {
        self.append_styled(s, Style::default())
    }

    fn append_styled<T: Into<Cow<'a, str>>>(&mut self, s: T, style: Style) -> &mut Self {
        match s.into() {
            Cow::Borrowed(s) => {
                let mut lines = s.lines();
                if let Some(last) = self.lines.last_mut() {
                    if let Some(line) = lines.next() {
                        last.0.push(Span::styled(line, style));
                    }
                }

                self.lines.extend(lines.map(|line| Spans(vec![Span::styled(line, style)])));
            }
            Cow::Owned(s) => {
                let mut lines = s.lines();
                if let Some(last) = self.lines.last_mut() {
                    if let Some(line) = lines.next() {
                        last.0.push(Span::styled(line.to_owned(), style));
                    }
                }

                self.lines.extend(lines.map(|line| Spans(vec![Span::styled(line.to_owned(), style)])));
            }
        }

        self
    }
}

@fdehau
Copy link
Owner

fdehau commented Aug 23, 2020

@TheLostLambda

One of my biggest grievances is how difficult it's become to apply a style to a multi-line string

Ok I can understand that and as I suggested and as @dotdash showed, this might simply be a matter of providing "builder" methods on Text to ease the process of creating multi-line styled strings. Controls characters would be interpreted and converted to the internal representations of Text. WDYT ?

Some of this ultimately falls in again with the last PR I opened I suppose, when the user has a string with whitespaces and newlines, it's very unexpected for the library to go stripping them and changing the formatting.

I don't agree with this statement when the library clearly defines Span and Spans as single line strings. Keep in mind these types are used in all other widgets to advertise the capabilities of each widget text properties, e.g a Block title can only be on a single line while a ListItem content can be over multi-line, etc.

@extrawurst

now to be able to upgrade to tui-rs 0.10 I need to parse the line breaks in there manually

Would the proposed solution work with your use case ?

create a ton of before unneeded allocations

Are you simply worried about that or is it a perf issue for gitui ?

@dotdash

Converting a &str that contains newlines to Spans using From/Into silently messes the result up, but directly converting it to a Text handles the newlines just fine.

Again, the documentation clearly states Spans content is supposed to be on a single line.

One note on allocations: about 95% of my Spans only contain a single span, which makes me feel like that's a valid case to optimize for, i.e. avoiding the Vec in the Spans for this case.

I would like to see numbers before applying that kind of optimization which will make Text harder to work with internally.

@TheLostLambda
Copy link
Contributor Author

TheLostLambda commented Aug 23, 2020

@fdehau @extrawurst and @dotdash , I've tossed together a PR (#361) that I hope addresses the ergonomics of this issue (I've not included any optimisation changes). The added code is short, consistent with the rest of the library (particularly Span), and uses standard Rust traits. With that being said, I'm sure there is room to improve, so I'd love to hear what you all think of this solution!

I'll post some code snippets here soon showing off this new abstraction and how it addresses some of the issues I'd been having.

@TheLostLambda
Copy link
Contributor Author

I'm happy to report that, working with this new abstraction, I've ended up with even nicer code than before 0.10 while retaining all of the guarantees of the new text system.

Here is a code snippet from earlier in this conversation, using my old system that just allowed newlines in Spans:

fn print_question(quiz: &QuizRef) -> Vec<Span> {
    let style = Style::default().add_modifier(Modifier::BOLD);
    vec![Span::styled(format!("{}\n\n", dbg!(quiz.borrow().ask())), style)]
}

And here is the current working version (with my new PR):

fn print_question(quiz: &QuizRef) -> Text {
    let style = Style::default().add_modifier(Modifier::BOLD);
    Text::styled(format!("{}\n\n", quiz.borrow().ask()), style)
}

It's shorter, more semantic, and maintains the abstraction.

Some other wonderful things? Implementing Extend meant that this code (screenshot for those rust-analyser annotations):

Screenshot from 2020-08-23 21-45-43

Hardly needed to change at all:
Screenshot from 2020-08-23 21-47-21

Yup! Almost exactly the same as the vector system! Even shorter because I could drop all the silly Span and Spans fluff on the paragraph line.

Perhaps my favourite result?
image

If you are only ever dealing with external, pre-formatted, multi-line strings, like I am and (I presume) git-gui is, you never need to see or think about the underlying Span and Spans abstractions. This fixes the Text abstraction previously being leaky and revealing its implementation details. By fixing that, this also paves the road for fewer breaking changes if we ever do implement something like the enum optimisation proposed by @dotdash .

I'd love to know what you think about all of this! I'm quite pleased myself, but I'm only a sample-size of one.

@TheLostLambda
Copy link
Contributor Author

Closed by #361 ! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants