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

lightbox: Download images with correct names and MIME types. #4140

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

agrawal-d
Copy link
Member

@agrawal-d agrawal-d commented Jun 5, 2020

Currently, the downloadImage function saves images with the
wrong names, extensions and MIME types. This commit fixes that
by introducing a new argument for the function - fileName,
and by determining the MIME type from the file extension.

Simply having the proper extension does not fix the issue, as the file is still interpreted as a text file by Android. The proper MIME type is still required.

Fixes: #4138
Fixes: #4137

@gnprice
Copy link
Member

gnprice commented Jun 8, 2020

Thanks @agrawal-d for the fast fix!

I tested with my original repro (so in Android 8 in an emulator.)

This fixes two of the three things mentioned in #4138:

  • On downloading, the user should ideally see a choice of apps to open the file in that's appropriate to the fact it's an image.
  • When viewing the file in the Files app, it should be seen as an image: should get a thumbnail in the Files app's browse view, and tapping on one should open it as an image.

When I open the download notification, the image gets opened straight away in the Photos app. And the Files app sees it as an image.

  • The saved filename should not include the query-part of the URL. It should be based only on the path of the URL, and specifically the last path component.

But the filename still has the query-part of the URL -- it looks like IMG_20200508_152619.jpg?AWSAccessKeyId=AKIAIEVMBCAT2WD3M5KQ&Signature=anNN6M%2B2bIsRid45dmCR0Qs%2B3aU%3D&Expires=1591642660.

This PR probably contains most of the plumbing already to fix that -- just need to use the new fileName parameter to tell it what filename to save the download as. Would you make that fix too (and test that it works for you)?

@agrawal-d agrawal-d force-pushed the filenames branch 2 times, most recently from b8a8340 to 1223fdc Compare June 9, 2020 05:31
@agrawal-d
Copy link
Member Author

Thanks @gnprice, updated the PR.
Now, it also fixes #4137.

Currently, the `downloadImage` function saves images with the
wrong names, extensions and MIME types. This commit fixes that
by introducing a new argument for the function - `fileName`,
and by determining the MIME type from the file extension.

Fixes: zulip#4137
Fixes: zulip#4138
@gnprice gnprice merged commit 2439997 into zulip:master Jun 9, 2020
@gnprice
Copy link
Member

gnprice commented Jun 9, 2020

Looks great, thanks -- merged!

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