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 Windows Media Element in MultiWindow Mode on exit Crash #2259

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

Conversation

ne0rrmatrix
Copy link
Contributor

@ne0rrmatrix ne0rrmatrix commented Oct 5, 2024

  • Bug fix

Description of Change

Track window count and manage snackbar notifications

Added a static NumberOfWindows property to the Options class to track the number of windows. Updated SetShouldEnableSnackbarOnWindows to increment/decrement NumberOfWindows on window creation/closure. Conditional registration/unregistration of AppNotificationManager for snackbar notifications based on NumberOfWindows count.

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

This PR uses a simple counter to track the number of windows that exist and only when one is remaining will it unregister Snackview

Added a static `NumberOfWindows` property to the `Options` class to track the number of windows. Updated `SetShouldEnableSnackbarOnWindows` to increment/decrement `NumberOfWindows` on window creation/closure. Conditional registration/unregistration of `AppNotificationManager` for snackbar notifications based on `NumberOfWindows` count.
@ne0rrmatrix ne0rrmatrix self-assigned this Oct 5, 2024
@ne0rrmatrix ne0rrmatrix added the 📽️ MediaElement Issue/PR that has to do with MediaElement label Oct 5, 2024
Copy link

@McSaverInvestments McSaverInvestments left a comment

Choose a reason for hiding this comment

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

This changed fixed #2279
Hoping it will be released soon

@@ -66,13 +66,25 @@ public void SetShouldEnableSnackbarOnWindows(bool value)
builder.ConfigureLifecycleEvents(events =>
{
events.AddWindows(windows => windows
.OnWindowCreated((_) =>
{
NumberOfWindows++;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if just the UIThread can create an window or if can be done by another thread. I believe that's the second, so the Window1.UIThread != Window2.UIThread, and that can cause concurrency issues.

I would suggest to use the Interlocked.Increment and Interlocked.Decrement to control this counter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! Interlocked.Increment and Interlocked.Decrement are perfect for this scenario.

In this implementation, I think we should actually remove internal static int NumberOfWindows { get; private set; } and use Application.Current.Windows.Count to avoid creating a duplicate property forcing CommunityToolkit.Maui to increment/decrement properly.

I don't see a problem removing internal static in Options.NumberOfWindows { get; } since it is not being used anywhere else in the code.

I'll update the code to remove this property 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📽️ MediaElement Issue/PR that has to do with MediaElement
Projects
None yet
4 participants