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

[emby] Initial contribution #13340

Closed
wants to merge 23 commits into from
Closed

[emby] Initial contribution #13340

wants to merge 23 commits into from

Conversation

volfan6415
Copy link

This request picks up on the initial contribution from [WIP][emby] Initial Contribution by volfan6415 · Pull Request #7575 · openhab/openhab-addons (github.com) but updates the binding for support in openhab3 as well as other code clean up and improvements.

This binding interfaces openHAB with a running instance of EMBY Media Server (https://emby.media/). EMBY is a PLEX alternative and the binding provides many of the same features for users of EMBY as the PLEX binding does for uses of PLEX.
I have used the version of the binding which was previously submitted for over two years now (albeit in openHAB 2.X as the binding was not compatible until now with openHAB 3)(and as referenced in this thread [New Binding] EMBY Server Binding - Alpha - Add-ons / Bindings - openHAB Community I believe others in the community have been using it for awhile now as well. It has worked well for the users that have tested it for me.

I have made the necessary updates to make the binding operate in openHAB 3 and now feel that this binding is ready to be incorporated. The core works well. While there are certainly features that can be added over time the binding in its current state more than covers the basics of what someone using openHAB would want to get from EMBY. It allows for interaction with the EMBY server to get currently playing devices/media which can then be used inside of openHAB as triggers for automations and the like.
I would like to flag that there will need to be clarification on is the author signing. In developing this binding I started with the KODI binding code, as EMBY provides a similar websocket api. While I did make extensive changes to work with the API provided by EMBY, there is definitively a structure left from the KODI binding and there are some methods and classes where the only real change was method/class names. Unfortunately in making these changes did not keep the author tags on the classes and now have all of the classes showing me as the original contribution. It is not my intention to not give credit where credit is due, and so will need some guidance on what edits need to be made to the author tags in the file to rectify this situation.

I know there are significant updates which need to be made to the readme and I will be working on those put wanted to start this pull request so i could get some comments and suggestions on the underlying code before i spend too much time on the readme as that may very well change.

@volfan6415 volfan6415 requested a review from a team as a code owner August 31, 2022 19:44
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/new-binding-emby-server-binding-alpha/54010/117

@wborn wborn added the new binding If someone has started to work on a binding. For a new binding PR. label Aug 31, 2022
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
Signed-off-by: Zachary Christiansen <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented Dec 20, 2022

If you can fix the merge conflict, i can do a first review.

Signed-off-by: Zachary Christiansen <[email protected]>
@volfan6415
Copy link
Author

volfan6415 commented Jan 6, 2023

@lsiepel ok so i think i have fixed the merge conflict

@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be adapted to the new addon.xml, the PR to adapt the developer docs is not merged yet, so if you need more details, the corresponding 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

@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.

Partial review.
There are some SAT errors and i think the log levels need to be adjusted. See: https://www.openhab.org/docs/developer/guidelines.html#f-logging

c.send(sendPacket);
logger.trace(">>> Request packet sent to: {} ({})", REQUEST_MSG, REQUEST_PORT);
} catch (Exception e) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentionally not logging anything?

REQUEST_PORT);
c.send(sendPacket);
} catch (Exception e) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentionally not logging anything?

} catch (IOException ex) {
logger.debug("The exception was: {}", ex.getMessage());
} finally {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to close the port / clear resource in this finally block?

if (obj instanceof Number) {
return ((Number) obj).intValue();
} else if (obj instanceof String) {
return Integer.parseInt(obj.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

As the key is not checked, don't you want to catch the NumberFormatException?

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"Connection could not be established");
}
}, 360000, 360000, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor codestyle, make it a binding constant

ScheduledExecutorService setScheduler, int setBufferSize, WebSocketClient sharedWebSocketClient) {
eventHandler = setEventHandler;
uri = setUri;
// client = new WebSocketClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

*/
public synchronized void open() throws Exception {
if (isConnected()) {
logger.warn("connect: connection is already open");
Copy link
Contributor

Choose a reason for hiding this comment

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

reduce to debug

public class EmbyWebSocketListener {
@OnWebSocketConnect
public void onConnect(Session wssession) {
logger.info("EMBY client socket is Connected to emby server");
Copy link
Contributor

Choose a reason for hiding this comment

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

As the thing would go online, it allready signals the correct connection. So please reduce to debug

try {
eventHandler.onConnectionOpened();
} catch (Exception e) {
logger.error("Error handling onConnectionOpened(): {}", e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

reduce to debug. if the binding cannot recover the thing should be set to offline and provided with errordetails.

@@ -0,0 +1,21 @@
# FIXME: please substitute the xx with a proper locale, ie. de
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run mvn i18n:generate-default-translations

@lsiepel lsiepel added the awaiting feedback Awaiting feedback from the pull request author label Mar 11, 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 Jul 24, 2023

@volfan6415 last commit is 6 months old. Are you able to finish the binding?

@lsiepel
Copy link
Contributor

lsiepel commented Apr 29, 2024

ping @volfan6415 do you plan to finish this PR?

@lsiepel lsiepel added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Jul 9, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Sep 6, 2024

Does not look like this PR will get to the finish line. @volfan6415 please re-open (or comment) on this PR if you want to continue. Due to no responses i close this PR for now.

@lsiepel lsiepel closed this Sep 6, 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. stale As soon as a PR is marked stale, it can be removed 6 months later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants