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

Autoradio Averager + enhancement #248

Merged
merged 3 commits into from
Feb 11, 2019
Merged

Autoradio Averager + enhancement #248

merged 3 commits into from
Feb 11, 2019

Conversation

trekiteasy
Copy link
Contributor

Added autoradio averager

  • autoradio code was separated to a different file
  • autoradio is now based on similar tracks (and not only similar artists)
  • autoradio takes into account a defined number of tracks from the queue to compute next track
    Next steps :
  • the different parameters are currently const variables. This should be simplified and made changeable through apps settings (something like : how random should be the autoradio : 0 = very conservative 1=big variations)

Added autoradio averager
- autoradio code was separated to a different file
- autoradio is now based on similar tracks (and not similar artists)
- autoradio takes into account a defined number of tracks from the queue to compute next track
Next steps :
- the different parameters are currently const variables. This should be simplified and made changeable through apps settings (something like : how random should be the autoradio : 0 = very conservative 1=big variations)
@nukeop nukeop self-assigned this Feb 8, 2019
@nukeop nukeop added the under review This pull request is being reviewed by maintainers. label Feb 8, 2019
@nukeop
Copy link
Owner

nukeop commented Feb 9, 2019

I'm reluctant about coupling all that autoradio code with SoundContainer, since that container was supposed to be very simple: give it a stream URL, and playback parameters, and you get a playing song. I know there was already some last.fm-related scrobbling code in there, but this is a whole new ball game.

To solve it in a cleaner way, it would be better to move everything that's not related to playing songs outta there - I mean, last.fm stuff, autoradio stuff, and even switching songs on stream end, etc. Instead, make a list of subscribers to SoundContainer events, and on each event SoundContainer notifies the subscribers with relevant info instead of directly calling their methods. This way we can decouple all that unrelated code and make it easier to extend in the future.

If you want, you can implement this yourself, but if you don't feel like doing it (since this would require a significant effort), I can just merge this PR and do it later myself. Up to you which way you'd like to go with this.

@trekiteasy
Copy link
Contributor Author

I understand and was initially reluctant as well to put everything here (that's also why I separated some of the code to a new file).
Being fairly new to react (and not being a professionnal developper), I'm not sure I am confident enough to reorganize the code properly. I could help but would need more guidance on what to put where and how to put in place subscribers etc.

@nukeop nukeop merged commit cf1c1f3 into master Feb 11, 2019
@nukeop
Copy link
Owner

nukeop commented Feb 11, 2019

Ok, it's fine, we can rework this later.

nukeop added a commit that referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
under review This pull request is being reviewed by maintainers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants