[Merged by Bors] - Fix the `Size` helper functions using the wrong default value and improve the UI examples #7626

ickshonpe · 2023-02-11T11:53:02Z

Objective

Size::width sets the height field to Val::DEFAULT which is Val::Undefined, but the default for Size height is Val::Auto.
Size::height has the same problem, but with the width field.

The UI examples specify numeric values in many places where they could either be elided or replaced by composition of the Flex enum properties.

related: #7468
fixes: #6498

Solution

Change Size::width so it sets height to Val::AUTO and change Size::height so it sets width to Val::AUTO.
Added some tests so this doesn't happen again.

Changelog

Changed Size::width so it sets the height to Val::AUTO.
Changed Size::height so it sets the width to Val::AUTO.
Added tests to geometry.rs for Size and UiRect to ensure correct behaviour.
Simplified the UI examples. Replaced numeric values with the Flex property enums or elided them where possible, and removed the remaining use of auto margins.

Migration Guide

The Size::width constructor function now sets the height to Val::Auto instead of Val::Undefined.
The Size::height constructor function now sets the width to Val::Auto instead of Val::Undefined.

* fixed `Size::width` and `Size::height` using `Val::DEFAULT` instead of `Val::AUTO` * added tests to ensure correct defaults. * Simplified the UI examples. Replaced explicit values with the flex property enums where possible, removed values that can be ellided, and removed the remaining use of auto margins.

ickshonpe · 2023-02-11T11:58:01Z

Not sure whether the height/width fix needs a migration guide, as the functions are recent additions and it takes them to the natural behaviour you'd expect.

github-actions · 2023-02-11T14:34:55Z

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.

nicoburns

A couple of comments. Otherwise LGTM.

nicoburns · 2023-02-11T15:07:28Z

examples/ui/ui.rs

-                            margin: UiRect {
-                                left: Val::Auto,
-                                right: Val::Auto,
-                                ..default()
-                            },


Auto margins are not the default (or should not be). For margins, "auto" means "take up available space". The default margin is zero.

nicoburns · 2023-02-11T15:07:32Z

examples/ui/ui.rs

-                                                margin: UiRect {
-                                                    left: Val::Auto,
-                                                    right: Val::Auto,
-                                                    ..default()
-                                                },


Auto margins are not the default (or should not be). For margins, "auto" means "take up available space". The default margin is zero.

Oh I couldn't work out what you meant here for a moment.
The default for margins is Px(0.) as you'd expect.
In both cases, if you look above you can see that I replaced the auto margins with an AlignItems::Center on the parent.

Ah, gotcha.

No wait the default is still Val::Undefined, that fix is in the remove Val::Undefined PR and hasn't been merged yet. It's getting hard to keep track, really do need these tests.

nicoburns · 2023-02-11T15:08:22Z

crates/bevy_ui/src/geometry.rs

+    #[test]
+    fn test_size_width() {
+        let width = Val::Px(10.);
+
+        assert_eq!(
+            Size::width(width),
+            Size {
+                width,
+                ..Default::default()
+            }
+        );
+    }
+
+    #[test]
+    fn test_size_height() {
+        let height = Val::Px(7.);
+
+        assert_eq!(
+            Size::height(height),
+            Size {
+                height,
+                ..Default::default()
+            }
+        );
+    }


Seems to me that it might be better if these tested that you get Auto rather than ..Default::default(). If the default changed this test wouldn't catch the issue.

The purpose of these two tests is to ensure that the helper functions return the same result as setting the field directly and leaving the rest of the struct as default (whatever default it is) as a user would naturally assume.

I'll put in a separate assertion to ensure the default is Auto.

…e correct

alice-i-cecile · 2023-02-11T23:07:00Z

bors r+

…rove the UI examples (#7626) # Objective `Size::width` sets the `height` field to `Val::DEFAULT` which is `Val::Undefined`, but the default for `Size` `height` is `Val::Auto`. `Size::height` has the same problem, but with the `width` field. The UI examples specify numeric values in many places where they could either be elided or replaced by composition of the Flex enum properties. related: #7468 fixes: #6498 ## Solution Change `Size::width` so it sets `height` to `Val::AUTO` and change `Size::height` so it sets `width` to `Val::AUTO`. Added some tests so this doesn't happen again. ## Changelog Changed `Size::width` so it sets the `height` to `Val::AUTO`. Changed `Size::height` so it sets the `width` to `Val::AUTO`. Added tests to `geometry.rs` for `Size` and `UiRect` to ensure correct behaviour. Simplified the UI examples. Replaced numeric values with the Flex property enums or elided them where possible, and removed the remaining use of auto margins. ## Migration Guide The `Size::width` constructor function now sets the `height` to `Val::Auto` instead of `Val::Undefined`. The `Size::height` constructor function now sets the `width` to `Val::Auto` instead of `Val::Undefined`.

bors · 2023-02-11T23:22:31Z

Pull request successfully merged into main.

Build succeeded:

…rove the UI examples (bevyengine#7626) # Objective `Size::width` sets the `height` field to `Val::DEFAULT` which is `Val::Undefined`, but the default for `Size` `height` is `Val::Auto`. `Size::height` has the same problem, but with the `width` field. The UI examples specify numeric values in many places where they could either be elided or replaced by composition of the Flex enum properties. related: bevyengine#7468 fixes: bevyengine#6498 ## Solution Change `Size::width` so it sets `height` to `Val::AUTO` and change `Size::height` so it sets `width` to `Val::AUTO`. Added some tests so this doesn't happen again. ## Changelog Changed `Size::width` so it sets the `height` to `Val::AUTO`. Changed `Size::height` so it sets the `width` to `Val::AUTO`. Added tests to `geometry.rs` for `Size` and `UiRect` to ensure correct behaviour. Simplified the UI examples. Replaced numeric values with the Flex property enums or elided them where possible, and removed the remaining use of auto margins. ## Migration Guide The `Size::width` constructor function now sets the `height` to `Val::Auto` instead of `Val::Undefined`. The `Size::height` constructor function now sets the `width` to `Val::Auto` instead of `Val::Undefined`.

ickshonpe changed the title ~~changes:~~ Fix the Size helper functions using the wrong default value and improve the UI examples Feb 11, 2023

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A simple quality-of-life change that makes Bevy easier to use labels Feb 11, 2023

alice-i-cecile approved these changes Feb 11, 2023

View reviewed changes

alice-i-cecile added this to the 0.10 milestone Feb 11, 2023

alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 11, 2023

nicoburns suggested changes Feb 11, 2023

View reviewed changes

ickshonpe added 2 commits February 11, 2023 15:38

add assertions to tests checking that the Size and UiRect defaults ar…

a18f362

…e correct

cargo fmt --all

2eaf695

nicoburns approved these changes Feb 11, 2023

View reviewed changes

fixed test

fa76b63

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 Feb 11, 2023

bors bot changed the title ~~Fix the Size helper functions using the wrong default value and improve the UI examples~~ [Merged by Bors] - Fix the Size helper functions using the wrong default value and improve the UI examples Feb 11, 2023

bors bot closed this Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fix the `Size` helper functions using the wrong default value and improve the UI examples #7626

[Merged by Bors] - Fix the `Size` helper functions using the wrong default value and improve the UI examples #7626

ickshonpe commented Feb 11, 2023 •

edited

Loading

ickshonpe commented Feb 11, 2023 •

edited

Loading

github-actions bot commented Feb 11, 2023

nicoburns left a comment

nicoburns Feb 11, 2023

nicoburns Feb 11, 2023

ickshonpe Feb 11, 2023

nicoburns Feb 11, 2023

ickshonpe Feb 11, 2023 •

edited

Loading

nicoburns Feb 11, 2023

ickshonpe Feb 11, 2023

alice-i-cecile commented Feb 11, 2023

bors bot commented Feb 11, 2023

[Merged by Bors] - Fix the Size helper functions using the wrong default value and improve the UI examples #7626

[Merged by Bors] - Fix the Size helper functions using the wrong default value and improve the UI examples #7626

Conversation

ickshonpe commented Feb 11, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

ickshonpe commented Feb 11, 2023 • edited Loading

github-actions bot commented Feb 11, 2023

nicoburns left a comment

Choose a reason for hiding this comment

nicoburns Feb 11, 2023

Choose a reason for hiding this comment

nicoburns Feb 11, 2023

Choose a reason for hiding this comment

ickshonpe Feb 11, 2023

Choose a reason for hiding this comment

nicoburns Feb 11, 2023

Choose a reason for hiding this comment

ickshonpe Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

nicoburns Feb 11, 2023

Choose a reason for hiding this comment

ickshonpe Feb 11, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Feb 11, 2023

bors bot commented Feb 11, 2023

[Merged by Bors] - Fix the `Size` helper functions using the wrong default value and improve the UI examples #7626

[Merged by Bors] - Fix the `Size` helper functions using the wrong default value and improve the UI examples #7626

ickshonpe commented Feb 11, 2023 •

edited

Loading

ickshonpe commented Feb 11, 2023 •

edited

Loading

ickshonpe Feb 11, 2023 •

edited

Loading