Disallow NaNs in Rect's Width and Height properties#18971
Closed
BioTurboNick wants to merge 5 commits into
Closed
Conversation
|
Hey there @BioTurboNick! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Member
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hartez
suggested changes
Nov 28, 2023
Contributor
hartez
left a comment
There was a problem hiding this comment.
I like the idea, just needs some minor changes.
Co-authored-by: E.Z. Hart <hartez@users.noreply.github.com>
hartez
approved these changes
Jan 10, 2024
Member
|
/rebase |
Member
|
I am going to close this PR because I am not sure that we should do this. I would rather go back to the issue and have a discussion on whether we should do this or even change Point and/or Size instead. Or everything. |
NaNs in Rect sizesNaNs in Rect's Width and Height properties
PureWeen
added a commit
that referenced
this pull request
Jun 20, 2024
### Description of Change The other primitives are fine with `NaN`: * `Point` * `PointF` * `Rect` * `RectF` In fact, `Rect.Size` may throw because the `Rect` handles `NaN` but the `Size` instance does not. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #16571 <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. --> Possible alternative: - #18971
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
Recthas aSizeproperty that returns aSizeobject that wraps theWidthandHeightproperties. However,Sizeobjects do not allow construction withNaNvalues. This means thatRects can be created that have a property getter that throws an exception, which is against .Net design guidelines: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/propertyApart from just being against guidelines, a downstream issue is that exceptions are thrown at the point of access, rather than the point of creation, inhibiting the ability to troubleshoot where these
NaNs are coming from.This change found one location in MAUI that was creating
Rects withNaNs, and includes a fix. There may well be more locations that require adjustment. This would potentially be a breaking change inGraphics.Alternatively, if a breaking change is not desired, documenting the issue and ensuring any location that consumes the
Sizeproperty guards against the Rect containing NaN values could be pursued. That would be tricky because of instances likeElement.Bounds.Size.ToSizeF()being used.Issues Fixed
Fixes #16571
Possible alternative:
SizeandSizeFshould not throw onNaN#22890