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

Change the calling conditions of Popup's Dismiss method on Android #2159

Merged
merged 6 commits into from
Sep 8, 2024

Conversation

cat0363
Copy link
Contributor

@cat0363 cat0363 commented Aug 29, 2024

This PR resolves the issue where an exception occurs when the Popup's Close method is called when the Activity that displays the Popup has already been destroyed.

Description of Change

This issue occurs when the Popup's Dismiss method is called when the Activity that displays the Popup has already been discarded.

[mono-rt] [ERROR] FATAL UNHANDLED EXCEPTION: Java.Lang.IllegalArgumentException: View=DecorView@357f8b6[MainActivity] not attached to window manager

If the Activity that displays the Popup has already been disposed of, I will modify it so that the Dismiss method of the Popup is not called.

[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopUpHandler.android.cs]

	public static void MapOnClosed(PopupHandler handler, IPopup view, object? result)
	{
		var popup = handler.PlatformView;

		if (!popup.Context.GetActivity().IsDestroyed())
		{
			if (popup.IsShowing)
			{
				popup.Dismiss();
			}
		}

		view.HandlerCompleteTCS.TrySetResult();

		handler.DisconnectHandler(popup);
	}

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

Below are the verification results.
I tested the verification code for Issue #2135 by setting the time to close the Popup to 5 seconds.

Android.Emulator.-.pixel_2_-_api_30_5554.2024-08-29.10-36-57.mp4

Even if the Popup's Close method is called 5 seconds after the Popup is displayed, no exception is thrown.
When you return from standby, you will see that the Activity has been newly started.

The following is the verification result when waiting 5 seconds without setting it to standby.

Android.Emulator.-.pixel_2_-_api_30_5554.2024-08-29.10-41-16.mp4

You can see that the popup is closed as intended.

Copy link
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

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

Thank you @cat0363

@bijington bijington enabled auto-merge (squash) August 29, 2024 04:50
@bijington bijington disabled auto-merge August 29, 2024 04:50
@bijington
Copy link
Contributor

I assume this is ready to merge?

@cat0363
Copy link
Contributor Author

cat0363 commented Aug 29, 2024

When I checked, the following unit test failed.

CommunityToolkit.Maui.UnitTests.Views.PopupTests.PopupDismissedByTappingOutsideOfPopup

It is unclear whether the unit test imitates Activity like the Android emulator.

@bijington , In this unit test, does the activity exist?
If the activity does not exist, the Popup's Dismiss method will not be called, so the unit test is expected to fail with a timeout.

@cat0363
Copy link
Contributor Author

cat0363 commented Sep 1, 2024

@bijington , I mistakenly thought that the error was occurring in the unit test on the Android side.
This seems to be occurring on the macOS side, which is unrelated to this change.

CommunityToolkit.Maui.UnitTests.Views.PopupTests.PopupDismissedByTappingOutsideOfPopup

@bijington
Copy link
Contributor

@cat0363 thansk for the explanation. I'm not sure why it would be failing on macOS and not windows. I'll try and take a look this week 👍

@bijington
Copy link
Contributor

The test passes locally on my mac

@bijington bijington enabled auto-merge (squash) September 8, 2024 20:13
@bijington bijington merged commit fe7b59e into CommunityToolkit:main Sep 8, 2024
7 checks passed
@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] [ANDROID] Popup kills application when trying to close after Activity.OnDestroy
2 participants