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

chore(capabilities): Revamp Capabilities #2076

Closed
wants to merge 8 commits into from
Closed

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Jul 27, 2023

This PR revamps the system of capabilities and the updateOptions parameters. It associates all related objects together to simplify and give finer control over options.

Capabilities now differentiate (particularly on Android) between items that:

  1. should be used as media session actions
  2. should be included in the notification (and whether they are allowed in compact mode)

The Android side requires to be used in conjunction with: doublesymmetry/KotlinAudio#86

@puckey
Copy link
Collaborator

puckey commented Jul 27, 2023

I really like the direction this is headed! On the typescript side of things, I was wondering what you think of a format like:

await TrackPlayer.updateOptions({
  capabilities: {
    play: true,
    pause: true,
    skipToNext: true,
    skipToPrevious: {
      // optional: enabled: true,
      compact: false,
      icon: require('./previous.png')
    },
    seekTo: true
  }
})
  • we are no longer dealing with order, so an object is more fitting than an array
  • no need for any extra imports
  • no difference between what you see (function calls) and the data that it produces
  • nicely typeable

@dcvz
Copy link
Contributor Author

dcvz commented Aug 1, 2023

I really like the direction this is headed! On the typescript side of things, I was wondering what you think of a format like:

await TrackPlayer.updateOptions({
  capabilities: {
    play: true,
    pause: true,
    skipToNext: true,
    skipToPrevious: {
      // optional: enabled: true,
      compact: false,
      icon: require('./previous.png')
    },
    seekTo: true
  }
})
  • we are no longer dealing with order, so an object is more fitting than an array
  • no need for any extra imports
  • no difference between what you see (function calls) and the data that it produces
  • nicely typeable

I like the typeability aspect but it's also very rigid - we're setting a contract for the keys and fields in there, which is ok I guess in an API but I think it limits us a bit. Changing these will more easily result in being a breaking change.

For example: renaming or adding additional fields will change the typing. Any new keys introduced to objects will have to be ones that are initial set to undefined to not break previous versions. With the currently proposed approach since you use the function builders like Capability.Play() you can rename properties or introduce new ones with default values and not get any breaking changes.

It also makes the structure a bit complicated - for some capabilities that don't support notification at all we might have something like: playFromId: true | false but for capabilities that also support capabilities we'll need:

play: {
  enabled: true | false,
  notification: true | false,
  compact: true | false,
  icon: asset | undefined
}
or 
play: undefined | {
  notification: true | false,
  compact: true | false,
  icon: asset | undefined
}

Keep in mind that on Android there are three distinctions. Just because we have a capability for the media it does not mean it has to be in the notification, nor in the compact one -- so those are three cases we have to enable enabling/disabling.

In the current proposed solution whether a capability is present or not denotes the capability - and then we have the actual object which dictates if it also appears in the notification/compact.

Copy link
Contributor

github-actions bot commented Nov 6, 2023

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 6, 2023
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Nov 14, 2023
@dcvz dcvz reopened this Nov 14, 2023
@github-actions github-actions bot removed the Stale label Nov 15, 2023
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 14, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants