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

IBorder now works again for iOS/Android #4909

Merged
merged 3 commits into from
Feb 25, 2022
Merged

Conversation

Clancey
Copy link
Contributor

@Clancey Clancey commented Feb 25, 2022

On iOS, the overlay view never had it's frame set.

On Android, we were not calling all the correct properties, if it's only an iBorder

Copy link
Member

@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.

Seems good to me, but then @jsuarezruiz is probably more of an expert.

BringSubviewToFront(BorderView);
BorderView.Frame = Bounds;
Copy link
Member

Choose a reason for hiding this comment

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

We no longer need to bring it to front? @jsuarezruiz?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried and is not necessary. Is already done in other place.

@Clancey
Copy link
Contributor Author

Clancey commented Feb 25, 2022

We were bringing it to the front twice, instead of setting the frame. So I changed one to frame. I think it was a bad merge.

@@ -5,6 +5,26 @@ namespace Microsoft.Maui.Platform
{
public static class StrokeExtensions
{
public static void UpdateBorderStroke(this AView platformView, IBorderStroke border)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do Windows require changes too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? Was traveling without a windows machine. Was going to check tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, taking a look to WrapperView on Windows the logic to manage the border is missing. I think we should use the BorderChanged method to detect changes and ContentPanel control instead Grid in the WrapperView.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will send another PR with the Windows implementation. In that way we can merge and not block this PR.

BringSubviewToFront(BorderView);
BorderView.Frame = Bounds;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried and is not necessary. Is already done in other place.

Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
@pictos
Copy link
Contributor

pictos commented Feb 25, 2022

@PureWeen looks my suggestion was wrong :(
D:\a\_work\1\s\src\Core\src\Platform\Android\StrokeExtensions.cs(18,4): error CS8602: Dereference of a possibly null reference. [D:\a\_work\1\s\src\Core\src\Core.csproj]

I didn't know why since there is a check for null right before

@PureWeen
Copy link
Member

@pictos oh yea it's an && check not an || check so mauiDrawable could still be null

@PureWeen PureWeen enabled auto-merge (squash) February 25, 2022 21:08
@pictos
Copy link
Contributor

pictos commented Feb 25, 2022

@pictos oh yea it's an && check not an || check so mauiDrawable could still be null

Silly me >.<

@PureWeen PureWeen merged commit 5d8b798 into dotnet:main Feb 25, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
@samhouts samhouts added the fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants