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

BrainzPlayer album-aware music search #3014

Open
wants to merge 5 commits into
base: ansh/improve-apple-music-match
Choose a base branch
from

Conversation

anshg1214
Copy link
Member

@anshg1214 anshg1214 commented Oct 29, 2024

This PR implements an enhanced track matching feature by utilising track links from the API (spotify & appleMusic) data. When available, album data is used to retrieve the correct URL for upcoming tracks, improving the accuracy and completeness of track information in the queue.

// Filter the tracks in the album that are not yet matched
const queueTracksInAlbum = currentQueueAfterCurrent.filter((track) => {
return (
getReleaseMBID(track) === releaseMBID &&
Copy link
Member Author

Choose a reason for hiding this comment

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

@MonkeyDo Should we exclude tracks without a releaseMBID? In some cases, a listen might lack this mapping data but still have the same trackName we’re searching for.

Copy link
Member

Choose a reason for hiding this comment

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

I'm reading all this for the first time, so i hope I won't say anything too stupid XD

I don't think we should filter out non-mapped tracks, but I also don't understand why you want to do that in the first place.
Considering the mapping is a best-effort sort of feature, assume it won't always be present.

You can additionally use the releaseName and do some fuzzy matching on that if you are trying to group all tracks from the same album.

frontend/js/src/common/brainzplayer/AppleMusicPlayer.tsx Outdated Show resolved Hide resolved
frontend/js/src/common/brainzplayer/BrainzPlayer.tsx Outdated Show resolved Hide resolved
// Filter the tracks in the album that are not yet matched
const queueTracksInAlbum = currentQueueAfterCurrent.filter((track) => {
return (
getReleaseMBID(track) === releaseMBID &&
Copy link
Member

Choose a reason for hiding this comment

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

I'm reading all this for the first time, so i hope I won't say anything too stupid XD

I don't think we should filter out non-mapped tracks, but I also don't understand why you want to do that in the first place.
Considering the mapping is a best-effort sort of feature, assume it won't always be present.

You can additionally use the releaseName and do some fuzzy matching on that if you are trying to group all tracks from the same album.

type: "UPDATE_MATCHED_TRACKS",
data: {
dataSource,
queueMatchedTracks: newQueueMatchedTracks,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be easier, instead of storing this info in the listens in the queue, to have a separate object belonging to the BP component, which can be used to retrieve a URI (for a specific music source) using an MBID or name string.
Something like:

albumMappings: {
  [releaseMBID || releaseName]: {
    appleMusic: appleMusicAlbumURI,
    spotify: spotifyAlbumURI,
    etc.
  }
}

That way, for example if you have the same album spread over two pages of listens, when you go to page 2 you already have the album info you need without having to do the whole process again.
Also means not updating the whole queue, which might re-render the queue UI needlessly.

On the music components side, basically in playListen you would need to call a BP method to check if a value exists in the albumMappings object for this release (by MBID/releasName), and use that if there is a hit; otherwise do the search as usual.

What do you think?

Copy link
Member Author

@anshg1214 anshg1214 Nov 1, 2024

Choose a reason for hiding this comment

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

There is a problem in this implementation. We'll have a album mapping in this case, which might not be relevant. In order to use a album mapping, we'll have to make an API call to the data source provider every time in order to use this information.

Right now, what I'm doing is that fetching all the tracks of the current album, and then using the trackName <> URI mapping to update the upcoming tracks in the queue.

So, if we want to keep this information separate from queueItem, we can save all this information like

albumMapping: {
  [releaseName]: {
    [trackName]: {
      appleMusic: appleMusicURI,
      spotify: spotifyURI,
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation, I misunderstood the code at first.

Do you think my suggestion makes sense,as opposed to modifying the queue items?

Copy link
Member Author

@anshg1214 anshg1214 Nov 18, 2024

Choose a reason for hiding this comment

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

Yes, this makes sense, as we don't have to update queue items again and again, and will have a better performance with ambient queues

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.

2 participants