Skip to content

Make plex media players update position on state change#43759

Closed
shocklateboy92 wants to merge 1 commit intohome-assistant:devfrom
shocklateboy92:fixes/plex-position-paused
Closed

Make plex media players update position on state change#43759
shocklateboy92 wants to merge 1 commit intohome-assistant:devfrom
shocklateboy92:fixes/plex-position-paused

Conversation

@shocklateboy92
Copy link
Copy Markdown
Contributor

@shocklateboy92 shocklateboy92 commented Nov 30, 2020

Proposed change

Currently, media players for plex do throttled updates for the media_position attribute.
This means that when a player is paused, it displays the incorrect position and there's no way to work out what the actual position is.

This change changes the throttle logic slightly so that if the player state changed (e.g paused to playing, or vice-versa), it updates the position anyway.

This way, frontend components can always work out the current exact media position with the following logic:

  • if player state is paused, then the current value is correct
  • if player state is playing, then current position is media_position + time delta between now and media_position_updated_at.

This will reduce the throttling slightly, and I hope that's okay. If not, please let me know and we can discuss alternate ways of allowing frontend components to determine the exact position of a plex media player.
Also happy to make any improvements to the proposed code change; just let me know 😄

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @jjlawren, mind taking a look at this pull request as its been labeled with an integration (plex) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@jjlawren
Copy link
Copy Markdown
Contributor

Thanks, this is definitely needed. I've addressed this specific gap as part of #42332. Since it seems like you're starting to dig through the code, a sanity check on that PR would be greatly appreciated if you're up for it.

Copy link
Copy Markdown
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

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

Looks good

@shocklateboy92
Copy link
Copy Markdown
Contributor Author

@jjlawren Thanks. I've looked through the PR you mentioned and it looks like it will address my needs.
Do you reckon that'll get merged soon? If so, I'd rather just close this PR and reduce the amount of merge conflicts that have to be dealt with overall 😁

However, if you reckon that won't make it to the next release of HA then would prefer that this get merged 😊

@jjlawren
Copy link
Copy Markdown
Contributor

jjlawren commented Dec 3, 2020

The other PR has been merged and is live in the beta. Ok to close this?

@github-actions github-actions Bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants