Skip to content

Fix IOS bug in media Element when using Popup#1597

Merged
pictos merged 24 commits intoCommunityToolkit:mainfrom
ne0rrmatrix:FixIOSException
Feb 15, 2024
Merged

Fix IOS bug in media Element when using Popup#1597
pictos merged 24 commits intoCommunityToolkit:mainfrom
ne0rrmatrix:FixIOSException

Conversation

@ne0rrmatrix
Copy link
Copy Markdown
Member

@ne0rrmatrix ne0rrmatrix commented Dec 11, 2023

  • Bug fix for Media Element trying to add controls to wrong View Controller.

Description of Change

  • Adds checks to make sure video controls do not get added to wrong View Controller. This works if using Shell, Page, Navigation Page, Tabbed Page, Popup, CollectionView, and CarouselView

Linked Issues

PR Checklist

Additional information

Fixes CTD when using media element in a Popup. The incorrect Parent ViewController is being used when adding controls to parent view controller. A sample for popup has been added to samples for media element. This document has been edited since written to better explain the changes and what the fix does.

@bijington
Copy link
Copy Markdown
Contributor

@ne0rrmatrix thank you for submitting this. Sorry to ask this but is there an easy way to test this fix? I'm guessing there isn't anything in the sample that can be tested?

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

ne0rrmatrix commented Dec 11, 2023

@bijington you can load this PR in the quoted issues example repo. This fix requires targeting dotnet 8.0.x.

@bijington
Copy link
Copy Markdown
Contributor

@bijington you can load this PR in the quoted issues example repo. This fix requires targeting dotnet 8.0.x.

OK thanks. Do you think there is any value in us introducing a similar example in the sample application? I'm happy to assist getting it in as part of the testing.

@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

Do you think there is any value in us introducing a similar example in the sample application?

Absolutely! Always 💯

Anytime we fix a bug, we should add an example of its reproduction to the Sample App. This helps us verify the bug fix and ensures that we won't have a regression when testing a future PR.

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

ne0rrmatrix commented Dec 12, 2023

@brminnick I will work on adding reproduction to the sample app this week. I assume adding with PR is a bad idea? I am thinking separate because if we end up reverting PR the example goes away.

@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

Thanks! Yes, please add it to this PR

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

@brminnick I rewrote some View Controller logic for IOS. This change allows the controls to actually work. I still have a bunch of testing to do so please wait to review. I added a Popup with mediaElement inside it to the Media element page as a button on the far right called Popup in the sample app.

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

Have done some testing and fixed a bug with Navigation Page logic. I still have to test Flyout Page and Tabbed Page. Once I confirm they are working I will tag someone to review.

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

ne0rrmatrix commented Dec 14, 2023

Ok I have tested against shell, navigation Page,Tabbed Page, Content Page, and Collection View. Does anyone have any other suggested tests?

After testing Collection View I realized this may be fixed too #1421 since testing with collection view worked. Should I add another test in sample for CollectionView? I am not sure where to put it xaml if we want to do that. It is easy to implement but I can't think of a good place to put it in layout.

If we are adding another test to sample app I would love help with doing that. I am not sure about layout and how we would want to add collection view to the page.

@bijington @brminnick If we are not adding another example to sample for collection view this PR is ready for review by maintainer.

@ne0rrmatrix
Copy link
Copy Markdown
Member Author

Fix for collection view controller when using Carousel View has been added. Anyone who looks at this for review please indicate whether the approach I use is appropriate. There is now at least one other PR out to fix same issue. I am not sure which approach is appropriate and what the maintainers feel is best to move forward. I am happy either way.

@ne0rrmatrix ne0rrmatrix requested a review from pictos January 12, 2024 10:15
Comment thread src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.macios.cs Outdated
@ne0rrmatrix ne0rrmatrix requested a review from pictos January 16, 2024 22:33
pictos
pictos previously approved these changes Feb 15, 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.

MediaElement causes crash when used inside a popup on iOS only [BUG] MediaElement Crash in CollectionView On IOS

4 participants