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

FEAT: Ability to show video in notification as a gif #2342

Merged
merged 5 commits into from Mar 8, 2022

Conversation

NickM-27
Copy link
Contributor

@NickM-27 NickM-27 commented Mar 5, 2022

Summary

I have added the ability for a user to define video as part of an Android notification. This video will be downloaded and home assistant will attempt to parse frames from this video to show in a notification. The goal is to find 5 frames but it will work for any number of frames < 5 as well.

Screenshots

device-2022-03-04-173416.mp4

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#705

@dshokouhi
Copy link
Member

This PR needs 1 additional PR to add the parameter to firebase as well, just need to add video here since its a new field

https://github.com/home-assistant/mobile-apps-fcm-push/blob/master/functions/android.js#L43-L52

@dshokouhi
Copy link
Member

This PR made me think about #1270 I wonder if later on we can leverage the code here to fix it?

@NickM-27
Copy link
Contributor Author

NickM-27 commented Mar 5, 2022

This PR made me think about #1270 I wonder if later on we can leverage the code here to fix it?

yeah, sounds like there is a first party fix that works for API 31 but perhaps the MediaMetaDataRetriever can parse frames from gifs as well. I wouldn't mind investigating as part of this PR but perhaps that would belong on its own PR.

@dshokouhi
Copy link
Member

I wouldn't mind investigating as part of this PR but perhaps that would belong on its own PR.

lets do a separate PR for that to keep this one as small as possible :)

@NickM-27
Copy link
Contributor Author

NickM-27 commented Mar 5, 2022

@dshokouhi Let me know what you think of the docs changes. One thing still in my head that I think may be worth adding in the notes is that videos less than 10 seconds long probably won't work very well since it may not find enough frames to show.

Also fcm PR: home-assistant/mobile-apps-fcm-push#705

@dshokouhi
Copy link
Member

I just left a comment on the docs PR :)

One thing still in my head that I think may be worth adding in the notes is that videos less than 10 seconds long probably won't work very well since it may not find enough frames to show

I agree I think thats worth mentioning as a note specific to android as I dont think iOS has to worry about that. The only thing here that we want to make sure is that the service call can be the same for iOS and android :) Functionality doesn't need to match just that a user can put the 2 devices in a group and send the notification there.

Comment on lines +1204 to +1210
data[TITLE]?.let { rawTitle ->
remoteViewFlipper.setTextViewText(R.id.title, rawTitle)
}

data[MESSAGE]?.let { rawMessage ->
remoteViewFlipper.setTextViewText(R.id.info, rawMessage)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't the handleText function handle this?

Copy link
Contributor Author

@NickM-27 NickM-27 Mar 5, 2022

Choose a reason for hiding this comment

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

No, when you set the style as DecoratedCustomViewStyle you actually control the entire notification viewport so the title and text part from the builder is overwritten / not included and the custom view needs to include it itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify on that last point, the docs here: https://developer.android.com/reference/android/app/Notification.DecoratedCustomViewStyle show how you can override specific parts of the notification. In this case I am overriding the bigContentView so the heads up view and the standard content view will use the handleText part but when the notification is expanded it needs to be handled by the custom view here.

@NickM-27 NickM-27 requested a review from JBassett March 5, 2022 19:29
@NickM-27
Copy link
Contributor Author

NickM-27 commented Mar 7, 2022

@dshokouhi Is there anything else needed for this PR? Just curious what the timeline may be 😄

@dshokouhi
Copy link
Member

Right now this PR is just waiting for a more official code review, depends on how much time the reviewer has. Usually 1-2 weeks for a PR to be reviewed but could be longer, maybe faster if its fixing a critical bug :) There is a low PR count so its on the radar

@NickM-27
Copy link
Contributor Author

NickM-27 commented Mar 7, 2022

Right now this PR is just waiting for a more official code review, depends on how much time the reviewer has. Usually 1-2 weeks for a PR to be reviewed but could be longer, maybe faster if its fixing a critical bug :) There is a low PR count so its on the radar

Thanks! Sounds good. By the way I looked into use the MediaMetaDataRetriever to parse gifs but unfortunately I don't believe that format is supported :/

I'll have to look and see if there are any other approaches that are lightweight. (I know Glide can do it but that's a big library for that problem alone)

@JBassett JBassett merged commit 508893f into home-assistant:master Mar 8, 2022
@conorlap
Copy link

conorlap commented Mar 8, 2022

I can't seem to get this working. I've installed beta-2150-261ae31f but no image/gif is appearing. Here's how I'm calling the notification:

service: notify.mobile_app_s20
data:
  message: Test
  data:
    video: "https://download.samplelib.com/mp4/sample-20s.mp4"

Am I doing something wrong?

@dshokouhi
Copy link
Member

I can't seem to get this working. I've installed beta-2150-261ae31f but no image/gif is appearing. Here's how I'm calling the notification:

service: notify.mobile_app_s20
data:
  message: Test
  data:
    video: "https://download.samplelib.com/mp4/sample-20s.mp4"

Am I doing something wrong?

Please do not comment on merged PRs, instead create a new issue. Also I think videos need to be more than 10 seconds long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants