feat(block): add Block::bordered

[Valentin271]

This avoid creating a block with no borders and then settings Borders::ALL. i.e.

- Block::default().borders(Borders::ALL);
+ Block::all_borders();

Searching on github, 1.3K use .borders(Borders:ALL) for a total of 1.5K .borders( usage.

Names I considered:

• all: too implicit, Block::all() in the wild isn't clear
• with_borders: would be confused with with_borders(b: Borders) i.e. with a parameter
• with_all_borders: the most explicit but too long imo
• borders_all: to match with Borders::ALL enum, nothing wrong with that, I just feel like Block::all_borders is easier to read and more natural

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (50374b2) 92.3% compared to head (2d82e0e) 92.3%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #736   +/-   ##
=====================================
  Coverage   92.3%   92.3%           
=====================================
  Files         48      48           
  Lines      14807   14817   +10     
=====================================
+ Hits       13673   13683   +10     
  Misses      1134    1134           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[orhun]

Intuitively I would say the correct name for this function would be with_borders or with_all_borders but I see why you chose all_borders. One issue is that it sounds like block.all_borders to me. How about Block::bordered?

[Valentin271]

How about Block::bordered?

Sounds great, I change it now.

[joshka]

It would be worth taking a look through the following to get some idea of how people are calling this outside and inside ratatui:

• https://github.com/search?q=ratatui+%22Block%3A%3Anew%22+NOT+repo%3Aratatui-org%2Fratatui+&type=code
• https://github.com/search?q=ratatui+%22Block%3A%3Anew%22+repo%3Aratatui-org%2Fratatui+&type=code

Perhaps it would be worth breaking backwards compatibility and introducing borders, and title params to the new function? This would be a two step change - deprecate new() (so users can replace with default()) and then change it to add the new params.

let block = Block::new("title", Borders:ALL);

P.S. .bordered is good and likely worth it regardless. This suggestion is just another option to consider.

[kdheepak]

There's enough ways to create titles (left align, center align, right align; multiple titles etc) that I don't think I'd want to do this.

let block = Block::new("title", Borders:ALL);

Also, as an example, Paragraph::new(text) always confuses me because I type Paragraph::new() first. For better or for worse, ergonomic rust is using builder methods.

But I could be convinced that Block::default() should do Block::new().borders(Border::ALL) instead.

[joshka]

For better or for worse, ergonomic rust is using builder methods.

I'm in strong disagreement with this statement. A constructor should accept the most common or most necessary items that a type needs. Builder methods are there for the things which are more optional than necessary. If 90% (unchecked number) of the usages of block set borders and a title, then that is what should be in the new() constructor.

There's enough ways to create titles (left align, center align, right align; multiple titles etc)

That's fine - title should be just fn new<T: Into<Line>(title: T, ...).

"Multiple titles" is a lower priority item as it falls in the 5% bucket.

Also, I've been rethinking the need for block::Title to have an alignment field. Line already has this, so it's unnecessary. Then if we look at position, this is a property of the block, not of the title itself, so that could just be Block::top_title() and Block::bottom_title(), which would remove the Title type entirely. It's 100% unnecessary and sits in a weird place in the widget hierarchy.

Paragraph::new(text) always confuses me because I type Paragraph::new()

Not sure what you mean by this. Paragraph is 100% useless without content, so why would it ever be good to have an empty constructor for it?

But I could be convinced that Block::default() should do Block::new().borders(Border::ALL) instead.

This would be a very bad idea as it would break the UI of every app out there.

[joshka]

https://rust-lang.github.io/api-guidelines/predictability.html#constructors-are-static-inherent-methods-c-ctor

[joshka]

Added an issue for fixing up title:

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

[kdheepak]

In languages like Python, all of these can build a Block:

Block(title = "title", borders = "all")
Block(borders = "all", title = "title")
Block(borders = "all")
Block()

In Rust, we have to use this pattern for the same benefits (not breaking future features, configuration of defaults, changing order in which you set fields, etc):

Block::new().title("title").borders(Borders::ALL)

which my understanding of is why it the convention for building structs.

Also, you can build a Block without a title using Block::new().borders(Borders::ALL). Along the same lines of the Paragraph point you pointed out, Block::new(title, borders) implies these are required and doesn't hint at other options (e.g. padding). It also is easier to end up breaking in the future if we decide to add a new field that is of equal importance to title and borders.

fwiw, I like that Paragraph::new takes text because as you said it is useless without content. But I also don't remember every new function in ratatui and their signatures. I don't want to change Paragraph::new or for example the constructor for Table::new, I was trying to give an example where what I type on autopilot doesn't work and the cognitive overhead of different ways of constructing something.

Also, it is generally way faster for me to type SomeWidget::new().<TAB> and wait for autocomplete to give me options than it is to type SomeWidget::new(<TAB>, then complete the required arguments, and then continue with SomeWidget::new(...).<TAB> to look for more options. I find it ergonomic when I get . autocomplete for all available optional configuration.

[kdheepak]

It'd be nice if GitHub allowed us to fork a part of thread into a separate issue :) We can continue this discussion off this PR.

[joshka]

It'd be nice if GitHub allowed us to fork a part of thread into a separate issue :) We can continue this discussion off this PR.

[joshka]

Discussion continues in #747

[joshka]

Reading https://rust-lang.github.io/api-guidelines/predictability.html#constructors-are-static-inherent-methods-c-ctor, with_all_borders() probably does make the most sense. Approving this though as I don't feel strongly about it in comparison to bordered().

[Valentin271]

I'd be in favor of with_all_borders if it wasn't for with_selected and such in states (https://github.com/ratatui-org/ratatui/blob/803a72df27190e273556e089e42036bfc001f003/src/widgets/list.rs#L81).

I like bordered too so I think I'll merge this later, waiting to see if anyone has any more objections.

[Valentin271]

Responding to relevant comments regarding breaking new and cognitive overhead in bulk:

If breaking changes weren't a concern, I would have changed the default of Borders to ALL, because this makes the most sense to me.
This would have changed Block::default to build a bordered block, which is a sane default IMO.

But I get @kdheepak's point where you basically want a clean sheet and add onto it. So I'd keep new to build the most clean block possible.

The opposite could do too, i.e. new builds bordered and default clean. That way we could have "autopilot builder" but also sane/easy default.

---

As for adding parameters to Block::new, I think block is used in too many different ways to do that. i.e. some use a simple string as title, some configure the title ..., whereas Borders is almost always simply ALL, making it boilerplate from my point of view.
Also that would make the constructor hard to read in user code. And agree to some extent with @kdheepak, it makes sense to build a block without title or border, whereas is doesn't to build a Paragraph without text (but you could and that's a good thing) :

implies these are required and doesn't hint at other options

---

Anyway I'm afraid this will be too much of a breaking change and thus not worth it. Which is why I choose a specialized bordered constructor as alternative.