ryanheise / audio_service

Flutter plugin to play audio in the background while the screen is off.
796 stars 476 forks source link

support more actions in media notification #633

Open nt4f04uNd opened 3 years ago

nt4f04uNd commented 3 years ago

Is your feature request related to a problem? Please describe. for android, i'd like to have a loop switch button in notification, this is currently impossible https://github.com/ryanheise/audio_service/blob/9d0a2bdd0e1a57b366d1bff3ab1c728968555304/audio_service/lib/audio_service.dart#L80-L91

for other platforms, as far as i can understand this is simply impossible at all

Describe the solution you'd like on android (and other platforms that allow to do this, if any) support all other MediaActions + allow binding custom actions, not included in this list

Describe alternatives you've considered none

Additional context no

ryanheise commented 3 years ago

This would be good to have.

At the moment, we use onMediaButtonEvent in Java to handle all notification clicks and these are created in the notification via:

    NotificationCompat.Action action(String resource, String label, long actionCode) {
        int iconId = getResourceId(resource);
        return new NotificationCompat.Action(iconId, label,
                buildMediaButtonPendingIntent(actionCode));
    }

So it's for simplicity that we do it this way. Anything that can't be mapped to a limited set of MEDIA_* keycodes cannot be routed through the media session mechanism, those events get filtered out. So we'd need to implement a separate mechanism just for these other buttons.

nt4f04uNd commented 3 years ago

i noticed you override onMediaButtonEvent. android provides an implementation for you, you simply didn't know that? https://stackoverflow.com/a/29002335/9710294

nt4f04uNd commented 3 years ago

i believe removing this override and removing the click callback in the handler is what has to be done. then we better create a Notification reciever that simply registers pending intents with custom extras from MediaActions and calls that back to the dart and eventually some callback like onNotificationClick is called, that would have a default implementation that handles all the media buttons out of the box

MediaAction should be remade to custom enum to be able to create custom notification actions

nt4f04uNd commented 3 years ago

currently click allows to listen to button events, sort of. this is limited and i doubt anybody uses that. if we want to provide an ability to listen to them, it should be a separate callback, e.g. onMediaButtonEvent, as on android

ryanheise commented 3 years ago

You have stumbled onto the reason why I have overridden it. All of that work is specifically so that I can present button click events to the app rather than let the support libraries try to convert them into onPlay/onPause commands depending on the current state. This library has a wide range of use cases, and the media button on Android is used by some fitness trackers, for example to send a signal to the app of when the training has ended, etc. There are so many different apps besides music and podcast players that have found innovative ways to use the headset buttons, and they also sometimes have preferences to allow reprogramming the headset buttons. Interestingly, the Android APIs used to be more like this, you could create a broadcast receiver to listen to the media button intent and interpret the click as your app liked. The support libraries tried to make things easier for you, but ended up losing information on the original event.

In any case, I think I'm happy with the overall API I have arrived at, where click corresponds to an actual headset button click. It supports all of the use cases. The implementation has gone through a few iterations so it may be worth my going through and checking that it all still works as intended.

The problem with exposing Android's onMediaButtonEvent directly is that this masks the source of the event. You cannot tell if they pressed the media button or whether they pressed a notification button. At a minimum, the goal is to be able to distinguish those and provide a separate callback for the hardware buttons. audio_service isn't intended to necessarily be an exact mirror of Android's support libraries, but rather to provide access to the events on both Android and iOS, and on iOS things work a bit more closely to how I would like, in that when the user presses the play/pause button, the OS doesn't try to transform that into a play event or pause event for us, it leaves it as a "playPauseToggle" event and lets the app decide what to do with it.

Edit: just to clarify the above, it's not really the hardware button I care about so much not having the OS automatically convert the "playPause" event into a "play" or "pause" event so that it become impossible to distinguish between an actual "play" command from a client or the notification, vs a media button click.

nt4f04uNd commented 3 years ago

ok, i see. after all, i still think that notification should be handled through a custom reciever

ryanheise commented 3 years ago

I do use a custom receiver now. If you look through the history of different hacks I used, I originally coopted some of the rarely used MEDIA_* keycodes and remapped them so that I could distinguish between the ones that the support libraries were rewriting and the original untouched ones. This eventually ran into its limits, and then I implemented my own broadcast receiver, but that broadcast receiver at the moment leverages the support libraries to do most processing, and then can provide overrides just in specific cases where the support library route would lose or throw out critical information. So for this feature request, we can maybe eventually slot this in here: MediaButtonReceiver.

nt4f04uNd commented 3 years ago

this is real wrong to me. why do you remap actions into KEYCODEMEDIA and then feed them into MediaButtonReceiver, in the first place. why not just custom broadcast receiver? this would remove all your pain with remapping, this is so strange

nt4f04uNd commented 3 years ago

if you struggle to understand what i mean, here's an example i used in my app https://github.com/nt4f04uNd/sweyer/blob/master/android/app/src/main/java/com/nt4f04uNd/sweyer/receivers/NotificationReceiver.java

in the case of the plugin, as i see it, it would just receive an intent, get extra from it and call the callback to the dart side

nt4f04uNd commented 3 years ago

to clear it out even more - this would NOT remove the need in MediaButtonReceiver, which handles physical button clicks, this is just extracting the logic that handles notification into a separate receiver

ryanheise commented 3 years ago

The main reason why I have not yet removed that historical hack is because the overriding goal is to serve the app developers and provide the features and stability that make their apps possible. Internal changes are not the highest priority things for me to work on, though it would be satisfying to work on them, such is the way of priorities. If an app can't progress without feature x or bug fix y, that's generally what I work on first, and save the internal changes for when there is downtime.

Here, this feature request could benefit from an internal redesign but I'm not sure yet whether we want to have a second receiver. The last time I added a custom receiver it caused lasting headaches as people missed the README instructions on what to do. I guess one option is to include a <receiver> in the plugin's manifest and it will get merged in in the build process but before doing that I would want to consider whether we can implement the feature request with minimal changes/impact and do it in a way that is backwards compatible.

Note that my priorities right now are on getting the one-isolate release out there (focusing on stabilising the API and fixing critical bugs), so this feature is not critical for that release, and not on my priority list for the moment, but that is of course if I do everything myself. Contributions would always be welcome to make things move faster.

nt4f04uNd commented 3 years ago

receivers can be registered programmatically

i just searched it up and it looks there's a room to extend notification handling API on all supported platforms. i will give it a shot in the next few days

nt4f04uNd commented 3 years ago

ok i think i'm gonna contribute quite a bit to the plugin. in relation to this - it seems like you are using dartfmt, have you considered manual formatting in conjunction with some guidlines from the flutter's https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo, like these

it makes code a lot cleaner and i absolutely love how it works out

ryanheise commented 3 years ago

ok i think i'm gonna contribute quite a bit to the plugin.

Great to hear! As you're probably aware, I am struggling with a medical condition now so I am really glad to see more open source contributions.

in relation to this - it seems like you are using dartfmt, have you considered manual formatting in conjunction with some guidlines from the flutter's https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

For the same reason as above, I would like to make progress in the most economical fashion. From the document you linked:

We do not yet use dartfmt. Flutter code tends to use patterns that the standard Dart formatter does not handle well. We are working with Dart team to make dartfmt aware of these patterns.

Let's wait until the Dart team make dartfmt aware of these patterns.

nt4f04uNd commented 3 years ago

aren't you against i will format it for you and sumbit a PR amongst with other code-quality improvements? or this will be hard for you to write the code then?

Let's wait until the Dart team make dartfmt aware of these patterns.

i don't see this coming any time soon, my prediction is that is might be adressed the very least in the next year, if it ever will be. formatter might never become smart enough

ryanheise commented 3 years ago

Learning the style guide takes time (for all contributors), communicating this to all contributors takes time, enforcing this among all contributors takes time, dealing with PRs by people who use dartfmt and mess up the diffs takes time, and discussing this decision takes time. There is very little benefit (and the benefit is also subjective), and even if it takes more than a year, dartfmt is still fine and is what the Flutter team recommends for us at the moment. Let's focus on things that directly support people building great apps. Disagreeing or debating over formatting conventions is the least productive way for a software engineer to spend time, and after moving from company to company, seeing weird and unaesthetic conventions (to my tastes at least), I have accepted that these are unimportant decisions in the grand scheme of things. Life's short (really!) so let's focus on the things that matter.

nt4f04uNd commented 3 years ago

prototyping possible API i came to conclusion that we should do the following:

  1. MediaAction represents PlaybackStateCompat actions, which "Set the current capabilities available on this session.". this is Android only and there's nothing similar on other platforms. therefore we should:
    • rename MediaAction -> AndroidServiceAction
    • rename systemActions -> androidServiceActions
    • androidServiceActions should be ignored on other platforms and don't affect them in any way
  2. notification controls are very specific to each platform and therefore plugin must provide an interface to take advantages of each:
    • create NotificationControl - an abstract base class for notification controls
    • create controls classes for each platform
      • AndroidNotificationControl
      • CupertinoNotificationControl (which will be used on both iOS and macOS)
      • WebNotificationControl
    • create UniversalNotificationControl - this will combine most common controls to work by default and cross-platform
    • rename controls to notificatrionControls
  3. i'm not yet sure how to handle the seekbar, but it should likely either be:
    • a separate field in PlaybackState
    • or a separate subclass of NotificationControl, something like SeekControl and fed into notificatrionControls
  4. all notification actions will be reported to onNotificationAction(NotificationControl control) of AudioHandler, which will have a default implementation
ryanheise commented 3 years ago

People are already starting to use this branch and I have promised that things will not change in any "major" way before the release. So any radical changes like this will have to wait for the next major release which will hopefully not be for a long time to help with stability. So until then, I would like new features to be added in a way that is backwards compatible. Keep all the existing parameter names, etc, but find the most appropriate place to include these new media actions.

nt4f04uNd commented 3 years ago

surely i agree this feature is not for this release

ryanheise commented 3 years ago

I see this as two separate things: a feature and a renaming. The feature can actually be added in a non-breaking way, and could probably happen in the 0.18.x series but the renaming part could be considered independently later.

nt4f04uNd commented 3 years ago

renaming i proposed can be integrated in the 100% non-breaking way

however, i still would not hurry up with this, rather release 0.18.x and add all new APIs after that. this will be too much for this release, and personally i already implemented the part i needed in my app in a fork. now i would like to focus more on the app itself and let plugin to stabilize for some time

ryanheise commented 3 years ago

i don't actually see what breaking changes i suggested? everything i proposed can be integrated in the 100% non-breaking way.

Even if renaming systemActions (etc.) can be done in a non-breaking way (I assume you mean marking it as deprecated), I just don't want to deal with both the feature and the renaming at the same time. During the major(*) release cycle, the goal should be to fix bugs and add new features without renaming anything. Once the feature has been added, we can take a look at whether the renaming is worth it for the next breaking release.

(*) by which I just mean for example the 0.18.x series up until we bump that to 0.19.

nt4f04uNd commented 3 years ago

actually, i personally don't see a consistent way to split this. for me this would mean introducing ambiguity in what's old and what's new (if we add new API and don't deprecate the old as you suggest)

if you do know a better way and want to do this in 0.18.x, i will be only happy. if you were expecting me to implement this, i'm not going to do that before the next version

kurtisnelson commented 3 years ago

What is the current status of this? I'd like to add a custom action for our podcast player. (One big feature is that we allow you to bookmark content within a podcast for further research/listening)

ryanheise commented 3 years ago

Hi @kurtisnelson

This is not one of the features scheduled for inclusion in the next release, but in the meantime, you have a couple of options:

  1. Help by submitting a pull request.
  2. Hack around it by co-opting one of the existing supported media actions that your app is not already using, and just change it's icon to your own purposes.

According to the documentation:

  /// The list of currently enabled controls which should be shown in the media
  /// notification. Each control represents a clickable button with a
  /// [MediaAction] that must be one of:
  ///
  /// * [MediaAction.stop]
  /// * [MediaAction.pause]
  /// * [MediaAction.play]
  /// * [MediaAction.rewind]
  /// * [MediaAction.skipToPrevious]
  /// * [MediaAction.skipToNext]
  /// * [MediaAction.fastForward]
  /// * [MediaAction.playPause]
  final List<MediaControl> controls;

So for example if you are not using the rewind button in your notification, you could co-opt this for your purposes, and implement the corresponding action in your rewind() method.

ryanheise commented 3 years ago

I should add that if your Flutter UI actually has a rewind button, you will need to remember that it won't be able to bind to the rewind media action since that now has a different function. Instead you would need to create a custom action for your genuine rewind.