Dealer: Rework context retrieval #1414
Conversation
- resolved contexts independently, by that we don't block our main loop - move resolve logic into own file - polish handling for play and transfer
|
When finishing a context it does seem to reset the current track correctly, but doesn't update the next tracks correclty. probably wrong:
|
|
Quickly seeking to beginning (just pressing prev) and pausing can update the state incorrectly. Log Snippet |
- rename `handle_context` to `handle_next_context` - disconnect should only pause the playback - find_next should not exceed queue length
There was a problem hiding this comment.
Copilot reviewed 6 out of 12 changed files in this pull request and generated 2 comments.
Files not reviewed (6)
- connect/src/state/options.rs: Evaluated as low risk
- connect/src/model.rs: Evaluated as low risk
- core/src/spclient.rs: Evaluated as low risk
- connect/src/state/transfer.rs: Evaluated as low risk
- connect/src/state/handle.rs: Evaluated as low risk
- connect/src/state/tracks.rs: Evaluated as low risk
Comments suppressed due to low confidence (1)
connect/src/state/context.rs:267
- Ensure that the behavior of returning
Ok(None)whennext_contextsis empty is covered by tests.
if next_contexts.is_empty() { return Ok(None); }
|
Currently the initial transfer is not as smooth as it was before these changes. I will fix that, and afterwards remove the draft state. Is probably be a small fix. |
- fix context_metadata and restriction incorrect reset - do some state updates earlier - add more logging
|
Hopefully last points:
|
just mimics official client behavior
# Conflicts: # connect/src/spirc.rs
|
Besides the incorrect tests, there seems to be a missing track update when starting a playback with shuffle and an specified track. Besides that the state feels quite stable again, so I will probably mark the PR as ready this evening. |
|
Hey @photovoltex. I tested the PR and it seems to work (at least that what I tested) :D Don't know if this belongs in this PR or if I should rather create an issue: I discovered that when playing a search result with autoplay, then sometimes the same song is played again when skipping to the next song. Sometimes it is not played directly again but after like 2-4 songs: Log after clicking on the search result: No log after clicking skip. Btw: Congrats on becoming a maintainer :D |
see here #1414 (comment) hehe
I also encountered that Problem and didn't think much about it. But seems to be a Problem of the current solution/branch. I think I already know where the problem lies heh. So that shouldn't be much of a problem to solve. As always thanks for testing this branch. Helps a lot :D |
Now I feel stupid, completely overlooked that somehow xD |
|
Huh... the autoplay endpoint seems to always provide the initial track again. That seems to be the "normal" behavior. I wonder why that didn't happen before... Well time to fix that. |
|
Wait I take that back... I have a track where it behaves like that and another's where it doesn't... the hell I suppose when it only affects autoplay we can filter out the track from which we requested the autoplay (so for singles/tracks just the track). Weird behavior tho... I wonder why it didn't seem to happen before. I checked the code that changed, but the relevant parts didn't change. |
Yup. Now to make it more confusing, it isn't even always the first track after skipping, as I described above (example: Feliz Navidad, at least for me). Will test your recent changes tomorrow. |
I look at "Feliz Navidad" and there is another "Feliz Navidad" in the autoplay context, but with a different |
- removes previously added workaround
|
This should "fix" the weird behavior, as we now provide all recently played songs, in which the current track will also be contained, and by that should be not included anymore in the autoplay tracks. But thats for spotify to decide in the end^^ |
@photovoltex I tested it and the tracks are no longer duplicated. I also was quite sure that when I tried playing Feliz Navidad, that there was the exact same track duplicated, now it's a track with even a different name... so issue fixed I guess :D Selecting a song when playing shuffled also works now. |
There was a problem hiding this comment.
Copilot reviewed 6 out of 13 changed files in this pull request and generated 1 comment.
Files not reviewed (7)
- connect/src/state/options.rs: Evaluated as low risk
- connect/src/state/transfer.rs: Evaluated as low risk
- core/src/spclient.rs: Evaluated as low risk
- connect/src/state/handle.rs: Evaluated as low risk
- examples/play_connect.rs: Evaluated as low risk
- connect/src/lib.rs: Evaluated as low risk
- connect/src/state/tracks.rs: Evaluated as low risk
Comments suppressed due to low confidence (5)
connect/src/context_resolver.rs:170
- The debug message contains a minor grammar issue. It should be 'context was requested {} seconds ago'.
debug!("context was requested {}s ago, trying again to resolve the requested context", last_try.expect("checked by condition").as_secs());
connect/src/context_resolver.rs:232
- The behavior of
get_next_contextwhenUpdateContext::Autoplayis used should be covered by tests to ensure that it handles unsupported contexts for podcasts correctly.
pub async fn get_next_context(&self, recent_track_uri: impl Fn() -> Vec<String>) -> Result<Context, Error> {
connect/src/context_resolver.rs:33
- [nitpick] The name
ResolveContextmight be ambiguous. It could be renamed toContextResolutionto better reflect its purpose.
pub(super) struct ResolveContext {
core/src/dealer/protocol.rs:177
- The word "send" should be corrected to "sent".
trace!("message was send with {encoding} encoding ");
connect/src/state.rs:358
- The update_context_index method is called with new_index + 1, which might cause an off-by-one error. It should be called with new_index.
self.update_context_index(self.active_context, new_index + 1)?;
- use drain instead of for with pop - use for instead of loop - use or_else instead of match - use Self::Error instead of the value - free memory for metadata and restrictions
|
I added another fix around |
|
And another small fix that adjusts the setting of the Last change as I will not add anything more. |
# Conflicts: # connect/src/model.rs # connect/src/spirc.rs # connect/src/state.rs # connect/src/state/context.rs # connect/src/state/transfer.rs # protocol/src/lib.rs
|
@roderickvd, Could you take a look over the applied changes (and maybe merge it afterwards if everything seems fine)? Thanks in advance :) |
There was a problem hiding this comment.
Copilot reviewed 7 out of 18 changed files in this pull request and generated no comments.
Files not reviewed (11)
- connect/src/state/restrictions.rs: Evaluated as low risk
- core/src/spclient.rs: Evaluated as low risk
- connect/src/state/options.rs: Evaluated as low risk
- connect/src/state/handle.rs: Evaluated as low risk
- connect/src/lib.rs: Evaluated as low risk
- examples/play_connect.rs: Evaluated as low risk
- connect/src/state/tracks.rs: Evaluated as low risk
- connect/src/state.rs: Evaluated as low risk
- connect/src/state/transfer.rs: Evaluated as low risk
- core/src/dealer/protocol.rs: Evaluated as low risk
- protocol/src/lib.rs: Evaluated as low risk
Comments suppressed due to low confidence (5)
connect/src/context_resolver.rs:91
- Using unwrap_or_default might hide issues if the URI is not correctly set. Ensure the URI is valid before proceeding.
Resolve::Context(ref ctx) => ctx.uri.as_deref().unwrap_or_default(),
connect/src/context_resolver.rs:232
- Simplify the error handling in get_next_context.
Err(ContextResolverError::NotAllowedContext(resolve_uri.to_string()))?
connect/src/context_resolver.rs:113
- Check the spelling and grammar in the error messages.
UnexpectedPagesSize(usize),
connect/src/context_resolver.rs:260
- [nitpick] The check for context.pages.len() == 1 might be too restrictive. Consider revising the logic.
ContextAction::Append if context.pages.len() == 1
connect/src/state/context.rs:335
- Using
unwrap_or_defaultfor the fallback index might not be appropriate in all cases. Consider ensuring that the default value is suitable for the context.
let fallback_index = self
.player()
.index
.as_ref()
.map(|i| i.track)
.unwrap_or_default();
| /// see [ConnectState::set_status] | ||
| pub fn is_pause(&self) -> bool { | ||
| let player = self.player(); | ||
| player.is_playing && player.is_paused && player.is_buffering |
There was a problem hiding this comment.
As a sanity check: so if we are 100% buffered, we cannot be in pause state?
There was a problem hiding this comment.
Hmm, my method naming might be a bit misleading as the player here isn't the actual player and rather the state of the player that the server knows of.
And by that these bool flags are all independent from the actual player logic. Which also means we can set them as we like to, and for pausing the mobile and desktop want all of these flags set to true. I can't tell you why, but setting them to the expected values (so only pause would be true if we are pausing) we can't resume playback as the play button will be disabled.
I have written down that explanation somewhere by the reference method.
But yeah as a mixture of my confusing naming and Spotify's confusing expectations this looks rather wrong^^;
Thanks for the final check-up! I tested it over the last days/weeks so it should be rather stable to merge. So will probably merge it the following days. |
This resolves the issue that a context with multiple pages couldn't be completely shuffled. I had to rework most of the context resolving part, because resolving the entire context at once wasn't an option. Pretty much blocked the entire main loop for like ~1-30sec depending on the context. Which blocked all processing of commands and or events.
Each context is now requested independently so that commands can fit between requests :D
additional fixes:
matchblocksplayback_speedaccording to previously mentioned here Spirc: Replace Mecury with Dealer #1356 (comment)I will let it stay in draft for some days, so I can still test it a bit more during the day. And maybe someone else even wants to try it^^