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

[plex] Initial contribution #12135

Closed
wants to merge 1 commit into from
Closed

Conversation

aronbeurskens
Copy link

@aronbeurskens aronbeurskens commented Jan 27, 2022

New binding: Plex

Description

This adds a new binding for communication with Plex.
It allows multiple players to be added to OpenHab and provide information about what is currently playing on the play.

When using this binding one can:

  • Create pages with meta information about the media that is begin played
  • Create rules to automatically control lights when a movie is started / paused / stopped

Related links

#6179
#4949
https://community.openhab.org/t/plex-binding-for-oh3-will-it-be-ported/111487

Testing

Latest .jar file available add: https://github.com/aronbeurskens/openhab-addons/releases/tag/v0.13

@aronbeurskens aronbeurskens requested a review from a team as a code owner January 27, 2022 18:25
@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 27, 2022
@fwolter fwolter changed the title New binding - added plex binding for openhab 3 [plex] Initial Contribution Jan 27, 2022
@Hilbrand Hilbrand added the oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 label Jan 27, 2022
@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/plex-binding-for-oh3-will-it-be-ported/111487/178

@aronbeurskens
Copy link
Author

Can somebody help/point me solving the build failures? It seems to fail, but not on the plex part.
How can I reproduce this locally (what command to run) to debug what is causing the failed build?

@lolodomo
Copy link
Contributor

lolodomo commented Jan 28, 2022

Maybe due to the full copyright you added in comments in the file feature.xml?
Look at another binding how should look like this file.

@aronbeurskens
Copy link
Author

Thanks, I will have a look. Is there any way to reproduce this locally, to avoid having to commit and wait each time...

@lolodomo
Copy link
Contributor

lolodomo commented Jan 28, 2022

Yes, by running mvn clean install at the root of your git repo.

@aronbeurskens
Copy link
Author

Thank you @lolodomo . Managed to get it passing the build checks!

@aronbeurskens
Copy link
Author

This is my first contribution / pr so I'm not quite familiar with the way of working. Do I need to do anything to get this merged?

@aronbeurskens
Copy link
Author

Hi all, it has been a while, but letting you all know i am working on an update compatible for version 4.0

@aronbeurskens
Copy link
Author

@fwolter thanks for the feedback. I've updated the PR as much as possible. Please have second look at it.

@aronbeurskens aronbeurskens requested review from fwolter and removed request for jlaur January 3, 2023 15:18
@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/plex-binding-for-oh3-stuck/136598/13

@jlaur
Copy link
Contributor

jlaur commented Mar 21, 2023

I have added "awaiting feedback" because there is still an unresolved comment for quite some time: #12135 (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.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html after building it with mvn clean install.

There are some compiler warnings about null access and dead code left. You could take a look at mvn clean install. To fix the null warnings, you need to store the field to a local variable and do the null check on that.

[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[114,17] The method setupDefaultSecurity(com.thoughtworks.xstream.XStream) from the type com.thoughtworks.xstream.XStream is deprecated
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[162,17] Redundant null check: The variable user cannot be null at this location
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[166,20] Dead code
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[226,16] Null type mismatch (type annotations): 'null' is not compatible to the free type variable 'T'
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[284,28] The constructor org.eclipse.jetty.websocket.client.WebSocketClient(org.eclipse.jetty.util.ssl.SslContextFactory) is deprecated
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[335,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexApiConnector.java:[357,39] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexServerHandler.java:[56,54] The value of the field org.openhab.binding.plex.internal.handler.PlexServerHandler.stateDescriptionProvider is not used
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexServerHandler.java:[159,44] Potential null pointer access: The variable tmpMeta may be null at this location
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexServerHandler.java:[268,35] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexServerHandler.java:[301,36] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexServerHandler.java:[302,13] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\PlexHandlerFactory.java:[76,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexPlayerHandler.java:[48,30] The value of the field org.openhab.binding.plex.internal.handler.PlexPlayerHandler.plexAPIConnector is not used
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexPlayerHandler.java:[70,49] Potential null pointer access: The method getBridge() may return null
[WARNING] C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.plex\src\main\java\org\openhab\binding\plex\internal\handler\PlexPlayerHandler.java:[85,45] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] Code Analysis Tool has found:
 0 error(s)!
 8 warning(s)
 11 info(s)
[WARNING] .binding.plex\README.md:[6]
The line before a Markdown list must be empty.
[WARNING] org.openhab.binding.plex.discovery.PlexDiscoveryService.java:[27]
First javadoc author should have "Initial contribution" contribution description.
[WARNING] org.openhab.binding.plex.discovery.PlexDiscoveryService.java:[29]
Classes/Interfaces/Enums should be annotated with @NonNullByDefault
[WARNING] org.openhab.binding.plex.internal.dto.MediaContainer.java:[23]
First javadoc author should have "Initial contribution" contribution description.
[WARNING] org.openhab.binding.plex.internal.handler.PlexApiConnector.java:[103]
Bei einem equals()-Vergleich sollten String-Literale auf der linken Seite stehen.
[WARNING] org.openhab.binding.plex.internal.handler.PlexPlayerHandler.java:[26]
Stern-Importe der Form '.*' sollten vermieden werden - org.openhab.core.thing.*.
[WARNING] org.openhab.binding.plex.internal.handler.PlexApiConnectorTest.java:[31]
First javadoc author should have "Initial contribution" contribution description.
[WARNING] org.openhab.binding.plex.internal.handler.PlexApiConnectorTest.java:[33]
Classes/Interfaces/Enums should be annotated with @NonNullByDefault

PlexPlayerConfiguration config = getConfigAs(PlexPlayerConfiguration.class);
foundInSession = false;
playerID = config.playerID;
logger.warn("Initializing PLEX player : {}", playerID);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logger.warn("Initializing PLEX player : {}", playerID);

private final Map<String, PlexPlayerHandler> playerHandlers = new ConcurrentHashMap<>();

private PlexServerConfiguration config = new PlexServerConfiguration();
// private @Nullable PlexServerHandler bridge;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// private @Nullable PlexServerHandler bridge;

</channel-type>
<channel-type id="player">
<item-type>Player</item-type>
<label>Player Control Play/Pause/Next/Previous</label>
Copy link
Member

Choose a reason for hiding this comment

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

Labels are expected to be as short as possible. Guideline is 2-3 words with max 20-25 chars. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

Suggested change
<label>Player Control Play/Pause/Next/Previous</label>
<label>Player Control</label>

@mlobstein
Copy link
Contributor

How long does this remain in awaiting feedback status before being closed? Although I don't use plex, I wouldn't mind seeing this make it into the official distribution. I would be willing to finish the changes needed to get this done.

@jlaur
Copy link
Contributor

jlaur commented Jun 7, 2023

How long does this remain in awaiting feedback status before being closed? Although I don't use plex, I wouldn't mind seeing this make it into the official distribution. I would be willing to finish the changes needed to get this done.

@aronbeurskens - would that be okay with you? And @fwolter, do you think you would be able to finish the review if @mlobstein would help resolving the last comments?

@fwolter
Copy link
Member

fwolter commented Jun 7, 2023

Yes!

@aronbeurskens
Copy link
Author

Of course, no problem. When merged its also easier for me to maintain

@mlobstein
Copy link
Contributor

replaced by #15057

@mlobstein mlobstein closed this Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback 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.

[plex] Add Plex Binding