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

[mpd]: Music Player Daemon initial contribution #7870

Merged
merged 10 commits into from
Aug 21, 2020

Conversation

stefanroellin
Copy link
Contributor

Signed-off-by: Stefan Roellin stefan@roellin-baumann.ch

This binding controls Music Player Daemons (https://www.musicpd.org/) and is meant to replace the corresponding binding from openhab1-addons (https://www.openhab.org/addons/bindings/mpd1/)

Almost all functionality from the old binding is supported, except

  • playsong
  • playsongid

However, the new implementation has actions, which allow to send arbitrary commands to a Music Player Daemon. With those actions, the missing functionality can be replaced easily and is much more flexible.

Example with actions:

rule "turn on morning music"
when
        Item morning_music changed to ON
then
        val actions = getActions("mpd","mpd:mpd:music")
        if(actions === null) {
                logWarn("myLog", "actions is null")
                return
        }

        actions.sendCommand("clear")
        actions.sendCommand("load", "MorningMusic");
        actions.sendCommand("shuffle");
        actions.sendCommand("play");
end

@stefanroellin stefanroellin requested a review from a team as a code owner June 7, 2020 09:14
@TravisBuddy
Copy link

Travis tests were successful

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

@Hilbrand Hilbrand added the oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 label Jun 7, 2020
@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 13, 2020
@TravisBuddy
Copy link

Travis tests were successful

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

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Your clean code makes reviewing easy! There is one checkstyle warning left.

@stefanroellin
Copy link
Contributor Author

@fwolter thanks for the review. I have made the requested changes.

@TravisBuddy
Copy link

Travis tests were successful

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

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Your changes look good! I found a few minor things I overlooked during the first review. Sorry for that.

@TravisBuddy
Copy link

Travis tests were successful

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

@stefanroellin
Copy link
Contributor Author

Your changes look good! I found a few minor things I overlooked during the first review. Sorry for that.

Thanks again @fwolter for the review and no problem at all. I appreciate any feedback.
I have implemented the suggested changes except for the last one - see my comment #7870 (comment)

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

@fwolter fwolter added the cre Coordinated Review Effort label Jul 19, 2020
Copy link
Member

@martinvw martinvw left a comment

Choose a reason for hiding this comment

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

Hello @stefanroellin,

Thanks again for your contribution the code looks good!

I added some comments regarding the error messages which are displayed when things go wrong and some other details. Please let me know if you have any questions!

}

private void closeSocket() {
logger.debug("Closing socket");
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need to close all of them? Will closing some of them not take a long the rest. E.g closing both the reader and inputStreamReader is AFAIK not needed.

A quick peak in the javadoc suggested that closing the socket will do all the work at one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have realized that the writer (DataOutputStream) is not necessary, since one can use socket.getOutputStream() directly.
@fwolter has suggested in #7870 (comment) to also close the InputStreamReader, which is in contrast to your suggestion. From the mentioned javadoc it is clear that the InputStream and the OutputStream will also be closed, when the socket is closed. I do not close these streams explicitly.
However, I would at least close the BufferedReader. But it does not harm, if I also close the InputStreamReader. I prefer to close too much than too little. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with keeping it like this. But there is a reason they mention it. Sometimes closing twice could lead to errors/exceptions and when not explicitly documented I would rather not.

@martinvw martinvw self-assigned this Aug 12, 2020
@stefanroellin
Copy link
Contributor Author

@martinvw thanks for your review. I have implemented your suggestions and commented some of them. I would appreciate your feedback.

throw new MPDException("Missing parameter ipAddress");
}
if (port < 1 || port > 65335) {
throw new MPDException("Invalid parameter port");
Copy link
Member

Choose a reason for hiding this comment

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

Very minor:

Suggested change
throw new MPDException("Invalid parameter port");
throw new MPDException("Invalid port parameter");

Otherwise, it might suggest that the parameter port is invalid and should not be supplied.

Copy link
Member

Choose a reason for hiding this comment

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

@stefanroellin please let me know if you want to fix this tiny comment otherwise I'm fine merging as is, thanks for your work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will push a fix for it.

if (logger.isTraceEnabled()) {
logger.trace("send command '{}'", command.asLine());
}
final Socket socket = this.socket;
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to fetch the socket.getOutputStream() here so that you don't have to fetch it twice or was there a specific reason to do it like this?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe socket can be nulled out and socket. getOutputStream not, sensible enough :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of using a local variable for getOutputStream(), but I find the current code is easier to read and circumvents the problem you describe.

@martinvw martinvw added the rebuild Triggers Jenkins PR build label Aug 20, 2020
@martinvw
Copy link
Member

FTR: Jenkins reports:

[ERROR] .binding.mpd/pom.xml:[0]
The pom version is different from the parent pom version
[INFO] Detailed report can be found at: file:/home/jenkins/jenkins-agent1/workspace/PR-openHAB2-Addons/bundles/org.openhab.binding.mpd/target/code-analysis/report.html

I would guess that is because the branch was not fully rebased, the version of the MPD binding seems fine.

@martinvw martinvw removed the rebuild Triggers Jenkins PR build label Aug 20, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
@stefanroellin
Copy link
Contributor Author

@martinvw I have pushed some changes:

  • text for an invalid port is clearer
  • change of text for an invalid host
  • do not leak password to log file
  • solved a problem, when no password is specified, but one is required, i.e. permission problem
  • rebased to origin/2.5.x

Copy link
Member

@martinvw martinvw left a comment

Choose a reason for hiding this comment

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

Thanks a lot

@martinvw martinvw merged commit 5d100e9 into openhab:2.5.x Aug 21, 2020
@martinvw martinvw added this to the 2.5.8 milestone Aug 21, 2020
@stefanroellin stefanroellin deleted the mpd branch August 21, 2020 15:48
taboneclayton pushed a commit to taboneclayton/openhab-addons that referenced this pull request Aug 23, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Clayton Tabone <taboneclayton@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Signed-off-by: Stefan Roellin <stefan@roellin-baumann.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants