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] Removed dependency on 'org.apache.commons.io.IOUtils' #7730

Closed
wants to merge 2 commits into from

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented May 22, 2020

Relative to #7722

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo lolodomo requested a review from kgoderis as a code owner May 22, 2020 09:15
@lolodomo
Copy link
Contributor Author

This was pushed without a working maven buiuld (Maven build is currently not working due to download failures).

Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Laurent Garnier <lg.hc@]free.fr>
@lolodomo
Copy link
Contributor Author

This was pushed without a working maven buiuld (Maven build is currently not working due to download failures).

Using the offline mode (mvn -o clean install), the local build succeeded.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@@ -115,7 +114,11 @@ public void process(AudioStream audioStream)
logger.warn("We do not have any callback url, so Sonos cannot play the audio stream!");
}
} else {
IOUtils.closeQuietly(audioStream);
try {
audioStream.close();
Copy link
Contributor

@cweitkamp cweitkamp May 23, 2020

Choose a reason for hiding this comment

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

I once again have to jump into the discussion if we really have to close the passed AudioStream in AudioSink implementations. IIRC Chromecast and Kodi do not close it and I am not aware of any resource leak. Maybe we can find a final decision on it in openhab/openhab-core#532.

At the end we should make it clear in the JavaDoc for the Audiosink#process(AudioStream) method.

@lolodomo
Copy link
Contributor Author

As it is apparently not clear if the audio stream has to be closed or not, I close my change proposal until this point is clearer.

@lolodomo lolodomo closed this May 25, 2020
@lolodomo lolodomo deleted the sonos_ioutils branch May 25, 2020 12:42
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.

None yet

3 participants