Add long press context menu for images with option to download image #179
Conversation
subsymbolic
left a comment
subsymbolic
left a comment
There was a problem hiding this comment.
Nice work, I really like the long press menu.
A couple of requests
• Can we update the pdf download to also be a long press menu operation for consistency?
• Can we give the images better names? I am seeing 'downloadfile.bin' for all of them
| override fun onContextItemSelected(item: MenuItem): Boolean { | ||
| webView.hitTestResult?.let { | ||
| var url = it.extra | ||
| if(viewModel.userSelectedItemFromLongPressMenu(url, item)) { |
There was a problem hiding this comment.
What site/image are you seeing that's giving you downloadfile.bin? I'm seeing sensible filenames by default when I'm testing.
There was a problem hiding this comment.
I had tried a few images on espc.com and the guardian.co.uk and they are all downloadfile.bin in our app but fine in Chrome. I just tried bbc and that looks good in our app too
There was a problem hiding this comment.
Thanks; will investigate
| } | ||
|
|
||
| fun userLongPressedInWebView(target: WebView.HitTestResult, menu: ContextMenu) { | ||
| Timber.i("Long pressed on %s, (extra=%s)", target.type, target.extra) |
There was a problem hiding this comment.
Can we use kotlin style strings? It is the project standard and I'd rather not mix styles. I know it's slightly less efficient but on the upside they are more readable and if efficiency becomes an issue we can fix it with some proguard config.
There was a problem hiding this comment.
I've removed other instances of the Timber style in favour of the Kotlin style too.
I'm not sure if we can? it looks like the PDF download is a more integral action whereby the We could perhaps start whitelisting types we suspect the browser can't handle (and PDF would be one) and handle that in a special way, but seems like a bigger thing than should be done in this task. |
It was just a thought, I should have marked it as optional as it really goes beyond the scope of this task. |
…image_downloading
…image_downloading
|
@subsymbolic I've added code to save the images to "public external storage directory" so that it will outlive the system's download cache as well as attempting to name the files better. Doing so requires the runtime permission for Re-review please |
…rom DB (#179) Co-authored-by: Craig Russell <CDRussell@users.noreply.github.com>
2 participants
Asana Issue URL: https://app.asana.com/0/414730916066338/538612605030335
Description
Add ability to long press on an image in a WebView and see a context menu allowing for the image to be downloaded
Note, images won't be able to be downloaded from DDG SERP yet due to an ongoing issue there.
Steps to Test this PR:
BBCicon (very top left of the page) and that you can download this image - this is a different type of image from the FTSE chart but both should download successfully.From here, it would be wise to revoke the storage permission, and test that PDFs still download successfully (using espc.com for instance, and clicking on a property, and then "schedule" to trigger a PDF download)