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

Fix Crash on iOS when using MediaElement inside a CarouselView #2187

Merged
merged 8 commits into from
Sep 8, 2024

Conversation

brminnick
Copy link
Collaborator

Description of Change

This Pull Request adds support for MediaElement inside of DataTemplate.

This Pull Request also adds MediaElementCarouselViewPage to the Sample App.

Simulator Screen Recording - iPhone 15 Pro - 2024-09-08 at 14 59 50

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

Additional information

This fix requires reflection because the Controller property that we need to reference when MediaElement is inside of a DataTemplate is a protected internal property in the ItemsViewContoller class: https://github.com/dotnet/maui/blob/cf002538cb73db4bf187a51e4786d7478a7025ee/src/Controls/src/Core/Handlers/Items/ItemsViewHandler.iOS.cs#L39

Because this uses reflection, it is important to manually test the MediaElementCarouselViewPage whenever we update our .NET MAUI Workload (eg net8.0-ios -> net9.0-ios) to ensure there are no breaking changes caused by reflection and/or the renaming of internal properties.

@ne0rrmatrix
Copy link
Contributor

I am very happy to see this PR! I just finished testing it in IOS and for Mac Catalyst. It works as intended and everything is fully working now. The only thing I could think of that could be done in a different PR maybe is adding another page and testing CollectionView. If you want I can add that view or you can do it? It is up to you. But we need to test for that too. I don't know how many times I have manually just created both this view and also Collection View for testing specifically in IOS/Mac. I tested on an Ipad running current version of IOS. It is all working!

Copy link
Contributor

@ne0rrmatrix ne0rrmatrix left a comment

Choose a reason for hiding this comment

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

As I said in the comment. It all looks good. I am also very happy to report I tested it against a mac and ipad. No issues on either CaraouselView or regular MediaElement. My only wish is to now either have you add another sample page with CollectionView. If you don't want to I would be happy to do it.

@brminnick
Copy link
Collaborator Author

As I said in the comment. It all looks good. I am also very happy to report I tested it against a mac and ipad. No issues on either CaraouselView or regular MediaElement. My only wish is to now either have you add another sample page with CollectionView. If you don't want to I would be happy to do it.

Sure - go for it! I'll go ahead and merge this PR in the meantime to get the bug fix into the code base.

You can assign your PR adding a CollectionView sample to me when it's ready for review 👍

@brminnick brminnick enabled auto-merge (squash) September 8, 2024 22:43
@brminnick brminnick merged commit 35c01fb into main Sep 8, 2024
7 checks passed
@brminnick brminnick deleted the iOS-MediaElement-CarouselView branch September 8, 2024 22:44
@AndreasHennig
Copy link

@brminnick I also just wanted to express my gratitude for the fix - thank you!! 🥳

@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Crash on iOS when using MediaElement inside a CarouselView
3 participants