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

[ClementineRemote] Initial contribution #14110

Closed
wants to merge 35 commits into from

Conversation

StephanRichter
Copy link
Contributor

ClementineRemote Binding

This new binding allows controlling Clementine Player from OpenHAB.

The binding is located @ bundles/org.openhab.binding.clementineremote.

Description

Connection between Handler and Clementine Player is done via Google Protocol Buffers.
The protobuf definition file is included in the folder proto.

Based on this definition file, a class ClementineRemote.java has been generated in the package bundles/org.openhab.binding.clementineremote/src/main/java/de/qspool/clementineremote/backend/pb via the command

bundles/org.openhab.binding.clementineremote$> protoc --java_out=src/main/java/ proto/remotecontrolmessages.proto

OpenHAB version

As per request from Maintainer, this commit is now based on lastest 4.0.0 main.

Static Code Analysis

All code in org/openhab/binding/clementineremote should conform to the requirements given in the Coding Guidelines.

However the code in de/qspool/clementineremote/backend/pb does not! As this class is script-generated, I think it should be excluded from code analysis.
There already is a thread for discussion of that topic.

openhab-bot and others added 2 commits June 26, 2022 21:01
This is a squash commit to tidy up experiments.

Signed-off-by: Stephan Richter <github@srsoftware.de>
@StephanRichter StephanRichter requested a review from a team as a code owner December 29, 2022 17:16
@jlaur jlaur changed the title [ClementineRemote] new binding! [ClementineRemote] Initial contribution Dec 29, 2022
@jlaur jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 29, 2022
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, did a basic review. Didn't look at the ...lementineremote/src/main/java/de/qspool/clementineremote/backend/pb/ClementineRemote.java 36k lines :-) and two other files, Maybe look at that later.

There also has to come a solution for the auto-generated files as the current build fails due to these files.

note: im not an addons maintainer


* Project home: https://www.openhab.org

== Declared Project Licenses
Copy link
Contributor

Choose a reason for hiding this comment

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

No license expert here, just want to mention that i see a different license for the auto generated files like: /src/main/java/de/qspool/clementineremote/backend/pb/ClementineRemote.java

The dependency on com.google.protobuf might need to be mentioned, but as said, i'm no expert here.

Copy link
Contributor Author

@StephanRichter StephanRichter Jan 8, 2023

Choose a reason for hiding this comment

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

I've updated the NOTICE file in a recent commit. But I'm no expert in these things. Again, I would be grateful for any suggestion how to improve this text.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlaur can you advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this to the end of this file

== Third-party Content

protobuf-java
* License: BSD License
* Project: https://developers.google.com/protocol-buffers
* Source:  https://github.com/protocolbuffers/protobuf

Stephan Richter added 6 commits January 6, 2023 08:48
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
@StephanRichter
Copy link
Contributor Author

StephanRichter commented Jan 8, 2023

I fixed all suggestions by lsiepel – with on exception:

I did not change the channel type of playback-control to the predefined system.media-control, as that does not seem to support STOP command.

@lsiepel
Copy link
Contributor

lsiepel commented Jan 8, 2023

I fixed all suggestions by lsiepel – with on exception:

I did not change the channel type of playback-control to the predefined system.media-control, as that does not seem to support STOP command.

Great, some small issues left. Maybe look at some other binding regarding the player / media control. Don't have a real opinion on it, juist know there is a dedicated control for such tasks.

I leave this now for a real maintainer to review. :-)

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Have also commented on some parts that could improve in the handler, one of the files
i skipped earlier.

Comment on lines +1 to +9
<?xml version="1.0" encoding="UTF-8"?>
<binding:binding id="clementineremote" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:binding="https://openhab.org/schemas/binding/v1.0.0"
xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd">

<name>Clementine Remote Control</name>
<description>This is a binding to remotely control Clementine Player instances.</description>

</binding:binding>
Copy link
Member

Choose a reason for hiding this comment

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

Please convert to the new addon.xml.

Copy link
Contributor

Choose a reason for hiding this comment

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

This addon.xml is not yet common knowledge and the PR to adapt the developer docs is not merged yet, so if you need more details, the corrosponding issue is here: openhab/openhab-core#2058
But you can also update your branch and look at another binding's addon.xml and adapt it to this initial contribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Stephan Richter and others added 11 commits February 7, 2023 21:05
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
…Channels

Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
This is a squash commit to tidy up experiments.

Signed-off-by: Stephan Richter <github@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Copy link
Contributor

@lsiepel lsiepel 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 making the improvements. There are some builds warnings left (Potential null pointer access). Hopefully @jlaur can advice how to exclude the 3rd party generated file from SAT analysis. as it is currently cluttering the build log.

Stephan Richter added 14 commits February 8, 2023 23:50
…c channel (for all channels).

Refactored connection methods, implemented failover by using sheduler

Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
Signed-off-by: Stephan Richter <s.richter@srsoftware.de>
@StephanRichter
Copy link
Contributor Author

Hm, any progress here? Did I miss something when fixing the denoted issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was probably added by mistake, can you remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I'm a bit short in time. I will look into this later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should probably be moved to bundles/org.openhab.binding.clementineremote/src/3rdparty/proto/remotecontrolmessages.proto since it belongs to a different namespace with a difference license.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should probably be moved to bundles/org.openhab.binding.clementineremote/src/3rdparty/java/backend/pb/ClementineRemote.java since it belongs to a different namespace with a difference license.

@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Apr 23, 2023
<parent>
<groupId>org.openhab.addons.bundles</groupId>
<artifactId>org.openhab.addons.reactor.bundles</artifactId>
<version>4.0.0-SNAPSHOT</version>
Copy link
Member

@wborn wborn Jul 24, 2023

Choose a reason for hiding this comment

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

Suggested change
<version>4.0.0-SNAPSHOT</version>
<version>4.3.0-SNAPSHOT</version>

@lsiepel
Copy link
Contributor

lsiepel commented Sep 9, 2024

@StephanRichter there has been some time without any updated / commits to this PR. Do you think you will be able to finish this or did you abandon this PR? At some time we will close this PR due to inactivity.

@StephanRichter
Copy link
Contributor Author

StephanRichter commented Sep 29, 2024

Sorry for the late answer. I'm currently completely overloaded and have no spare time to invest here. The code is working (I use it on a daily base) – if anyone can make editions to fulfill your requirements: feel free to do so.
I tried to implement all requirements requested so far, with no success. I give up.

@lsiepel
Copy link
Contributor

lsiepel commented Oct 2, 2024

Thank you for your reply. While this PR is not that far from being merged, i hope you finish it somewhere in the future. But as you said to give up at this moment i think it is better to close it here. Obvious the code remains available in your branch, so whenever you or another contributer want to move this forward the code can be re-used.

@lsiepel lsiepel closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Awaiting feedback from the pull request author 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.

5 participants