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

Add support for multiple artists #234

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bgaesslein
Copy link
Contributor

@bgaesslein bgaesslein commented Mar 30, 2023

In response to feature request #198.

This makes it necessary to differentiate between artists and album artists so I created an album artist fragment. The assumption is that at most one albumartist is set but multiple artists in the artist tag are possible.

I also added a GetSearchHintsAsync call to the search activity because GetItemsAsync will not return artists with no albums. GetSearchHintsAsync does not work well with albums though so I split the search into two calls, GetSearchHintsAsync for artists and GetItemsAsync for albums and songs. There might be a better way to do this but this is the only way I could make it work.
This should solve issue #176.

Needed to support multiple artists.
Assumption: only one AlbumArtist, possibly more than one Artist tag.
Classes that directly queried a Song's artistname field now must call a
function that concatenates all artist names into a single string.
Necessary to fully support the artists/album artists split
The ItemQuery api call does not return artists with no albums while a
SearchHints call gets both artists and album artists. I tried to use
GetSearchHintsAsync for everything but couldn't get it to work properly
for albums (art was always missing) so this is what I got to make it
work.
@bgaesslein bgaesslein force-pushed the feature/multi-artist-support branch from 4f00439 to 1284e35 Compare March 30, 2023 10:04
@dkanada
Copy link
Owner

dkanada commented Apr 2, 2023

Haven't looked through everything yet but I'll review and merge in the next week or two. I'm guessing that the "Go To Artist" button on a song will send people to the album artist, and the same goes for the artist button in the album details view. Let me know if you have any other features planned and I'll make sure to include them in the same release!

@bgaesslein
Copy link
Contributor Author

bgaesslein commented Apr 2, 2023

Yes, the various "go to artist" links will still go to the album artist.

There's currently no way to go to individual artists' pages for songs from compilation albums, maybe there should be? Maybe I'll think of a way to implement this and do a further pull request in the future but this one should suffice for now.

What I would personally really like to see is artist names for songs in the album view and also in the song queue. For album views it might be sufficient to show individual artists only whenever the artist tag differs form the album artist tag. But the song queue should always show the artist in addition to the album to be more informative imho. Shouldn't be too hard to implement, I will look into it.

I've been looking into adding chromecast capabilities but haven't gotten very far. I'm still learning how to even do this.

@dkanada
Copy link
Owner

dkanada commented Jun 11, 2023

Did you test upgrading an existing install with these changes? Just want to make sure the database doesn't need any migrations because I've seen that issue before.

@bgaesslein
Copy link
Contributor Author

bgaesslein commented Jun 20, 2023

It's been a while, I'll try upgrading from the current release again later today and let you know.
Given that the Song model had to be changed to support an ArrayList for artistId and artistName I'm guessing there might be some issues. Is there anything that can be done to alleviate potential upgrade problems? If it's necessary, I'll look into Room database migration and try to add it to this pull request.

@jakobkukla
Copy link
Contributor

jakobkukla commented Jun 20, 2023

@bgaesslein Have a look at this for db migrations. For every model you change, you need to drop/recreate or alter the respective table.

@bgaesslein
Copy link
Contributor Author

Thanks, I will look into it and update the PR accordingly.

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.

3 participants