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

Feature request: use Android 11+ media style for controls notification #11

Closed
NorthFred opened this issue May 8, 2023 · 38 comments
Closed

Comments

@NorthFred
Copy link

Referring to the Cordova equivalent of this plugin: https://github.com/ghenry22/cordova-plugin-music-controls2

In this plugin, the notification has been updated to show different media style for the notification. This Capacitor plugin could also benefit from this, especially now since the controls notification does not show up anymore on API 33+ (see issue #7).

@ingageco
Copy link
Owner

ingageco commented May 8, 2023

@NorthFred Very nice, i'll look into the differences and start working on it

@NorthFred
Copy link
Author

@ingageco Great 👍!

@NorthFred
Copy link
Author

@ingageco How's this coming along? I'm available and eager to start testing this, if you have anything ready!

@NorthFred
Copy link
Author

@ingageco Any updates? I'm eager to have a working solution for API levels 33+...

@ingageco
Copy link
Owner

I do t have any updates at the moment. I plan on making some app updates this week, and will update this to meet any needs we have for Android 13

@ingageco
Copy link
Owner

@NorthFred Also currently in the android-13-style branch and in need of testing

@NorthFred
Copy link
Author

@ingageco Build fails with following error in CapacitorMusicControls.java: cannot find symbol variable newMConnection

`activity.bindService(startServiceIntent, newMConnection, Context.BIND_AUTO_CREATE);`

@ingageco
Copy link
Owner

Ok I'll find that reference. Must have missed something

@ingageco
Copy link
Owner

@NorthFred Updated - re-pull the branch. i confirmed that it'll build on its own, so there shouldn't be any simple errors like this.

@NorthFred
Copy link
Author

@ingageco Issue verified! Tested further with the Android 13 emulator (still don't have devices), and the media controls are appearing when playing music! The Pause button however has no effect. When pressing it, it changes to the Play button, but the music keeps playing. Any subsequent press of the Play button has no effect either.

@ingageco
Copy link
Owner

@NorthFred Awesome! could you screenshot just so i have a reference?

Does it still ask for permissions of any sort?

Im going to try to do my own testing on this the remainder of the week, but it's a crazy personal week + we are on watch for our 2nd child. Send whatever you find, and i'll address it asap!

@NorthFred
Copy link
Author

NorthFred commented Jun 14, 2023

@ingageco Wish you and your family all the best with the expectancy of the baby!

This is what it looks like on Android 13:
image

There are no dialogs asking for permissions any more when starting the app for the first time.

There's also a 2nd issue on Android 13: when dismissing the media controller, the music doesn't stop playing, even though the media controller is removed from the view.

@ingageco
Copy link
Owner

ingageco commented Jun 15, 2023

@NorthFred in my initial test, on android 33, a few things:

  1. the notification permission is still asked the first time the controls are created
  2. the play/pause button work as expected
  3. the stop button (the X beside play/pause) correctly ends the audio in app, but the notification doesn't dismiss
  4. dismissing the notification by swiping it away does not end the audio

(edited to distinguish between x button and dismiss via swipe)

@NorthFred
Copy link
Author

@ingageco Did you test on Android 13? On Android 8-12, the controls are working, but I'm facing the problems with Android 13... I haven't seen the notification permission on any version.

@ingageco
Copy link
Owner

Api level 33 is android 13

@NorthFred
Copy link
Author

Please make sure to also target API 33 when building the app (in variables.gradle). I get a different outcome when I still have API 32 marked as target.

These are my settings:

    minSdkVersion = 22
    compileSdkVersion = 33
    targetSdkVersion = 33

@ingageco
Copy link
Owner

@NorthFred Ahhhhh... now im on the same page as you.

@ingageco
Copy link
Owner

@NorthFred I've copied over just about everything that I can from the cordova version of the plugin. With the exception of the cordova-specific code, i think they are close to identical. However, the play/pause button still doesn't work in 13.

One thing i have discovered about 13 - swiping the media controls DOES make it go away, but it is not an actual dismissal of the notification. It hides it. Swipe down again after swiping it away, and i'll see it still exists there.

@NorthFred
Copy link
Author

@ingageco There are no errors printed to the console for the Play/Pause button, only following message is registered:

I/CapacitorMusicControls: controlsNotification fired music-controls-media-button-pause
V/Capacitor/CapacitorMusicControls: Notifying listeners for event controlsNotification

@NorthFred
Copy link
Author

As for the hiding of the media controls: when swiping down again, the media controls are still shown in the status page and on 2nd try, they can no longer be dismissed.

@ingageco
Copy link
Owner

@NorthFred Read this: ionic-team/capacitor#6234

Appears to be a capacitor bug with notifying the listeners. "Solution" is to use the legacy android JS bridge, by placing:

android: { useLegacyBridge: true }

in your capacitor.config.ts config file

@ingageco
Copy link
Owner

@NorthFred doing this causes issues for me. I believe the capacitor bug may be related to running the notification as a foreground service. before this branch, we didn't run it as a foreground service, but i integrated those changes from the other repo because of reports that the audio was stopping after 5min for that plugin. Not knowing if that was the case with our plugin / our apps, i went ahead and integrated it. Turns out, my apps aren't currently affected by that - at least with the current version of this plugin / Android 12.

So i guess the next thing I can do is create a new branch and ONLY integrate the android 13 style changes to see if that will work, without changing it to run as a foreground service. If that doesn't work, i dont have any other solutions, and we both may just have to see if the cordova version will run in our projects for the near future.

@ingageco
Copy link
Owner

@NorthFred No dice on android 13 style only. Looks like we're going to have to see what we can do about the notifyListeners not sending the button events back to the app via the JS bridge.

@ingageco
Copy link
Owner

I've done everything i can to try to get this to work. At this point, there's no more progress to make unless that JS bridge bug is fixed. Even using the legacy Bridge has not worked for me, but i'll keep trying that angle to see if i can get it to work until capacitor issues the fix.

@ingageco
Copy link
Owner

@NorthFred Good news! It really bothered me to have this roadblock - i have multiple media apps that we have for clients. I just can't sit around and wait for the Cap team to fix this. So i dug into the docs, and found that there is another way to communicate with the application via Java - triggerJSEvent

I just tested it, and .... the application actually is notified, unlike the recommended method by Capacitor! I'll post an update soon - it does require that you change how you listen for updates.

@NorthFred
Copy link
Author

@ingageco Wow! That is great news and I am happy to hear that there might be a solution after all. I have been testing with the capacitor.config.ts flag that you mentioned, but was hitting a wall with all the attempts. I can't wait to try out your solution, and I am feeling hopeful again that this will work by the time of the API level 33 target deadline this August!

@ingageco
Copy link
Owner

ingageco commented Jun 20, 2023

@NorthFred Update your version of the android-13-style branch and differentiate between android and ios using this code:

the listener for android

        document.addEventListener('controlsNotification', (event) => {

          console.log('event received:');
          console.log(event);
          const action = { message : event.message, position: 0 };
          self.handleControlsEvent(action);
        });	

the listener for ios (unchanged)

        CapacitorMusicControls.addListener('controlsNotification', (action) => {

          self.handleControlsEvent(action);
        });

@NorthFred
Copy link
Author

@ingageco First test is successful! This looks promising indeed! I will do some further in-depth testing during the next 2 days and will report back if there are any findings. Thanks a lot altready, is there a way I could buy you a coffee? :)

@ingageco
Copy link
Owner

@NorthFred I would let you for sure... BUT I've benefitted from so many working on open source, im happy to just accept your appreciation!

@ingageco
Copy link
Owner

@NorthFred This plugin would not be anywhere near usable if it wasn't for your testing and research. That's extremely valuable to me. You found things i never would have found, honestly, because i'm not as thorough of a tester!

@NorthFred
Copy link
Author

@ingageco Thanks for the kind words :) I'm actually a software tester by profession, but I do some light coding as a hobby :)

@NorthFred
Copy link
Author

@ingageco On Android API 34 I am running into a lot of messages like the one below. The app is hardly usable on the emulator because of it. Now, Android API 33 is reporting the same but it doesn't show as many of them as on API 34. It seems these are starting to come when the plugin gets registered during start-up of the application. I didn't have this with the main branch of the app:

I/Choreographer: Skipped 68 frames!  The application may be doing too much work on its main thread.

@ingageco
Copy link
Owner

It happens when you call adding an event listener or immediately on open?

@ingageco
Copy link
Owner

@NorthFred I'm not seeing any of that in my logs on 33 - zero. Booting up a 34 emulator to check.

When i researched this, most of the consensus was this was either harmless or would go away with Invalidate Caches/Rebuild

@ingageco
Copy link
Owner

@NorthFred Android API 34 is in beta and not going to reach general availability for a long time. So I suggest we keep focus to 33 for now, which is making it's way to devices now.

It's possible that the issues on 34 are Capacitor related and have nothing to do with our code. We are prob a little ahead of the curve there.

@NorthFred
Copy link
Author

@ingageco We can leave it out of scope for the time being, it also requires some further investigation from my side.

@NorthFred
Copy link
Author

NorthFred commented Jun 21, 2023

One observation with the android-13-style branch of this plugin is that the controls are not destroyed when closing the application. This happens in 2 instances:

A.

  • Open your application
  • Play some music for some time (media controls are created)
  • Close the application
    ==> The media controls persist.

B.

  • Open the application
  • Play some music and stop the music (media controls are created, no active streams)
  • Put the application to the background
  • Wait for e.g. 15 minutes
    ==> The system will close the application, but the media controls are still visible.

This reproduces on Android 12 as well as 13.

@ingageco
Copy link
Owner

@NorthFred lets move your last comments to a new issue and close this one

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

No branches or pull requests

2 participants