-
-
Notifications
You must be signed in to change notification settings - Fork 656
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
base: master
Are you sure you want to change the base?
Conversation
Ads the http header check after a GIF File
There was a problem hiding this 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!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
app/src/main/java/io/homeassistant/companion/android/notifications/MessagingManager.kt
Outdated
Show resolved
Hide resolved
val connection = url.openConnection() | ||
connection.setRequestProperty("User-Agent", HomeAssistantApis.USER_AGENT_STRING) | ||
val contentType = connection.contentType |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
I think you've misunderstood what I was saying... There is a |
As you are clearly still developing I'm going to change this to draft, mark it as ready once you're done :) |
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.