Add support for ListenBrainz Audio Scrobbler Service #1224

ibmibmibm · 2019-09-04T20:49:13Z

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

Nice and clean pull-request, thanks!

ibmibmibm · 2019-09-05T09:21:31Z

Try to make changes for Java8 now...

eharris

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.

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

airsonic-main/src/main/resources/org/airsonic/player/i18n/ResourceBundle_en.properties

ibmibmibm · 2019-09-06T15:43:50Z

looks like #1218 ?

eharris · 2019-09-09T12:12:16Z

@ibmibmibm What does that ticket have to do with this?

ibmibmibm · 2019-09-09T13:05:21Z

@ibmibmibm What does that ticket have to do with this?

Looks like that occurs in CI runs, too.

eharris · 2019-09-09T18:39:28Z

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.

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

eharris

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.

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

airsonic-main/src/main/resources/org/airsonic/player/i18n/ResourceBundle_bg.properties

eharris

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.

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

fxthomas

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! 😉)

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

ibmibmibm · 2019-10-04T07:32:02Z

I've separated AudioScrobblerService into two helper class, to handle different audio scrobbler target.

fxthomas

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!

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

fxthomas · 2019-10-04T19:26:06Z

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

            }
+            lastFMScrobbler.register(mediaFile, userSettings.getLastFmUsername(), userSettings.getLastFmPassword(), submission, time);


It would be better to pass uername and password through the constructor.

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.

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.

airsonic-main/src/main/java/org/airsonic/player/service/AudioScrobblerService.java

airsonic-main/src/main/java/org/airsonic/player/service/scrobbler/ListenBrainzScrobbler.java

eharris · 2019-10-21T22:33:52Z

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.

eharris · 2019-10-21T22:47:47Z

I've separated AudioScrobblerService into two helper class, to handle different audio scrobbler target.

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.

ibmibmibm · 2019-10-28T07:37:46Z

I've separated AudioScrobblerService into two helper class, to handle different audio scrobbler target.

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.

fxthomas · 2019-10-29T21:12:47Z

Thank you, great job on building this! 😄 Aside from my last comment we should be good to go.

eharris · 2019-11-11T22:43:22Z

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>

muff1nman · 2019-11-23T20:06:48Z

Merged in 84df4e3

ibmibmibm closed this Sep 5, 2019

ibmibmibm reopened this Sep 5, 2019

jvoisin requested a review from eharris September 5, 2019 08:36

jvoisin approved these changes Sep 5, 2019

View reviewed changes

eharris suggested changes Sep 5, 2019

View reviewed changes

eharris mentioned this pull request Sep 10, 2019

Data lost when changing file name #1190

Closed

eharris suggested changes Sep 12, 2019

View reviewed changes

eharris added type: enhancement-closed What was previously labeled enhancement. For archiving. Will be organized later. type: request There is no implementer. Or there is no support from the maintainer. Please implement it and appeal. labels Sep 15, 2019

eharris suggested changes Sep 16, 2019

View reviewed changes

eharris added this to the 10.5.0 milestone Sep 16, 2019

eharris mentioned this pull request Sep 18, 2019

Migrate to new last.fm scrobbling API #776

Open

ibmibmibm requested a review from eharris September 22, 2019 03:39

eharris suggested changes Sep 22, 2019

View reviewed changes

fxthomas approved these changes Sep 24, 2019

View reviewed changes

ibmibmibm requested review from eharris, jvoisin and fxthomas October 4, 2019 07:30

fxthomas suggested changes Oct 4, 2019

View reviewed changes

ibmibmibm added 2 commits November 13, 2019 13:31

Move Last.fm logic into separate helper class

79d6993

- Split AudioScrobblerSevice into two classes - Change api to https in last.fm Audio Scrobbler Signed-off-by: Shen-Ta Hsieh <ibmibmibm.tw@gmail.com>

muff1nman closed this Nov 23, 2019

eharris mentioned this pull request Dec 16, 2019

No Last.fm scrobbling at all #785

Closed

randomnicode mentioned this pull request Mar 2, 2020

Fix last fm issue where scrobbling does not happen airsonic-advanced/airsonic-advanced#112

Merged

		}
		lastFMScrobbler.register(mediaFile, userSettings.getLastFmUsername(), userSettings.getLastFmPassword(), submission, time);

Add support for ListenBrainz Audio Scrobbler Service #1224

Add support for ListenBrainz Audio Scrobbler Service #1224

Uh oh!

Conversation

ibmibmibm commented Sep 4, 2019

Uh oh!

jvoisin left a comment

Choose a reason for hiding this comment

Uh oh!

ibmibmibm commented Sep 5, 2019

Uh oh!

eharris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibmibmibm commented Sep 6, 2019

Uh oh!

eharris commented Sep 9, 2019

Uh oh!

ibmibmibm commented Sep 9, 2019

Uh oh!

eharris commented Sep 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eharris left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eharris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fxthomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibmibmibm commented Oct 4, 2019

Uh oh!

fxthomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fxthomas Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

ibmibmibm Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

fxthomas Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eharris commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eharris commented Oct 21, 2019

Uh oh!

ibmibmibm commented Oct 28, 2019

Uh oh!

fxthomas commented Oct 29, 2019

Uh oh!

eharris commented Nov 11, 2019

eharris left a comment •

edited

Loading

eharris commented Oct 21, 2019 •

edited

Loading