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

Don't use inconsistent `albumId` or `artistId` #233

Merged
merged 6 commits into from Mar 10, 2020
Merged

Conversation

@jjok
Copy link
Contributor

jjok commented Mar 1, 2020

Fixes #139
Fixes #206

The albumId or artistId doesn't always exist. They sometimes exists for some tracks in an album, but not others. This causes new IDs to be generated, which means those tracks are shown as being by a separate artist and in a separate album.

All I've done here is make it always generate an artistId and albumId, never using the one that may exist from gmusic.

Does anyone know if this is an issue for All Access (called subscription now?)? I'm thinking that the IDs might always be sent back for AA tracks. If that's the case, we should probably do something like:

if all_access:
    album_id = song.get("albumId")
else:
    album_id = create_id(artist.name + name + date)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 1, 2020

Codecov Report

Merging #233 into master will increase coverage by 0.50%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   59.92%   60.42%   +0.50%     
==========================================
  Files           9        9              
  Lines         751      748       -3     
==========================================
+ Hits          450      452       +2     
+ Misses        301      296       -5     
Impacted Files Coverage Δ
mopidy_gmusic/library.py 51.76% <50.00%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88e71a1...07578f7. Read the comment docs.

@jodal jodal requested a review from belak Mar 1, 2020
album_id = create_id(artist.name + name + date)
#album_id = song.get("albumId")
#if album_id is None:
album_id = create_id(artist.name + name + date)

This comment has been minimized.

Copy link
@jodal

jodal Mar 1, 2020

Member

Assuming that this change is good to go and works with All Access too (I can't confirm, as I don't have a subscription and don't use GMusic), the commented out code should be removed before this is merged.

This comment has been minimized.

Copy link
@belak

belak Mar 6, 2020

Member

This allows for conflicts if the artist, album name, and date are the same. Unlikely, but still possible. One of the reasons I had for sticking to pulling IDs like this from Google if they exist were specifically because of this. One specific example would be for two Various Artists albums with the same name in the same year.

This comment has been minimized.

Copy link
@jjok

jjok Mar 7, 2020

Author Contributor

Yeah, I get that there's possibility of conflict. For me though, with only uploaded tracks, this change is way better, and makes the extension work really well, rather than having to wade through all the duplicates.
I don't know how well it works for subscription. I know that, with Spotify, they sometimes have the same album on there multiple times. If gmusic does that, then that could potentially cause problems.

This comment has been minimized.

Copy link
@jjok

jjok Mar 7, 2020

Author Contributor

FYI Weezer released two self-titled albums last year. 🙄

This comment has been minimized.

Copy link
@jjok

jjok Mar 7, 2020

Author Contributor

Other than that, I think it's extremely unlikely that someone will have uploaded two conflicting albums. And, if they have, this change is still an overall improvement, in my opinion.

This comment has been minimized.

Copy link
@belak

belak Mar 8, 2020

Member

I'm not a huge fan of knowingly breaking things, but considering it's broken in other more obvious ways, I can get behind this.

My one request - can you use some sort of separator (like : or |) between the fields? I think it will make it a bit more stable and make the IDs easier to read if we're ever debugging.

This comment has been minimized.

Copy link
@jjok

jjok Mar 8, 2020

Author Contributor

can you use some sort of separator (like : or |) between the fields?

Sound like a good idea. I'll do that.

This comment has been minimized.

Copy link
@jjok

jjok Mar 8, 2020

Author Contributor

I'll go with the | as I've got at least one album with : in the title.

Various Artists|Fat Music, Vol. 5: Live Fat Die Young|2001
@jodal jodal added this to the v4.0.1 milestone Mar 1, 2020
logger.info("Loaded "
f"{len(self.artists)} artists, "
f"{len(self.albums)} albums, "
f"{len(self.tracks)} tracks from Google Play Music")

This comment has been minimized.

Copy link
@jjok

jjok Mar 9, 2020

Author Contributor

I added some logging. Now you get:

INFO     2020-03-09 00:12:42,160 [1:GMusicBackend-6] mopidy_gmusic.session
  Logged in to Google Play Music
INFO     2020-03-09 00:12:42,161 [1:Thread-8] mopidy_gmusic.library
  Refreshing library
INFO     2020-03-09 00:12:55,224 [1:Thread-8] mopidy_gmusic.library
  Loaded 442 artists, 1036 albums, 8439 tracks from Google Play Music
INFO     2020-03-09 00:12:57,881 [1:Thread-9] mopidy_gmusic.playlists
  Loaded 5 playlists from Google Play Music
jjok added 2 commits Mar 9, 2020
@belak
belak approved these changes Mar 10, 2020
@jodal jodal merged commit a4a7846 into mopidy:master Mar 10, 2020
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 50.00% of diff hit (target 59.92%)
Details
codecov/project 60.42% (+0.50%) compared to 88e71a1
Details
test Workflow: test
Details
@jodal

This comment has been minimized.

Copy link
Member

jodal commented Mar 10, 2020

Thank you :-)

@jjok jjok deleted the jjok:fix/139 branch Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.