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 stop the music #1681

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Don't stop the music #1681

wants to merge 7 commits into from

Conversation

MarvinSchenkel
Copy link
Contributor

Automatically play similar music at the end of a queue

@@ -345,6 +359,10 @@ async def play_media(
else:
media_item = item

# Save requested media item to play on the queue so we can use it as a source
# for Don't stop the music
queue.played_media_item = media_item
Copy link
Member

Choose a reason for hiding this comment

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

So if I play a few tracks, it will be based on the last track of the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we could also change it to be a list so that it grabs all the tracks that were added.

Copy link
Member

Choose a reason for hiding this comment

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

yeah maybe a length limited list of max 10 items or so

Copy link

@T3chArmy T3chArmy Oct 4, 2024

Choose a reason for hiding this comment

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

Out of curiosity, would this theoretically resolve my issue with the current implementation where it pulls the same 20(or so) songs from Tidal into the queue every time the queue reaches 5 songs. So instead of pulling songs based on the song the radio station was started on, it would pull a list of songs based on say the most recently played 5-10 songs?

Copy link
Contributor Author

@MarvinSchenkel MarvinSchenkel Oct 7, 2024

Choose a reason for hiding this comment

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

No, radio mode bases the radio based on the 'radio media item' (item used when the user clicked 'start radio'), or in the case of 'dont stop the music', the items that are enqueued by the user. However, I rewrote the logic a few beta releases ago, so that it uses individual songs to search for 'similar songs' instead of an album or an artist. Did you test if this behaviour still occurs with Tidal in the latest beta?

@marcelveldt
Copy link
Member

@MarvinSchenkel please mark this PR as ready for review once you are ready for a final review.

@MarvinSchenkel
Copy link
Contributor Author

@MarvinSchenkel please mark this PR as ready for review once you are ready for a final review.

I will. Still need some final tweaks + the UI bit. Will try to finalize it this week.

@MarvinSchenkel MarvinSchenkel marked this pull request as ready for review October 7, 2024 14:31
@@ -36,6 +37,7 @@ class PlayerQueue(DataClassDictMixin):
current_item: QueueItem | None = None
next_item: QueueItem | None = None
radio_source: list[MediaItemType] = field(default_factory=list)
played_media_item: list[MediaItemType] = field(default_factory=list)
Copy link
Member

Choose a reason for hiding this comment

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

hmmm, not sure about this variable name as its a bit misleading Can you at least add some surrounding comments what its for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. How about enqueued_media_items ?

dont_stop_the_music_enabled = self.mass.config.get_raw_core_config_value(
self.domain,
CONF_DEFAULT_DONT_STOP_THE_MUSIC,
True,
Copy link
Member

Choose a reason for hiding this comment

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

Why not define the configentry as constant and refer to it here ?
Also.. not so sure about the default set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look at the config entry. About the default value, YT Music has 'autoplay' enabled by default. Apple music has it as well in the mobile app. So I thought to just turn it on by default. Happy to change it to False if you don't agree. Not sure how Spotify does this.

and (queue.items - queue.current_index) <= 1
):
# Enable radio mode on the originally requested MediaItem in play_media
queue.radio_source = queue.played_media_item
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a super complicated way of doing this ?
We have all the already played tracks available in the queue_items

Copy link
Contributor Author

@MarvinSchenkel MarvinSchenkel Oct 8, 2024

Choose a reason for hiding this comment

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

Well we do not want the recently played tracks as a source for our radio. We only want to use the originally enqueued media items as a source for the radio, because they represent the closest of what the user intended to play.

The logic here checks if we are playing the last item in the queue and, if DSTM is enabled, uses those originally enqueued media items as a radio source to dynamically fill the queue.

Copy link
Member

Choose a reason for hiding this comment

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

Sure but radio mode is always based on the tracks right ?
So radio mode for playlist --> will be based on the tracks
So radio mode for album--> will be based on the tracks

Copy link
Contributor Author

@MarvinSchenkel MarvinSchenkel Oct 8, 2024

Choose a reason for hiding this comment

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

Sure but radio mode is always based on the tracks right ? So radio mode for playlist --> will be based on the tracks So radio mode for album--> will be based on the tracks

No, radio mode is based on MediaItems. So the radio_source can be a list of let's say [Album, Track, Playlist]. The radio logic translates this into a list of base tracks (random 5 tracks per MediaItem), in this case 5 album tracks + the enqueued track + 5 random playlist tracks. Then it will create the dynamic playlist based on similar tracks for all of those base tracks.

This ensures that the originally enqueued items are remembered so we can always go back to them for generating a fresh list of dynamic tracks. This prevents the 'algorithm' to deviate from what was originally played.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants