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

[unifiedremote] Initial contribution #8546

Merged
merged 9 commits into from
Oct 24, 2020
Merged

[unifiedremote] Initial contribution #8546

merged 9 commits into from
Oct 24, 2020

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented Sep 22, 2020

Copied from initial pr(#8494):

Unified Remote Binding.

This binding allow send some command to the unified remote server through its web client interface.
It create some channels to control the mouse, some navigation keys, some media keys, and not much more.

Good day. This is my first contribution to this amazing product. Some days ago I started search for a way to control my computer with openHAB and I decided to try to write this binding that seems to finally be working.
Please let me know if there are any suggestions to improve the binding as I'm still getting in touch with this technology. I will also open a thread in the Add-ons forum pointing to this pr, in case someone wants to try it or have an idea for a better implementation. Thanks in advance for your time.

download binding jar

@GiviMAD GiviMAD requested a review from a team as a code owner September 22, 2020 15:53
Signed-off-by: GiviMAD <miguelwork92@gmail.com>
@TravisBuddy
Copy link

TravisBuddy commented Sep 22, 2020

Travis tests were successful

Hey @GiviMAD,
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 changed the title [unifiedremote] add binding code [unifiedremote] Initial contribution Sep 22, 2020
@Hilbrand Hilbrand added the new binding If someone has started to work on a binding. For a new binding PR. label Sep 22, 2020
@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 23, 2020
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.

Thanks for your contribution! I reviewed your code and found only minor things. Good code in general.

@fwolter fwolter self-assigned this Oct 10, 2020
Signed-off-by: GiviMAD <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from fwolter October 18, 2020 14:26
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.

There are some markdown warnings left. You could take a look at target/code-analysis/report.html.

Signed-off-by: GiviMAD <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from fwolter October 18, 2020 18:35
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 Oct 18, 2020
@fwolter fwolter removed their assignment Oct 18, 2020
@fwolter
Copy link
Member

fwolter commented Oct 18, 2020

Can you fix the conflicting CODEOWNERS file?

@GiviMAD
Copy link
Member Author

GiviMAD commented Oct 18, 2020

Done!

[MeteoAlerte] Update for OH3 (#8801)
@cpmeister
Copy link
Contributor

Can you fix the conflicting pom.xml file?

@GiviMAD
Copy link
Member Author

GiviMAD commented Oct 22, 2020

the build finally works!

@cpmeister
Copy link
Contributor

Almost done, just resolve these warnings and you are finished!

[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[70,36] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[73,36] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[80,21] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[106,61] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[115,21] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[118,41] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteHandler.java:[136,13] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.unifiedremote/src/main/java/org/openhab/binding/unifiedremote/internal/UnifiedRemoteDiscoveryService.java:[100,20] The value of the field org.openhab.binding.unifiedremote.internal.UnifiedRemoteDiscoveryService.UnifiedRemoteUdpDiscovery.ServerInfo.publicIp is not used

To deal with the null warnings you should assign the field to a local variable and perform your operations on that local variable instead.

Signed-off-by: GiviMAD <miguelwork92@gmail.com>
@GiviMAD
Copy link
Member Author

GiviMAD commented Oct 23, 2020

I think I get rid of all the warnings. Please also note I make a small change in the catch clause in the fie UnifiedRemoteHandler.java to consider the thing as offline is the TimeoutException is thrown. And thanks for the tip on how to deal with the nullable type warnings,I wasn't sure about how to solve those.

Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

I thought I was done before, but I took another look and found these two things.
These will be my last change suggestions through so fix these and I'll approve and merge this.
Thanks for all your work!

Signed-off-by: GiviMAD <miguelwork92@gmail.com>
@GiviMAD
Copy link
Member Author

GiviMAD commented Oct 24, 2020

Done! Please note I change the exception control in the UnifiedRemoteHandler.java again, I was testing the binding this morning in my home installation of openHAB (arm Kubernetes cluster) and I'm getting a NotRouteToHostException when thing turns offline. Sorry for the late change and thanks for your help reviewing the pr.

@cpmeister cpmeister merged commit 8b8b79c into openhab:main Oct 24, 2020
@cpmeister
Copy link
Contributor

Done

@cpmeister cpmeister added this to the 3.0.0.M2 milestone Oct 24, 2020
nowaterman pushed a commit to nowaterman/openhab-addons that referenced this pull request Jan 19, 2021
boehan pushed a commit to boehan/openhab-addons that referenced this pull request Apr 12, 2021
Signed-off-by: GiviMAD <miguelwork92@gmail.com>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: GiviMAD <miguelwork92@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants