Skip to content

Add Android TV screen capture option and use library screencap #34074

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 14, 2020
Merged

Add Android TV screen capture option and use library screencap #34074

merged 4 commits into from
Apr 14, 2020

Conversation

i00
Copy link
Contributor

@i00 i00 commented Apr 12, 2020 •
edited
Loading

Breaking change

None

Proposed change

Add attribute option to turn off screen captures for Android TV; for devices that have permanent HDCP (hdmi protection) or slow screen updates.

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

Example entry for configuration.yaml:

# Example configuration.yaml
media_player:
  - platform: androidtv
    #Existing options here
    screencap: false

Additional information

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.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant probot-home-assistant bot added integration: androidtv small-pr PRs with less than 30 lines. labels Apr 12, 2020
@probot-home-assistant
Copy link

Hey there @JeffLIrion, mind taking a look at this pull request as its been labeled with a integration (androidtv) you are listed as a codeowner for? Thanks!

JeffLIrion
JeffLIrion reviewed Apr 12, 2020
View reviewed changes
@JeffLIrion
Copy link
Contributor

You need to bump the requirements in requirements_test_all.txt (I think that's the right filename).

@i00 i00 mentioned this pull request Apr 12, 2020
Merged
8 tasks
@JeffLIrion
Copy link
Contributor

You should make the title more descriptive.

Also, you'll need a test to pass the coverage check. You can use this: a0e145f. You might need more.

@i00 i00 changed the title Android TV Android TV add screen capture option and implement new screencap in androidtv 0.40 Apr 12, 2020
@i00
Copy link
Contributor Author

i00 commented Apr 12, 2020

@JeffLIrion ... sorry can you explain why the tests are failing ... even without code coverage; I believe it should have passed ... correct me if I am wrong; but it appears that the issues relate to other things that are already in the dev branch?

@JeffLIrion
Copy link
Contributor

@JeffLIrion ... sorry can you explain why the tests are failing ... even without code coverage; I believe it should have passed ... correct me if I am wrong; but it appears that the issues relate to other things that are already in the dev branch?

Bad luck😉

Either push another commit or close and re-open the pull request.

@i00
Copy link
Contributor Author

i00 commented Apr 12, 2020 •
edited
Loading

Also, you'll need a test to pass the coverage check. You can use this: a0e145f. You might need more.

Also (this may seem a little dumb ... but) where is that commit located ... when i fetch the upstream then run
git branch --contains a0e145f
it says the commit doesn't eixst.

@JeffLIrion
Copy link
Contributor

Also, you'll need a test to pass the coverage check. You can use this: a0e145f. You might need more.

Also (this may seem a little dumb ... but) where is that commit located ... when i fetch the upstream then run
git branch --contains a0e145f
it says the commit doesn't eixst.

It's on my fork. You could either add my fork as a git remote, or you could just copy & paste the whole file.

https://github.com/JeffLIrion/home-assistant/blob/patch-37/tests/components/androidtv/test_media_player.py

@MartinHjelmare MartinHjelmare changed the title Android TV add screen capture option and implement new screencap in androidtv 0.40 Add Android TV screen capture option and use library screencap Apr 12, 2020
@JeffLIrion JeffLIrion mentioned this pull request Apr 12, 2020
Closed
20 tasks
springstan
springstan approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

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

LGTM ✌

@springstan springstan merged commit 994e838 into home-assistant:dev Apr 14, 2020
@frenck frenck mentioned this pull request Apr 15, 2020
Closed
@JeffLIrion JeffLIrion mentioned this pull request Apr 15, 2020
Merged
8 tasks
@lock lock bot locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JeffLIrion JeffLIrion JeffLIrion approved these changes

@springstan springstan springstan approved these changes

Assignees
No one assigned
Labels
cla-signed dependency integration: androidtv small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@i00 @JeffLIrion @springstan @homeassistant