From 92f45484d4144436f9c19c2ad9ddabc28d4406ea Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Wed, 5 Aug 2020 20:46:28 +0100 Subject: [PATCH 1/8] Improve algorithm used to guess filenames for downloaded files --- .../UriUtilsFilenameExtractorTest.kt | 115 ++++++++++++++++++ .../browser/DownloadConfirmationFragment.kt | 6 +- .../app/browser/downloader/FileDownloader.kt | 7 +- .../browser/downloader/FilenameExtractor.kt | 78 ++++++++++++ .../downloader/NetworkFileDownloader.kt | 4 +- 5 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt create mode 100644 app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt new file mode 100644 index 000000000000..20def25911d0 --- /dev/null +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2020 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser.downloader + +import org.junit.Assert.assertEquals +import org.junit.Test + +class UriUtilsFilenameExtractorTest { + + private val testee: FilenameExtractor = FilenameExtractor() + + @Test + fun whenUrlEndsWithFilenameAsJpgNoMimeOrContentDispositionThenFilenameShouldBeExtracted() { + val url = "https://example.com/realFilename.jpg" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.jpg", extracted) + } + + @Test + fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsThenFilenameShouldBeExtracted() { + val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.jpg", extracted) + } + + @Test + fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsAndQueryParamsThenFilenameShouldBeExtracted() { + val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.jpg", extracted) + } + + @Test + fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsAndQueryParamsWhichLookLikeAFilenameThenFilenameShouldBeExtracted() { + val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123.com" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.jpg", extracted) + } + + @Test + fun whenUrlContainsFilenameButContentDispositionSaysOtherwiseThenExtractFromContentDisposition() { + val url = "https://example.com/filename.jpg" + val mimeType: String? = null + val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg" + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("fromDisposition.jpg", extracted) + } + + @Test + fun whenFilenameActuallyEndsInBinThenThatIsExtracted() { + val url = "https://example.com/realFilename.bin" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.bin", extracted) + } + + @Test + fun whenUrlIsEmptyStringAndNoOtherDataProvidedThenDefaultNameAndBinFiletypeReturned() { + val url = "" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("downloadfile.bin", extracted) + } + + @Test + fun whenUrlIsEmptyStringAndMimeTypeProvidedThenDefaultNameAndFiletypeFromMimeReturned() { + val url = "" + val mimeType: String? = "image/jpeg" + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("downloadfile.jpg", extracted) + } + + @Test + fun whenUrlIsEmptyStringAndContentDispositionProvidedThenExtractFromContentDisposition() { + val url = "" + val mimeType: String? = null + val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg" + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("fromDisposition.jpg", extracted) + } + + @Test + fun whenNoFilenameAndNoPathSegmentsThenDomainNameReturned() { + val url = "http://example.com" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("example.com", extracted) + } +} diff --git a/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt index db17eb8b5b8e..dcf86293538e 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt @@ -27,6 +27,7 @@ import androidx.core.content.FileProvider.getUriForFile import com.duckduckgo.app.browser.downloader.FileDownloader import com.duckduckgo.app.browser.downloader.FileDownloader.FileDownloadListener import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload +import com.duckduckgo.app.browser.downloader.FilenameExtractor import com.duckduckgo.app.browser.downloader.guessFileName import com.duckduckgo.app.browser.downloader.isDataUrl import com.duckduckgo.app.global.view.gone @@ -46,6 +47,9 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() { @Inject lateinit var downloader: FileDownloader + @Inject + lateinit var filenameExtractor: FilenameExtractor + lateinit var downloadListener: FileDownloadListener private val pendingDownload: PendingFileDownload by lazy { @@ -67,7 +71,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() { } private fun setupDownload() { - file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, pendingDownload.guessFileName()) else null + file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, pendingDownload.guessFileName(filenameExtractor)) else null } private fun setupViews(view: View) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt index 826ed1340be5..ff69dcaae4cf 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt @@ -27,7 +27,8 @@ import javax.inject.Inject class FileDownloader @Inject constructor( private val dataUriDownloader: DataUriDownloader, - private val networkFileDownloader: NetworkFileDownloader + private val networkFileDownloader: NetworkFileDownloader, + private val filenameExtractor: FilenameExtractor ) { @WorkerThread @@ -57,8 +58,8 @@ class FileDownloader @Inject constructor( } } -fun FileDownloader.PendingFileDownload.guessFileName(): String { - val guessedFileName = URLUtil.guessFileName(url, contentDisposition, mimeType) +fun FileDownloader.PendingFileDownload.guessFileName(filenameExtractor: FilenameExtractor): String { + val guessedFileName = filenameExtractor.extract(url, contentDisposition, mimeType) Timber.i("Guessed filename of $guessedFileName for url $url") return guessedFileName } diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt new file mode 100644 index 000000000000..601284b9a366 --- /dev/null +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2020 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.app.browser.downloader + +import android.webkit.URLUtil +import androidx.core.net.toUri +import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.* +import timber.log.Timber +import javax.inject.Inject + +class FilenameExtractor @Inject constructor() { + + fun extract(url: String, contentDisposition: String?, mimeType: String?): String { + val firstGuess = guessFromUrl(url, contentDisposition, mimeType) + var pathSegments = pathSegments(url) + var bestGuess = firstGuess + + while (true) { + + when (determineGuessQuality(bestGuess, pathSegments)) { + is GoodEnough -> return bestGuess + is OutOfOptions -> return firstGuess + is NotGoodEnough -> { + pathSegments = pathSegments.dropLast(1) + bestGuess = guessFromUrl(pathSegments.rebuildUrl(), contentDisposition, mimeType) + } + } + + } + } + + private fun determineGuessQuality(guess: String, pathSegments: List): GuessQuality { + Timber.v("Best guess is now $guess") + + if (!guess.endsWith(DEFAULT_FILE_TYPE)) return GoodEnough + if (pathSegments.isEmpty()) return OutOfOptions + + return NotGoodEnough + } + + private fun guessFromUrl(url: String, contentDisposition: String?, mimeType: String?): String { + return URLUtil.guessFileName(url, contentDisposition, mimeType) + } + + private fun pathSegments(url: String): List { + val uri = url.toUri() + return uri.pathSegments ?: emptyList() + } + + private fun List.rebuildUrl(): String { + return joinToString(separator = "/") + } + + companion object { + private const val DEFAULT_FILE_TYPE = ".bin" + } + + sealed class GuessQuality { + object GoodEnough : GuessQuality() + object NotGoodEnough : GuessQuality() + object OutOfOptions : GuessQuality() + } + +} diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt index 08e1c732a445..0be61b2d2c71 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt @@ -25,7 +25,7 @@ import com.duckduckgo.app.browser.R import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload import javax.inject.Inject -class NetworkFileDownloader @Inject constructor(private val context: Context) { +class NetworkFileDownloader @Inject constructor(private val context: Context, private val filenameExtractor: FilenameExtractor) { fun download(pendingDownload: PendingFileDownload, callback: FileDownloader.FileDownloadListener) { @@ -34,7 +34,7 @@ class NetworkFileDownloader @Inject constructor(private val context: Context) { return } - val guessedFileName = pendingDownload.guessFileName() + val guessedFileName = pendingDownload.guessFileName(filenameExtractor) val request = DownloadManager.Request(pendingDownload.url.toUri()).apply { allowScanningByMediaScanner() From f4a1927a63d1fba88574ce43ba4132e962f15c35 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Wed, 5 Aug 2020 21:49:32 +0100 Subject: [PATCH 2/8] Tidy extraction logic and add additional test cases --- .../UriUtilsFilenameExtractorTest.kt | 22 +++++++- .../browser/downloader/FilenameExtractor.kt | 53 +++++++++++-------- 2 files changed, 50 insertions(+), 25 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index 20def25911d0..9516439eadce 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -69,7 +69,7 @@ class UriUtilsFilenameExtractorTest { } @Test - fun whenFilenameActuallyEndsInBinThenThatIsExtracted() { + fun whenFilenameEndsInBinThenThatIsExtracted() { val url = "https://example.com/realFilename.bin" val mimeType: String? = null val contentDisposition: String? = null @@ -77,13 +77,31 @@ class UriUtilsFilenameExtractorTest { assertEquals("realFilename.bin", extracted) } + @Test + fun whenFilenameEndsInBinWithASlashThenThatIsExtracted() { + val url = "https://example.com/realFilename.bin/" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.bin", extracted) + } + + @Test + fun whenFilenameContainsBinThenThatIsExtracted() { + val url = "https://example.com/realFilename.bin/foo/bar" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("realFilename.bin", extracted) + } + @Test fun whenUrlIsEmptyStringAndNoOtherDataProvidedThenDefaultNameAndBinFiletypeReturned() { val url = "" val mimeType: String? = null val contentDisposition: String? = null val extracted = testee.extract(url, contentDisposition, mimeType) - assertEquals("downloadfile.bin", extracted) + assertEquals("downloadfile", extracted) } @Test diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt index 601284b9a366..0c7571b5a786 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt @@ -18,47 +18,53 @@ package com.duckduckgo.app.browser.downloader import android.webkit.URLUtil import androidx.core.net.toUri -import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.* -import timber.log.Timber +import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.NotGoodEnough +import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.TriedAllOptions import javax.inject.Inject class FilenameExtractor @Inject constructor() { fun extract(url: String, contentDisposition: String?, mimeType: String?): String { val firstGuess = guessFromUrl(url, contentDisposition, mimeType) + val guesses = Guesses(bestGuess = null, latestGuess = firstGuess) + val baseUrl = url.toUri().host ?: "" var pathSegments = pathSegments(url) - var bestGuess = firstGuess - - while (true) { - - when (determineGuessQuality(bestGuess, pathSegments)) { - is GoodEnough -> return bestGuess - is OutOfOptions -> return firstGuess - is NotGoodEnough -> { - pathSegments = pathSegments.dropLast(1) - bestGuess = guessFromUrl(pathSegments.rebuildUrl(), contentDisposition, mimeType) - } - } + while (evaluateGuessQuality(guesses, pathSegments) != TriedAllOptions) { + pathSegments = pathSegments.dropLast(1) + guesses.latestGuess = guessFromUrl(baseUrl + "/" + pathSegments.rebuildUrl(), contentDisposition, mimeType) } + return guesses.bestGuess ?: guesses.latestGuess + } - private fun determineGuessQuality(guess: String, pathSegments: List): GuessQuality { - Timber.v("Best guess is now $guess") + private fun evaluateGuessQuality(guesses: Guesses, pathSegments: List): GuessQuality { + val latestGuess = guesses.latestGuess + + // if it contains a '.' then it's a good chance of a filetype and we can update our best guess + if (latestGuess.contains(".")) { + guesses.bestGuess = latestGuess + } - if (!guess.endsWith(DEFAULT_FILE_TYPE)) return GoodEnough - if (pathSegments.isEmpty()) return OutOfOptions + if (pathSegments.isEmpty()) return TriedAllOptions return NotGoodEnough } private fun guessFromUrl(url: String, contentDisposition: String?, mimeType: String?): String { - return URLUtil.guessFileName(url, contentDisposition, mimeType) + val tidiedUrl = url.removeSuffix("/") + var guessedFilename = URLUtil.guessFileName(tidiedUrl, contentDisposition, mimeType) + + // we only want to keep the default .bin filetype on the guess if the URL actually has that too + if (guessedFilename.endsWith(DEFAULT_FILE_TYPE) && !tidiedUrl.endsWith(DEFAULT_FILE_TYPE)) { + guessedFilename = guessedFilename.removeSuffix(DEFAULT_FILE_TYPE) + } + + return guessedFilename } private fun pathSegments(url: String): List { - val uri = url.toUri() - return uri.pathSegments ?: emptyList() + return url.toUri().pathSegments ?: emptyList() } private fun List.rebuildUrl(): String { @@ -70,9 +76,10 @@ class FilenameExtractor @Inject constructor() { } sealed class GuessQuality { - object GoodEnough : GuessQuality() object NotGoodEnough : GuessQuality() - object OutOfOptions : GuessQuality() + object TriedAllOptions : GuessQuality() } + data class Guesses(var latestGuess: String, var bestGuess: String? = null) + } From 00ec84b73d3a2175d4fe28fea8c4c0fb3ecab0c1 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Wed, 5 Aug 2020 22:00:11 +0100 Subject: [PATCH 3/8] Break evaluation earlier and add new test --- .../browser/downloader/UriUtilsFilenameExtractorTest.kt | 9 +++++++++ .../app/browser/downloader/FilenameExtractor.kt | 4 +++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index 9516439eadce..9cd077fb75f9 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -77,6 +77,15 @@ class UriUtilsFilenameExtractorTest { assertEquals("realFilename.bin", extracted) } + @Test + fun whenUrlContainsNoFileNameButLotsOfPathsSegmentsThenFirstSegmentNameIsUsed() { + val url = "https://example.com/foo/bar/files" + val mimeType: String? = null + val contentDisposition: String? = null + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("foo", extracted) + } + @Test fun whenFilenameEndsInBinWithASlashThenThatIsExtracted() { val url = "https://example.com/realFilename.bin/" diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt index 0c7571b5a786..14dcd6126db4 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt @@ -20,6 +20,7 @@ import android.webkit.URLUtil import androidx.core.net.toUri import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.NotGoodEnough import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.TriedAllOptions +import timber.log.Timber import javax.inject.Inject class FilenameExtractor @Inject constructor() { @@ -46,7 +47,7 @@ class FilenameExtractor @Inject constructor() { guesses.bestGuess = latestGuess } - if (pathSegments.isEmpty()) return TriedAllOptions + if (pathSegments.size < 2) return TriedAllOptions return NotGoodEnough } @@ -60,6 +61,7 @@ class FilenameExtractor @Inject constructor() { guessedFilename = guessedFilename.removeSuffix(DEFAULT_FILE_TYPE) } + Timber.v("From URL [$url], guessed [$guessedFilename]") return guessedFilename } From 943f91dfc0e31908ac45e3d476f61e5b1ca3bdfe Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Fri, 7 Aug 2020 15:57:17 +0100 Subject: [PATCH 4/8] Return .bin if no other filetype available --- .../downloader/UriUtilsFilenameExtractorTest.kt | 4 ++-- .../app/browser/downloader/FilenameExtractor.kt | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index 9cd077fb75f9..ab52b2347015 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -83,7 +83,7 @@ class UriUtilsFilenameExtractorTest { val mimeType: String? = null val contentDisposition: String? = null val extracted = testee.extract(url, contentDisposition, mimeType) - assertEquals("foo", extracted) + assertEquals("foo.bin", extracted) } @Test @@ -110,7 +110,7 @@ class UriUtilsFilenameExtractorTest { val mimeType: String? = null val contentDisposition: String? = null val extracted = testee.extract(url, contentDisposition, mimeType) - assertEquals("downloadfile", extracted) + assertEquals("downloadfile.bin", extracted) } @Test diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt index 14dcd6126db4..aacd29197c45 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt @@ -26,17 +26,17 @@ import javax.inject.Inject class FilenameExtractor @Inject constructor() { fun extract(url: String, contentDisposition: String?, mimeType: String?): String { - val firstGuess = guessFromUrl(url, contentDisposition, mimeType) + val firstGuess = guessFilename(url, contentDisposition, mimeType) val guesses = Guesses(bestGuess = null, latestGuess = firstGuess) val baseUrl = url.toUri().host ?: "" var pathSegments = pathSegments(url) while (evaluateGuessQuality(guesses, pathSegments) != TriedAllOptions) { pathSegments = pathSegments.dropLast(1) - guesses.latestGuess = guessFromUrl(baseUrl + "/" + pathSegments.rebuildUrl(), contentDisposition, mimeType) + guesses.latestGuess = guessFilename(baseUrl + "/" + pathSegments.rebuildUrl(), contentDisposition, mimeType) } - return guesses.bestGuess ?: guesses.latestGuess + return bestGuess(guesses) } private fun evaluateGuessQuality(guesses: Guesses, pathSegments: List): GuessQuality { @@ -52,7 +52,7 @@ class FilenameExtractor @Inject constructor() { return NotGoodEnough } - private fun guessFromUrl(url: String, contentDisposition: String?, mimeType: String?): String { + private fun guessFilename(url: String, contentDisposition: String?, mimeType: String?): String { val tidiedUrl = url.removeSuffix("/") var guessedFilename = URLUtil.guessFileName(tidiedUrl, contentDisposition, mimeType) @@ -65,6 +65,14 @@ class FilenameExtractor @Inject constructor() { return guessedFilename } + private fun bestGuess(guesses: Guesses): String { + val guess = guesses.bestGuess ?: guesses.latestGuess + if (!guess.contains(".")) { + return guess + DEFAULT_FILE_TYPE + } + return guess + } + private fun pathSegments(url: String): List { return url.toUri().pathSegments ?: emptyList() } From 4d73cc88304041c2748b6b4922a044f60e7e8d22 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Fri, 7 Aug 2020 15:57:39 +0100 Subject: [PATCH 5/8] Remove unused constructor param --- .../com/duckduckgo/app/browser/downloader/FileDownloader.kt | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt index ff69dcaae4cf..fa43b4ea0161 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt @@ -25,11 +25,7 @@ import java.io.File import java.io.Serializable import javax.inject.Inject -class FileDownloader @Inject constructor( - private val dataUriDownloader: DataUriDownloader, - private val networkFileDownloader: NetworkFileDownloader, - private val filenameExtractor: FilenameExtractor -) { +class FileDownloader @Inject constructor(private val dataUriDownloader: DataUriDownloader, private val networkFileDownloader: NetworkFileDownloader) { @WorkerThread fun download(pending: PendingFileDownload, callback: FileDownloadListener) { From 6cb5bda5cc7f5c78ce6a5c7b1e9b578ff53de41c Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Fri, 7 Aug 2020 17:21:43 +0100 Subject: [PATCH 6/8] Add additional test --- .../browser/downloader/UriUtilsFilenameExtractorTest.kt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index ab52b2347015..11cec402ab3c 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -131,6 +131,15 @@ class UriUtilsFilenameExtractorTest { assertEquals("fromDisposition.jpg", extracted) } + @Test + fun whenUrlIsEmptyStringAndConflictingContentDispositionAndMimeTypeProvidedThenExtractFromContentDisposition() { + val url = "" + val mimeType: String? = "image/jpeg" + val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.json" + val extracted = testee.extract(url, contentDisposition, mimeType) + assertEquals("fromDisposition.jpg", extracted) + } + @Test fun whenNoFilenameAndNoPathSegmentsThenDomainNameReturned() { val url = "http://example.com" From d05b32feff2dfb05dc570af01a83b4f90f8c6307 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Fri, 7 Aug 2020 17:41:03 +0100 Subject: [PATCH 7/8] Removed confusing extension function from pendingDownload --- .../UriUtilsFilenameExtractorTest.kt | 38 ++++++++++++------- .../browser/DownloadConfirmationFragment.kt | 3 +- .../app/browser/downloader/FileDownloader.kt | 7 ---- .../browser/downloader/FilenameExtractor.kt | 7 +++- .../downloader/NetworkFileDownloader.kt | 2 +- 5 files changed, 32 insertions(+), 25 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index 11cec402ab3c..dec85bb1ac52 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -28,7 +28,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/realFilename.jpg" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.jpg", extracted) } @@ -37,7 +37,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.jpg", extracted) } @@ -46,7 +46,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.jpg", extracted) } @@ -55,7 +55,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123.com" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.jpg", extracted) } @@ -64,7 +64,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/filename.jpg" val mimeType: String? = null val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg" - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("fromDisposition.jpg", extracted) } @@ -73,7 +73,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/realFilename.bin" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.bin", extracted) } @@ -82,7 +82,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/foo/bar/files" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("foo.bin", extracted) } @@ -91,7 +91,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/realFilename.bin/" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.bin", extracted) } @@ -100,7 +100,7 @@ class UriUtilsFilenameExtractorTest { val url = "https://example.com/realFilename.bin/foo/bar" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("realFilename.bin", extracted) } @@ -109,7 +109,7 @@ class UriUtilsFilenameExtractorTest { val url = "" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("downloadfile.bin", extracted) } @@ -118,7 +118,7 @@ class UriUtilsFilenameExtractorTest { val url = "" val mimeType: String? = "image/jpeg" val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("downloadfile.jpg", extracted) } @@ -127,7 +127,7 @@ class UriUtilsFilenameExtractorTest { val url = "" val mimeType: String? = null val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg" - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("fromDisposition.jpg", extracted) } @@ -136,7 +136,7 @@ class UriUtilsFilenameExtractorTest { val url = "" val mimeType: String? = "image/jpeg" val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.json" - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("fromDisposition.jpg", extracted) } @@ -145,7 +145,17 @@ class UriUtilsFilenameExtractorTest { val url = "http://example.com" val mimeType: String? = null val contentDisposition: String? = null - val extracted = testee.extract(url, contentDisposition, mimeType) + val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) assertEquals("example.com", extracted) } + + private fun buildPendingDownload(url: String, contentDisposition: String?, mimeType: String?): FileDownloader.PendingFileDownload { + return FileDownloader.PendingFileDownload( + url = url, + contentDisposition = contentDisposition, + mimeType = mimeType, + subfolder = "aFolder", + userAgent = "aUserAgent" + ) + } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt index dcf86293538e..9ed33a6e9022 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt @@ -28,7 +28,6 @@ import com.duckduckgo.app.browser.downloader.FileDownloader import com.duckduckgo.app.browser.downloader.FileDownloader.FileDownloadListener import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload import com.duckduckgo.app.browser.downloader.FilenameExtractor -import com.duckduckgo.app.browser.downloader.guessFileName import com.duckduckgo.app.browser.downloader.isDataUrl import com.duckduckgo.app.global.view.gone import com.duckduckgo.app.global.view.leftDrawable @@ -71,7 +70,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() { } private fun setupDownload() { - file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, pendingDownload.guessFileName(filenameExtractor)) else null + file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, filenameExtractor.extract(pendingDownload)) else null } private fun setupViews(view: View) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt index fa43b4ea0161..c381ff617f75 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt @@ -20,7 +20,6 @@ import android.net.Uri import android.os.Environment import android.webkit.URLUtil import androidx.annotation.WorkerThread -import timber.log.Timber import java.io.File import java.io.Serializable import javax.inject.Inject @@ -54,12 +53,6 @@ class FileDownloader @Inject constructor(private val dataUriDownloader: DataUriD } } -fun FileDownloader.PendingFileDownload.guessFileName(filenameExtractor: FilenameExtractor): String { - val guessedFileName = filenameExtractor.extract(url, contentDisposition, mimeType) - Timber.i("Guessed filename of $guessedFileName for url $url") - return guessedFileName -} - val FileDownloader.PendingFileDownload.isDataUrl get() = URLUtil.isDataUrl(url) val FileDownloader.PendingFileDownload.isNetworkUrl get() = URLUtil.isNetworkUrl(url) diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt index aacd29197c45..78c170ae6de7 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt @@ -18,6 +18,7 @@ package com.duckduckgo.app.browser.downloader import android.webkit.URLUtil import androidx.core.net.toUri +import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.NotGoodEnough import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.TriedAllOptions import timber.log.Timber @@ -25,7 +26,11 @@ import javax.inject.Inject class FilenameExtractor @Inject constructor() { - fun extract(url: String, contentDisposition: String?, mimeType: String?): String { + fun extract(pendingDownload: PendingFileDownload): String { + val url = pendingDownload.url + val mimeType = pendingDownload.mimeType + val contentDisposition = pendingDownload.contentDisposition + val firstGuess = guessFilename(url, contentDisposition, mimeType) val guesses = Guesses(bestGuess = null, latestGuess = firstGuess) val baseUrl = url.toUri().host ?: "" diff --git a/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt b/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt index 0be61b2d2c71..82d8740a4dfb 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt @@ -34,7 +34,7 @@ class NetworkFileDownloader @Inject constructor(private val context: Context, pr return } - val guessedFileName = pendingDownload.guessFileName(filenameExtractor) + val guessedFileName = filenameExtractor.extract(pendingDownload) val request = DownloadManager.Request(pendingDownload.url.toUri()).apply { allowScanningByMediaScanner() From e6f77ae692484584e9f43529b5f06ae930135dd3 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Fri, 7 Aug 2020 19:51:50 +0100 Subject: [PATCH 8/8] Remove test that can be OS-dependent --- .../browser/downloader/UriUtilsFilenameExtractorTest.kt | 9 --------- 1 file changed, 9 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt index dec85bb1ac52..7da76d7a3227 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt @@ -131,15 +131,6 @@ class UriUtilsFilenameExtractorTest { assertEquals("fromDisposition.jpg", extracted) } - @Test - fun whenUrlIsEmptyStringAndConflictingContentDispositionAndMimeTypeProvidedThenExtractFromContentDisposition() { - val url = "" - val mimeType: String? = "image/jpeg" - val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.json" - val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType)) - assertEquals("fromDisposition.jpg", extracted) - } - @Test fun whenNoFilenameAndNoPathSegmentsThenDomainNameReturned() { val url = "http://example.com"