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

Check metadata title for URI hints before discarding #906

Merged
merged 2 commits into from Mar 2, 2022

Conversation

jjlawren
Copy link
Contributor

@jjlawren jjlawren commented Mar 1, 2022

A change in #896 caused certain titles (most obviously from CIFS shares) to be incorrectly discarded as they were contained in the track URI. This was naive as even single-word strings would trigger the discard behavior. This PR adds an additional hurdle which requires the track to contain some URI components before it is discarded, such as path and query arguments or an .m3u8 extension.

I've also added tests to validate expected processing of metadata from a variety of sources. This should help ensure we're going all we can to get correct results and also prevent future regressions.

Note: This includes the commit from #904 in order to pass tests. Once that PR is merged this can be rebased to simplify the changes.

Fixes: #903

@pwt pwt self-assigned this Mar 1, 2022
@pwt pwt added this to the 0.27.0 milestone Mar 1, 2022
"AbsCount": "2147483647"
},
"result": {
"title": "text=\"Spot Block End\" amgTrackId=\"1234567\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not handled ideally, but it mirrors what is displayed in the native Sonos app. Perhaps should also be discarded?

@jjlawren
Copy link
Contributor Author

jjlawren commented Mar 1, 2022

Radio stations usually provide the station name in a separate call (SoCo.get_current_media_info()["channel"]) which could be used as a fallback title, but that should probably be handled by the downstream application.

@pwt
Copy link
Contributor

pwt commented Mar 1, 2022

Radio stations usually provide the station name in a separate call (SoCo.get_current_media_info()["channel"]) which could be used as a fallback title, but that should probably be handled by the downstream application.

Just to note that to get radio show names, I fall back to using avTransport events with something like:

sub = speaker.avTransport.subscribe()
event = sub.events.get(timeout=0.5)
radio_show = event.variables[
       "current_track_meta_data"
].radio_show.rpartition(",")[0]

@jjlawren
Copy link
Contributor Author

jjlawren commented Mar 1, 2022

Just to note that to get radio show names, I fall back to using avTransport events with something like:

Yep, that's the exact same data as what get_current_media_info() provides and how I solve this problem downstream today. I was talking in terms of a pure polling workflow using the built-in methods and making this "automatic" for simpler use-cases.

@pwt
Copy link
Contributor

pwt commented Mar 1, 2022

Just to note that to get radio show names, I fall back to using avTransport events with something like:

Yep, that's the exact same data as what get_current_media_info() provides and how I solve this problem downstream today. I was talking in terms of a pure polling workflow using the built-in methods and making this "automatic" for simpler use-cases.

get_current_media_info() gets the radio channel, but not the radio show (i.e., the equivalent of the title). At least, not in my experimentation.

@jjlawren
Copy link
Contributor Author

jjlawren commented Mar 1, 2022

get_current_media_info() gets the radio channel, but not the radio show (i.e., the equivalent of the title). At least, not in my experimentation.

Ah, yes, I didn't read the attributes closely enough. You're absolutely right and I have not seen that payload in polling calls, either.

I was thinking about the radio station, which would require an additional call to the speaker. The radio show is simply not available without separately consuming subscription events.

Assuming the user would want to use the radio station/channel as the title may not be a safe assumption and should not be done here.

@pwt
Copy link
Contributor

pwt commented Mar 2, 2022

I've merged #904. @jjlawren Do you want to rebase this PR, or should I commit it as-is? Ah ... you just answered my question :)

Once committed, I'll issue a patch release.

Nice work. Great to see all the validation tests.

@pwt pwt merged commit 5c2cd48 into SoCo:master Mar 2, 2022
@jjlawren jjlawren deleted the check_title_for_uri_hints branch March 2, 2022 20:05
pwt pushed a commit that referenced this pull request Mar 2, 2022
* Only discard title if it contains URI components
* Move string URI component test to utils
* Fixes #903
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.

Missing title in get_current_track_info
2 participants