Improve algorithm to generate a filename from an image download #896
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CDRussell
changed the title
cmonfortep
reviewed
Aug 7, 2020
Contributor
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
I really liked to review the PR starting with the unit tests you prepared, it made me easier to understand the algorithm. I posted a few questions though.
Also, while testing this I found a difference between current develop (and main) vs this branch.
- Go to DDG and search something (e.g: wombat)
- Go to image tab
- Long press on an image
- Develop will download something like
downloadfile.binthat can be opened by any app gallery. - This branch will download something like
downloadfile(without extension), and file can't be opened.
app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt
Outdated
Show resolved
Hide resolved
| import org.junit.Assert.assertEquals | ||
| import org.junit.Test | ||
|
|
||
| class UriUtilsFilenameExtractorTest { |
Contributor
There was a problem hiding this comment.
I still need to look the real implementation, but after going through the test cases, I have a question: What happens
- if
urlis empty,mimeTypeandcontentDispositionare not, which one of them as prevalence over the other? - if
urlis not empty and contains a filename, butmimetypesays another thing. which one will be returned?
Member
Author
There was a problem hiding this comment.
- we would still fall back on
URLUtil.guessFileNamewhich is ultimately deciding on the precedence between them. I think that might even be varying the precedence based on OS version which isn't ideal, but that is unchanged from current behavior - I see
mimetypebeing preferred, but again, deferring that logic toURLUtil.guessFileName. so for this scenario, the behavior is unchanged from the current behavior.
| return NotGoodEnough | ||
| } | ||
|
|
||
| private fun guessFromUrl(url: String, contentDisposition: String?, mimeType: String?): String { |
Contributor
There was a problem hiding this comment.
That made me think this method was going to use only url: String to guess the fileName, but I think we will also use also the other params. Wanted to check if my assumption is true
Member
Author
There was a problem hiding this comment.
Good point; changed to guessFilename() which is hopefully clearer
app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt
Outdated
Show resolved
Hide resolved
…craig/improve_download_filename_guessing
…craig/improve_download_filename_guessing # Conflicts: # app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt # app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt
cmonfortep
approved these changes
Aug 10, 2020
Contributor
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
LGTM thanks for the changes!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/0/1187737416677993/f
Tech Design URL:
CC:
Description:
When trying to download images, our app currently tries to guess what a good filename for the download will be, and often fails to produce something nice; it'll often end in a .*bin file type as its current implementation is somewhat naive in turning a URL into a filename.
This PR improves that situation by walking back the URL path, trying each to make a filename, and then returning the best filename produced. Worst case, this should produce the same result as the old algorithm, but best case it will pick out a much better filename.
Steps to test this PR:
FHJ84.jpgand notlatest.binas it was beforeInternal references:
Software Engineering Expectations
Technical Design Template