Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Dimension::Undefined in favour of Option<Dimension> #148

Closed
wants to merge 20 commits into from
Closed

Remove Dimension::Undefined in favour of Option<Dimension> #148

wants to merge 20 commits into from

Conversation

Weibye
Copy link
Collaborator

@Weibye Weibye commented Jun 11, 2022 •
edited
Loading

Objective

Fixes #114
Fixes #154

Enabled by the work from #144

Changelog

  • Dimension::Undefined has been removed
  • Which means we need to deal with undefined / defined nature of the dimensions in one level higher, using Option<Dimension>

Size

The geometry::Size<T> has been changed from

pub struct Size<T> {
    /// The x extent of the rectangle
    pub width: T,
    /// The y extent of the rectangle
    pub height: T,
}

to

pub struct Size<T> {
    /// The x extent of the rectangle
    pub width: Option<T>,
    /// The y extent of the rectangle
    pub height: Option<T>,
}

Rect

Has changed from

pub struct Rect<T> {
    pub start: T,
    pub end: T,
    pub top: T,
    pub bottom: T,
}

to

pub struct Rect<T> {
    pub start: Option<T>,
    pub end: Option<T>,
    pub top: Option<T>,
    pub bottom: Option<T>,
}

Feedback wanted

  • TODO

@alice-i-cecile alice-i-cecile added the code quality Make the code cleaner or prettier. label Jun 11, 2022
@Weibye Weibye mentioned this pull request Jun 11, 2022
Closed
@alice-i-cecile
Copy link
Collaborator

@Weibye I've done my best to resolve the merge conflicts and rebase for you :)

Weibye
Weibye commented Jun 12, 2022
View reviewed changes
src/geometry.rs Outdated Show resolved Hide resolved
@Weibye Weibye added the breaking-change A change that breaks our public interface label Jun 12, 2022
Weibye
Weibye commented Jun 12, 2022
View reviewed changes
Copy link
Collaborator Author

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

Making progress, uncertain of a few things

src/flexbox.rs
@@ -95,7 +95,7 @@ struct AlgoConstants {
padding_border: Rect<f32>,

/// The size of the internal node
node_inner_size: Size<Option<f32>>,
node_inner_size: Size<f32>,
/// The size of the surrounding container
container_size: Size<f32>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at this, do we need a Size that does not have Optional fields? One that is guaranteed to have both width and height?

src/flexbox.rs
Comment on lines +212 to +217
let margin =
self.nodes[node].style.margin.map(|n| n.map(|d| d.resolve(parent_size.width)).unwrap_or(Some(0.0)));
let padding =
self.nodes[node].style.padding.map(|n| n.map(|d| d.resolve(parent_size.width)).unwrap_or(Some(0.0)));
let border =
self.nodes[node].style.border.map(|n| n.map(|d| d.resolve(parent_size.width)).unwrap_or(Some(0.0)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm very uncertain if this is the correct logic. Still have more work to do before everything compiles and tests can start to yell at me.

src/flexbox.rs Outdated
Comment on lines 227 to 228
width: Some(node_size.width.maybe_sub(padding_border.horizontal_axis_sum())),
height: Some(node_size.height.maybe_sub(padding_border.vertical_axis_sum())),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Compiler complains here: How can I satisfy the trait constraints of MaybeMath so that these sums work?

Weibye
Weibye commented Jun 12, 2022
View reviewed changes
src/geometry.rs Outdated
Comment on lines 65 to 67
T: MaybeMath<T, Option<T>> + Copy + Clone,
T: MaybeMath<Option<T>, Option<T>> + Copy + Clone,
T: MaybeMath<Option<T>, T> + Copy + Clone,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this to get the sums to work MaybeMath but it didn't solve it.

src/geometry.rs
pub(crate) fn horizontal_axis_sum(&self) -> T {
self.start + self.end
pub(crate) fn horizontal_axis_sum(&self) -> Option<T> {
self.start.and_then(|start| start.maybe_add(self.end))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With MaybeMath I should be able to just use self.start.maybe_add(self.end) here (?) but not sure exactly how to get that to work.

Traits this involved are new to me, so bear with me while I try to figure this out 🙃

@Weibye Weibye mentioned this pull request Jun 12, 2022
Closed
@Weibye Weibye mentioned this pull request Jun 23, 2022
Closed
@Weibye
Copy link
Collaborator Author

Weibye commented Jun 23, 2022

Replaced by #188

@Weibye Weibye closed this Jun 23, 2022
@Weibye Weibye mentioned this pull request Nov 25, 2022
Closed
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 left review comments

Assignees
No one assigned
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change width and height of Size to Option<T> instead of T Consider removing Dimension::Undefined
2 participants
@Weibye @alice-i-cecile