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

Remove block::Title and block::Position, add Block::top_title() and Block::bottom_title() for ergonomics #738

Open
1 of 6 tasks
joshka opened this issue Jan 5, 2024 · 17 comments
Assignees
Labels
bug Something isn't working

Comments

@joshka
Copy link
Member

joshka commented Jan 5, 2024

Description

block::Title and block::Position sit weirdly in the API. They can't be included in the prelude as the names would clash pretty annoyingly with many other type names in common use in apps, but also Title does not actually need to exist.

Title has 3 fields - content (Line), alignment and position. But Line already has an alignment field, so the title's alignment is superfluous. Position is a property of where the title is rendered, and so is a property of the Block, not the title itself. So if you remove the superfluous fields, that leaves Title just being a Line.

We should remove Title and Position from the API entirely.

Suggested fixes

Transition to having just:

let block = Block::default()
    .title(Line::from("foo")) // keep for backwards compatibility - equivalent top_title
    .top_title(Line::from("foo").alignment(Alignment::Center))
    .bottom_title(Line::from("bar").alignment(Aligment::Right))

To do this nicely, we'd need to:

  • add top_title<T: Into<Line>>(line: T) and similarly bottom_title(...)
  • mark Title as deprecated
  • mark Title::position() deprecated and point at Block::top_title() / Block::bottom::title
  • mark Position as deprecated`
  • change Block::titles to store top_titles and bottom_titles separately
  • remove title_on_bottom (already deprecated)
  • deprecate and then remove Block::title_position
  • fix up the rendering methods

It's possible that we could add a From<Title> for Line implementation that drops the position and make existing code still work except for things with bottom titles for a while too.

It's a good idea to split this up into a few PRs:

  • add the top_ and bottom_ fields and setters, update the rendering to use these instead of the current titles field (non breaking change)
  • Implement From<Title> for Line
  • Mark all deprecations and write up a transition guide for the change in BREAKING changes
  • Remove existing deprecated setter
  • Change Block::title to accept Into<Line> instead of Into<Title>
  • in a later release remove the deprecated stuff.

The external impact of this is 77 files, so fairly small right now:
https://github.com/search?q=ratatui+AND+block+AND+%22Title%3A%3Afrom%22+AND+%28NOT+repo%3Aratatui-org%2Fratatui%29+AND+%28NOT+is%3Afork%29&type=code

@joshka joshka added the bug Something isn't working label Jan 5, 2024
@joshka joshka changed the title Fix the Block Title ergonomics Fix the Block Title ergonomics (Remove block::Title and block::Position, add Block::top_title() and Block::bottom_title() Jan 5, 2024
@joshka joshka changed the title Fix the Block Title ergonomics (Remove block::Title and block::Position, add Block::top_title() and Block::bottom_title() Remove block::Title and block::Position, add Block::top_title() and Block::bottom_title() for ergonomics Jan 5, 2024
@kdheepak
Copy link
Collaborator

kdheepak commented Jan 6, 2024

I like this idea!

I think additionally Alignment should be Start, Center, End instead of Left, Center, Right. This would potentially allow us to use Alignment along a vertical axis too. For example in the future we may have:

let block = Block::default()
    .left_title(Line::from("xxx").alignment(Alignment::Center))
    .right_title(Line::from("xxx").alignment(Alignment::End))

This language would map more closely to CSS align-self property

https://developer.mozilla.org/en-US/docs/Web/CSS/align-self

If we wanted to be fully consistent, we would include Alignment::Stretch as well, which probably will only affect background color for us.

@Valentin271
Copy link
Member

I think it's a good idea too.

Could you make your list a task list so we have this as a tracking issue.

It's possible that we could add a From<Title> for Line implementation that drops the position and make existing code still work except for things with bottom titles for a while too.

This is a good idea IMO.

4. Change title to accept Into instead of Into<Title>

Could you clarify this one? I'm not sure what you're referencing. Perhaps you meant Change Block::title to accept Into<Line> instead of Into<Title> ?

@joshka
Copy link
Member Author

joshka commented Jan 6, 2024

Done. (markdown ate the angle brackets.)

joshka added a commit that referenced this issue Feb 9, 2024
This adds the ability to add titles to the top and bottom of a block
without having to use the `Title` struct (which will be removed in a
future release - likely v0.28.0).

Fixes a subtle bug if the title was created from a right aligned Line
and was also right aligned. The title would be rendered one cell too far
to the right.

```rust
Block::bordered()
    .title_top(Line::raw("A").left_aligned())
    .title_top(Line::raw("B").centered())
    .title_top(Line::raw("C").right_aligned())
    .title_bottom(Line::raw("D").left_aligned())
    .title_bottom(Line::raw("E").centered())
    .title_bottom(Line::raw("F").right_aligned())
    .render(buffer.area, &mut buffer);
