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

@JeffLIrion JeffLIrion 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 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)}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@MartinHjelmare MartinHjelmare Aug 5, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@MartinHjelmare MartinHjelmare 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!

A couple of comments that could improve it.

Co-Authored-By: Martin Hjelmare <[email protected]>
@MartinHjelmare MartinHjelmare merged commit 0449132 into home-assistant:dev Aug 5, 2019
@JeffLIrion
Copy link
Contributor Author

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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants