[Feature Request] Provide a more stable API for PlaybackTrackChanged

[uzegonemad]

Describe the bug My app sometimes rebuilds the track queue under certain circumstances (e.g. losing internet connectivity). To keep things simple, I loop through the entire queue and call remove() on each track, then re-add the tracks that are currently playable.

When you remove a track that is earlier in the queue than the current track, the PlaybackTrackChanged event is called.

I don't think this is correct because although the track's index is now different in the queue, the track itself did not actually change.

I've tested this on iOS, not sure if it's an issue on Android yet or not.

To Reproduce

1. Add tracks to queue.
2. Go to any track after the first track, e.g. 3
3. Remove any track earlier in the queue, e.g. 1

Environment (please complete the following information): react-native info

System:
    OS: macOS 10.15.7
    CPU: (8) x64 Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
    Memory: 30.45 MB / 16.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 10.13.0 - /var/folders/9v/b24jnwd51ws4y9f21tn9syfm0000gn/T/yarn--1633108112407-0.8994549613595215/node
    Yarn: 1.22.5 - /var/folders/9v/b24jnwd51ws4y9f21tn9syfm0000gn/T/yarn--1633108112407-0.8994549613595215/yarn
    npm: 6.14.8 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.10.2 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: iOS 14.4, DriverKit 20.2, macOS 11.1, tvOS 14.3, watchOS 7.2
    Android SDK:
      API Levels: 23, 25, 26, 27, 28, 29
      Build Tools: 23.0.1, 25.0.0, 25.0.2, 25.0.3, 26.0.1, 26.0.2, 27.0.3, 28.0.2, 28.0.3, 29.0.2, 31.0.0
      System Images: android-23 | Google APIs Intel x86 Atom, android-29 | Google APIs Intel x86 Atom, android-29 | Google Play Intel x86 Atom, android-30 | Google Play Intel x86 Atom
      Android NDK: Not Found
  IDEs:
    Android Studio: 4.0 AI-193.6911.18.40.6821437
    Xcode: 12.4/12D4e - /usr/bin/xcodebuild
  Languages:
    Java: 1.8.0_65 - /usr/bin/javac
    Python: 2.7.16 - /usr/bin/python
  npmPackages:
    @react-native-community/cli: Not Found
    react: 16.13.1 => 16.13.1
    react-native: 0.63.4 => 0.63.4
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

What react-native-track-player version are you using? 2.0.3

Are you testing on a real device or in the simulator? Which OS version are you running? Both. sim: iOS 14.4, physical: iOS 14.7.1

Code Something like this should be simple enough to repro:

await TrackPlayer.skipToNext();
const currentTrackIndex = await TrackPlayer.getCurrentTrack();
await TrackPlayer.remove(currentTrackIndex-1);

[bradfloodx]

Good catch @uzegonemad

Any chance you could raise a PR to fix this?

Cheers, Brad

[jspizziri]

@uzegonemad can you please see if v2.2.0-rc3 fixes your issue?

[jspizziri]

I discussed this issue with @dcvz . We're both of the opinion that any good solution here would require a breaking change to the library.

This is definitely something that needs to be fixed, however it's probably not something that can be fixed until we start iterating on a 3.0 version (which could be shortly after 2.2 lands)

[github-actions[bot]]

This issue 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.

[jspizziri]

As mentioned earlier, we're going to consider this as a breaking feature for v4.0 I've added it to the list for discussion here

https://github.com/doublesymmetry/react-native-track-player/issues/1637

[jspizziri]

A point of discussion when considering this refactor. Some of these events don't entirely make sense. See my comments on this issue for more detail.

[puckey]

In the following pr, I am deprecating PlaybackTrackChanged and replacing it with Event.PlaybackActiveTrackChanged which will be emitted with the following data:

export interface PlaybackActiveTrackChangedEvent {
  /** The index of previously active track. */
  lastIndex: number | undefined;

  /**
   * The previously active track or `undefined` when there wasn't a previously
   * active track.
   * */
  lastTrack: Track | undefined;

  /**
   * The position of the previously active track in seconds or undefined if
   * there wasn't a previously active track.
   * */
  lastPosition: number | undefined;

  /**
   * The newly active track index or `undefined` if there is no longer an
   * active track.
   * */
  index: number | undefined;

  /**
   * The newly active track or `undefined` if there is no longer an
   * active track.
   **/
  track: Track | undefined;
}

(This pr also implements TrackPlayer.setQueue, which is probably what you want to be using.)

But you raise a good point: should we emit the event if only the track's index changed? I am undecided on this, but leaning toward only emitting when the track changed from undefined or another track. @jspizziri do you have thoughts on this specific question?

[jspizziri]

EDIT: I've realized this is a bad idea. See a revision below.

---

@puckey @dcvz @mpivchev

I'd like to take a step back and examine both the queue and all the queuing events that users might care about and implement them from a holistic perspective. Here are the ones that come to mind for me with some proposals for their behavior. I also want to add that I think having automated tests to implement this correctly is going to be absolutely necessary. It's a complete nightmare trying to test all of this manually with all the different queue states and repeat modes. Sorry in advance for how lengthy this is.

Queue Behaviour

First I'd like to talk about how the queue behaves. This is how I think it should operate:

1. The Queue is FIFO
2. In general when the head item unloads from the queue, it is gone. All the indexes effectively change.
3. IFF RepeatMode.Off is configured THEN when the head of the queue unloads it should disappear. This effectively would change all the indexes in the queue each time an item unloads.
4. IFF RepeatMode.Queue is configured THEN when the head of the queue unloads, it should be requeued at the end of the queue. This effectively would change all the indexes in the queue each time an item unloads.
5. IFF RepeatMode.Track is configured THEN when the head of the queue unloads, that item should be immediately re-inserted at the head of the queue again and begin to play. This effectively WOULD NOT change all the indexes in the queue.

I propose that the each track in the Queue should be automatically assigned a "Queue UUID" that uniquely represents that queue item. I want to be clear here that I don't mean it represents a Track but a unique instance of a track in the queue, similar to a message ID. I think this should not be assignable by the user. When tracks repeat, they would be assigned a new UUID.

export interface QueuedTrack {
  quid: string; // unique uuid for this track in the queue generated by the queue itself on `add`
  track: Track;
}

This might seem strange at first, but here's the rationale:

Queue indexes are fundamentally unreliable, they're changing constantly. Additionally, when you deal with the recurrence or repeat of track index can be even more unreliable. What happens when you try to remove a track from the queue by index right when the head element unloads? The index is now different (or IMO it should be). Moreover, I think the existing interface has some issues when it comes to having a queue with a repeated track sprinkled throughout it. Users likely don't want to entirely remove a track from the queue, but rather only specific queued positions of that track.

With that said, the following is my proposal for Queue methods & events.

---

Methods

Effectively, allow for the use of indexes but use at your own risk:

Method	Before	After
add	add(tracks: Track[], insertBeforeIndex)	add(tracks: Track[], insertBeforeQUIDOrIndex: string \| number): Promise<QueuedTrack[]>
remove	remove(tracks: Track[])	remove(quidsOrIndexes: string[] \| number[])
skip	skip(index, initialPosition)	skip(quidOrIndex: string \| number, initialPosition: number)
updateMetadataForTrack	updateMetadataForTrack(index, metadata)	updateMetadataForTrack(quidOrIndex: string \| number, metadata)
getCurrenTrack	getCurrenTrack(): Promise<Track>	getActiveQueuedTrack(): Promise<QueuedTrack>
getQueue	getQueue(): Promise<Track[]>	getQueue(): Promise<QueuedTrack[]>
getTrack	getTrack(index: number): Promise<Track>	getQueuedTrack(quidOrIndex: string \| number): Promise<QueuedTrack>
removeUpcomingTracks	removed in favor of reset()	removed in favor of reset()

Events

Queue Order Changed

As we look towards adding things like the ability to natively shuffle, we should notify the user if the order of the queue changed. If they care about that they can pull the queue to update their UI's. This also addresses the cascading index change problem that we have when you insert a track at a specific position in the queue. If you care about it, simply observe the event and refetch the queue.

Proposed Interface

(empty event)

export interface QueueOrderChanged {}

Queue Length Changed

This one I'm not so sold on, but I have had multiple users request it. I tell them that they need to infer it based on when they're calling add/remove/etc.

NOTE: this could get tricky when you're requeueing tracks for RepeatMode.Queue

Proposed Interface

export interface QueueLengthChanged {
  previousLength: number;
  currentLength: number;
}

Queue Ended

Proposed Interface

