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

notifications - adds a test if a image-url source contains a GIF file #4326

Draft
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

BitWuehler
Copy link

@BitWuehler BitWuehler commented Apr 6, 2024

Adds a test if a image-url source contains a GIF file

Summary

Today I wanted to show a GIF in a notification an Android 14. I saw that this was added befor two weeks, Sadly this only works with URLs that ends with ".gif". Some URLs don't have the filename in the URL so i added a http header check for that.

Link to pull request in Documentation repository

The Documentation should be fine. The function is already described there.

Edit: I just checked the generated APK on my phone. Now the GIF works fine in notifications also with an URL without the filename.

Ads the http header check after a GIF File
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @BitWuehler

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

home-assistant bot commented Apr 6, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@BitWuehler BitWuehler marked this pull request as ready for review April 6, 2024 01:38
@BitWuehler BitWuehler changed the title Adds http header check after a GIF File notifications - adds a test if a image url contains a GIF file Apr 6, 2024
@BitWuehler BitWuehler changed the title notifications - adds a test if a image url contains a GIF file notifications - adds a test if a image-url source contains a GIF file Apr 6, 2024
Comment on lines 1293 to 1295
val connection = url.openConnection()
connection.setRequestProperty("User-Agent", HomeAssistantApis.USER_AGENT_STRING)
val contentType = connection.contentType
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this results in the entire image (or whatever is behind the URL) being downloaded. As the image is also fetched later, that is work that should be avoided as it duplicates data used. Try only getting the headers which should contain the content type. This also doesn't handle the authentication required images (hosted on the HA server).

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I commit a "connection.requestMethod = "HEAD" for that.
But what du you mean with "authentication required images (hosted on the HA server)"? I only know direct image paths but these ones should be handled with the filename ending.
Do you maybe mean the use of Image entities? For that this really does not work. But i'm not shure how to resolve this problem. Maybe we could check the file header here?

Copy link
Author

Choose a reason for hiding this comment

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

I now added a byte checkup for the Entity urls. Im not sure if this works good like this, but i will check it with the generated apk!

Copy link
Author

Choose a reason for hiding this comment

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

Because I dont have a image entitiy and i dont now how to add one manually i can't test the byte GIF-check with the image entity. I tryed it with the camera entity, because i used a gif in my camera entity but that doesn't work. Do you now, if the camera entity converts the input somehow? If yes i will delete the camera gif-check.

@jpelgrom
Copy link
Member

jpelgrom commented Apr 7, 2024

I think you've misunderstood what I was saying... There is a requiresAuth parameter that should be handled. At this point, paths are never relative because it is transformed here. If you're saying there is no way to serve a GIF from one of HA's built in proxies, that's also fine! No need for these complicated checks I think.

@jpelgrom
Copy link
Member

jpelgrom commented Jun 20, 2024

As you are clearly still developing I'm going to change this to draft, mark it as ready once you're done :)

@jpelgrom jpelgrom marked this pull request as draft June 20, 2024 05:05
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.

2 participants