-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix iOS infinite layout invalidation loop from bit-exact Frame equality (#35142) #35144
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| #nullable enable | ||
| Microsoft.Maui.Graphics.Rect.EqualsApproximately(Microsoft.Maui.Graphics.Rect other, double epsilon) -> bool | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 CRITICAL — Missing PublicAPI entries for platform-specific TFMs (3/3 reviewers)
Fix: Add
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| #nullable enable | ||
| Microsoft.Maui.Graphics.Rect.EqualsApproximately(Microsoft.Maui.Graphics.Rect other, double epsilon) -> bool |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,22 @@ | |
| return X.Equals(other.X) && Y.Equals(other.Y) && Width.Equals(other.Width) && Height.Equals(other.Height); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines whether each component of this rectangle is within <paramref name="epsilon"/> | ||
| /// of the corresponding component of <paramref name="other"/>. Use this to compare rectangles | ||
| /// produced by floating-point arithmetic where bit-exact equality would treat ULP-level | ||
| /// differences as material changes. | ||
| /// </summary> | ||
| /// <param name="other">The rectangle to compare against.</param> | ||
| /// <param name="epsilon">The maximum absolute difference, per component, that is treated as equal.</param> | ||
| public bool EqualsApproximately(Rect other, double epsilon) | ||
|
Check failure on line 87 in src/Graphics/src/Graphics/Rect.cs
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 MINOR — NaN semantics diverge from
In the Frame setter context: if a platform produced a NaN frame, the old code would no-op (dedup), but the new code would call Not a practical risk (NaN frames indicate an upstream bug), but worth documenting. Suggestion: Add a |
||
| { | ||
| return Math.Abs(X - other.X) <= epsilon | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 MINOR — No epsilon validation on public API (2/3 reviewers)
The internal caller uses a hardcoded Suggestion: Either add an |
||
| && Math.Abs(Y - other.Y) <= epsilon | ||
| && Math.Abs(Width - other.Width) <= epsilon | ||
| && Math.Abs(Height - other.Height) <= epsilon; | ||
| } | ||
|
|
||
| public override bool Equals(object obj) | ||
| { | ||
| if (obj is null) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Maui.Graphics.Tests | ||
| { | ||
| public class RectTests | ||
| { | ||
| [Fact] | ||
| public void EqualsApproximatelyReturnsTrueForIdenticalRects() | ||
| { | ||
| var rect = new Rect(10, 20, 30, 40); | ||
| Assert.True(rect.EqualsApproximately(new Rect(10, 20, 30, 40), epsilon: 1e-9)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void EqualsApproximatelyAbsorbsUlpDifferences() | ||
| { | ||
| // Values from the dotnet/maui#35142 trace: border heights captured on consecutive | ||
| // iOS layoutSubviews passes that differ by ~22 ULP. | ||
| var a = new Rect(0, 0, 390, 556.00000063578295); | ||
| var b = new Rect(0, 0, 390, 556.00000063578273); | ||
|
|
||
| Assert.False(a.Equals(b)); // bit-exact equality treats them as different | ||
| Assert.True(a.EqualsApproximately(b, epsilon: 1e-9)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void EqualsApproximatelyReturnsFalseWhenAnyComponentExceedsEpsilon() | ||
| { | ||
| var rect = new Rect(0, 0, 100, 100); | ||
| const double epsilon = 1e-9; | ||
|
|
||
| Assert.False(rect.EqualsApproximately(new Rect(0 + 2 * epsilon, 0, 100, 100), epsilon)); | ||
| Assert.False(rect.EqualsApproximately(new Rect(0, 0 + 2 * epsilon, 100, 100), epsilon)); | ||
| Assert.False(rect.EqualsApproximately(new Rect(0, 0, 100 + 2 * epsilon, 100), epsilon)); | ||
| Assert.False(rect.EqualsApproximately(new Rect(0, 0, 100, 100 + 2 * epsilon), epsilon)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void EqualsApproximatelyTreatsHalfEpsilonDifferenceAsEqual() | ||
| { | ||
| var rect = new Rect(0, 0, 100, 100); | ||
| const double epsilon = 1e-9; | ||
|
|
||
| Assert.True(rect.EqualsApproximately(new Rect(0, 0, 100, 100 + epsilon * 0.5), epsilon)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MODERATE —
UpdateBoundsComponentsinternal guard is now dead code (2/3 reviewers)UpdateBoundsComponents(called on line 1895) has an internalif (_frame == bounds) return;bit-exact guard. With this PR, the Frame setter now exits early for any difference ≤ 1e-9, soUpdateBoundsComponentsis only called when the difference exceeds the epsilon — making the internal bit-exact check guaranteed false (dead code).This isn't a correctness bug today (confirmed
UpdateBoundsComponentsis only called from the Frame setter), but it's a maintenance trap: a future caller bypassing the Frame setter would re-expose the original oscillation.Suggestion: Add a brief comment inside
UpdateBoundsComponentsnoting that the Frame setter is the sole caller and the primary deduplication path is the approximate check above.