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

Added support for Notifications for Android TV / FireTV #3978

Merged
merged 6 commits into from Oct 25, 2016
Merged

Added support for Notifications for Android TV / FireTV #3978

merged 6 commits into from Oct 25, 2016

Conversation

danielperna84
Copy link
Contributor

@danielperna84 danielperna84 commented Oct 21, 2016

Description:
Notification service for Notifications for Android TV and Notifications for FireTV.
The app usually consists of the server-part running on a compatible Android TV or FireTV device + a client-app running on an Android smartphone, which publishes its notifications (WhatsApp, incoming calls etc.) to the TV.
With this service we don't need the client running on the smartphone and instead can push our HASS notifications to the TV. The advantage compared to e.g. Kodi notifications is, that these notifications are global in the context of the Android TV device. So they show up even while watching satellite TV or YouTube.
There are some styling-options available. I've decided to allow a default-configuration via configuration.yaml, but also overriding them with the data-attribute of notifications. The parameters (color, transparency etc.) seem to be more "flexible" at first sight, but are actually limited by the server. Only the values within the platform-code work. So we can't set the color to #ff0000 for red, but have to use #4caf50. All those possible values can be seen in the code, and in the end it might even be more user friendly to call the service with "color":"red" instead of "color":"#ff0000". Just want to clarify this so reviewers are aware why it's done this way.
I'll take care of the documentation soon. If you wan't to try it out, here's a fully filled service-data-dict:

{
"message": "Messagetext",
"title": "My Notification",
"data":{
"position":"center",
"duration":2,
"transparency":"0%",
"color": "red",
"interrupt": 0
}
}

Most parameters explain themselves. The interrupt one displays a different kind of notification (looks almost the same, but has a border). When set to true (or on, 1, it's cv.boolean), the notification can be dismissed or selected and viewed. According to the App-description this might even stop playback, although that didn't happen to me with YouTube and Netflix.

One further note: the app description refers to premium features for unlimited messages etc. Those limitations only apply to the smartphone app. So when you're just using it for HASS notifications no premium features are required.

I really dislike me disabeling the too-many-branches check for what I do beginning at line 147, but I'm unsure how I should handle the passed in data more elegantly. Suggestions welcome!

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#1296

Example entry for configuration.yaml (if applicable):

notify:
- platform: nfandroidtv
  name: Kitchen
  ip: 192.168.1.12
  color: grey
  transparency: 25%
  position: top-left

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

interrupt = config.get(CONF_INTERRUPT)
timeout = config.get(CONF_TIMEOUT)

return NFAndroidTVNotificationService(remoteip,
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible that you check if the config is correct before returning the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, I'm not sure what you mean by that. The PLATFORM_SCHEMA handles and validates all possible options and sets defaults if not present. Wouldn't additional validation in get_service make the PLATFORM_SCHEMA obsolete? For example: the color option has to be one of the keys in the COLORS-dict. Currently I am under the assuption, that if that fails get_service won't even be called.

Copy link
Member

Choose a reason for hiding this comment

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

That is a correct assumption. I meant that you check if the given IP actually contains a Notification for Android TV service

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 could check if TCP connections to the host are possible. But that would lead to the drawback, that all TVs would need to be turned on when restarting HASS.
This would actually be a platform where notification delivery can't be guaranteed because the device (in case of a TV with integrated Android TV) can be turned off. In that case the warning caused by the exception would be triggered.

@balloob
Copy link
Member

balloob commented Oct 22, 2016

Code looks all good. One minor comment

@danielperna84
Copy link
Contributor Author

danielperna84 commented Oct 23, 2016

Since this didn't end up in 0.31, should I have a look at making the code use aiohttp instead of requests? Or is async in an too early stage to port the platform?
The App running on the Android device doesn't play well with async requests, so I'll leave this as it is.

@balloob
Copy link
Member

balloob commented Oct 25, 2016

Notify component does not work with async yet, so better stick with requests for now.

@balloob balloob merged commit 1707cdf into home-assistant:dev Oct 25, 2016
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
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.

None yet

3 participants