Fix: Use the provided index or the first track as fallback value #1599
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where tracks wouldn't play when the given URI and album URI don't match by implementing a fallback mechanism. The fix ensures that either the provided index or the first track (index 0) is used as a fallback when track resolution fails.
- Adds fallback index extraction from
skip_tooptions - Modifies track index resolution to use Result type and handle failures gracefully
- Updates function signatures to pass the fallback index through the call chain
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| connect/src/spirc.rs | Implements fallback index logic and error handling for track resolution |
| discovery/src/lib.rs | Removes unnecessary string reference for DNS_SD_SERVICE_NAME parameter |
| CHANGELOG.md | Documents the bug fix in the changelog |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I've been testing this and there seems to still be some bug; I don't have a good repro but sometimes when hopping between albums/singles librespot plays the wrong thing, from not even the correct context. Logs show the new warning added by this PR but with the fallback index being 0. I'm going to try to get a repeatable scenario to share here. |
|
This is the command that causes the wrong thing to play |
|
I have a repro: while connected to librespot: have something playing, then go to an artist page, go to All Releases or similar, scroll down a bit and play something from there. It plays something from something else on the same page. |
|
You seem to have found a case where the |
|
Could you give this a try @drobotk? I couldn't tested it this time, but it should probably also account for the edge case now. If not we might have to do a bit more then I would hope for. |
|
Same command as above still doesn't play the right thing, but what it plays is different than before. I try to play this but this plays instead. There also appears to be a regression. With this latest commit, trying to play songs from the album I linked in the Issue it now always plays the first track, index 0. |
|
Thanks for the testing, I see where my issue for the fix is. I will look into the regression, might be a minor oversight hopefully. |
|
So the second issue isn't an easy fix as simply providing a fallback index... It probably makes more sense to limit this PR for now to the initial problem and offload the second issue into a separate topic. A wrong behavior is more acceptable currently then something that prevents playback entirely. |
photovoltex
force-pushed
the
fix/use-index-as-fallback
branch
from
3681bb3 to
f0d6afe
Compare
|
@drobotk Could you create a new issue for the second issue you encountered here? It's worth fixing but would take some more time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pretty much just forwards the index given by
skip_toso that a track is always played, even when the given uri and the album uri don't match. This seems to be a rare case for some albums.Fixes #1591