feat(Rect): add offset method

[Valentin271]

Description

As discussed here.

Add inset and offset methods to respectively adjust a Rect size and move it.
This is common logic in renders and apps.

Implementation choices

I used generics to allow direct calculation in the call, like here. This just avoids having to wrap calculations in parenthesis and calling into().
I used two different generics to allow giving different types (e.g. u16 calculation and i32 literal).

A signed int allows to move the Rect forward or backward.

I used an i64 i32 as parameter to allow moving x and y from their minimal value to their maximal value (0 -> u16::MAX), even though this is something that very rarely occur (if at all).
An i32 i16 would not be enough (reference).

Due to the preceding choice, we're casting an i64 i32 to an u16. If the calculation could not be converted back to u16, values will wrap around. This is documented, I think this is legitimate since this case should not happen in the first place, we could provide variants later if needed (saturating_..., checked_...).
Alternatives are:

• saturating (seems good, harder to implement)
• returning Option (easy to implement, kind of breaks builder pattern, unpleasant to use)
• panic (easy to implement)

Let me know what you think. IMO, this is basically choosing which behavior should be the default as we could provide variant later.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fe632d7) 90.7% compared to head (323bde7) 90.7%.

❗ Current head 323bde7 differs from pull request most recent head 67acc04. Consider uploading reports for the commit 67acc04 to get more accurate results

Files	Patch %	Lines
src/layout/rect/offset.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #533   +/-   ##
=====================================
  Coverage   90.7%   90.7%           
=====================================
  Files         41      42    +1     
  Lines      12130   12170   +40     
=====================================
+ Hits       11003   11042   +39     
- Misses      1127    1128    +1     

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

[joshka]

Can you explain why "An i32 would not be enough"? isn't (0u16 as i32 + i32::MAX) > (u16::MAX as i32) (and likewise the subtraction.

Regarding wrap vs saturating - consider what Rect::new(0,0, 10, 10).inset(-1, -1) results in.

On thinking a bit more about this and seeing the implementation, I wonder if my problem is just that inner() taking a Margin is not quite flexible enough. I suspect there's a few cases where we want an inset rectangle to be on the top or left instead of the bottom/right. Could this be better expressed as inset amounts for left,right,top,bottom (if so offset() would just then become inset(x,y, 0, 0)). Alternatively perhaps defining a struct Offset { left, right, top, bottom } might be the more idiomatic rust approach (with a conversion from a tuple to make calling this easy) - I'm not sure what the right order of this would be (left, top, right, bottom), (top, left, bottom, right), ...?

[Valentin271]

Can you explain why "An i32 would not be enough"? isn't (0u16 as i32 + i32::MAX) > (u16::MAX as i32) (and likewise the subtraction.

Oops my bad, it is. For some reason I red the line for u32 instead of u16 in the table. Fixing that.

Regarding wrap vs saturating - consider what Rect::new(0,0, 10, 10).inset(-1, -1) results in.

That's my problem ^^, I don't feel like one is more relevant / intuitive / useful than the other. You may want to saturate to not go out of the terminal ; you may want to loop around the terminal for some sort of banner.

---

inner with Margin is not flexible enough because you couldn't put negative margin to enlarge the Rect (which is good, it would be confusing with the method name). Moreover Margin is only horizontal / vertical. I feel like inset should be more precise.

Offset struct could be nice, all manipulation to Rect could then be done with it. I think the order should be as in HTML, conversions from tuples could use this syntax too.

[Valentin271]

898769c contains a Proof-of-Concept for the Offset struct. I think this is the right way, but still need some work. Just pushing the commit for early feedbacks.

The fields order in Offset seems fine to me. However, conversion from tuples as in HTML wasn't too useful here. Maybe keep it for consistency for newcomers? Otherwise, nothing would make more sense I think.
Offset needs more constructors (e.g. uniform, horizontal, vertical, zero, top, left ...)

Also, it can we weird to some, positive numbers move a border to the right (negative to the left). So something like that

Offset {
  top: 1,
  right: 0,
  bottom: 1,
  left: 0

would actually move the Rect and not reduce or enlarge it by 2.
Should field means a movement to exterior/interior of the Rect instead of right/left? The original solution seems better to me.

---

offset could call resize internally like so

self.resize((y.into(), x.into()))

but I feel like this is confusing and somewhat less understandable.

[joshka]

That's my problem ^^, I don't feel like one is more relevant / intuitive / useful than the other. You may want to saturate to not go out of the terminal ; you may want to loop around the terminal for some sort of banner.

The rect struct doesn't know the size of the terminal. Right now this wraps using integer wrap to u16:MAX, not the terminal size.

Can you write a test that shows what a Rect at 0,0 wraps to?

Put another way, does Rect { x: u16:MAX, y: u16::MAX, width: 10, height: 10 } have any meaning in the context of a TUI app? Where would it ever be useful?

I suspect the right approach for this is to always saturate, though I'm not sure if we should make this explicit in the name or not.

[joshka]

Also, it can we weird to some, positive numbers move a border to the right (negative to the left). So something like that

Offset {
  top: 1,
  right: 0,
  bottom: 1,
  left: 0

would actually move the Rect and not reduce or enlarge it by 2.
Should field means a movement to exterior/interior of the Rect instead of right/left? The original solution seems better to me.

Perhaps we could consider naming this move() / Move instead of offset() / Offset?

A small heads up - I just slapped together a refactor in #534 that extracts Rect out into layout/rect.rs to make it easier to just look at Rect related stuff instead of all the complexity of the layout module.

[joshka]

I think it's probably more useful to saturate any values rather than wrapping, and consider that we should end up with a Rect that is useful to the app (i.e. if the x value become u16::MAX then the width must become 0)

Would it be the worst thing to consider making these 3 methods:

• move(amount: Move { x, y } ) // just changes the x, y values
• resize(amount: Size { width, height } ) // just changes the width, height values
• offset(amount: Offset { left, top, right, bottom }) // can change all

The rect returned in each case should be valid (x,y <= u16::MAX and x+width,y+height <= u16::MAX)

[Valentin271]

I added resize, offset and relocate (see this comment on name) methods.

I am unsure whether structs are really needed for relocate and resize. I don't see the clear benefits over two simple integers.

PS. I'll add unit tests once the content is validated

[joshka]

Ok, so reading the current iteration, what if instead we end up with:

struct Offset { x: i32, y: i32 }
/// moves the rect by the given offset
fn offset(offset: Offset) {}

I like using a struct here rather than two values as it avoids the situation where people inadvertently flip the two.

Resize could have some added complexity. Do the numbers in the resize refer to the delta or the final value (resize_by / resize_to). How about we just do move in this PR and split the resize to another one?

[Valentin271]

This seems like a good first step yes. This also enables us to wait for more feedback on Rect's usage.

[joshka]

Looking good.

I think we probably still need to define some behavior around what to do when we hit the max though. Say we have a 100x100 width rect, I think it's probably better to not move it beyond u16::MAX - 100 instead in order to keep the height and width the same and ensure the bounds are within u16.

This is unlikely to be ever hit - but in the odd event someone with a display the size of a house does hit it, I'd prefer to render badly than to have their app crash due to trying to address a part of a rect that doesn't exist.

Can you add the behavior and a test for this please?

[Valentin271]

I've added that quickly, but then I think it is inconsistent with Rect::new which does not check if the rect goes outside u16::MAX.

I would have added it to new too but with the 'keep the ratio' thing its a bit harder. Plus it would probably be a breaking change (one we can ignore though as no one will realistically run into it).

I wanted to hear your opinion on it before implementing. We would also have to perform this check every time a value is modified. But ultimately that would be best imo, keep the Rect inside u16 in any case (area, right, bottom).

[joshka]

I've added that quickly, but then I think it is inconsistent with Rect::new which does not check if the rect goes outside u16::MAX.

I would have added it to new too but with the 'keep the ratio' thing its a bit harder. Plus it would probably be a breaking change (one we can ignore though as no one will realistically run into it).

I wanted to hear your opinion on it before implementing. We would also have to perform this check every time a value is modified. But ultimately that would be best imo, keep the Rect inside u16 in any case (area, right, bottom).

TBH I don't understand the rationale for the ratio code (I haven't dug into the history of it), so I'm not really sure.

[joshka]

LGTM

[joshka]

Message for CHANGELOG:

---

feat(Rect): add offset method

The offset method creates a new Rect that is moved by the amount
specified in the x and y direction. These values can be positive or
negative. This is useful for manual layout tasks.

let rect = area.offset(Offset { x: 10, y: -10 });

[Valentin271]

TBH I don't understand the rationale for the ratio code (I haven't dug into the history of it), so I'm not really sure.

Lets just merge this PR as is then.

We can then find out the rationale for the ratio code and streamline the desired behavior on all Rect methods

[joshka]

TBH I don't understand the rationale for the ratio code (I haven't dug into the history of it), so I'm not really sure.

Lets just merge this PR as is then.

We can then find out the rationale for the ratio code and streamline the desired behavior on all Rect methods

Yup - was just checking with the other devs that offset was fine :)

[joshka]

Thanks again for this PR.

[Valentin271]

You're welcome! Glad to be participating in such an amazing project!