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 MediaElement not working on FlyoutPage in IOS/MAC Catalyst #2532

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ne0rrmatrix
Copy link
Contributor

  • Bug fix

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

  • 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
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

Windows and android already support playback on a FlyoutPage.

@TheCodeTraveler
Copy link
Collaborator

@ne0rrmatrix How are you able to test + verify this fix works for a FlyoutPage?

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

@ne0rrmatrix
Copy link
Contributor Author

I will update the PR with a sample and create tests. Ty.

@ne0rrmatrix
Copy link
Contributor Author

I tested against the provided sample showing isdue

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler left a 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:

  1. Update PageExtensions.GetCurrentPage() to match the most-recent implementation of Microsoft.Maui.Controls.Platform.PageExtensions
  2. Update TryGetCurrentPage to use the GetCurrentPage() extension method which already checks for FlyoutPage:
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
@ne0rrmatrix
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS MediaElement in CollectionView on FlyoutPage
2 participants