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

Val arithmetic changes #9534

Closed
wants to merge 2 commits into from
Closed

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Aug 22, 2023

Objective

  • A numeric Val with an inner value of 0. should be compatible in operations with other numeric Vals regardless of variant.

    For example, Val::Px(10.).try_add(Val::Percent(0.)) fails with a ValArithmeticError::NonIdenticalVariants error but this operation should succeed since a numeric Val with an inner value of 0. always resolves to 0. regardless of its variant.

  • Val arithmetic operations with Auto variants should always fail with a ValArithmeticError::NonEvaluatable error but addition and subtraction of a Val::Auto by a Val::Auto returns Val::Auto.


Changelog

Val::is_zero:
New method which returns true if the Val is numeric with an inner value of 0..

Val::try_add/try_sub changes:

  • If the reciever or argument is Val::Auto returns Err(ValArithmeticError::NonEvaluateable).
  • If the receiver or argument is numeric with an inner value of 0. convert it to the variant of the other Val before performing the arithmetic operation.
  • Implemented support for the Vw, Vh, VMin and VMax variants.

* `is_zero` method returns true if the `Val` is numeric and it's inner value is `0.`.
* `try_add/try_sub` methods: If reciever or argument is `Val::Auto` return Err(ValArithmeticError::NonEvaluateable). If reciever or argument is a numeric variant equal to `0.` convert it to a variant compatible with the other `Val`.
@ickshonpe ickshonpe changed the title Allow Val's with zero v Val arithmetic changes Aug 22, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Aug 22, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I'd probably prefer a couple more tests, but this change looks good.

Mostly trying to think about what tests we can write that will be more robust than the original code 🤔

@ickshonpe ickshonpe mentioned this pull request Aug 22, 2023
@ickshonpe
Copy link
Contributor Author

We could just make an issue for more tests, it might be an ok non-threatening task for a new contributor to take on etc.

Val's evaluate function needs to be updated to support viewport coordinates as well. I'll add an issue for it.

@bushrat011899
Copy link
Contributor

In #9566, I modify the implementation for PartialEq to explicitly equate zero-values of any unit, which may simplify some of the changes in this PR (e.g., is_zero could be removed and replaced with value == Val::ZERO)

@ickshonpe
Copy link
Contributor Author

Removed in favour of #9609

@ickshonpe ickshonpe closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants