Add support for ListenBrainz Audio Scrobbler Service#1224
Conversation
ibmibmibm
commented
Sep 4, 2019
- Add textbox in user settings page for ListenBrainz token
- Add changes to db
- Add db colume to store MusicBrainz Recording ID (improves Support MusicBrainz tags #164)
- Add db colume to store ListenBrainz token
- Add test for reading id
- Add tag on testing file
- Add localization entry
jvoisin
left a comment
There was a problem hiding this comment.
Nice and clean pull-request, thanks!
|
Try to make changes for Java8 now... |
eharris
left a comment
There was a problem hiding this comment.
Good clean PR, thanks!
Please add the help messages to the rest of the internationalization language files (in English if you don't know the language). The translations can be handled later.
|
looks like #1218 ? |
|
@ibmibmibm What does that ticket have to do with this? |
Looks like that occurs in CI runs, too. |
|
The CI runs do not use a media player, they just pull the stream directly from the API. We don't know yet why Travis is having issues, but there's no way that MEJS player issues are involved. |
There was a problem hiding this comment.
You probably want some other way to trigger retries if scrobbling fails, since there are now two different scrobble destinations. The way the logic is now, if one fails, both will get re-posted. I'm not sure if that would cause duplicate scrobbles on the one that didn't fail, but I'm sure the services would prefer not to get duplicate scrobble attempts just the same. So there should probably be either separate queues, or keep flags on the queued item that says which service needs retrying.
eharris
left a comment
There was a problem hiding this comment.
I didn't have time to fully review everything, but don't want to hold up you fixing the already identified issues, so going ahead and submitting now.
fxthomas
left a comment
There was a problem hiding this comment.
A few questions on how things work, but otherwise it sounds like a great PR.
(...and thank you for a feature I had been waiting for for a long time! 😉)
|
I've separated AudioScrobblerService into two helper class, to handle different audio scrobbler target. |
fxthomas
left a comment
There was a problem hiding this comment.
Good changes, thanks! But now that things are split, why not make a complete interface (something like ScrobblerInterface?) with consistent method signatures? This makes things even clearer!
| if (lastFMScrobbler == null) { | ||
| lastFMScrobbler = new LastFMScrobbler(); | ||
| } | ||
| lastFMScrobbler.register(mediaFile, userSettings.getLastFmUsername(), userSettings.getLastFmPassword(), submission, time); |
There was a problem hiding this comment.
It would be better to pass uername and password through the constructor.
There was a problem hiding this comment.
Is this service construct one for every users? I was guessing that this service object shared between all users, so I passed this data through register function.
There was a problem hiding this comment.
Ah, I see, if you see it as a long-running service that's shared for everyone it makes sense.
What do you think of turning it into a real Spring service then? IMO this makes it more clear that it's intended this way and avoids the if (lastFMScrobbler == null) dance a few lines above.
|
I'd really like to see this get merged soon. @ibmibmibm can you address @fxthomas concerns? I will also try to allocate time to review after that happens. |
We really appreciate your work on this, it's really shaping up well! That being said, I'd really like to see the code rearrangement (moving Last.fm to its own helper class) done as a separate commit (or even its own PR) before the changes to add ListenBrainz. |
Now in separated commits. |
|
Thank you, great job on building this! 😄 Aside from my last comment we should be good to go. |
|
This has a merge conflict that needs to be resolved. |
- Split AudioScrobblerSevice into two classes - Change api to https in last.fm Audio Scrobbler Signed-off-by: Shen-Ta Hsieh <ibmibmibm.tw@gmail.com>
- Add textbox in user settings page for ListenBrainz token
- Add changes to db
- Add db colume to store MusicBrainz Recording ID
- Add db colume to store ListenBrainz token
- Add test for reading id
- Add tag on testing file
- Add localization entry
Signed-off-by: Shen-Ta Hsieh <ibmibmibm.tw@gmail.com>
|
Merged in 84df4e3 |