Skip to content

Remove Bindings on Popup Close#1614

Merged
TheCodeTraveler merged 8 commits intoCommunityToolkit:mainfrom
cat0363:Issue-1608
Feb 17, 2024
Merged

Remove Bindings on Popup Close#1614
TheCodeTraveler merged 8 commits intoCommunityToolkit:mainfrom
cat0363:Issue-1608

Conversation

@cat0363
Copy link
Copy Markdown
Contributor

@cat0363 cat0363 commented Dec 20, 2023

In this PR, we will correct it so that SetColor of a Popup that has already been destroyed is not called.

Description of Change

If the SetColor method is called after closing the Popup, an exception will occur at the following location.

[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]

public static void SetColor(this Dialog dialog, in IPopup popup)
{
    if (popup.Color is null)
    {
        return;
    }

    var window = GetWindow(dialog);    // <- here
    window.SetBackgroundDrawable(new ColorDrawable(popup.Color.ToPlatform(AColorRes.BackgroundLight, dialog.Context)));
}

Add a guard condition to the call to the SetColor method.

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

public static void MapColor(PopupHandler handler, IPopup view)
{
    if (!handler.PlatformView.IsDisposed())
    {
        handler.PlatformView.SetColor(view);
    }
}

Similarly, add guard conditions to calls to the SetAnchor and SetSize methods.

public static void MapAnchor(PopupHandler handler, IPopup view)
{
    if (!handler.PlatformView.IsDisposed())
    {
        handler.PlatformView.SetAnchor(view);
    }
}

public static void MapSize(PopupHandler handler, IPopup view)
{
    ArgumentNullException.ThrowIfNull(handler.Container);

    if (!handler.PlatformView.IsDisposed())
    {
        handler.PlatformView.SetSize(view, handler.Container);
        handler.PlatformView.SetAnchor(view);
    }
}

void OnLayoutChange(object? sender, EventArgs e)
{
    if (VirtualView?.Handler?.PlatformView is Dialog dialog && Container is not null)
    {
        if (!dialog.IsDisposed())
        {
            PopupExtensions.SetSize(dialog, VirtualView, Container);
        }
    }
}

Linked Issues

PR Checklist

Additional information

Below is the verification video.

Android.Emulator.-.pixel_2_-_api_30_5554.2023-12-20.15-32-08.mp4

Even if I change the AppTheme, I can see that it is working as intended without any crashes.

@cat0363 cat0363 changed the title Add disposed check on Android Add Popup disposed check on Android Dec 20, 2023
@pictos
Copy link
Copy Markdown
Member

pictos commented Jan 18, 2024

Hey @cat0363, thanks for PR... For me, this looks more like a workaround than an actual fix. We dispose of the PlatformView when the DisconnectHandler is called, so the Map methods shouldn't trigger anymore, for that specific instance. So I believe the error is elsewhere. I suggest we check if there's another path to PlatformView.Dispose first, then start working from there

@cat0363
Copy link
Copy Markdown
Contributor Author

cat0363 commented Jan 18, 2024

@pictos , Thank you for your comment.
I will reconsider. I would like to try to find the root cause of the problem.

@cat0363
Copy link
Copy Markdown
Contributor Author

cat0363 commented Jan 19, 2024

The root cause of this problem is that the Binding is not released after the Popup is closed.
Because the Binding was not released, the MapColor method was called via the Color property when the theme was changed after the Popup was closed.

Revert the modifications you made previously and make the following changes.

[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]

protected virtual async Task OnClosed(object? result, bool wasDismissedByTappingOutsideOfPopup, CancellationToken token = default)
{
	((IPopup)this).OnClosed(result);
	((IResourceDictionary)resources).ValuesChanged -= OnResourcesChanged;

	RemoveBinding(Popup.ContentProperty);
	RemoveBinding(Popup.ColorProperty);
	RemoveBinding(Popup.SizeProperty);
	RemoveBinding(Popup.CanBeDismissedByTappingOutsideOfPopupProperty);
	RemoveBinding(Popup.VerticalOptionsProperty);
	RemoveBinding(Popup.HorizontalOptionsProperty);
	RemoveBinding(Popup.StyleProperty);

	await popupDismissedTaskCompletionSource.Task.WaitAsync(token);
	Parent = null;

	dismissWeakEventManager.HandleEvent(this, new PopupClosedEventArgs(result, wasDismissedByTappingOutsideOfPopup), nameof(Closed));
}

Below is the verification video.

Android.Emulator.-.pixel_2_-_api_30_5554.2024-01-19.11-42-36.mp4

You can see that it doesn't crash even after changing the theme.

@TheCodeTraveler TheCodeTraveler enabled auto-merge (squash) February 17, 2024 21:48
@TheCodeTraveler TheCodeTraveler enabled auto-merge (squash) February 17, 2024 21:58
@TheCodeTraveler TheCodeTraveler changed the title Add Popup disposed check on Android Remove Bindings on Popup Close Feb 17, 2024
@TheCodeTraveler TheCodeTraveler merged commit c6654f3 into CommunityToolkit:main Feb 17, 2024
@durandt durandt mentioned this pull request Mar 6, 2024
Open
@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.

Reviewers

@TheCodeTraveler TheCodeTraveler TheCodeTraveler approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Application crashes on theme change after using Popup with AppThemeBinding under Android

3 participants

@cat0363 @pictos @TheCodeTraveler