(empty event) - see my note on PlaybackActiveTrackChanged as to why this is empty.

export interface QueueEnded {}

Active Track Changed

This event would fire IFF the actively loaded/playing track was mutated (via loading or unloading). Please Note: I propose we do away with the index on the event. Indexes in queues change constantly, especially with my proposal on how the queue should work in general. As such I don't feel that index is really helpful. The only thing it's really being used for is fetching the track object, which is why I think we should simply send the track objects instead.

Proposed Interface

// NOTE: not sure of a good name for this
export interface TrackEvent {
  /** the relevant track object **/
  /** NOTE: this should really be `QueuedTrack` not `Track` based on the above. **/
  track: Track;

  /** position of the track at the time of the event **/
  position: number;
}

export interface PlaybackActiveTrackChanged {
  /** if loaded is `undefined` then no track has loaded signifying no playback **/
  loaded: TrackEvent | undefined;

   /** if unloaded is `undefined` then no track was previously playing **/
  unloaded: TrackEvent  | undefined;
}

Furthermore this is how I propose it works in various repeat modes (for reference see all the possible cases here):

1. RepeatMode.Track - each time the track repeats a new event fires both loaded and unloaded would contain the track if appropriate.
2. RepeatMode.Queue - Behaves normally in multitrack queues. Behaves like RepeatMode.Track in single track queues.
3. RepeatMode.Off - called when the final track unloads. Both loaded and unloaded would be undefined in this scenario.

Incidentally, I think this event could replace PlaybackQueueEnded entirely as if both loaded and unloaded are undefined, you know the queue has terminated. I think this event should be observed if you care about what the last track was in the queue.

RFC @bradleyflood @uzegonemad

[puckey]

Thanks for your extensive thoughts on this subject, I will need some time to fully grep what you are proposing.. I think this kind of proposal should be discussed in the context of a major rethinking of the library / api as it fundamentally rethinks how tracks are considered (both id and index) and how a queue functions (FIFO). Perhaps we should think about how to create a space that allows for this kind of fundamental work – in any case I think it deserves its own thread / issue.

Within the context of what the functionality of RNTP currently is, do you have thoughts on whether we should emit the event if only the track's index changed?

[jspizziri]

@puckey no, I don't think we should emit events if the only thing that changes is an index. I think we should emit events when the active track loads/unloads and if the queue order changes we should emit a QueueOrderChanged event (very similar to my above proposal, but just a minimal version of it).

[bradfloodx]

Good thoughts @jspizziri.

Re FIFO

How would a .skipToPrevious() work?

For my use-case I'm essentially copying Apple Music. So a user might be half way through a playlist of songs when they decided to skip back to previous songs. I rely on the history of items in the queue to skip back to them.

Also when skipping back, I need to know when I am back to the beginning of the Playlist, so that the user can't skip back anymore as there is nothing earlier.

I'm not sure if these scenarios could be handled well if there was no "historic" queue due to fifo.

Thoughts?

Re Events

As I've mentioned before, the PlaybackTrackChanged event is difficult to use in its current state. I'm having to debounce() it to get the actual playing track. So your proposal of a PlaybackActiveTrackChanged would be good. Especially if it emits the new Track... awesome!

I agree with fetching the track too, getCurrentTrack() would ideally return a Track not an index.

Re Queue UUID

I agree here too. Having a UUID key would be great.

For reference, in my current implementation our User will have a Playlist, and each Song in that Playlist will have a key: uuidV4() value assigned to it (even before I add it to TrackPlayer). So if the exact same Song is added to my Playlist multiple times then I can distinguish each individual item in the Playlist by the key value. For instance when a User deletes a Song from the Playlist I need to find that particular key and delete it from the TrackPlayer queue too.

Likewise when I am showing in the UI which Song is currently playing in the Playlist UI, I match the key value in the Playhead with that in my Playlist. (I have added the key to the Track object, and I listen for the PlaybackTrackChanged event and fetch the currently playing Track and it's key to know what Playlist Item is currently playing).

Removing removeUpcomingTracks()

I often use this method. For instance if a user toggles Shuffle > On, then I need to clear all upcoming tracks (whilst continuing to play the current track) and then shuffle the original Playlist they were playing and queue ALL the Songs in that Playlist ahead of the current track (I can't just shuffle the remaining queue). Likewise when they toggle Shuffle > Off, I have to removeUpcomingTracks(), find the location of the currently playing Song. in the active Playlist, and then queue the remaining playlist items in their original order from the place of the current track.

I also wish there was a removePreviousTracks() method too, as I sometimes need to rewrite the history (for instance when a user reorders items in a Playlist I need to reorder them in the queue too (either ahead or behind the currently playing track).

[bradfloodx]

Just thinking about this in bed tonight... 🤓

IF RepeatMode.Off is configured THEN when the head of the queue unloads it should disappear

But what happens when repeat mode is turned on after several tracks have already popped off the queue? You can't repeat them because they don't exist anymore. But the user would expect them to be repeated.

[jspizziri]

@bradleyflood thanks for your feedback. The problems you mention are valid and could be solved by an additional LIFO history queue... but after thinking about this more, I finally had a critical realization about this lib (late to the party I'm sure). We don't actually have a queue at all (which I realize now is what @mpivchev has been discussing on other tickets). We really just have a dynamic ordered playlist. Actually making this lib behave like a queue would be a bad idea as I think it's very much against the design, and it's really just a confusion of terminology.

In the context of a playlist, there really is no such thing as "history", only the playlist. For instance, if a user plays a track, than clears the playlist and plays something else, this library won't know how to go back to what they were listening to before. Which could be a problem, but I think is just the way it is.

I've thought up a variation of my proposal last night that is far less radical. I'll post here soon.

[jspizziri]

@puckey @dcvz @mpivchev @bradleyflood

Here's my updated proposal based on my realizing some fundamental misunderstandings I had about the library (yep, I'm a dope).

Queue

Queue vs Playlist

First, as @mpivchev has mentioned in other tickets, we don't have a Queue (this is the key fact that I'm finally catching up to, which is either proof of my ignorance or of the level of confusion this can cause). We have a dynamic & ordered list or Playlist. As Milen has suggested we need to rename everything having to do with Queues to Playlists.

Problems With Indexes

In a dynamic list, indexes are fundamentally unreliable, as small mutations in the list can cause cascading changes in the index. Consider the following examples of valid playlists:

NOTE: all letters represent unique instances of tracks, indexing is the same as it would be in an array.

// prior to mutation
[A, B, C]

// insert at position 1
[A, D, B, C]
    ^ 

The above example illustrates the example of an insert causing a cascading index update. This is a hard problem for users to solve given the event-oriented and async nature of this library. If they now want to remove an element that used to be at index 2 the need to fetch the entire Playlist and search for their Track to find that it's at position 3 now.

I propose that the each track added to the Playlist should be represented by a PlaylistItem which would have a unique Playlist UUID automatically assigned to it. I want to be clear here that I don't intend a PlaylistItem to represent a Track, but rather a unique instance of a track in the playlist. I think this should not be assignable by the user, and it is immutable for the time the item exists in the Playlist.

export interface PlaylistItem {
  pid: string; // unique uuid for this track in the playlist generated by the playlist itself on `add`
  track: Track;
}

Problems With Track Removal

Incidentally, the PlaylistItem object solves another problem I see, which is the removal of Tracks that occur multiple times in a playlist. Consider the following example:

[A, B, C, A]

Notice the duplicate occurrence of track A, which is perfectly valid. Currently, we provide a remove(tracks: Track[]) function. But as you can see from the above example, this could cause problems. I may not want to remove A entirely from the playlist, but rather only remove a specific occurrence of it. Removal of an item from the Playlist should only be possible by specifying a set of unique PlaylistItems rather than Tracks.

Methods

Effectively, allow for the use of indexes but use at your own risk:

Method	Before	After
add	add(tracks: Track[], insertBeforeIndex)	add(tracks: Track[], insertBeforeQUIDOrIndex: string \| number): Promise<PlaylistItem[]>
remove	remove(tracks: number[])	remove(pidsOrIndexes: string[] \| number[])
skip	skip(index, initialPosition)	skip(pidOrIndex: string \| number, initialPosition: number)
updateMetadataForTrack	updateMetadataForTrack(index, metadata)	updateMetadataForTrack(pidOrIndex: string \| number, metadata)
getCurrenTrack	getCurrenTrack(): Promise<Track>	getActivePlaylistItem(): Promise<PlaylistItem>
getPlaylist	getQueue(): Promise<Track[]>	getPlaylist(): Promise<PlaylistItem[]>
getTrack	getTrack(index: number): Promise<Track>	getPlaylistItem(pidOrIndex: string \| number): Promise<PlaylistItem>
removeUpcomingTracks	removeUpcomingTracks()	removeUpcomingPlaylistItems()

Events

Playlist Order Changed

As we look towards adding things like the ability to natively shuffle, we should notify the user if the order of the playlist changes. If they care about that they can pull the playlist to update their UI's. This also addresses the cascading index change problem that we have when you insert a track at a specific position in the playlist. If a user really wants to user indexes, simply observe the event and refetch the playlist and do the rest of the work on their side.

Proposed Interface

(empty event)

export interface PlaylistOrderChanged {}

Playlist Length Changed

This one I'm not so sold on, but I have had multiple users request it. I tell them that they need to infer it based on when they're calling add/remove/etc.

Proposed Interface

export interface PlaylistLengthChanged {
  previousLength: number;
  currentLength: number;
}

Playlist Ended

Proposed Interface

(empty event) - see my note on PlaybackActivePlaylistItemChanged as to why this is empty.

export interface PlaylistEnded {}

Active Track Changed

This event would fire IFF the actively loaded/playing track was mutated (via loading or unloading). Please Note: I propose we do away with the index on the event. Indexes in playlists change constantly, especially with my proposal on how the playlist should work in general. As such I don't feel that index is really helpful. The only thing it's really being used for is fetching the track object, which is why I think we should simply send the track objects instead.

Proposed Interface

// NOTE: not sure of a good name for this
export interface PlaylistItemEvent {
  /** the relevant PlaylistItem object **/
  item: PlaylistItem;

  /** position of the track at the time of the event **/
  position: number;
}

export interface PlaybackActivePlaylistItemChanged {
  /** if loaded is `undefined` then no track has loaded signifying no playback **/
  loaded: PlaylistItemEvent | undefined;

   /** if unloaded is `undefined` then no track was previously playing **/
  unloaded: PlaylistItemEvent  | undefined;
}

Furthermore, this is how I propose it works in various repeat modes (for reference see all the possible cases here):

1. RepeatMode.Track - each time the track repeats a new event fires both loaded and unloaded would contain the track if appropriate.
2. RepeatMode.Playlist - Behaves normally in multitrack playlists. Behaves like RepeatMode.Track in single track playlists.
3. RepeatMode.Off - called when the final track unloads. Both loaded and unloaded would be undefined in this scenario.

Incidentally, I think this event could replace PlaybackPlaylistEnded entirely as if both loaded and unloaded are undefined, you know the playlist has terminated. I think this event should be observed if you care about what the last track was in the playlist.

RFC @uzegonemad

[uzegonemad]

@jspizziri This looks fine to me.

A couple minor typos I noticed:

• insertBeforeQUIDOrIndex should be insertBeforeUUIDOrIndex
• getCurrenTrack should be getCurrentTrack

PlaylistItem has a pid while it's refered to as UUID elsewhere, so we would just want to settle on one naming convention.

Ultimately, I don't have a strong preference on whether the library implements a UUID or not. However you may or may not know that this library used to function on IDs, and during the v2 development cycle IDs were removed and replaced with indexes. See #688

I don't recall all of the reasons why the change was made but maybe @Guichaguri or @dcvz can chime in with some of the original pain points which lead to the removal of IDs in the first place? Maybe managing the additional layer required for having IDs was just too much overhead at the time.

---

For my use case, it would be neat to be able to use the UUID of the current track to update the rest of the queue/playlist. Consider the following pseudo-code.

const playlist = await getPlaylist(); // [A, B, C, D]

const currentItem = await getActivePlaylistItem(); // B
const currentId = currentItem.pid;

const idsToRemove = playlist.filter(i => i.id != currentId).map(i => i.pid);

await remove(idsToRemove); // Queue is now [B]

let newQueue = [
    {url: '...', title: 'E'},
    {pid: currentId},
    {url: '...', title: 'F'}
];

add(newQueue); // Queue is now [E, B, F]

I do realize that adds additional code for this library to have to maintain, so we're probably better off just having people call add() twice in that scenario. Just figured I'd throw it out.

[github-actions[bot]]

This issue 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.

[jspizziri]

Closed by https://github.com/doublesymmetry/react-native-track-player/pull/1713