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

just_audio_background: notification builder #912

Open
Zekfad opened this issue Dec 31, 2022 · 9 comments · May be fixed by #914
Open

just_audio_background: notification builder #912

Zekfad opened this issue Dec 31, 2022 · 9 comments · May be fixed by #914
Assignees
Labels
1 backlog enhancement New feature or request

Comments

@Zekfad
Copy link
Contributor

Zekfad commented Dec 31, 2022

Is your feature request related to a problem? Please describe.
It would be very helpful to have customizable notification behavior for just_audio_background.

Describe the solution you'd like
Expose builder for modifying notification config.

Describe alternatives you've considered
Its's possible to add config? But I think it wound add unnecessary complexity.

Additional context
Currently notification properties is controlled via this chunk of code:

final controls = [
if (hasPrevious) MediaControl.skipToPrevious,
if (_playing) MediaControl.pause else MediaControl.play,
MediaControl.stop,
if (hasNext) MediaControl.skipToNext,
];
playbackState.add(playbackState.nvalue!.copyWith(
controls: controls,
systemActions: {
MediaAction.seek,
MediaAction.seekForward,
MediaAction.seekBackward,
},
androidCompactActionIndices: List.generate(controls.length, (i) => i)
.where((i) => controls[i].action != MediaAction.stop)
.toList(),

So we can make a builder that will receive micro state snapshot (has previous, has next, etc), and use it's result in this method.

I think its overkill to use audio_service for this single change, because you would need to reinvent (or copy-paste) 99% of already existing code from just_audio_background.

@Zekfad Zekfad added 1 backlog enhancement New feature or request labels Dec 31, 2022
@Zekfad
Copy link
Contributor Author

Zekfad commented Dec 31, 2022

I'm panning to fire PR for this in a few days, what the best solution you'd suggest, so I'll put my efforts into desired approach?

@ryanheise
Copy link
Owner

My thoughts:

It is simple enough to add customisation options for which controls to display, but it is not as simple to decide how to customise the behaviour of those buttons. For example, play and pause are straight forward, but then to fast forward or skip forward, you need to think about the skip interval. Whatever is done should be carefully thought out so as to minimise any future corrections of the API design that would break backwards compatibility.

If you are keen to create a PR for this, it would be worth first describing and discussing your proposed API design below before implementing it. It may also be worth searching through past issues where people have requested customisation features for just_audio_background as that may inform the best API design.

@Zekfad
Copy link
Contributor Author

Zekfad commented Jan 1, 2023

Hi, thank you for your response.
I've done some search before opening issue, couldn't find related issues (I guess I missed some?)

I want to propose following API (names subject to change):

Class for notification button

@immutable
class NotificationAction {
  const NotificationAction({
    required this.action,
    this.showInCompact = false,
  });

  final MediaAction action;
  final bool showInCompact;
}

Builder function (new optional argument to initializer)

typedef NotificationActionsBuilder = Iterable<NotificationAction> Function(AudioPlayer player);

So we will pass function that will take our player instance (allowing us to inspect player state, e.g. has next, has previous, playing or paused, etc.). This function will return list of action configs. By examining this list we can decide on actions and whether they'll be shown in compact mode.

This proposal allows user to decide on logic and can be easily modified, without breaking compatibility between future versions.

@ryanheise
Copy link
Owner

Why not just mirror the audio service API for updating the notification rather than invent a new API?

@Zekfad
Copy link
Contributor Author

Zekfad commented Jan 1, 2023

In my vision just_audio_background is a simple drop in solution for background playback, so I think it would be nice to have slightly simpler API, than just mirroring low level abstraction. (And this will make it more independent of audio service API changes in future).

@ryanheise
Copy link
Owner

The API for compact actions has been discussed within audio_service, and some people wanted to be able to set the order of the compact actions, and hence the list of compact action indices was favoured over a boolean flag on each action. I see the appeal of trying to simplify that, but there appear to be use cases that would favour mirroring the audio_service API here.

There is also a distinction between media actions and media controls. If you choose not to mirror this, probably people will later request it since there are valid use cases for these.

I wouldn't worry about making the API resilient to changes to audio_service, since just_audio_background is intended to remain in sync with audio_service.

@Zekfad
Copy link
Contributor Author

Zekfad commented Jan 1, 2023

Ah, so that's why it was made with indices. About media controls, I've missed them, so here's new proposal:

@immutable
class NotificationConfig {
  const NotificationConfig({
    this.controls = const [],
    this.systemActions = const {},
    this.androidCompactActionIndices,
  });

  final List<int>? androidCompactActionIndices;
  final List<MediaControl> controls;
  final Set<MediaAction> systemActions;
}
typedef NotificationConfigBuilder = NotificationConfig Function(AudioPlayer player);

@ryanheise
Copy link
Owner

One other twist is the latest changes in Android 13 which deprecate the compact action indices:

https://developer.android.com/about/versions/13/behavior-changes-13#playback-controls

Although I guess that doesn't really affect the API design choices here because the new behaviour is all automatic based on the playback state, while in terms of the old behaviour, it will take years for the older versions of Android to be phased out and we'll still need to support them.

I'd be happy for you to submit a PR proposal to see how it would work in practice, where I assume the notification builder mechanism could also be used to define the default behaviour via an internal default builder. We can discuss the class names later, and we can also later have a look at whether a builder is the right mechanism here.

@Zekfad Zekfad linked a pull request Jan 5, 2023 that will close this issue
@BraveEvidence
Copy link

This will help https://www.youtube.com/watch?v=IMQdSTlTXjA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 backlog enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants