-
Notifications
You must be signed in to change notification settings - Fork 424
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 MediaElement not working on FlyoutPage in IOS/MAC Catalyst #2532
base: main
Are you sure you want to change the base?
Fix MediaElement not working on FlyoutPage in IOS/MAC Catalyst #2532
Conversation
@ne0rrmatrix How are you able to test + verify this fix works for a This PR doesn't include an update to the sample app or additional unit tests. Any time we fix a bug, we should add a unit test, if possible, and add update the sample app to demonstrate it working (aka a Regression Test). |
I will update the PR with a sample and create tests. Ty. |
I tested against the provided sample showing isdue |
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.
I recommend doing two things to make this more robust:
- Update
PageExtensions.GetCurrentPage()
to match the most-recent implementation ofMicrosoft.Maui.Controls.Platform.PageExtensions
- Update
TryGetCurrentPage
to use theGetCurrentPage()
extension method which already checks forFlyoutPage
:
static bool TryGetCurrentPage([NotNullWhen(true)] out Page? currentPage)
{
currentPage = null;
if (Application.Current?.Windows is null)
{
return false;
}
if (Application.Current.Windows.Count is 0)
{
throw new InvalidOperationException("Unable to find active Window");
}
if (Application.Current.Windows.Count > 1)
{
// We are unable to determine which Window contains the ItemsView that contains the MediaElement when multiple ItemsView are being used in the same page
// TODO: Add support for MediaElement in an ItemsView in a multi-window application
throw new NotSupportedException("MediaElement is not currently supported in multi-window applications");
}
var window = Application.Current.Windows[0];
// If using Shell, return the current page
if (window.Page is Shell { CurrentPage: not null } shell)
{
currentPage = shell.CurrentPage;
return true;
}
// If not using Shell, use the ModelNavigationStack to check for any pages displayed modally
if (TryGetModalPage(window, out var modalPage))
{
currentPage = modalPage;
return true;
}
if (window.Page is Page page)
{
currentPage = page.GetCurrentPage();
return true;
}
return false;
static bool TryGetModalPage(Window window, [NotNullWhen(true)] out Page? page)
{
page = window.Navigation.ModalStack.LastOrDefault();
return page is not null;
}
}
Update TryGetCurrentPage to use GetCurrentPage
I have updated page extensions and tryGetCurrentPage as requested. I will start trying to figure out how to implement a FlyoutPage with sample app. Implementing the logic to handle a flyout page and a non shell app will be challenging. Device specific tests for IOS/Mac to test this can be done. I am not sure if that is what you are looking for? If so I can do that. I hope I am not too off track with that line of reasoning. |
Description of Change
Support for Media Element on a
FlyoutPage
does not currently work on IOS. This adds support for it on IOS and Mac Catalyst.Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
Windows and android already support playback on a FlyoutPage.