// renders
"┌A─────B──────C",
"│             │",
"└D─────E──────F",
```

Addresses part of #738
joshka added a commit that referenced this issue Feb 9, 2024
This adds the ability to add titles to the top and bottom of a block
without having to use the `Title` struct (which will be removed in a
future release - likely v0.28.0).

Fixes a subtle bug if the title was created from a right aligned Line
and was also right aligned. The title would be rendered one cell too far
to the right.

```rust
Block::bordered()
    .title_top(Line::raw("A").left_aligned())
    .title_top(Line::raw("B").centered())
    .title_top(Line::raw("C").right_aligned())
    .title_bottom(Line::raw("D").left_aligned())
    .title_bottom(Line::raw("E").centered())
    .title_bottom(Line::raw("F").right_aligned())
    .render(buffer.area, &mut buffer);
// renders
"┌A─────B─────C┐",
"│             │",
"└D─────E─────F┘",
```

Addresses part of #738
joshka added a commit that referenced this issue Feb 9, 2024
This adds the ability to add titles to the top and bottom of a block
without having to use the `Title` struct (which will be removed in a
future release - likely v0.28.0).

Fixes a subtle bug if the title was created from a right aligned Line
and was also right aligned. The title would be rendered one cell too far
to the right.

```rust
Block::bordered()
    .title_top(Line::raw("A").left_aligned())
    .title_top(Line::raw("B").centered())
    .title_top(Line::raw("C").right_aligned())
    .title_bottom(Line::raw("D").left_aligned())
    .title_bottom(Line::raw("E").centered())
    .title_bottom(Line::raw("F").right_aligned())
    .render(buffer.area, &mut buffer);
