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

Bump androidtv to 0.0.21; add 'state_detection_rules' config parameter #25647

Merged

Conversation

@JeffLIrion
Copy link
Contributor

commented Aug 2, 2019

Description:

Bump androidtv to 0.0.20 and add the state_detection_rules config parameter.

I have tested this code and it works. A tester on the forum also reported that it works. In addition, I added unit tests to the androidtv package to make sure that these user-specified state detection rules work as expected (JeffLIrion/python-androidtv#55).

I'd really like to get this merged in because there are plenty of posts on the forum about "Android TV reports the wrong state for xyz app," but only a few users have submitted pull requests to the androidtv repo to help address this. The logic for determining the state differs from app to app and from one device to the next, so a universal solution is unrealistic. This will allow users to fine tune the state detection logic on their own, without the need for an update to the androidtv package followed by a version bump in the HA integration.

Related issue (if applicable): #23346

It fixes this issue by allowing the user to specify their own rules for determining the state.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10036

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: androidtv
    name: Fire TV (Bedroom)
    device_class: firetv
    host: 192.168.0.10
    state_detection_rules:
      'com.amazon.tv.launcher':
        - 'standby'
      'com.netflix.ninja':
        - 'media_session_state'
      'com.ellation.vrv':
        - 'audio_state'
      'com.plexapp.android':
        - 'playing':
            'media_session_state': 3  # this indentation is important!
            'wake_lock_size': 3       # this indentation is important!
        - 'paused':
            'media_session_state': 3  # this indentation is important!
            'wake_lock_size': 1       # this indentation is important!
        - 'standby'

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.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.

@JeffLIrion JeffLIrion changed the title Bump androidtv to 0.0.19; add 'state_detection_rules' config parameter Bump androidtv to 0.0.20; add 'state_detection_rules' config parameter Aug 4, 2019

JeffLIrion added 3 commits Aug 5, 2019

@JeffLIrion JeffLIrion changed the title Bump androidtv to 0.0.20; add 'state_detection_rules' config parameter Bump androidtv to 0.0.21; add 'state_detection_rules' config parameter Aug 5, 2019

@@ -99,6 +103,9 @@
vol.Optional(CONF_APPS, default=dict()): vol.Schema({cv.string: cv.string}),
vol.Optional(CONF_TURN_ON_COMMAND): cv.string,
vol.Optional(CONF_TURN_OFF_COMMAND): cv.string,
vol.Optional(CONF_STATE_DETECTION_RULES, default=dict()): vol.Schema(
{cv.string: ha_state_detection_rules_validator(vol.Invalid)}

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Aug 5, 2019

Member

The validator would be more general purpose if it accepted a function instead of an exception. But it's ok like this too.

This comment has been minimized.

Copy link
@JeffLIrion

JeffLIrion Aug 5, 2019

Author Contributor

I wasn’t 100% clear on what you meant by your previous post, but I thought this would fit the bill. It's basically the same function as before. I tested it out and it worked as expected, reporting error messages in the "Check config" box when something wasn't right.

I'd like to leave it as is, since I already tested it and published it on Pypi. But I'm also curious what you meant by your previous post.

Even better would be if we moved the whole validator to the library. We can do that by wrapping it in a factory function, that accepts a callback to be called when the validation fails. The callback should accept the message for the failed validation.

Then the user of the library can create a validator by calling the factory passing the callback. In our case the callback will raise vol.Invalid.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Aug 5, 2019

Member

If we have the validator factory accept a function as callback. We can do this:

def handle_invalid_rule(msg):
    raise vol.Invalid(msg)

...

vol.Optional(CONF_STATE_DETECTION_RULES, default={}): vol.Schema(
    {cv.string: ha_state_detection_rules_validator(handle_invalid_rule)}
...

A function is more flexible since another user of the library might want to do something else than raise an error.

We can keep it like it is.

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

A couple of comments that could improve it.

Dev automation moved this from Needs review to Reviewer approved Aug 5, 2019

dict() -> {}
Co-Authored-By: Martin Hjelmare <marhje52@kth.se>

@MartinHjelmare MartinHjelmare merged commit 0449132 into home-assistant:dev Aug 5, 2019

11 checks passed

CI Build #20190805.29 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 8a57255...607cd90
Details
codecov/project 94.01% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Aug 5, 2019

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Thanks for the review @MartinHjelmare!

@lock lock bot locked and limited conversation to collaborators Aug 6, 2019

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