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

[WIP][emby] Initial Contribution #7575

Closed
wants to merge 3 commits into from
Closed

[WIP][emby] Initial Contribution #7575

wants to merge 3 commits into from

Conversation

volfan6415
Copy link

So i have developed a binding to interface with 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 had users using an earlier version of the binding (prior to cleanup commits that i did before making this pull request) for over a 1.5 now and i have been using it myself for that long as well. It has worked well for the users that have tested it for me. (see https://community.openhab.org/t/new-binding-emby-server-binding-alpha/54010)

I would like to start making the necessary changes that i know need to be made to get this code cleaned up and optimized so that it can be incorporated as an official binding.

I know i need to do some things still but i know there will be others that i don't now about, hence, the pull request. I would say the binding in its current state is beta. The core works well but there are features that i could possibly add and changes which need to be made.

One thing i know I will need some clarification on is the author signing. In developing this binding i started with the KODI binding code, as EMBY provides a similar websocket api. I did have to make extensive changes to make it work with EMBY, however, 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. I 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. I know this is wrong. What is the best way to handle this?

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.

Open to all input as to what needs to be done to get this binding where it can be submitted.

volfan6415 added 2 commits May 6, 2020 00:04
*add additional control options
*various bug fixes and code cleanup to prepare for pull request

Signed-off-by: Zachary Christiansen <[email protected]>
* Code cleanup after running code analysis
* optimizations toimprove null handling


Signed-off-by: Zachary Christiansen <[email protected]>
@volfan6415 volfan6415 requested a review from a team as a code owner May 8, 2020 17:27
@J-N-K J-N-K changed the title [EMBY][WIP] Initial Contribution [WIP][emby] Initial Contribution May 9, 2020
@J-N-K J-N-K added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels May 9, 2020
@fwolter
Copy link
Member

fwolter commented Jul 26, 2020

@volfan6415 What's the current state of this PR? Are you interested in a code review?

@volfan6415
Copy link
Author

Yes i would like a code review. I have a hunch that there will need to be some changes so would love the review to get an idea on what i should do.

Thanks

-updates to allow for control play command to be sent when nothing is actively playing

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

I know the review can take awhile. But will i get a review on this binding or will it have to wait until 3.0 is out?

@fwolter
Copy link
Member

fwolter commented Sep 3, 2020

It's on my TODO list. I'll try to get ready before 2.5.9. In the meantime, you could solve the conflicting pom.xml.

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.

Sorry for the delay, I reviewed your code and here is my feedback.

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

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

Comment on lines +9 to +11
_Please describe the different supported things / devices within this section._
_Which different types are supported, which models were tested etc.?_
_Note that it is planned to generate some part of this based on the XML files within ```ESH-INF/thing``` of your binding._
Copy link
Member

Choose a reason for hiding this comment

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

Can you mention the Thing Type IDs?

| Parameter | Description |
|---------|----------------------------------------------------------|
| API Key | This is the API key generated from EMBY used for Authorization. (Generated from your emby server at Dashboard -> Expert -> Advanced -> Security) |
| Web Socket Buffer Size | Here you can define a custom size for the websocket buffer size. Default is 100000. Increasing this can descrease server timeouts in certain cases |
Copy link
Member

Choose a reason for hiding this comment

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

When do the user need to change this? Under which circumstances are the timeouts too high?

Comment on lines +43 to +46
| Image URL | String | Image Max Height
Image Max Width
Image Type
Percent Played | This will produce a URL to the currently playing media. More information about the config parameters can be found at https://github.com/MediaBrowser/Emby/wiki/Images. |
Copy link
Member

Choose a reason for hiding this comment

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

Multiline rows are not supported, at least by the Github markdown renderer. Can you mention the exact names of the config parameters?

Comment on lines +49 to +50
| Media Type | String | None | Description |
| Send Play | String | None | Description |
Copy link
Member

Choose a reason for hiding this comment

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

This is WIP?

<channel-type id="sendplay">
<item-type>String</item-type>
<label>Send Play</label>
<description>String to send a comma-delimited list of item id's that will be played by the client. The string should be in JSON format with all desired parameters from the EMBY API. https://github.com/MediaBrowser/Emby/wiki/Remote-control</description>
Copy link
Member

Choose a reason for hiding this comment

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

I think this is rather an Action, than a Channel.

Copy link
Author

Choose a reason for hiding this comment

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

you are correct will revise

Comment on lines +118 to +137
<option value="MoveUp">Move Up</option>
<option value="MoveDown">Move Down</option>
<option value="MoveLeft">Move Left</option>
<option value="MoveRight">Move Right</option>
<option value="PageUp">Page Up</option>
<option value="PageDown">Page Down</option>
<option value="PreviousLetter">Previous Letter</option>
<option value="NextLetter">Next Letter</option>
<option value="ToggleOsdMenu">Toggle Osd Menu</option>
<option value="ToggleContextMenu">Toggle Context Menu</option>
<option value="ToggleMute">Toggle Mute</option>
<option value="Select">Select</option>
<option value="Back">Back</option>
<option value="TakeScreenshot">Take Screenshot</option>
<option value="GoHome">Go Home</option>
<option value="GoToSettings">Go To Settings</option>
<option value="VolumeUp">Volume Up</option>
<option value="VolumeDown">Volume Down</option>
<option value="ToggleFullscreen">Toggle Full Screen</option>
<option value="GoToSearch">Go To Search</option>
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, these would be rather Actions, too. Same for below. Then, you don't need to rely on the JSON code, too.

Copy link
Author

Choose a reason for hiding this comment

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

You are correct this makes this much cleaner. Will revise

@fwolter
Copy link
Member

fwolter commented Sep 11, 2020

I don't want to push you, but the merge window for 2.5.9 comes closer and closer. If we miss that, the PR must be ported to OH3 manually.

@kaikreuzer
Copy link
Member

Closing this PR as it is not in a mergeable state. Please follow the instructions to port this PR to the main branch for openHAB 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants