BrainzPlayer datasources improvements #1539
Conversation
Implement two methods on each source to 1. determine if a track was played from that service and 2. determine if that datasource can be used to search and play a track
Otherwise skip to the next song without any message
Each datasource returns null in the render method when not activated or not selected
when no datasource can be used to search for songs
We have to create the iframe on load (even if we are not currently using this datasource), otherwise the youtube player does not get initialized correctly for later use
My previous attempts were optimistic; we do definitely need to try with the next datasource if the first one failed.
Throttling the calls to trackInfoChange (youtube calls it twice in succession) and show title on screen in an 'info' toast.
And removing lingering access_token in test fixtures. After merging master
| const { onCurrentListenChange } = this.props; | ||
| onCurrentListenChange(listen); |
There was a problem hiding this comment.
This moved up to the beginning of the function
| @@ -280,44 +295,6 @@ export default class BrainzPlayer extends React.Component< | |||
| }); | |||
| }; | |||
|
|
|||
| getSourceIndexByListenData = (listen: Listen | JSPFTrack): number => { | |||
There was a problem hiding this comment.
This was moved to the datasources themselves that now implement an isListenFromThisService method
| // Try playing the listen with the next dataSource | ||
| this.playListen(currentListen, currentDataSourceIndex + 1); | ||
| } else { | ||
| this.handleWarning( |
There was a problem hiding this comment.
This was just too annoying.
| {artist && ` — ${artist}`} | ||
| </> | ||
| ); | ||
| this.handleInfoMessage(message); |
There was a problem hiding this comment.
Show a notification with the song title on a blue background.
I also want to implement (in another PR) native browser notifications, with this home-made notification as a fallback.
There was a problem hiding this comment.
Follow up on that feature in PR #1561
| }; | ||
|
|
||
| // eslint-disable-next-line react/sort-comp | ||
| throttledTrackInfoChange = _throttle(this.trackInfoChange, 2000, { |
There was a problem hiding this comment.
The Youtube player was calling this method twice in short succession when loading a song.
| const hasDatasourceToSearch = | ||
| this.dataSources.findIndex((ds) => | ||
| ds.current?.canSearchAndPlayTracks() | ||
| ) !== -1; |
There was a problem hiding this comment.
This (and the associated message line 468) might or might not be useful depending on wether we have a default player (like youtube using API keys) or if they all require auth. It does appear for a flash we the page loads, but suppose it doesn't hurt much to have clear indications in any case.
Associated ticket: https://tickets.metabrainz.org/browse/LB-907
Instead of trying blindly, we can check if there is a possibility a datasource can attempt to play the track (looking at you SoundCloud…) and skip it if it can't.
This PR changes a few things with how we play music with BrainzPlayer:
Instead of trying to play a track with each datasource until one works, we check if any of the datasources will be able to play the track, and if not we skip it.That was a dumb idea and broke our fallback strategy of trying to play on any of the datasources it could.
The previous functionality of determining the source of a listen and finding a usable datasource is improved and partly moved from the BrainzPlayer component to two new methods in the DataSource interface:
isListenFromThisServiceandcanSearchAndPlayTrackswe also reduce the verbosity of warnings, removing the "We couldn't find a matching song on any music service we tried" messages
while adding a new toast notification with the song title when we change track
we ensure that nothing happens with the player until a direct user interaction to play a song:
don't render anything if a datasource hasn't been selected to play (rendering iframes before a track is played sometimes creates problems)This broke the Youtube player whose iframe needs to be initialized first even if it is not yet in usefinally, we add a helpful message when no service is registered to search for and play songs.
Suggestions welcome on the wording !
This may or may not be relevant depending on how the whole Youtube auth things goes.