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.

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

Change width and height of Size to Option<T> instead of T #154

Closed
Weibye opened this issue Jun 11, 2022 · 6 comments
Closed

Change width and height of Size to Option<T> instead of T #154

Weibye opened this issue Jun 11, 2022 · 6 comments
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier. usability Make the library more comfortable to use

Comments

@Weibye
Copy link
Collaborator

Weibye commented Jun 11, 2022

What problem does this solve or what need does it fill?

The geometry::Size<T> is currently defined as

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

While digging through the code in relation to #144 and #148 I've started wondering if we should change the definition of Size to the following:

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

The width and height fields need to be independently defined or not, which means that replacing everything with some_field: Option<Size> will not cover all use-cases throughout the code.

I'm not yet able to see the full picture how everything fits together once we:
Dimension::Undefined -> Option<Dimension>
Number::undefined -> Option<f32>

but it seems it is a must for this to work (?)

What alternative(s) have you considered?

Leave it as is, but I expect it to change in some way or another.

Are there alternative designs we can explore?

@Weibye Weibye added enhancement New feature or request code quality Make the code cleaner or prettier. breaking-change A change that breaks our public interface labels Jun 11, 2022
@alice-i-cecile
Copy link
Collaborator

alice-i-cecile commented Jun 11, 2022

This seems like about the right approach. In general it really seems like we're typically storing options here.

@alice-i-cecile alice-i-cecile added usability Make the library more comfortable to use and removed enhancement New feature or request labels Jun 11, 2022
@alice-i-cecile
Copy link
Collaborator

I'm tackling this now.

@alice-i-cecile
Copy link
Collaborator

Actually, this appears to be blocked on #148. @Weibye, feel free to attempt this in a follow-up, or bug me to do it :)

@Weibye
Copy link
Collaborator Author

Weibye commented Jun 11, 2022

Actually, this appears to be blocked on #148. @Weibye, feel free to attempt this in a follow-up, or bug me to do it :)

First I thought we'd need so solve #148 before this, then I thought this needed to come first. I supsect they both need to be done at the same time for the whole system to work, but I'll keep tinkering with it tomorrow morning :)

@nicoburns
Copy link
Collaborator

This doesn't won't work now we're also using Size with AvailableSpace. And Size<Option<f32>> is working well as is :)

@nicoburns nicoburns closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2022
@Weibye
Copy link
Collaborator Author

Weibye commented Nov 24, 2022

👍 It seemed to make sense at the time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier. usability Make the library more comfortable to use
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants