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 radio-browser search integration #2108

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

Conversation

kgarner7
Copy link
Contributor

@kgarner7 kgarner7 commented Jan 23, 2023

This is a draft integration of the Radio Browser API.

It currently does:

  • Periodic fetching of all station data from radio-browser (currently around 8.78 MB in DB)
  • Allow searching through various filters
  • Allow previewing a stream (playing it)
  • Allow creating a new Internet Radio from the search. This also required additional fields in the radio table

Things missing:

  • Translation. Most of the API is English, but it could be possible to translate the country code and language code?
  • Click Metrics. It is requested that clients using this API send a "Click" request when using a server. This is not yet implemented
  • [ ] Proxy/store favicons? Some of them set third-party cookies, which isn't nice. Cons: extra load on Navidrome (to be handled in another request)
  • Better handling of non-functional streams. Because of CORS (as well as just being broken), some streams will just not play in the web client. I've added a warning, but it would be cool if there is some alternative

@github-actions
Copy link

github-actions bot commented Jan 23, 2023

Download the artifacts for this pull request:

Copy link

@deluan-tc deluan-tc left a comment

Choose a reason for hiding this comment

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

Hey @kgarner7 why this is still a draft? Anyway, check my comments. Thanks!

Re: "Translation. Most of the API is English, but it could be possible to translate the country code and language code?", I think it makes sense. We could maybe add the country code in the translation files, and get the translations from there.

@@ -276,6 +307,10 @@ func init() {
viper.SetDefault("spotify.secret", "")
viper.SetDefault("listenbrainz.enabled", true)
viper.SetDefault("listenbrainz.baseurl", "https://api.listenbrainz.org/1/")
viper.SetDefault("radiobrowser.baseurl", "https://de1.api.radio-browser.info")
viper.SetDefault("radiobrowser.enabled", "true")
viper.SetDefault("radiobrowser.refreshschedule", "@every 24h")

Choose a reason for hiding this comment

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

I'd make the default @weekly

)

const (
fetchAgentTimeout = 100000000000000 // 100s

Choose a reason for hiding this comment

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

Can't you set this as a proper duration? 100 * time.Second


toDelete := []string{}

for id := range existing {

Choose a reason for hiding this comment

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

I think you can use the utils.RangeByChunks function here.

@@ -2,7 +2,8 @@ package model

const (
// TODO Move other prop keys to here
PropLastScan = "LastScan"
PropLastScan = "LastScan"
PropLastRefresh = "LastRefresh"

Choose a reason for hiding this comment

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

Maybe rename this to PropLastRadioBrowserRefresh, to be more explicitly ?

})
}

func RadioBrowserConstructor(ds model.DataStore) *radioBrowserAgent {

Choose a reason for hiding this comment

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

"Constructors" in Go, by convention, are named NewXxxx() ;)


DROP TABLE radio_tmp;
`)
// This code is executed when the migration is applied.

Choose a reason for hiding this comment

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

Remove this comment.

}

func downAddCachedRadioinfo(tx *sql.Tx) error {
// This code is executed when the migration is rolled back.

Choose a reason for hiding this comment

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

Remove this comment.

maxQueryVariables = 1000 // You can go higher than this, but just a precaution
)

type RadioBrowserAgent interface {

Choose a reason for hiding this comment

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

Not sure if this classifies as an "Agent", as it is very specific, probably won't be implemented with other service. But if you feel it can be implemented with other services, then it should not update the DB directly, it should only query the API and return a parsed list of radios. The synchronization with the DB should be done in a core service.

@@ -366,6 +385,9 @@
"musicbrainz": "Open in MusicBrainz"
},
"lastfmLink": "Read More...",
"originalFormat": "Download in original format",

Choose a reason for hiding this comment

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

I don't think we are using this string in the PR.

ui/src/App.js Outdated
permissions === 'admin' ? (
<Resource name="radioInfo" {...radioInfo} />
) : undefined,
config.devEnableShare && <Resource name="share" {...share} />,

Choose a reason for hiding this comment

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

Merge error?

- Merge in master
- `/radio` => `/radios`. For some reason `/radio/${id}` just gave me a status=0? If the original one works that'd be great
- Move radio search to empty page/radio list
- Edit button for existing entries instead
@kgarner7
Copy link
Contributor Author

kgarner7 commented Apr 1, 2023

This was still in draft mainly for the translations (and in the hope that proxying radios would be looked at first). One weird thing I noticed when testing, for some reason /radio/${id}/ would return with status 0, which is why I changed to /radios. If you can fix that it'd be great. I have no idea why the rest api was not called .-.

@kgarner7 kgarner7 marked this pull request as ready for review April 1, 2023 05:58
@deluan
Copy link
Member

deluan commented Apr 6, 2023

and in the hope that proxying radios would be looked at first

Do you think the radio proxy PR should go first?

@kgarner7
Copy link
Contributor Author

kgarner7 commented Apr 6, 2023

If possible, yes. My reasoning is that, currently, the internet radio feature basically requires you to have an existing one in mind. With this feature, it makes searching more likely, which while a good thing, also means that the odds of someone finding a stream that doesn't support work currently more likely...

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

This PR has been automatically marked as stale because it has not had recent activity. The resources of the Navidrome team are limited, and so we are asking for your help.
Please check https://github.com/navidrome/navidrome/blob/master/CONTRIBUTING.md#pull-requests and verify that this code contribution fits with the description. If yes, tell it in a comment.
This PR will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@github-actions github-actions bot added the stale label Oct 4, 2023
@github-actions github-actions bot closed this Nov 3, 2023
@deluan deluan reopened this Nov 8, 2023
@deluan deluan added keep and removed stale labels Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants