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

[sonos] Fix warnings and fix parsing of room name #15708

Merged
merged 5 commits into from
Jun 9, 2024

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Oct 6, 2023

Fix some compiler and checkstyle warnings:

  • XMLReaderFactory is deprecated.
  • Potential null pointer access.
  • Unsupported @SuppressWarnings("PMD.CompareObjectsWithEquals").
  • The import org.openhab.core.audio.AudioSink is never used.
  • UnsynchronizedStaticFormatter: Static Formatter objects should be accessed in a synchronized manner.

Additionally:

  • Fix error logged when trying to query an offline device.
  • Reduce log level from error to warn for some non-critical failures.

Fixes #6793

@jlaur jlaur marked this pull request as draft October 6, 2023 12:55
@jlaur jlaur added the work in progress A PR that is not yet ready to be merged label Oct 6, 2023
@jlaur jlaur force-pushed the sonos-fix-warnings branch from d0a4926 to bd9ef50 Compare October 6, 2023 22:37
@jlaur jlaur removed the work in progress A PR that is not yet ready to be merged label Oct 6, 2023
@jlaur jlaur marked this pull request as ready for review October 6, 2023 22:43
@jlaur jlaur marked this pull request as draft October 7, 2023 12:16
@jlaur
Copy link
Contributor Author

jlaur commented Oct 7, 2023

Short update: I'm struggling a bit with the SAXParser refactoring. Metadata parsing is currently broken, I'm debugging that.

@jlaur jlaur force-pushed the sonos-fix-warnings branch 4 times, most recently from 2adc8a7 to 22715e2 Compare October 7, 2023 14:51
@jlaur jlaur force-pushed the sonos-fix-warnings branch 3 times, most recently from 1670b0d to 61b30e4 Compare October 7, 2023 19:16
@jlaur jlaur marked this pull request as ready for review October 7, 2023 19:42
@jlaur jlaur requested a review from a team October 7, 2023 19:43
@jlaur
Copy link
Contributor Author

jlaur commented Oct 7, 2023

Finally ready for review. I found an interesting behavior when writing the test for getMetaDataFromXML: If the provided XML (in the test) is pretty printed, the parsing will fail. This XML:

<DIDL-Lite xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:upnp="urn:schemas-upnp-org:metadata-1-0/upnp/" xmlns:r="urn:schemas-rinconnetworks-com:metadata-1-0/" xmlns="urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/">
	<item id="-1" parentID="-1" restricted="true">
		<res protocolInfo="sonos.com-http:*:application/x-mpegURL:*" duration="0:03:33">x-sonosapi-hls-static:librarytrack%3ai.eoD8VQ5SZOB8QX7?sid=204&amp;flags=8232&amp;sn=9</res>
		<r:streamContent/>
		<r:radioShowMd/>
		<r:streamInfo>bd:16,sr:22050,c:3,l:0,d:0</r:streamInfo>
		<upnp:albumArtURI>/getaa?s=1&amp;u=x-sonosapi-hls-static%3alibrarytrack%253ai.eoD8VQ5SZOB8QX7%3fsid%3d204%26flags%3d8232%26sn%3d9</upnp:albumArtURI>
		<dc:title>Turn Down for What</dc:title>
		<upnp:class>object.item.audioItem.musicTrack</upnp:class>
		<dc:creator>DJ Snake &amp; Lil Jon</dc:creator>
		<upnp:album>Turn Down for What - Single</upnp:album>
	</item>
</DIDL-Lite>

will return album as "Turn Down for What - Single\n\t". This seems quite wrong, since the white space after the ending of the XML tag is included. However, it is outside the scope of this PR to fix/mess with. For that reason, to be on the safe side, I did a third commit to revert some white space changes, which could cause problems if put through the parser.

@lolodomo
Copy link
Contributor

lolodomo commented Oct 8, 2023

I propose to merge after milestone 2.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review of all except class SonosXMLParser

@jlaur jlaur force-pushed the sonos-fix-warnings branch from e1aabec to fe08d4e Compare October 16, 2023 21:46
@jlaur
Copy link
Contributor Author

jlaur commented Nov 3, 2023

@lolodomo - gentle ping. Might be nice to include in next milestone for some additional testing. 🙂

@lolodomo
Copy link
Contributor

lolodomo commented Nov 3, 2023

@lolodomo - gentle ping. Might be nice to include in next milestone for some additional testing. 🙂

I would prefer to test it myself before merging it for everyone. But I will finish my review soon.

@jlaur
Copy link
Contributor Author

jlaur commented Nov 30, 2023

@lolodomo - ping, only in case you forgot. :)

@jlaur jlaur force-pushed the sonos-fix-warnings branch from fe08d4e to 74ae093 Compare December 13, 2023 19:57
@jlaur jlaur force-pushed the sonos-fix-warnings branch 2 times, most recently from c237737 to 7846ce9 Compare December 31, 2023 11:02
@jlaur jlaur force-pushed the sonos-fix-warnings branch 2 times, most recently from ffcf1f5 to 2a06fbd Compare January 13, 2024 16:43
@jlaur jlaur requested a review from a team January 25, 2024 13:08
@jlaur jlaur force-pushed the sonos-fix-warnings branch from 2a06fbd to 5349d08 Compare February 7, 2024 21:50
@jlaur jlaur force-pushed the sonos-fix-warnings branch from 5349d08 to a0f3d23 Compare March 9, 2024 11:38
@lolodomo
Copy link
Contributor

lolodomo commented Apr 2, 2024

@jlaur : now that the problem with JUPnP impacting the Sonos binding is fixed, I will come back to you.

@jlaur jlaur force-pushed the sonos-fix-warnings branch from a0f3d23 to 2c84947 Compare April 3, 2024 18:34
@jlaur jlaur force-pushed the sonos-fix-warnings branch from 2c84947 to 06ee37b Compare May 4, 2024 20:46
jlaur added 5 commits June 1, 2024 14:58
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the sonos-fix-warnings branch from 06ee37b to b066a1e Compare June 1, 2024 12:58
@jlaur
Copy link
Contributor Author

jlaur commented Jun 6, 2024

@lolodomo - sorry for pinging you again, just want to check if this PR is still on your review list?

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.
I am going to deploy the changes and test before merging.

@lolodomo
Copy link
Contributor

lolodomo commented Jun 9, 2024

Tests are OK

@lolodomo lolodomo merged commit ed9fbf0 into openhab:main Jun 9, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Jun 9, 2024
@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Jun 9, 2024
@lolodomo lolodomo changed the title [sonos] Fix warnings [sonos] Fix warnings and error when parsing room name Jun 9, 2024
@jlaur
Copy link
Contributor Author

jlaur commented Jun 9, 2024

Tests are OK

Thanks for the review and testing.

@lolodomo lolodomo changed the title [sonos] Fix warnings and error when parsing room name [sonos] Fix warnings and fix parsing of room name Jun 10, 2024
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
* Fix warnings
* Fix discovery error logging for offline devices

Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
psmedley added a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
@jlaur jlaur deleted the sonos-fix-warnings branch June 23, 2024 21:45
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* Fix warnings
* Fix discovery error logging for offline devices

Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* Fix warnings
* Fix discovery error logging for offline devices

Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* Fix warnings
* Fix discovery error logging for offline devices

Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
* Fix warnings
* Fix discovery error logging for offline devices

Fixes openhab#6793

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sonos] "Could not parse Sonos room name from string" in logging
2 participants