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

docs: Update section on rendering 📚 #99

Merged
merged 13 commits into from
Oct 7, 2023
Merged

Conversation

kdheepak
Copy link
Contributor

@kdheepak kdheepak commented Oct 6, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-10-07 18:07 UTC

@kdheepak kdheepak force-pushed the update-concepts-rendering branch from 4e5163f to ad27254 Compare October 6, 2023 11:10
@kdheepak kdheepak requested review from joshka, orhun and Nickiel12 October 6, 2023 11:11
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.

I'm very much a top down (or outside-in) person when it comes to something like this. What do you think about flipping this article fully upside down (widget->frame->buffer->cell) (and then talking about the frame/terminal/backend last).

Why? Because I think it tells the person why they need to know the details before showing them that detail rather than the reverse.

src/concepts/rendering.md Outdated Show resolved Hide resolved
src/concepts/rendering.md Outdated Show resolved Hide resolved
src/concepts/rendering.md Outdated Show resolved Hide resolved
@kdheepak kdheepak force-pushed the update-concepts-rendering branch 2 times, most recently from 225c2e3 to 263f1e4 Compare October 7, 2023 01:11
@kdheepak kdheepak force-pushed the update-concepts-rendering branch from 263f1e4 to cbc21e9 Compare October 7, 2023 01:35
@kdheepak
Copy link
Contributor Author

kdheepak commented Oct 7, 2023

I removed the following content because I didn't know if it fit well in the description:

For example, when you create an instance of a `Paragraph` struct, any content you pass in is
converted to a [`Text`] struct.

```rust
pub struct Paragraph<'a> {
    /// The text to display
    text: Text<'a>,
    // --snip--
}
```

A `Text` struct consists of [`Line`]s which consists of [`Span`]s.

```rust
pub struct Text<'a> {
    pub lines: Vec<Line<'a>>,
}

pub struct Line<'a> {
    pub spans: Vec<Span<'a>>,
    pub alignment: Option<Alignment>,
}

pub struct Span<'a> {
    /// The content of the span as a Clone-on-write string.
    pub content: Cow<'a, str>,
    /// The style of the span.
    pub style: Style,
}
```

When you render an instance of `Paragraph`, the `Paragraph` widget is passed to a
[`Paragraph`'s `render`](https://github.com/ratatui-org/ratatui/blob/e5caf170c8c304b952cbff7499fd4da17ab154ea/src/widgets/paragraph.rs#L253-L268)
function along with the `Buffer`.

The implementation of `render` loops through the content of the `Text` struct in the `Paragraph`
widget and puts the content in the appropriate `Cell` locations using methods exposed on the
`Buffer` struct.


As another example, here's a snippet of the [`render` method for `Block`]:

```rust
    fn render(self, area: Rect, buf: &mut Buffer) {
        buf.set_style(area, self.style);
        let symbols = self.border_set;

        // Sides
        if self.borders.intersects(Borders::LEFT) {
            for y in area.top()..area.bottom() {
                buf.get_mut(area.left(), y)
                    .set_symbol(symbols.vertical_left)
                    .set_style(self.border_style);
            }
        }
        // --snip--

        // Corners
        if self.borders.contains(Borders::RIGHT | Borders::BOTTOM) {
            buf.get_mut(area.right() - 1, area.bottom() - 1)
                .set_symbol(symbols.bottom_right)
                .set_style(self.border_style);
        }
        // --snip--
    }
```

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.

Lots of comments at a very high level (there's some background discord chat on this that provides more context for anyone else reading).

Approving as it's much better than what was there and nothing in the comments is necessary to ship this.

Some general comments - there's a few sentences that start with nothing words ("basically what this", "essentially has a" and a lot of passive voice ("terminal.flush is called"). I think the ratio of code to text gets in the way of providing some good narrative info.

I think it would be worth presenting the methods from the caller's perspective rather than the ratatui code perspective (i.e. buf.set_string(...) rather than pub fn set_string ...)

src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
src/concepts/rendering-under-the-hood.md Outdated Show resolved Hide resolved
@kdheepak
Copy link
Contributor Author

kdheepak commented Oct 7, 2023

Thanks for the extremely helpful and detailed comments! I took another stab at it and pushed a new commit with a number of changes. I'm going to take a break and come back to it hopefully with a more fresh perspective.

@kdheepak kdheepak force-pushed the update-concepts-rendering branch 15 times, most recently from 40371e2 to ae8963b Compare October 7, 2023 08:03
@kdheepak kdheepak force-pushed the update-concepts-rendering branch from ae8963b to 4649d0f Compare October 7, 2023 08:09
@kdheepak kdheepak force-pushed the update-concepts-rendering branch from 10a9dab to 2375314 Compare October 7, 2023 17:26
@kdheepak kdheepak force-pushed the update-concepts-rendering branch from 0c8850f to 1789368 Compare October 7, 2023 17:55
@kdheepak
Copy link
Contributor Author

kdheepak commented Oct 7, 2023

@joshka I'm going to merge this. I'll address any additional comments you or others have in future PRs.

@kdheepak kdheepak merged commit c3cc400 into main Oct 7, 2023
2 checks passed
@kdheepak kdheepak deleted the update-concepts-rendering branch October 7, 2023 18:06
@joshka joshka mentioned this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants