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

Possible fix for Starting song radio from queue creates a queue based on the wrong song #263 #264

Merged
merged 5 commits into from
Feb 2, 2025

Conversation

Millencolin
Copy link
Contributor

What is it?

  • New feature (user facing)
  • Update to existing feature (user facing)
  • Bugfix (user facing)
  • Codebase improvements or refactors (dev facing)
  • Other

Description of the changes in your PR

This is a possible fix for #263.

However:
It would probably be better to call startRadioSeamlessly with mediaMetadata as parameter and handling the logic of selecting the right songs in there.

Also adding another method in queueBoard to handle adding the songs and start it without interrupting the currently playing song would probably be cleaner than calling queueBoard.addQueue / queueBoard.setCurrQueuePosIndex.

I didn't want to start modifying too many things and let you use this code as a base to fix the issue

@mikooomich
Copy link
Collaborator

Ah. I see you've stumbled upon mymultiqueue mess.

Other than continuation not being supported, seamless switching (I didn't know this existed) isn't either. For non-seamless, it isn't hard, just do this and queueboard should already handle it fine.

playerConnection.playQueue(YouTubeQueue.radio(song.toMediaMetadata()))

For seamless, media items in player can be updated with player.replaceMediaItems() without interrupting playback, then sync with queueboard as you said ---> the add function in queueboard needs to be modified for seamless...... and that should be it(?)

Also adding another method in queueBoard to handle adding the songs and start it without interrupting the currently playing song would probably be cleaner than calling queueBoard.addQueue / queueBoard.setCurrQueuePosIndex.

I think shuffle, enqueue, reorder all modify the player seamlessly, but do so in different ways. Would it be better to have one function to handle all this instead?

@Millencolin Millencolin force-pushed the fix-start-radio-from-queue branch 2 times, most recently from 9d3c953 to ee7d719 Compare January 30, 2025 20:30
@Millencolin
Copy link
Contributor Author

I updated the code:

It now uses playerConnection.playQueue(YouTubeQueue.radio(song.toMediaMetadata())) to start playback from the android auto song radio toggle as well ( I saw that you already changed that in one of the calls using startRadioSeamlessly )

To make seamless playback work I did the following:

Modified QueueBoard::setCurrQueue:

  • If the currently playing song matches the first item of the media items that we are adding => seamless playback: I remove all songs from the player except the currently playing one and add the new items. (player.replaceMediaItems does not work since it seems to stop playback)

  • If the first item of the media items does not match the currently playing song it replaces the whole list of media items in the player which matches the current behavior

@Millencolin Millencolin force-pushed the fix-start-radio-from-queue branch from 80a9382 to 85ef35a Compare January 31, 2025 07:29
@Millencolin Millencolin changed the title WIP: Possible fix for Starting song radio from queue creates a queue based on the wrong song #263 Possible fix for Starting song radio from queue creates a queue based on the wrong song #263 Jan 31, 2025
@mikooomich mikooomich force-pushed the fix-start-radio-from-queue branch from 55b2aa6 to 810badd Compare January 31, 2025 23:59
@mikooomich
Copy link
Collaborator

@Millencolin I added continuation back (so radio is endless now), and did some tinkering with the seamless stuff to accommodate that. In short, seamless works for any case where the current song == song to play after switching queues. Let me know your thoughts.

@Millencolin
Copy link
Contributor Author

I think it is not working properly, didn't have time to look into the code for details

Example

  1. Start a radio -> It adds 51 songs
  2. Manually start playing a song in the queue (eg. pos 4) and then click on start radio -> It creates a new queue and starts a radio seamlessly but the new queue has more than 51 songs in it. At the same time the queue from 1) suddenly has a few additional songs. I would have expected the old queue to be untouched and the new one to have 51 songs from the new song radio.

Other thing for seamless playback:
When it reaches the end of the queue it adds additional songs, in my case it adds another ~100 songs, why not ~50 like a normal radio? Also the first song of the additional songs matches the last one in the existing queue -> twice the same song in a row, probably the first song from the additional song radio songs has to be removed

@Millencolin
Copy link
Contributor Author

Found the issue for the additional songs in the queue...
It came from the caching of the playerIndex in the refactoring. The player index becomes the first item after the first if clause... since the songs before it have been removed => It does not clear all songs

val playerIndex = player.player.currentMediaItemIndex
// player.player.replaceMediaItems seems to stop playback so we
// remove all songs except the currently playing one and then add the list of new items
if (playerIndex> 0) {
    player.player.removeMediaItems(0, playerIndex)
}
if (playerIndex < player.player.mediaItemCount - 1) {
    player.player.removeMediaItems(playerIndex + 1, player.player.mediaItemCount)
}

@mikooomich mikooomich force-pushed the fix-start-radio-from-queue branch from 8fab8e7 to db7bc02 Compare February 2, 2025 18:51
Copy link
Collaborator

@mikooomich mikooomich left a comment

Choose a reason for hiding this comment

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

Shouldn't be duplicates anymore (oops by bad). I'll probably squash all the fix seamless commits, so we're left with initial seamless, improved seamless, and continuation as 3 commits only so the commit history is more readable

Let me know when ready to merge

@mikooomich mikooomich force-pushed the fix-start-radio-from-queue branch from db7bc02 to 257a4f8 Compare February 2, 2025 19:04
@Millencolin
Copy link
Contributor Author

Hmm, i could still see the duplicate song somehow. I pushed a small fix which I think fixes it. To reproduce the issue: Start a radio, click on the second last song so that the queue expands, you will see the last song twice

One thing that I also just saw is that when you use continuation multiple times it is adding the same list of songs again
YouTubeQueue(WatchEndpoint(songId)).nextPage() probably returns the same list

For me it is ok to reorder and squash commits

@mikooomich
Copy link
Collaborator

mikooomich commented Feb 2, 2025

SIGH. now it should no longer load the same mix over again

Millencolin and others added 3 commits February 2, 2025 14:52
Co-authored-by: Millencolin <[email protected]>
Co-authored-by: mikooomich <[email protected]>
* Aka auto load.
* Rebased implementation on InnerTune
* Closes DD3Boh#88 DD3Boh#144
@mikooomich mikooomich force-pushed the fix-start-radio-from-queue branch from 6411f81 to 9d5a984 Compare February 2, 2025 20:06
@Millencolin
Copy link
Contributor Author

I think it looks good now.

BTW: Continuation stops working if you close and reopen the app. Probably because is does not persist the playlistId yet

  /**
     * Song id to start watch endpoint
     * TODO: change this in database too
     */
    var playlistId: String? = null,

@mikooomich
Copy link
Collaborator

I think it looks good now.

BTW: Continuation stops working if you close and reopen the app. Probably because is does not persist the playlistId yet

  /**
     * Song id to start watch endpoint
     * TODO: change this in database too
     */
    var playlistId: String? = null,

eh? I thought I just fixed that (old queues will have to be removed and re-created)

@Millencolin
Copy link
Contributor Author

Your fixes work fine.

However if you play a song radio with continuation and then force close the app and reopen it, continuation is not working

@mikooomich
Copy link
Collaborator

now I think all the bugs have been squashed. Coding the initial radio while sick was a mistake.

@Millencolin
Copy link
Contributor Author

Haha, works fine now

@mikooomich mikooomich merged commit 82d2081 into DD3Boh:dev Feb 2, 2025
1 check passed
@Millencolin Millencolin deleted the fix-start-radio-from-queue branch February 2, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants