Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[iOS] Fix issue creating rectangle using Radius with a high value #11051

Merged
merged 10 commits into from
Nov 27, 2020
Merged

[iOS] Fix issue creating rectangle using Radius with a high value #11051

merged 10 commits into from
Nov 27, 2020

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Fix issue creating rectangle using Radius with a high value on iOS.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

fix11033

Testing Procedure

Launch Core Gallery and navigate to the issue 11033. If the Ellipse renders without problems, the tests has passed.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@samhouts samhouts added p/iOS 🍎 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often labels Jun 15, 2020
@samhouts samhouts added this to In Review in 4.7.0 Jun 20, 2020
@samhouts samhouts changed the base branch from 4.7.0 to 4.8.0 July 8, 2020 18:13
@samhouts samhouts removed this from In Review in 4.7.0 Jul 8, 2020
@samhouts samhouts added this to In Review in vCurrent (4.8.0) Jul 30, 2020
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jul 30, 2020
@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 4, 2020 22:12
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 4, 2020
@samhouts samhouts added this to the 5.0.0 milestone Aug 4, 2020
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 4, 2020
@samhouts samhouts removed this from In Review in vCurrent (4.8.0) Aug 4, 2020
@samhouts samhouts added this to In Review in vNext+1 (5.0.0) Aug 11, 2020
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 26, 2020
@rmarinho rmarinho added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Oct 30, 2020
@rmarinho rmarinho requested review from mattleibow and removed request for samhouts November 10, 2020 19:18
@rmarinho rmarinho assigned mattleibow and unassigned samhouts Nov 11, 2020
mattleibow
mattleibow reviewed Nov 16, 2020
View reviewed changes
Xamarin.Forms.Platform.iOS/Shapes/RectangleRenderer.cs Outdated
Comment on lines 51 to 61
void UpdateRadiusX()
{
if (Element.Width > 0)
Control.UpdateRadiusX(Element.RadiusX / Element.Width);
var radiusX = ValidateRadius(Element.RadiusX / Element.WidthRequest);
Control.UpdateRadiusX(radiusX);
}

void UpdateRadiusY()
{
if (Element.Height > 0)
Control.UpdateRadiusY(Element.RadiusY / Element.Height);
var radiusY = ValidateRadius(Element.RadiusY / Element.HeightRequest);
Control.UpdateRadiusY(radiusY);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Height/Width or HeightRequest/WidthRequest?

What happens if I am in a stack panel with the width set to stretch, but specified a width of say 10? The radius might be incorrect then?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does result in a different outcome on Android - they still use Width/Height.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see Tizen uses the requests. Testing UWP now.

Copy link
Contributor Author

@jsuarezruiz jsuarezruiz Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed and now HeightRequest and WidthRequest is used everywhere to fix the inconsistent issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsuarezruiz should we be using Width (actual layout size) or WidthRequest (what the user said, but could be anything and will usually be different when set to Expand?)

mattleibow
mattleibow suggested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

This is a breaking change, but also leaves the others inconsistent.

@rmarinho rmarinho merged commit 84f7a54 into 5.0.0 Nov 27, 2020
vNext+1 (5.0.0) automation moved this from In Review to Done Nov 27, 2020
@rmarinho rmarinho deleted the fix-11033 branch November 27, 2020 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattleibow mattleibow mattleibow approved these changes

@rmarinho rmarinho rmarinho approved these changes

Assignees

@mattleibow mattleibow

@rmarinho rmarinho

Labels
a/shapes blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/Android p/iOS 🍎 t/bug 🐛
Projects
No open projects
vNext+1 (5.0.0)
Done
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

[Bug] iOS Native crash when RadiusX/RadiusY > Width/Height of Shapes.Rectangle
4 participants
@jsuarezruiz @mattleibow @rmarinho @samhouts