Skip to content

Rename UiRect to Frame - #7969

Closed
ickshonpe wants to merge 5 commits into
bevyengine:mainfrom
ickshonpe:rename-uirect
Closed

Rename UiRect to Frame#7969
ickshonpe wants to merge 5 commits into
bevyengine:mainfrom
ickshonpe:rename-uirect

Conversation

@ickshonpe

@ickshonpe ickshonpe commented Mar 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Objective

UiRect is confusingly named (nothing it represents is a rectangle) and we have a consensus (as far as I can tell) that Frame is the best alternative.

If Frame seems ambiguous (not so much internally but with concepts from foreign UI apis), UiFrame could also be okay.

Alternative proposal: #7710

Changelog

  • UiRect is renamed to Frame

Migration Guide

UiRect has been renamed to Frame.

@ickshonpe
Rename UiRect to Frame
5240ab1 
# Objective

`UiRect` is confusingly named (nothing it represents is a rectangle) and we have a consensus (afaik) that `Frame` is the best alternative.

`UiFrame` could also be okay, if `Frame` seems ambiguous.

# Changelog
* `UiRect` is renamed to `Frame`

# Migration Guide
`UiRect` has been renamed to `Frame`.
ItsDoot
ItsDoot reviewed Mar 8, 2023
View reviewed changes
Comment thread CHANGELOG.md Outdated
ickshonpe added 3 commits March 8, 2023 10:52
@ickshonpe
renamed the test Frame_default_equals_const_default to `frame_defau…
9da5d31 
…lt_equals_const_default`
@ickshonpe
Rename UiRect to Frame
6e9ee6b 
# Objective

`UiRect` is confusingly named (nothing it represents is a rectangle) and we have a consensus (afaik) that `Frame` is the best alternative.

`UiFrame` could also be okay, if `Frame` seems ambiguous.

# Changelog
* `UiRect` is renamed to `Frame`

# Migration Guide
`UiRect` has been renamed to `Frame`.
@ickshonpe
Merge branch 'rename-uirect' of https://github.com/ickshonpe/bevy int…
14f33e1 
…o rename-uirect
@james7132 james7132 added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 8, 2023
@github-actions

github-actions Bot commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile

alice-i-cecile commented Mar 8, 2023

Copy link
Copy Markdown
Member

@TimJentzsch @nicoburns for review please :)

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 8, 2023
TimJentzsch
TimJentzsch approved these changes Mar 8, 2023
View reviewed changes

@TimJentzsch TimJentzsch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely better than UiRect:D

Comment thread crates/bevy_ui/src/geometry.rs
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 8, 2023
nicoburns
nicoburns approved these changes Mar 8, 2023
View reviewed changes

@nicoburns nicoburns left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. And I like the name Frame.

@hymm

hymm commented Mar 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Not sure about Frame seems like it'd be confused with the frame as in frames per second. I don't think UiFrame fixes the issue either.

@alice-i-cecile alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label Mar 9, 2023
@ickshonpe

ickshonpe commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

Not sure about Frame seems like it'd be confused with the frame as in frames per second. I don't think UiFrame fixes the issue either.

I can see what you mean, but won't it be obvious from the context that it means a geometric frame?

@james7132
james7132 requested a review from cart March 10, 2023 12:42
@alice-i-cecile

alice-i-cecile commented Mar 10, 2023

Copy link
Copy Markdown
Member

Cart's previously stated that "Bevy is treated as a global namespace", which I think that Frame runs a bit close to violating. I still think it's a much better name than UiRect. LayoutFrame would also be fine by me, but I'm increasingly leaning towards distinct types for margin/padding/border.

@ickshonpe

ickshonpe commented Mar 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Cart's previously stated that "Bevy is treated as a global namespace", which I think that Frame runs a bit close to violating. I still think it's a much better name than UiRect. LayoutFrame would also be fine by me, but I'm increasingly leaning towards distinct types for margin/padding/border.

Yeah I can see that in a global namespace Frame intuitively feels like it would be a type associated with animation and, although it might be workable now to have only one type, as taffy adds more features the margin, border, and padding interfaces will probably diverge even further making distinct types more desirable.

@ickshonpe

ickshonpe commented Mar 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I thought this would be an easily accepted non-controversial obvious improvement but I've come around to agreeing with hymm now.

@alice-i-cecile
Do you think we should close this one, and just concentrate on #7710 (or an incremental step towards it, maybe just a separate Margin type to start with)?

@ickshonpe

ickshonpe commented Mar 14, 2023

Copy link
Copy Markdown
Contributor Author

Thinking on it, maybe #7710 and #7569 don't make sense as distinct PRs and should be combined.

@alice-i-cecile

alice-i-cecile commented Mar 14, 2023

Copy link
Copy Markdown
Member

I think close this out and merge the other two :)

@ickshonpe ickshonpe mentioned this pull request Mar 24, 2023
Closed
@ickshonpe ickshonpe mentioned this pull request Sep 4, 2023
Open
@viridia

viridia commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

BTW, the word you are looking for is Insets. That's what it's called in iOS (UiEdgeInsets), Android (Insets) and Flutter (EdgeInsets).

@alice-i-cecile @ickshonpe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes
@cart cart Awaiting requested review from cart
+3 more reviewers
@ItsDoot ItsDoot ItsDoot left review comments
@nicoburns nicoburns nicoburns approved these changes
@TimJentzsch TimJentzsch TimJentzsch approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Milestone

0.11

Development

Successfully merging this pull request may close these issues.

8 participants

@ickshonpe @alice-i-cecile @hymm @viridia @nicoburns @ItsDoot @TimJentzsch @james7132