Fix crash when multiple Android Service are active #2418

mr5z · 2024-12-28T04:24:06Z

Description of Change

Code refactors
- Change of naming convention of constants
- Removal of confusing code
- Update Android Service name
Fix crash that occurs in Android Service

Linked Issues

Fixes [BUG] Using MediaElement in Android causes crash #2419

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

ne0rrmatrix · 2024-12-28T14:30:57Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

@@ -18,15 +18,20 @@
 namespace CommunityToolkit.Maui.Media.Services;

 [SupportedOSPlatform("Android26.0")]
-[Service(Exported = false, Enabled = true, Name = "communityToolkit.maui.media.services", ForegroundServiceType = ForegroundService.TypeMediaPlayback)]
+[Service(Exported = false, Enabled = true, Name = "CommunityToolkit.Maui.Media.Services", ForegroundServiceType = ForegroundService.TypeMediaPlayback)]


Changing the case to uppercase will break older API's compatibility and prevent app from running on Android 23 or below. We only support android 26+ now on media element but I do want to point out this would break some current apps. Also changing this here requires changing the string in the manifest to match which has not been done.

I really have no strong opinion on what naming convention should be done for android-net, but in native Android, it's package.name.ServiceName but it can be addressed relatively in AndroidManifest.xml as .ServiceName. Maybe we should follow that? I think it is still early for this library to think about the naming convention before the inconsistency spreads out even more, but if you think this is the convention we should follow moving forward, I'm happy to revert it.

pictos

Thanks for your PR, but I can't see what I should review. Where's the actual fix for the issue?
Can you also revert all code style changes that you made and follow ours?

pictos · 2025-01-09T18:01:46Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

+	public const string ActionPlay = "MediaAction.play";
+	public const string ActionPause = "MediaAction.pause";
+	public const string ActionUpdateUI = "CommunityToolkit.Maui.Services.action.UPDATE_UI";
+	public const string ActionUpdatePlayer = "CommunityToolkit.Maui.Services.action.UPDATE_PLAYER";
+	public const string ActionRewind = "MediaAction.rewind";
+	public const string ActionFastForward = "MediaAction.fastForward";


Why did you change the pattern? Can you revert and follow our pattern

Isn't PascalCasing the standard convention in C# to name consts?

@pictos I've left some comments on some of my code changes.

mr5z · 2025-01-11T15:46:59Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

 		{
 			notification.AddAction(actionPlay);
 		}
 		notification.AddAction(actionNext);
-		notification.Build();


This is an immutable operation. It basically does nothing so I think it's better to remove it.

mr5z · 2025-01-11T15:51:01Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

+	public const string ActionRewind = "MediaAction.rewind";
+	public const string ActionFastForward = "MediaAction.fastForward";
+
+	public const string NotificationChannelId = "Maui.MediaElement";


I have introduced a constant to easily spot the notification ID of this control. I think it's important to keep track of notification ID as we introduce more foreground Android services since the same notification ID could not co-exist in the same Android application. I also think it's important to name them appropriately instead of just "1" as what was previously implemented.

mr5z · 2025-01-11T15:53:17Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

+	public const string ActionFastForward = "MediaAction.fastForward";
+
+	public const string NotificationChannelId = "Maui.MediaElement";
+	public const string NotificationChannelName = "Transport Controls";


I have also introduced a const for channel name so we can easily track it. This is a user facing text so I think it's better to name it appropriately instead of just "notification". I am not sure about the exact text to put but I think title casing is the norm.

mr5z · 2025-01-11T15:54:39Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

+	public const string NotificationChannelId = "Maui.MediaElement";
+	public const string NotificationChannelName = "Transport Controls";
+
+	public const int NotificationId = 2024;


2024 I think is unique enough not to clash with other foreground service ID.

mr5z · 2025-01-11T15:58:47Z

src/CommunityToolkit.Maui.MediaElement/Services/MediaControlsService.android.cs

@@ -112,20 +117,15 @@ async ValueTask InitializeNotification(MediaSessionCompat mediaSession, Intent m
 		var style = new AndroidX.Media.App.NotificationCompat.MediaStyle();
 		style.SetMediaSession(token);

-		if (Build.VERSION.SdkInt >= BuildVersionCodes.S)
-		{
-			style.SetShowActionsInCompactView(0, 1, 2, 3);


I think this operation must be called after setting actions to the notification object, but it's empty at this point and it's the main reason for crash issues reported here. I have also tried including this after the notification action has been set but it just completely hides the buttons in the notification when I tested it so I think it's better to remove this until we have a better reason to keep it and have it implement properly.

Fix crash when multiple Android Service are active

0009187

mr5z marked this pull request as ready for review December 28, 2024 05:20

ne0rrmatrix reviewed Dec 28, 2024

View reviewed changes

Merge branch 'CommunityToolkit:main' into main

3d69d40

pictos requested changes Jan 9, 2025

View reviewed changes

mr5z commented Jan 11, 2025

View reviewed changes

mr5z requested review from pictos and ne0rrmatrix January 11, 2025 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when multiple Android Service are active #2418

Fix crash when multiple Android Service are active #2418

mr5z commented Dec 28, 2024 •

edited

Loading

ne0rrmatrix Dec 28, 2024

mr5z Jan 11, 2025

pictos left a comment

pictos Jan 9, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

mr5z Jan 11, 2025

Fix crash when multiple Android Service are active #2418

Are you sure you want to change the base?

Fix crash when multiple Android Service are active #2418

Conversation

mr5z commented Dec 28, 2024 • edited Loading

Description of Change

Linked Issues

PR Checklist

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pictos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr5z commented Dec 28, 2024 •

edited

Loading