Skip to content

Allow filtering of sources for Android TV#30994

Merged
pnbruckner merged 3 commits into
home-assistant:devfrom
JeffLIrion:androidtv-filter-sources
Jan 29, 2020
Merged

Allow filtering of sources for Android TV#30994
pnbruckner merged 3 commits into
home-assistant:devfrom
JeffLIrion:androidtv-filter-sources

Conversation

@JeffLIrion
Copy link
Copy Markdown
Contributor

@JeffLIrion JeffLIrion commented Jan 20, 2020

Description:

If you don't want an app to show up in your sources list, you can leave its friendly name empty or give it an empty string as its friendly name in the apps configuration entry. You can also set exclude_unnamed_apps: true so that all apps that do not have a friendly name provided by you or provided by default in the backend library will not be shown in the sources list.

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

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: androidtv
    host: 192.168.0.111
    apps:
      com.amazon.tv.launcher: "Fire TV"
      some.background.app:   # this will never show up in the sources list
      another.background_app:  # this will also never show up in the sources list

  - platform: androidtv
    host: 192.168.0.222
    exclude_unnamed_apps: true
    apps:
      com.amazon.tv.launcher: "Fire TV"

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 does not interact with devices:

  • Tests have been added to verify that the new code works.

@raman325
Copy link
Copy Markdown
Contributor

raman325 commented Jan 20, 2020

@JeffLIrion instead of blacklisting apps you don't want to see, it might be nicer/easier or at a minimum a nice alternative to be able to whitelist apps you do want to see. Just food for thought, happened to see this PR while checking on the status of another one. Thanks for adding this btw, makes support to launch an app much more useful!

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

@JeffLIrion instead of blacklisting apps you don't want to see, it might be nicer/easier or at a minimum a nice alternative to be able to whitelist apps you do want to see. Just food for thought, happened to see this PR while checking on the status of another one. Thanks for adding this btw, makes support to launch an app much more useful!

This was just a quick feature enhancement. If someone has a better way to allow for filtering sources, that would be welcome. My 2nd generation Fire TV stick never has more than a few apps open at a time, and I rarely view the sources list in Lovelace, so including all the running apps has never been an issue for me.

@raman325
Copy link
Copy Markdown
Contributor

I have an idea on how this could be implemented. Once this is approved I can put up a proposal PR for you to review

Comment thread homeassistant/components/androidtv/media_player.py
Comment thread tests/components/androidtv/test_media_player.py
Comment thread tests/components/androidtv/test_media_player.py
Copy link
Copy Markdown
Contributor

@raman325 raman325 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 to me once a decision is made on my comments

@dshokouhi
Copy link
Copy Markdown
Member

Just wanted to comment based on what I have seen other integrations such as the recorder or history do when it comes to filtering. They usually do one of 2 things an include or exclude list. If a user chooses exclude then everything but that list gets included. If user chooses include then ONLY those get filtered in and nothing else. I think that is the best of both worlds because we have 4 cases. You have lots of apps and only want to include a few, you have lots of apps and want to include most, you want to include all apps, you don't want to include any apps.

@raman325
Copy link
Copy Markdown
Contributor

The nice thing about @dshokouhi 's proposal is that it would handle the whitelist use case I had mentioned earlier

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

Just wanted to comment based on what I have seen other integrations such as the recorder or history do when it comes to filtering. They usually do one of 2 things an include or exclude list. If a user chooses exclude then everything but that list gets included. If user chooses include then ONLY those get filtered in and nothing else. I think that is the best of both worlds because we have 4 cases. You have lots of apps and only want to include a few, you have lots of apps and want to include most, you want to include all apps, you don't want to include any apps.

Omitting some config stuff:

# 1. Include all apps
apps:
  first.app: "First app"
  second.app: "Second app"

# 2. Don't include any apps
get_sources: false  # I think this would accomplish that because the only
                    # app in the sources list would be the current app

# 3. You have lots of apps and only want to include a few
exclude_unnamed_apps: true  # default value is false to preserve current behavior
apps:
  first.app: "First app"
  second.app: "Second app"

# 4. You have lots of apps and want to include most
apps:
  first.app: "First app"
  second.app: "Second app"
  third.app:  # will never be shown
  fourth.app: ""  # will never be shown

I think this solution of adding a config parameter exclude_unnamed_apps with a default value of false would accomplish this without over-complicating the configuration. Thoughts?

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

Follow-up

This commit implements the approach described in my previous post: JeffLIrion@0842b22

If you guys think that’s a good solution, I’ll merge it into the pull request branch.

@dshokouhi
Copy link
Copy Markdown
Member

Just wanted to comment based on what I have seen other integrations such as the recorder or history do when it comes to filtering. They usually do one of 2 things an include or exclude list. If a user chooses exclude then everything but that list gets included. If user chooses include then ONLY those get filtered in and nothing else. I think that is the best of both worlds because we have 4 cases. You have lots of apps and only want to include a few, you have lots of apps and want to include most, you want to include all apps, you don't want to include any apps.

Omitting some config stuff:

# 1. Include all apps
apps:
  first.app: "First app"
  second.app: "Second app"

# 2. Don't include any apps
get_sources: false  # I think this would accomplish that because the only
                    # app in the sources list would be the current app

# 3. You have lots of apps and only want to include a few
exclude_unnamed_apps: true  # default value is false to preserve current behavior
apps:
  first.app: "First app"
  second.app: "Second app"

# 4. You have lots of apps and want to include most
apps:
  first.app: "First app"
  second.app: "Second app"
  third.app:  # will never be shown
  fourth.app: ""  # will never be shown

I think this solution of adding a config parameter exclude_unnamed_apps with a default value of false would accomplish this without over-complicating the configuration. Thoughts?

I think this helps simplify things and allows users to decide how best to setup their apps depending on their use cases. The default value you chosen is also good to keep existing behavior.

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

I merged in that change and the checks passed. @dshokouhi would you mind reviewing this pull request?

Copy link
Copy Markdown
Member

@dshokouhi dshokouhi 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 to me

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

@dshokouhi and @raman325 thanks for reviewing! Now I just need someone with merge rights to review it👍

@pnbruckner
Copy link
Copy Markdown
Contributor

Well, I haven't merged other people's PRs yet, but this one looks pretty straight forward, and there are already two approvals, so I'll go ahead and merge it. If I'm going to help out more I guess I've got to jump in somewhere! 😃

@pnbruckner pnbruckner merged commit 31dc2ad into home-assistant:dev Jan 29, 2020
@JeffLIrion
Copy link
Copy Markdown
Contributor Author

Thanks @pnbruckner!

@lock lock Bot locked and limited conversation to collaborators Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants