Support for non-clients, NVidia shield, dynamic grouping, extra metad… #6054

JesseWebDotCom · 2017-02-17T03:50:41Z

This is my first contribution so forgive me if I misstep.

A full breakdown of the updates are here:

If you agree with the direction and or changes, I can start drafting the updated version of:
https://home-assistant.io/components/media_player.plex/

Let me know if there is an official process for updating documentation
Thanks

homeassistant · 2017-02-17T03:50:44Z

Hi @JesseWebDotCom,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

+        # or when casting to an Nvidia shield running a plex server
+        if self.local:
+            return None
+        #No mute since Shield only supports volume 2-100


block comment should start with '# '

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

+                thumb_response = requests.get(thumb_url, verify=False)
+                if thumb_response.status_code != 200:
+                    _LOGGER.debug(
+                        'Using art because thumbnail was not found: content id %s',


line too long (83 > 79 characters)

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

@@ -62,6 +91,17 @@ def config_from_file(filename, config=None):

 def setup_platform(hass, config, add_devices_callback, discovery_info=None):
    """Setup the Plex platform."""
+
+    #optional parameters


block comment should start with '# '

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

 from homeassistant.loader import get_component
 from homeassistant.helpers.event import (track_utc_time_change)
+import homeassistant.helpers.config_validation as cv


module level import not at top of file

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

-    STATE_UNKNOWN)
+    SUPPORT_PLAY, SUPPORT_VOLUME_MUTE, SUPPORT_TURN_OFF, SUPPORT_SEEK,
+    PLATFORM_SCHEMA, MediaPlayerDevice)
+from homeassistant.const import (DEVICE_DEFAULT_NAME, STATE_IDLE, STATE_OFF,


module level import not at top of file
'homeassistant.const.STATE_UNKNOWN' imported but unused

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

 import json
 import logging
 import os
 from datetime import timedelta
 from urllib.parse import urlparse

+import voluptuous as vol


module level import not at top of file

houndci-bot · 2017-02-17T03:50:46Z

homeassistant/components/media_player/plex.py

+from requests.packages.urllib3.exceptions import InsecureRequestWarning
+requests.packages.urllib3.disable_warnings(InsecureRequestWarning)
+
+import asyncio


module level import not at top of file

JesseWebDotCom · 2017-02-17T03:58:23Z

I can't sign the CLA - the web page spinner never stops (tried from multiple browsers, clients, and networks).

robbiet480 · 2017-02-18T10:28:11Z

@JesseWebDotCom Can you provide a full list of changes?

JesseWebDotCom · 2017-02-18T14:36:05Z

My original documentation is here:
https://github.com/JesseWebDotCom/home-assistant-configuration/blob/master/docs/media_player_plexdevices.md

change log (hopefully that’s everything):

Improved compatibility
- Added support for non-clients
  - remote users
  - PlexConnect Apple TV’s
  - Other non-controllable clients
- Improved support for channels (displays and is controllable providing the channel session closely resembles a movie or TV session)
Improved controls
- Added volume controls (volume up/down, mute)
  - Faking starting volume (as 100) as plexapi does not expose current volume
  - Faking muting as plexapi does not expose mute state
- Added Off
  - Faking by stopping media as plexapi does not provide an off method
- Added progress bar
- Restrict controls based on identified compatibility issues:
  - Remove controls if client is local (i.e. loopback with no controllable IP)
  - Remove mute for Nvidia shields (they do not support volume lower than 2)
- Added ability (enabled through optional config) to avoid control restriction and always show all controls
Improved metadata
- Display library name in “app_name” attribute
- Display movie name as “name (year)”, ex: “Blair Witch (2016)”
- Display TV show and episode number with leading 0 (ex. S02 instead of S2)
- Display music artist
- All music attributes (media artist, album name, album artist, track number)
  - Fallback to Album artist is media artist is blank
- Media position and duration attributes
- Added fallback to display TV season number when API seasons() call fails
- Added ability (enabled through optional config) to display the thumbnail for the playing TV episode as opposed to the show
- Display art if thumbnail is non-existent (useful for tv/movies without posters)
Added ability (enabled through optional config) to have entity ID’s named media_player.{plex player id} instead of media_player.{plex player name}.
Added PlayMedia service abilities to make a plex client play a movie, tv show, or music playlist
(enabled through optional config) Added dynamic “active” and “inactive” groups
- I’d like to establish a pattern here for automated grouping and to avoid conflicts with manual grouping. I chose to prefix my automated group names with underscores.
- There has to be some problem with how I created the groups as when I run this “{{ states.group._plex_devices_disconnected.attributes.entity_id | length }}” I get this “Error rendering template: SecurityError: access to attribute '_plex_devices_disconnected' of 'DomainStates' object is unsafe.”
Added optional config parameters (not sure I did this right but it works) to do the following:
- 'include_non_clients’: includes non-controllable clients (ex. PlexConnect Apple TV’s), defaults to false (for backwards compatibility)
- 'use_episode_art’: Use episode art instead of show art, defaults to false (for backwards compatibility)
- 'use_dynamic_groups’: Automatically group devices into active and inactive groups, defaults to false (for backwards compatibility)
- 'use_custom_entity_ids’: Name entities by device id (less ambiguous, more predictable names), defaults to false (for backwards compatibility)
- 'show_all_controls’: Always show all controls instead of only displaying ones within client capabilities, defaults to false (for backwards compatibility)
Misc improvements
- Improved refresh speed by querying sessions first, devices second
- Improved state accuracy by forcing idle status for old/invalid sessions
- Improved error avoidance by checking if object’s are not none and if they are callable
- Fixed bug where TV season would display even though device has gone idle
- Removed unnecessary code:
  - Removed “return STATE_UNKNON” code as it was unreachable
  - Removed “import Show” references as we already know the media type

houndci-bot · 2017-02-19T03:17:26Z

homeassistant/components/media_player/plex.py

+            # with a loopback address, the device must be local or casting
+            for client in self.device.server.clients():
+                if "127.0.0.1" in client.baseurl:
+                    if client.machineIdentifier == self.device.machineIdentifier:


line too long (81 > 79 characters)

This is more readable on one line but I'll change it if you need me to.

Please do, as we have strict requirements on PEP8 compliance.

ok, so I'm not sure how to fix this as I got an auto message each way I went

houndci-bot · 2017-02-19T03:26:41Z

homeassistant/components/media_player/plex.py

+            for client in self.device.server.clients():
+                if "127.0.0.1" in client.baseurl:
+                    if client.machineIdentifier == \
+                        self.device.machineIdentifier:


continuation line with same indent as next logical line

Boo, will fix (hopefully)

houndci-bot · 2017-02-19T03:29:20Z

homeassistant/components/media_player/plex.py

+            for client in self.device.server.clients():
+                if "127.0.0.1" in client.baseurl:
+                    if client.machineIdentifier == \
+                    self.device.machineIdentifier:


continuation line missing indentation or outdented

robbiet480 · 2017-02-19T03:42:12Z

@JesseWebDotCom You can use pylint and flake8 locally before pushing changes to ensure you are PEP8 compliant.

JesseWebDotCom · 2017-02-19T03:53:32Z

Thanks. Already use pylint in Atom.io combined with Atom beautify using yapf. It usually fixes everything automatically. Just added flake8 for good measure.

JesseWebDotCom · 2017-02-19T19:25:25Z

Never mind, I see how to update the documentation. I'll start doing that now and submit for review.

fabaff · 2017-02-20T07:14:11Z

Docs: home-assistant/home-assistant.io#2094

houndci-bot · 2017-02-26T02:40:17Z

homeassistant/components/media_player/plex.py

+>>>>>>> 4201005ca8ff3f7b584e5cbb9f3464f65c8ce2a9
+
+            # set groups with updated memberships
+            set_group_members(hass, GROUP_ACTIVE_DEVICES,


unexpected indentation

Ignore, I pushed a new update (fix3) that works and doesn't include the junk text above

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+                            active_entity_id_list.append(client.entity_id)
+>>>>>>> 4201005ca8ff3f7b584e5cbb9f3464f65c8ce2a9
+
+            # set groups with updated memberships


unexpected indentation (comment)

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+                            inactive_entity_id_list.append(client.entity_id)
+                        else:
+                            active_entity_id_list.append(client.entity_id)
+>>>>>>> 4201005ca8ff3f7b584e5cbb9f3464f65c8ce2a9


missing whitespace around operator

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+                            else:
+                                active_entity_id_list.append(client.entity_id)
+=======
+                    if not client.hidden:


unexpected indentation

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+                                    client.entity_id)
+                            else:
+                                active_entity_id_list.append(client.entity_id)
+=======


missing whitespace around operator

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+                if client.entity_id:
+<<<<<<< HEAD
+                    # do not add if hidden
+                    client_states = hass.states.get(client.entity_id)


unexpected indentation

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+            for machine_identifier, client in plex_clients.items():
+                if client.entity_id:
+<<<<<<< HEAD
+                    # do not add if hidden


unexpected indentation (comment)

houndci-bot · 2017-02-26T02:40:18Z

homeassistant/components/media_player/plex.py

+
+            for machine_identifier, client in plex_clients.items():
+                if client.entity_id:
+<<<<<<< HEAD


expected 2 blank lines after class or function definition, found 0
expected an indented block
IndentationError: expected an indented block
missing whitespace around bitwise or shift operator
missing whitespace around operator

JesseWebDotCom · 2017-02-26T18:08:15Z

Any word on when this and the documentation (home-assistant/home-assistant.io#2094) will get officially incorporated?

arsaboo · 2017-02-26T18:14:45Z

homeassistant/components/media_player/plex.py

+        if self.session and self.session.player:
+            if ((frozen_seconds >
+                 int(self.optional_config[CONF_MAX_FROZEN_PAUSED])) and
+                (self.session.player.state == 'paused' or


Need one space here.

arsaboo · 2017-02-26T18:16:07Z

@JesseWebDotCom Looks like the build is failing.....there are some simple errors that can be fixed.

arsaboo · 2017-02-26T18:17:00Z

homeassistant/components/media_player/plex.py


+def set_group_members(hass, group_entity_id, member_entity_id_list):
+    """Creates group if doesn't exist and sets memberships"""


Needs to end in a period
"""Create group if doesn't exist and set memberships."""

arsaboo · 2017-02-26T18:17:58Z

homeassistant/components/media_player/plex.py

+                        self.name.lower().replace('-', '_'))
+
+    def refresh(self, device, session):
+        """Refresh key device data"""


"""Refresh key device data."""

arsaboo · 2017-02-26T18:18:43Z

homeassistant/components/media_player/plex.py

+                self._make = make
+
+    def force_idle(self):
+        """Force client to idle"""


"""Force client to idle."""

arsaboo · 2017-02-26T18:19:18Z

homeassistant/components/media_player/plex.py

+
+    @property
+    def app_name(self):
+        """Library name of playing media"""


"""Return the library name of playing media."""

Teagan42 · 2017-03-15T22:34:14Z

I'm still getting a session based client and a regular client from the server.clients.

JesseWebDotCom · 2017-03-15T22:35:19Z

@Teagan42 - what's your recommendation? Also, how can I replicate this?

Teagan42 · 2017-03-15T22:36:12Z

homeassistant/components/media_player/plex.py

+                        SUPPORT_TURN_OFF | SUPPORT_VOLUME_MUTE)
+
+        # only show controls when we know what device is connecting
+        if not self._make:


Should this be an elif? It's overwriting the two ifs before this

good catch, those are missing return statements - will add

Teagan42 · 2017-03-15T22:41:19Z

homeassistant/components/media_player/plex.py

+        if config.get(CONF_INCLUDE_NON_CLIENTS):
+            for machine_identifier, session in plex_sessions.items():
+                if (machine_identifier not in plex_clients
+                        and machine_identifier is not None):


This should be to prevent dupes:

and session.player.machineIdentifier is not None):

I'll add, test, and check in (for this one spot) - thanks

Yup this fixes dupe issue

Ok so this is odd - at first load, with this change, I don't get dupes, but after it sits for a bit, I get dupes... let me investigate more...

Teagan42 · 2017-03-15T23:17:04Z

@JesseWebDotCom I'm not sure what is causing duplicate clients, I'm still digging.

JesseWebDotCom · 2017-03-15T23:20:18Z

@Teagan42 - Standing by

Teagan42 · 2017-03-15T23:23:14Z

So, I added the machine identifier to the state attributes - when it first loads, only the first two show up (yay), but after another pass, the second two show up (empty machine identifiers)

media_player.my_afts	idle	machineIdentifer: ead79392dc1a30ac-com-plexapp-android
app_name: 
friendly_name: My AFTS
media_player.my_afts_2	idle	machineIdentifer: 48d04c6c0bf62ed0-com-plexapp-android
app_name: 
friendly_name: My AFTS
media_player.my_afts_3	idle	supported_features: 20533
friendly_name: My AFTS
media_player.my_afts_4	idle	supported_features: 20533
friendly_name: My AFTS

So, some how, we're getting empty machine identifiers into the plex clients. Not sure where, again still digging - Not sure how no one else is seeing this issue...

Teagan42 · 2017-03-15T23:24:14Z

Even more interesting, realized I don't have the include non clients flag set in my config...

Teagan42 · 2017-03-15T23:29:11Z

@JesseWebDotCom if you got time in about an hour we can hop on a video call and I can show you what I'm seeing.

JesseWebDotCom · 2017-03-15T23:57:56Z

Unfortunately I'm at a family event. Perhaps you can send me a clip. Also, what's your setup (I think I have every plex client in the world - maybe I can reproduce the issue)?

Teagan42 · 2017-03-16T00:05:02Z

I am having issues with my Amazon Fire TVs (haven't even checked the two sticks I have up stairs).

On first pass, the AFTVs are added (as expected), but second pass (the refresh) it seems to duplicate them as non-devices.

When I turn on the INCLUDE_NON_CLIENT config, those (non-client) work just fine, no problems.

Teagan42 · 2017-03-16T00:07:27Z

~~TV (non-client) works, Chrome (client) works, AFTVs - duplicated... there's something I am missing somewhere...~~

Scratch that:

TV (Non-Client) Only shows up once ✅
Chrome (Client) Shows up duplicated 🔴
Amazon Fire TV (Client) Shows up duplicated 🔴

Here's an image (I have 2 Amazon Fire TVs - so you'll see both duplicated):

Teagan42 · 2017-03-16T00:20:49Z

Fire Sticks have same issue as the TVs

Teagan42 · 2017-03-16T00:24:49Z

Top to bottom:
Amazon Fire TV A (Duplicated - no machine identifier)
Amazon Fire TV B (Duplicated - no machine identifier)
Amazon Fire TV A (Machine Identifier)
Amazon Fire TV B (Machine Identifier)
Android Phone (Machine Identifier)
Chrome (Machine Identifier)
Chrome (Duplicated - no machine identifier)
Amazon Fire Stick A (Machine identifier)
Amazon Fire Stick A (Duplicated - no machine identifier)
Samsung TV A (Non-client, machine identifier)
Samsung TV B (Non-client, machine identifier)

Teagan42 · 2017-03-16T00:27:10Z

I have other things to work on tonight - if you can dig in, let me know otherwise I'll try to look tomorrow

JesseWebDotCom · 2017-03-16T00:58:25Z

When you see the duplicate devices, go to http://IP:PORT/status/sessions (ip = plex server) and post the results (make sure it doesn't include any sensitive information). My suspicion is you are either getting blank machine identifiers or duplicate identifiers. If so, that's a big problem as thats the only way to uniquely identify a player (other than a hack like using the client ip).

JesseWebDotCom · 2017-03-16T02:14:39Z

Also, what do you have for a plex server? Are you casting in any of these scenarios? And what is the compared behavior in the existing media_player.plex component?

It looks good

balloob · 2017-03-16T05:04:36Z

So the code looks good. This can be merged once the bugs found by @Teagan42 fixed or avoided.

Teagan42 · 2017-03-16T12:36:12Z

Bugs were all on my setup, my bad. All is good.

…

On Mar 15, 2017 11:04 PM, "Paulus Schoutsen" ***@***.***> wrote: So the code looks good. This can be merged once the bugs found by @Teagan42 <https://github.com/Teagan42> fixed or avoided. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6054 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC2fZV20QCbxBLwkEnGni3Km81yYj6Ykks5rmMLqgaJpZM4MD1_B> .

balloob · 2017-03-16T16:09:40Z

Thanks @JesseWebDotCom for the work and @Teagan42 for testing! 🎉 💃 🐬 🍺

JesseWebDotCom · 2017-03-16T23:07:29Z

Awesome, thanks for teaching

robbiet480 · 2017-03-16T23:09:36Z

@JesseWebDotCom Thanks for hanging on! We thought that this was such a great pull request and show of effort it was deserving of its own tweet! Cherish it, we don't hand those out lightly 😳

JesseWebDotCom · 2017-03-16T23:26:33Z

Thanks. Hey, I believe I have one more change to make which is a fix to the new show all controls option (it won't work as is). Can I still submit that as a new commit here or is it too late? I just tried to submit and it doesn't look like it worked:
https://github.com/JesseWebDotCom/home-assistant/commit/3bb8fc080d47904b2f8da4f299532b86c91066e1

Also, how can I tell if my associated documentation pull request is approved and ready to be released at the same time as this component?

homeassistant added platform: media_player.plex cla-needed labels Feb 17, 2017

houndci-bot reviewed Feb 17, 2017

View reviewed changes

homeassistant added cla-needed cla-signed and removed cla-needed labels Feb 17, 2017

homeassistant added the cla-signed label Feb 18, 2017

houndci-bot reviewed Feb 19, 2017

View reviewed changes

JesseWebDotCom mentioned this pull request Feb 20, 2017

Plex improvements home-assistant/home-assistant.io#2094

Merged

homeassistant added the cla-signed label Feb 20, 2017

houndci-bot reviewed Feb 26, 2017

View reviewed changes

arsaboo reviewed Feb 26, 2017

View reviewed changes

Teagan42 suggested changes Mar 15, 2017

View reviewed changes

balloob approved these changes Mar 16, 2017

View reviewed changes

balloob merged commit 5714f15 into home-assistant:dev Mar 16, 2017

JesseWebDotCom mentioned this pull request Mar 18, 2017

Fixed Show All Controls feature #6672

Merged

balloob mentioned this pull request Mar 24, 2017

0.41 #6756

Merged

home-assistant locked and limited conversation to collaborators Jul 17, 2017

ghost added integration: plex and removed platform: media_player.plex labels Mar 21, 2019


		def set_group_members(hass, group_entity_id, member_entity_id_list):
		"""Creates group if doesn't exist and sets memberships"""

Support for non-clients, NVidia shield, dynamic grouping, extra metad… #6054

Support for non-clients, NVidia shield, dynamic grouping, extra metad… #6054

Conversation

JesseWebDotCom commented Feb 17, 2017

homeassistant commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseWebDotCom commented Feb 17, 2017

robbiet480 commented Feb 18, 2017

JesseWebDotCom commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbiet480 commented Feb 19, 2017

JesseWebDotCom commented Feb 19, 2017

JesseWebDotCom commented Feb 19, 2017 • edited Loading

fabaff commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseWebDotCom commented Feb 26, 2017

Choose a reason for hiding this comment

arsaboo commented Feb 26, 2017

arsaboo Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teagan42 commented Mar 15, 2017

JesseWebDotCom commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teagan42 commented Mar 15, 2017

JesseWebDotCom commented Mar 15, 2017

Teagan42 commented Mar 15, 2017

Teagan42 commented Mar 15, 2017

Teagan42 commented Mar 15, 2017

JesseWebDotCom commented Mar 15, 2017

Teagan42 commented Mar 16, 2017 • edited Loading

Teagan42 commented Mar 16, 2017 • edited Loading

Teagan42 commented Mar 16, 2017

Teagan42 commented Mar 16, 2017

Teagan42 commented Mar 16, 2017

JesseWebDotCom commented Mar 16, 2017 • edited Loading

JesseWebDotCom commented Mar 16, 2017 • edited Loading

balloob commented Mar 16, 2017

Teagan42 commented Mar 16, 2017 via email

balloob commented Mar 16, 2017

JesseWebDotCom commented Mar 16, 2017

robbiet480 commented Mar 16, 2017 • edited Loading

JesseWebDotCom commented Mar 16, 2017 • edited Loading

JesseWebDotCom commented Feb 19, 2017 •

edited

Loading

arsaboo Feb 26, 2017 •

edited

Loading

JesseWebDotCom commented Mar 15, 2017 •

edited

Loading

Teagan42 commented Mar 16, 2017 •

edited

Loading

Teagan42 commented Mar 16, 2017 •

edited

Loading

JesseWebDotCom commented Mar 16, 2017 •

edited

Loading

JesseWebDotCom commented Mar 16, 2017 •

edited

Loading

robbiet480 commented Mar 16, 2017 •

edited

Loading

JesseWebDotCom commented Mar 16, 2017 •

edited

Loading