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

feat: notification config builder #914

Open
wants to merge 3 commits into
base: minor
Choose a base branch
from
Open

Conversation

Zekfad
Copy link
Contributor

@Zekfad Zekfad commented Jan 5, 2023

resolves #912

adds NotificationConfig that contains config
adds NotificationConfigBuilder that is used to accure config
adds PlayerStateSnapshot that provides access to state to builder

adds `NotificationConfig` that contains config
adds `NotificationConfigBuilder` that is used to accure config
adds `PlayerStateSnapshot` that provides access to state to builder
@Zekfad
Copy link
Contributor Author

Zekfad commented Jan 5, 2023

@ryanheise here's discussed prototype.
Default behavior is preserved and working (I've done testing on chrome and my android phone).
I've also added PlayerStateSnapshot to expose state, because _PlayerAudioHandler is internal, has many methods and properties that are undesirable to be exposed.


/// Snapshot of current player state.
/// Used to provide data to [NotificationConfigBuilder].
abstract class PlayerStateSnapshot {
Copy link
Owner

Choose a reason for hiding this comment

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

It's unfortunate to replicate existing state classes that are in just_audio, but just_audio_background should not depend on just_audio so this is understandable for now.

Eventually, I think these state classes should be defined in just_audio_platform_interface where they can be reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, is this in scope of this PR, if so, tell me where I should move it, please?

Copy link
Owner

Choose a reason for hiding this comment

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

That comment was just to recognise the need for something now even though it should be moved elsewhere in the future.

But as it is, is this really a "snapshot"? It looks like you're using this as a mixin (although you defined it as an abstract class instead of a mixin). That means that this package is now returning a reference to an internal class which it probably shouldn't. If the so-called snapshot is actually an object where the values may change behind the scenes, then it is not really a snapshot. I think it would be far less surprising to create an immutable state object, and to build a new instance of it each time a new state needs to be broadcast.

As for naming, and considering where these state objects will likely move in the future, I would say we do not know at this stage what that future state object will look like, and anything we might come up with now would be rather narrow in its vision. So to avoid any future problems, it might be best to choose a class name for this state that is obviously specific to this particular package rather than some name that we might want to reserve for the future ideal API.

since this class is internal, but exposed to provider access to the
interface
@Zekfad Zekfad requested a review from ryanheise January 13, 2023 23:11
@ryanheise
Copy link
Owner

Just as an update on where I'm at with this PR, I am not yet 100% sure on which way we want to go on the API design.

If we look at the default implementation of the notification builder:

_defaultNotificationConfigBuilder(PlayerStateSnapshot state) {
    final controls = [
      if (state.hasPrevious) MediaControl.skipToPrevious,
      if (state.playing) MediaControl.pause else MediaControl.play,
      MediaControl.stop,
      if (state.hasNext) MediaControl.skipToNext,
    ];
    return NotificationConfig(
        controls: controls,
        systemActions: const {
          MediaAction.seek,
          MediaAction.seekForward,
          MediaAction.seekBackward,
        },
        androidCompactActionIndices: List.generate(controls.length, (i) => i)
          ..removeAt(controls.indexOf(MediaControl.stop)));
  }

I still find this PlayerStateSnapshot problematic. Of course as discussed I think eventually just_audio should be restructured so that the state classes are moved into the platform interface package where they can be reused, so we don't have this redundant data structure. Right now, it is not possible to, say, declare the parameter type of this builder function to be AudioPlayer since just_audio_background cannot introduce a compile time dependency on just_audio. However, the app itself CAN have a compile time dependency on both just_audio and just_audio_background, and so one option is to simply remove this parameter, and allow the app to hold onto its own reference to the AudioPlayer instance and query its state.

In terms of API naming, an app may display other kinds of notifications, and so it may perhaps be a good idea to prefix these class names with Media. So, MediaNotificationConfig and MediaNotificationConfigBuilder (and also see if it's possible to delete PlayerStateSnapshot).

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

Successfully merging this pull request may close these issues.

just_audio_background: notification builder
2 participants