feat(layout): add Rect::split method

[joshka]

This method splits a Rect and returns a fixed-size array of the
resulting Rects. This allows the caller to use array destructuring
to get the individual Rects.

use Constraint::*;
let layout = &Layout::vertical([Length(1), Min(0)]);
let [top, main] = area.split(&layout);

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (803a72d) 92.3% compared to head (0155df8) 92.3%.

❗ Current head 0155df8 differs from pull request most recent head f2eaac9. Consider uploading reports for the commit f2eaac9 to get more accurate results

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #729   +/-   ##
=====================================
  Coverage   92.3%   92.3%           
=====================================
  Files         55      55           
  Lines      14811   14830   +19     
=====================================
+ Hits       13674   13693   +19     
  Misses      1137    1137           

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

[joshka]

I want two approvals on this one - mainly for working out whether the naming makes sense.

[kdheepak]

In terms of names, I would have preferred split_into() if area was stored in the layout, i.e.

let [top, main] =
    Layout::vertical([Constraint::Length(1), Constraint::Min(0)])
    .split_array(area);

versus

let [top, main] =
    Layout::vertical([Constraint::Length(1), Constraint::Min(0)])
    .area(area)
    .split_into();

With the functionality as is, I think the name split_array() is great!

[kdheepak]

I have a small preference for .split_into_array(area) as it would be more descriptive and representative of what the function does, i.e.

layout.split(area)
    .to_vec()
    .try_into()
    .unwrap()

is replaced by this:

layout.split_into_array(area)

[Valentin271]

I like split_into_array as a descriptive name but perhaps it feels too verbose.

it feels too verbose

Once merged, I assume this will be the main splitting method used.

[kdheepak]

Maybe it is worth introducing a breaking change for this and replacing .split(area) with this?

[joshka]

I'm 👎 on breaking .spilt - 1.1K calls

• https://github.com/search?q=ratatui+layout%3A%3A+.split%28+lang%3Arust+NOT+repo%3Aratatui-org%2Fratatui&type=code

But what about split_area() as a compromise on verbosity?

Once merged, I assume this will be the main splitting method used.

There's a PR that shows how the code would look using this #731

[kdheepak]

I like in order of preference:

1. let [a,b,c] = layout.split_into(area): it's as short as we can get while being descriptive, it hints at what's going on inside the function (i.e. try_into)
2. let [a,b,c] = layout.split_into_array(area): it's descriptive but verbose for something that might be the default way people use this function
3. let [a,b,c] = layout.split_array(area): it's short but I don't think I'd be able to guess what is going on in the function? It could be misinterpreted as split the argument which is an array which is why I like .split_into_array(area) more than this
4. let [a,b,c] = layout.split_area(area): it's short but repeats the argument name in the function name so it falls down the list

[joshka]

I think 1,2, and 3 are all problematic because they imply that the area is the (linguistic) object associated with the verb (split_into, split_into_array, split_array). Area is not the object of the sentence its the subject. Only a bare verb, or a verb phrase that repeats the subject communicates this properly.

Put in a sentence we want:

• The area is split by the layout
  Not:
• The layout is split into the area
• The layout is split into an array that is the area
• The layout splits an array that is the area

[joshka]

Chatting on Discord - @kdheepak made the observation that this then leads to:

area.split(Layout::horizontal([constraints]));

which I've added.
This now also includes the helper methods too.

[joshka]

Needs:

• docs
• updated changelog message
• examples in website to be changed to use this approach

[kdheepak]

This looks good to me!

But maybe useful to get one more review for feedback on area.split(&layout)?

[joshka]

This looks good to me!

But maybe useful to get one more review for feedback on area.split(&layout)?

Yep - definitely want one on this. I'm going to rebase and rewrite the commit for the CHANGELOG also.

[joshka]

rebased and commit message / PR text updated.
There's a clippy failure that is fixed in #743

[Valentin271]

Would be nice if we could write

area.split([Constraint::Length(10), Constraint::Length(10)]);
area.split([10, 10]);

Not too sure about the second one but the first one is definitely something I'd like.

Approving as this can be part of another PR.

[joshka]

Would be nice if we could write

area.split([Constraint::Length(10), Constraint::Length(10)]);
area.split([10, 10]);

Not too sure about the second one but the first one is definitely something I'd like.

Approving as this can be part of another PR.

Direction to split is necessary, so perhaps Rect::split_horizontal():

fn split_horizontal<const N: usize, T: IntoIterator<Item = Into<Constraint>>(self, constraints: T) -> [Rect; N] {
    self.split(Layout::horizontal(constraints));
}

But at that point we're just losing a single word (Layout), which is a good one to keep as it makes it a little more obvious what the [1,2,3] does. I like there to be a solid obvious way to do something. Do you think having both split(layout) and split_horizontal(constraints) might add one too many ways that would make it difficult to choose which method is the right one to use?

[Valentin271]

it makes it a little more obvious what the [1,2,3] does

I wasn't too sure about that, let's forget that for now.

Do you think having both split(layout) and split_horizontal(constraints) might add one too many ways that would make it difficult to choose which method is the right one to use?

Probably yes, this is confusing.

---

I though of my suggestion as

fn split<const N: usize, L: Into<Layout>>(self, layout: L)

That way you can still write .split(layout) which is the "solid obvious way", and is good if you want to store layout (for configuration perhaps). And for quick and easy split use Into.

That would require implementing From<Vec<Constraint>> for Layout or similar. But overall it makes sense to convert a bunch of constraint to a layout.

Direction to split is necessary

As for direction I'd just use the default one. Use layout if you want something else.

[joshka]

I'd prefer to avoid defaulting the direction implicitly as it makes the code difficult to read unless you know what that default is. Happy to do it with `default(), but it should be explicit otherwise.

[Valentin271]

Alright lets leave it as is for now. On second thought after reading the updated examples it seems fine to me.