// renders
"┌A─────B─────C┐",
"│             │",
"└D─────E─────F┘",
```

Addresses part of #738

<!-- Please read CONTRIBUTING.md before submitting any pull request. -->
@zjp-CN
Copy link

zjp-CN commented Feb 21, 2024

I just want to shout out block's title APIs are really inconvenient:

  • these APIs takes Self, so you can't modify them in place: this means you have to clone it (up to 216 bytes for Block type) to replace , like self.block = self.block.clone().title(title);
  • the biggest issue for me is the title elements are additive, and no way to remove or update them: this means you have to pass all the elements again by cloning themself and doing trivial type conversions...

For example, I'm writing a TUI for rustdoc, here's the popup:

  • at the right bottom of right block: I'd rather compute the offset and space on my own to genetate a new block. (Consider when your block is highly styled, you'd also have to clone the styles of itself and borders.)

block-bottom

// write the match result to the border bottom line
let text = format!(
    " got {} / total {} ",
    self.inner.total_len(),
    self.inner.lines.local.len()
);
self.border.render_with_bottom_right_text(buf, &text);

pub fn render_with_bottom_right_text(&self, buf: &mut Buffer, text: &str) {
   self.render(buf);
   let area = self.area;
   if let Some(offset) = (area.width as usize).checked_sub(2 + text.len()) {
      let x = area.x + offset as u16;
      let y = area.y + area.height - 1;
      render_line(Some((text, Style::new())), buf, x, y, text.len());
   }
}
  • on the top of left block: I tried using title APIs from ratatui, I just got more and more text from right to left...
    block-top

Actually, I mainly use Block only to get a bordered area with fixed (not variable) titles and a computed inner area.

Thanks for making decisions to improve title APIs ❤️

@joshka
Copy link
Member Author

joshka commented Mar 31, 2024

these APIs takes Self, so you can't modify them in place: this means you have to clone it (up to 216 bytes for Block type) to replace , like self.block = self.block.clone().title(title);

216 bytes doesn't matter (unless you're writing a TUI to run on a microcontroller). At apple prices for RAM that's $0.0000027

Methods taking self and returning Self is pretty much the standard way all setter methods work in Ratatui (and won't change due to this). We inherited this pattern from tui-rs and intend to keep consistent with the approach at least in the foreseeable future. The pattern has a name: Builder-lite.

That said we could consider setting all the titles at once, perhaps something like:

let block = Block::new().top_titles([Line::from("left").left_aligned(), Line::from("center").centered(),...);

There's possibly an idea where we could change the pattern to a more canonical builder pattern and gate that behind some feature flag, but it's something we'd want to do for every setter in the entire library, and it would be a huge mindset change that would require lots of docs and patience to get right.

@zjp-CN
Copy link

zjp-CN commented Mar 31, 2024

I happen to remember a general solution in Rust when you're stuck with temporary ownership taking like self.block = self.block.clone().title(title). It's replace_with.

self.block = replace_with::replace_with(
    &mut self.block, // take &mut, thus assignment works
    || custom_block_default(),
    |block| block.title(title) // block is owned, and no need to clone it
)

With this solution, we don't have to modify the current builder-lite APIs.

The reason things like replace_with are not in std is around panicking in the closure, thus there are variants of APIs as replace_with*.

@TadoTheMiner
Copy link
Contributor

I wanted to implement

impl<'a> From<Title<'a>> for Line<'a>

However, there is already

impl<'a, T> From<T> for Title<'a>
where
    T: Into<Line<'a>>

Which results in E0119 - conflicting trait implementations. I discussed the issue in the Rust community discord server, and they said we should make a breaking change immediately.
grafik

@joshka
Copy link
Member Author

joshka commented Mar 31, 2024

Yeah, there's going to be some breakage no matter what we do on this. It's worth always starting at the design stage, thinking about how each usage breaks as each increment part of the problem is released (and check the impact using github searches for code that calls the breaking methods). If you haven't done this before, it's worth doing the exercise in full detail rather than assuming the deprecation steps I listed above are correct (I get things wrong sometimes, and it was a while since I suggested the approach to deprecate).

Our goal is as much as possible to let people have ~2 major releases of notification of breaking changes whenever possible (so that they can generally upgrade to a release and be guided by build warnings about what changes need to be made in their code).

@TadoTheMiner
Copy link
Contributor

Should I make a separate branch and wait till we make a major release? Or it's too early? How often are major releases made? I mean the earlier I make the PR the more merging I will have to do.

@joshka
Copy link
Member Author

joshka commented Mar 31, 2024

Should I make a separate branch and wait till we make a major release? Or it's too early? How often are major releases made? I mean the earlier I make the PR the more merging I will have to do.

Don't stress too much about it. A PR is a branch. The work to merge / integrate this will be the same order of magnitude whether there's an formal branch taken and tracked at a particular point or not.

@TadoTheMiner
Copy link
Contributor

Okay assign me to this issue please. (I've already removed it, now just to edit docs and tests)

@joshka
Copy link
Member Author

joshka commented Apr 1, 2024

Can you pleas make a note of the plan you're following for this so we're on the same page. As a clarification, this is what I meant when I wrote the following:

It's worth always starting at the design stage, thinking about how each usage breaks as each increment part of the problem is released (and check the impact using github searches for code that calls the breaking methods)

@TadoTheMiner
Copy link
Contributor

I've done all of the checklisted stuff in #1012 since the changes can't be made separately.

@TadoTheMiner
Copy link
Contributor

@joshka The signature of title_top is the same as top_title<T: Into<Line>>(line: T), except that line is called title, same applies to bottom_title. So there's no reason to deprecate them, we should modify them to push to top_titles and bottom_titles.

@joshka
Copy link
Member Author

joshka commented May 6, 2024

Hey, sorry for letting this one drop. I've had other priorities on my time and this hasn't bubbled up to the top yet. I'll hopefully get back to looking at this soon.

@LucasPickering
Copy link

Heads up, I just opened #1090 which, if accepted, would impact this issue. In that issue I described a solution that accomplishes the part of the goal of this PR (removing Title) while leaving Position in place because of its flexibility.

@TadoTheMiner
Copy link
Contributor

In my opinion title should be deprecated aswell , because it's name is ambiguous, and people can just find and replace title( with title_top(, there are several utilities to do so. Instead, we should add a title_at_position function that accepts Into, and an argument of TitlePosition (located in block.rs), which will call title_top and title_bottom respectively. I've already implemented it in my deprecated title PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants