Skip to content

Commit a2bd93a

Browse files
ickshonpeickshonpe
authored
Make GridPlacement's fields non-zero and add accessor functions. (#9486)
# Objective * There is no way to read the fields of `GridPlacement` once set. * Values of `0` for `GridPlacement`'s fields are invalid but can be set. * A non-zero representation would be half the size. fixes #9474 ## Solution * Add `get_start`, `get_end` and `get_span` accessor methods. * Change`GridPlacement`'s constructor functions to panic on arguments of zero. * Use non-zero types instead of primitives for `GridPlacement`'s fields. --- ## Changelog `bevy_ui::ui_node::GridPlacement`: * Field types have been changed to `Option<NonZeroI16>` and `Option<NonZeroU16>`. This is because zero values are not valid for `GridPlacement`. Previously, Taffy interpreted these as auto variants. * Constructor functions for `GridPlacement` panic on arguments of `0`. * Added accessor functions: `get_start`, `get_end`, and `get_span`. These return the inner primitive value (if present) of the respective fields. ## Migration Guide `GridPlacement`'s constructor functions no longer accept values of `0`. Given any argument of `0` they will panic with a `GridPlacementError`.
1 parent 394e2b0 commit a2bd93a

File tree

2 files changed

+127
-29
lines changed

2 files changed

+127
-29
lines changed

‎crates/bevy_ui/src/layout/convert.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -278,8 +278,8 @@ impl From<GridAutoFlow> for taffy::style::GridAutoFlow {
278278

279279
impl From<GridPlacement> for taffy::geometry::Line<taffy::style::GridPlacement> {
280280
fn from(value: GridPlacement) -> Self {
281-
let span = value.span.unwrap_or(1).max(1);
282-
match (value.start, value.end) {
281+
let span = value.get_span().unwrap_or(1);
282+
match (value.get_start(), value.get_end()) {
283283
(Some(start), Some(end)) => taffy::geometry::Line {
284284
start: style_helpers::line(start),
285285
end: style_helpers::line(end),

‎crates/bevy_ui/src/ui_node.rs

Lines changed: 125 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ use bevy_render::{
1010
use bevy_transform::prelude::GlobalTransform;
1111
use serde::{Deserialize, Serialize};
1212
use smallvec::SmallVec;
13-
use std::ops::{Div, DivAssign, Mul, MulAssign};
13+
use std::{
14+
num::{NonZeroI16, NonZeroU16},
15+
ops::{Div, DivAssign, Mul, MulAssign},
16+
};
1417
use thiserror::Error;
1518

1619
/// Describes the size of a UI node
@@ -1470,100 +1473,145 @@ impl From<RepeatedGridTrack> for Vec<RepeatedGridTrack> {
14701473
/// <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout/Line-based_Placement_with_CSS_Grid>
14711474
pub struct GridPlacement {
14721475
/// The grid line at which the item should start. Lines are 1-indexed. Negative indexes count backwards from the end of the grid. Zero is not a valid index.
1473-
pub(crate) start: Option<i16>,
1476+
pub(crate) start: Option<NonZeroI16>,
14741477
/// How many grid tracks the item should span. Defaults to 1.
1475-
pub(crate) span: Option<u16>,
1476-
/// The grid line at which the node should end. Lines are 1-indexed. Negative indexes count backwards from the end of the grid. Zero is not a valid index.
1477-
pub(crate) end: Option<i16>,
1478+
pub(crate) span: Option<NonZeroU16>,
1479+
/// The grid line at which the item should end. Lines are 1-indexed. Negative indexes count backwards from the end of the grid. Zero is not a valid index.
1480+
pub(crate) end: Option<NonZeroI16>,
14781481
}
14791482

14801483
impl GridPlacement {
14811484
pub const DEFAULT: Self = Self {
14821485
start: None,
1483-
span: Some(1),
1486+
span: Some(unsafe { NonZeroU16::new_unchecked(1) }),
14841487
end: None,
14851488
};
14861489

14871490
/// Place the grid item automatically (letting the `span` default to `1`).
14881491
pub fn auto() -> Self {
1489-
Self {
1490-
start: None,
1491-
end: None,
1492-
span: Some(1),
1493-
}
1492+
Self::DEFAULT
14941493
}
14951494

14961495
/// Place the grid item automatically, specifying how many tracks it should `span`.
1496+
///
1497+
/// # Panics
1498+
///
1499+
/// Panics if `span` is `0`
14971500
pub fn span(span: u16) -> Self {
14981501
Self {
14991502
start: None,
15001503
end: None,
1501-
span: Some(span),
1504+
span: try_into_grid_span(span).expect("Invalid span value of 0."),
15021505
}
15031506
}
15041507

15051508
/// Place the grid item specifying the `start` grid line (letting the `span` default to `1`).
1509+
///
1510+
/// # Panics
1511+
///
1512+
/// Panics if `start` is `0`
15061513
pub fn start(start: i16) -> Self {
15071514
Self {
1508-
start: Some(start),
1509-
end: None,
1510-
span: Some(1),
1515+
start: try_into_grid_index(start).expect("Invalid start value of 0."),
1516+
..Self::DEFAULT
15111517
}
15121518
}
15131519

15141520
/// Place the grid item specifying the `end` grid line (letting the `span` default to `1`).
1521+
///
1522+
/// # Panics
1523+
///
1524+
/// Panics if `end` is `0`
15151525
pub fn end(end: i16) -> Self {
15161526
Self {
1517-
start: None,
1518-
end: Some(end),
1519-
span: Some(1),
1527+
end: try_into_grid_index(end).expect("Invalid end value of 0."),
1528+
..Self::DEFAULT
15201529
}
15211530
}
15221531

15231532
/// Place the grid item specifying the `start` grid line and how many tracks it should `span`.
1533+
///
1534+
/// # Panics
1535+
///
1536+
/// Panics if `start` or `span` is `0`
15241537
pub fn start_span(start: i16, span: u16) -> Self {
15251538
Self {
1526-
start: Some(start),
1539+
start: try_into_grid_index(start).expect("Invalid start value of 0."),
15271540
end: None,
1528-
span: Some(span),
1541+
span: try_into_grid_span(span).expect("Invalid span value of 0."),
15291542
}
15301543
}
15311544

15321545
/// Place the grid item specifying `start` and `end` grid lines (`span` will be inferred)
1546+
///
1547+
/// # Panics
1548+
///
1549+
/// Panics if `start` or `end` is `0`
15331550
pub fn start_end(start: i16, end: i16) -> Self {
15341551
Self {
1535-
start: Some(start),
1536-
end: Some(end),
1552+
start: try_into_grid_index(start).expect("Invalid start value of 0."),
1553+
end: try_into_grid_index(end).expect("Invalid end value of 0."),
15371554
span: None,
15381555
}
15391556
}
15401557

15411558
/// Place the grid item specifying the `end` grid line and how many tracks it should `span`.
1559+
///
1560+
/// # Panics
1561+
///
1562+
/// Panics if `end` or `span` is `0`
15421563
pub fn end_span(end: i16, span: u16) -> Self {
15431564
Self {
15441565
start: None,
1545-
end: Some(end),
1546-
span: Some(span),
1566+
end: try_into_grid_index(end).expect("Invalid end value of 0."),
1567+
span: try_into_grid_span(span).expect("Invalid span value of 0."),
15471568
}
15481569
}
15491570

15501571
/// Mutate the item, setting the `start` grid line
1572+
///
1573+
/// # Panics
1574+
///
1575+
/// Panics if `start` is `0`
15511576
pub fn set_start(mut self, start: i16) -> Self {
1552-
self.start = Some(start);
1577+
self.start = try_into_grid_index(start).expect("Invalid start value of 0.");
15531578
self
15541579
}
15551580

15561581
/// Mutate the item, setting the `end` grid line
1582+
///
1583+
/// # Panics
1584+
///
1585+
/// Panics if `end` is `0`
15571586
pub fn set_end(mut self, end: i16) -> Self {
1558-
self.end = Some(end);
1587+
self.end = try_into_grid_index(end).expect("Invalid end value of 0.");
15591588
self
15601589
}
15611590

15621591
/// Mutate the item, setting the number of tracks the item should `span`
1592+
///
1593+
/// # Panics
1594+
///
1595+
/// Panics if `span` is `0`
15631596
pub fn set_span(mut self, span: u16) -> Self {
1564-
self.span = Some(span);
1597+
self.span = try_into_grid_span(span).expect("Invalid span value of 0.");
15651598
self
15661599
}
1600+
1601+
/// Returns the grid line at which the item should start, or `None` if not set.
1602+
pub fn get_start(self) -> Option<i16> {
1603+
self.start.map(NonZeroI16::get)
1604+
}
1605+
1606+
/// Returns the grid line at which the item should end, or `None` if not set.
1607+
pub fn get_end(self) -> Option<i16> {
1608+
self.end.map(NonZeroI16::get)
1609+
}
1610+
1611+
/// Returns span for this grid item, or `None` if not set.
1612+
pub fn get_span(self) -> Option<u16> {
1613+
self.span.map(NonZeroU16::get)
1614+
}
15671615
}
15681616

15691617
impl Default for GridPlacement {
@@ -1572,6 +1620,29 @@ impl Default for GridPlacement {
15721620
}
15731621
}
15741622

1623+
/// Convert an `i16` to `NonZeroI16`, fails on `0` and returns the `InvalidZeroIndex` error.
1624+
fn try_into_grid_index(index: i16) -> Result<Option<NonZeroI16>, GridPlacementError> {
1625+
Ok(Some(
1626+
NonZeroI16::new(index).ok_or(GridPlacementError::InvalidZeroIndex)?,
1627+
))
1628+
}
1629+
1630+
/// Convert a `u16` to `NonZeroU16`, fails on `0` and returns the `InvalidZeroSpan` error.
1631+
fn try_into_grid_span(span: u16) -> Result<Option<NonZeroU16>, GridPlacementError> {
1632+
Ok(Some(
1633+
NonZeroU16::new(span).ok_or(GridPlacementError::InvalidZeroSpan)?,
1634+
))
1635+
}
1636+
1637+
/// Errors that occur when setting contraints for a `GridPlacement`
1638+
#[derive(Debug, Eq, PartialEq, Clone, Copy, Error)]
1639+
pub enum GridPlacementError {
1640+
#[error("Zero is not a valid grid position")]
1641+
InvalidZeroIndex,
1642+
#[error("Spans cannot be zero length")]
1643+
InvalidZeroSpan,
1644+
}
1645+
15751646
/// The background color of the node
15761647
///
15771648
/// This serves as the "fill" color.
@@ -1719,6 +1790,7 @@ impl Default for ZIndex {
17191790

17201791
#[cfg(test)]
17211792
mod tests {
1793+
use crate::GridPlacement;
17221794
use crate::ValArithmeticError;
17231795

17241796
use super::Val;
@@ -1867,4 +1939,30 @@ mod tests {
18671939
fn default_val_equals_const_default_val() {
18681940
assert_eq!(Val::default(), Val::DEFAULT);
18691941
}
1942+
1943+
#[test]
1944+
fn invalid_grid_placement_values() {
1945+
assert!(std::panic::catch_unwind(|| GridPlacement::span(0)).is_err());
1946+
assert!(std::panic::catch_unwind(|| GridPlacement::start(0)).is_err());
1947+
assert!(std::panic::catch_unwind(|| GridPlacement::end(0)).is_err());
1948+
assert!(std::panic::catch_unwind(|| GridPlacement::start_end(0, 1)).is_err());
1949+
assert!(std::panic::catch_unwind(|| GridPlacement::start_end(-1, 0)).is_err());
1950+
assert!(std::panic::catch_unwind(|| GridPlacement::start_span(1, 0)).is_err());
1951+
assert!(std::panic::catch_unwind(|| GridPlacement::start_span(0, 1)).is_err());
1952+
assert!(std::panic::catch_unwind(|| GridPlacement::end_span(0, 1)).is_err());
1953+
assert!(std::panic::catch_unwind(|| GridPlacement::end_span(1, 0)).is_err());
1954+
assert!(std::panic::catch_unwind(|| GridPlacement::default().set_start(0)).is_err());
1955+
assert!(std::panic::catch_unwind(|| GridPlacement::default().set_end(0)).is_err());
1956+
assert!(std::panic::catch_unwind(|| GridPlacement::default().set_span(0)).is_err());
1957+
}
1958+
1959+
#[test]
1960+
fn grid_placement_accessors() {
1961+
assert_eq!(GridPlacement::start(5).get_start(), Some(5));
1962+
assert_eq!(GridPlacement::end(-4).get_end(), Some(-4));
1963+
assert_eq!(GridPlacement::span(2).get_span(), Some(2));
1964+
assert_eq!(GridPlacement::start_end(11, 21).get_span(), None);
1965+
assert_eq!(GridPlacement::start_span(3, 5).get_end(), None);
1966+
assert_eq!(GridPlacement::end_span(-4, 12).get_start(), None);
1967+
}
18701968
}

0 commit comments

Comments
 (0)