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

Fix progress for Plex media_players #22224

Merged
merged 7 commits into from Mar 22, 2019

Conversation

Projects
None yet
6 participants
@jjlawren
Copy link
Contributor

commented Mar 20, 2019

Description:

Adds media_position and media_position_updated_at properties which are necessary for UI media players to properly display progress.

Example entry for configuration.yaml (if applicable):

media_player:
    - platform: plex

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@amelchio
Copy link
Contributor

left a comment

The position is supposed to only update when the playing starts/jumps.

Maybe we should just get home-assistant/architecture#101 implemented 🤔

@jjlawren jjlawren changed the title Fix progress for Plex media_players [WIP] Fix progress for Plex media_players Mar 22, 2019

@jjlawren jjlawren changed the title [WIP] Fix progress for Plex media_players Fix progress for Plex media_players Mar 22, 2019

@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

This seems to work well in my testing with ~5 different device/client types. As the component has a history of exposing edge cases, any additional help testing would be lovely.

Current limitations in this implementation:

  1. Progress is always slightly behind real-time, as a result of the Plex API reporting a slightly delayed timestamp and the use of a polling update method.
  2. The current position reported by the Plex API can be a little "jumpy" depending on the client type, leading to drifts away from the expected progress position. This can update the position attributes more often than absolutely required. This seems to affect web-based clients (and Plex Media Player) the most.

jjlawren and others added some commits Mar 22, 2019

Update homeassistant/components/plex/media_player.py
Co-Authored-By: jjlawren <jjlawren@users.noreply.github.com>

@jjlawren jjlawren force-pushed the jjlawren:plex_show_progress branch from 1a5d189 to c9527a4 Mar 22, 2019

@bachya

bachya approved these changes Mar 22, 2019

Copy link
Contributor

left a comment

Helped @jjlawren fix merge issues. If nothing has changed functionally since @balloob, @robbiet480, or @amelchio's reviews, this can be merged when tests pass.

@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Sorry all, not sure what happened. Confirmed the end result is identical to how it was before the failed rebase.

@bachya bachya merged commit ce55020 into home-assistant:dev Mar 22, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 93.677%
Details

@ghost ghost removed the in progress label Mar 22, 2019

@jjlawren jjlawren deleted the jjlawren:plex_show_progress branch Mar 22, 2019

mxworm added a commit to mxworm/home-assistant that referenced this pull request Mar 27, 2019

Merge branch 'dev' into current
* dev: (99 commits)
  show which component is causing translation errors (home-assistant#22340)
  fix where PLATFORM_SCHEMA gets pulled from (home-assistant#22334)
  Consolidate more platforms (home-assistant#22308)
  Update file header (home-assistant#22318)
  Remove occupancy as it is not available in API (home-assistant#22320)
  Google Assistant: Add camera stream trait (home-assistant#22278)
  Add Freebox switch platform (home-assistant#21710)
  Add homematicip cloud connection quality related attributes (home-assistant#21990)
  Update abbreviation (home-assistant#22317)
  Upgrade py-cpuinfo to 5.0.0 (home-assistant#22287)
  Upgrade pylast to 3.1.0 (home-assistant#22302)
  Fix for embedded MQTT server configuration (home-assistant#22305)
  Switch from using Google Maps API for elevation to Open Elevation API (home-assistant#22306)
  Update srpenergy library (home-assistant#22307)
  Sort code owners alphabetically (home-assistant#22304)
  Update trait to support auto without ranges. (home-assistant#21847)
  Fix Prometheus casting issues (home-assistant#22282)
  Add sort by config and tests for Reddit integration (home-assistant#22081)
  Fix progress for Plex media_players (home-assistant#22224)
  Fixing the api_streams sensor (home-assistant#22200)
  ...

@balloob balloob referenced this pull request Apr 3, 2019

Merged

0.91.0 #22688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.