Commit 7f57dffa authored by MozLando's avatar MozLando
Browse files

Merge #5047 #5133



5047: Fixes #5046: Updates guessFileName to use uniqueFIleName r=csadilek a=sblatz



5133: Closes #5077: Renames onDownloadCompleted r=NotWoods a=sblatz
Co-authored-by: default avatarSawyer Blatz <sdblatz@gmail.com>
......@@ -566,7 +566,8 @@ class SystemEngineView @JvmOverloads constructor(
internal fun createDownloadListener(): DownloadListener {
return DownloadListener { url, userAgent, contentDisposition, mimetype, contentLength ->
session?.internalNotifyObservers {
val fileName = DownloadUtils.guessFileName(contentDisposition, url, mimetype)
val fileName = DownloadUtils.guessFileName(contentDisposition, null, url, mimetype)
val cookie = CookieManager.getInstance().getCookie(url)
onExternalResource(url, fileName, contentLength, mimetype, cookie, userAgent)
}
......
......@@ -45,6 +45,7 @@ import mozilla.components.feature.downloads.facts.emitNotificationPauseFact
import mozilla.components.feature.downloads.facts.emitNotificationCancelFact
import mozilla.components.feature.downloads.facts.emitNotificationTryAgainFact
import mozilla.components.feature.downloads.facts.emitNotificationOpenFact
import mozilla.components.support.utils.DownloadUtils
import java.io.File
import java.io.FileOutputStream
import java.io.IOException
......@@ -329,15 +330,24 @@ abstract class AbstractFetchDownloadService : Service() {
*
* Encapsulates different behaviour depending on the SDK version.
*/
@Suppress("Deprecation")
internal fun useFileStream(
download: DownloadState,
append: Boolean,
block: (OutputStream) -> Unit
) {
// Update the file name to ensure it doesn't collide with one already on disk]
val downloadWithUniqueName = download.fileName?.let {
download.copy(fileName = DownloadUtils.uniqueFileName(
Environment.getExternalStoragePublicDirectory(download.destinationDirectory),
it
))
} ?: download
if (SDK_INT >= Build.VERSION_CODES.Q) {
useFileStreamScopedStorage(download, block)
useFileStreamScopedStorage(downloadWithUniqueName, block)
} else {
useFileStreamLegacy(download, append, block)
useFileStreamLegacy(downloadWithUniqueName, append, block)
}
}
......
......@@ -33,7 +33,7 @@ abstract class DownloadDialogFragment : AppCompatDialogFragment() {
fun setDownload(download: DownloadState) {
val args = arguments ?: Bundle()
args.putString(KEY_FILE_NAME, download.fileName
?: DownloadUtils.guessFileName(null, download.url, download.contentType))
?: DownloadUtils.guessFileName(null, download.destinationDirectory, download.url, download.contentType))
args.putString(KEY_URL, download.url)
args.putLong(KEY_CONTENT_LENGTH, download.contentLength ?: 0)
arguments = args
......
......@@ -5,7 +5,6 @@
package mozilla.components.feature.downloads
import android.content.Context
import android.os.Environment
import android.widget.Toast
import androidx.annotation.ColorRes
import androidx.annotation.VisibleForTesting
......@@ -22,8 +21,8 @@ import mozilla.components.browser.state.store.BrowserStore
import mozilla.components.feature.downloads.DownloadDialogFragment.Companion.FRAGMENT_TAG
import mozilla.components.feature.downloads.manager.AndroidDownloadManager
import mozilla.components.feature.downloads.manager.DownloadManager
import mozilla.components.feature.downloads.manager.OnDownloadCompleted
import mozilla.components.feature.downloads.manager.noop
import mozilla.components.feature.downloads.manager.onDownloadStopped
import mozilla.components.lib.state.ext.flowScoped
import mozilla.components.support.base.feature.LifecycleAwareFeature
import mozilla.components.support.base.feature.OnNeedToRequestPermissions
......@@ -31,7 +30,6 @@ import mozilla.components.support.base.feature.PermissionsFeature
import mozilla.components.support.ktx.android.content.appName
import mozilla.components.support.ktx.android.content.isPermissionGranted
import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged
import java.io.File
/**
* Feature implementation to provide download functionality for the selected
......@@ -42,7 +40,7 @@ import java.io.File
* @property onNeedToRequestPermissions a callback invoked when permissions
* need to be requested before a download can be performed. Once the request
* is completed, [onPermissionsResult] needs to be invoked.
* @property onDownloadCompleted a callback invoked when a download is completed.
* @property onDownloadStopped a callback invoked when a download is paused or completed.
* @property downloadManager a reference to the [DownloadManager] which is
* responsible for performing the downloads.
* @property store a reference to the application's [BrowserStore].
......@@ -59,7 +57,7 @@ class DownloadsFeature(
private val store: BrowserStore,
private val useCases: DownloadsUseCases,
override var onNeedToRequestPermissions: OnNeedToRequestPermissions = { },
onDownloadCompleted: OnDownloadCompleted = noop,
onDownloadStopped: onDownloadStopped = noop,
private val downloadManager: DownloadManager = AndroidDownloadManager(applicationContext),
private val customTabId: String? = null,
private val fragmentManager: FragmentManager? = null,
......@@ -70,12 +68,12 @@ class DownloadsFeature(
)
) : LifecycleAwareFeature, PermissionsFeature {
var onDownloadCompleted: OnDownloadCompleted
get() = downloadManager.onDownloadCompleted
set(value) { downloadManager.onDownloadCompleted = value }
var onDownloadStopped: onDownloadStopped
get() = downloadManager.onDownloadStopped
set(value) { downloadManager.onDownloadStopped = value }
init {
this.onDownloadCompleted = onDownloadCompleted
this.onDownloadStopped = onDownloadStopped
}
private var scope: CoroutineScope? = null
......@@ -96,15 +94,7 @@ class DownloadsFeature(
.collect { state ->
val download = state.content.download
if (download != null) {
// Update the file name to ensure it doesn't collide with one already on disk]
val downloadWithUniqueName = download.fileName?.let {
download.copy(fileName = uniqueFileName(
Environment.getExternalStoragePublicDirectory(download.destinationDirectory),
it
))
} ?: download
processDownload(state, downloadWithUniqueName)
processDownload(state, download)
}
}
}
......@@ -220,24 +210,6 @@ class DownloadsFeature(
block(Pair(state, download))
}
/**
* Checks if the file exists so as not to overwrite one already in downloads
*/
private fun uniqueFileName(directory: File, fileName: String): String {
val fileExtension = fileName.substringAfterLast(".")
val baseFileName = fileName.replace(fileExtension, "")
var potentialFileName = File(directory, fileName)
var copyVersionNumber = 1
while (potentialFileName.exists()) {
potentialFileName = File(directory, "$baseFileName($copyVersionNumber).$fileExtension")
copyVersionNumber += 1
}
return potentialFileName.name
}
/**
* Styling for the download dialog prompt
*/
......
......@@ -37,7 +37,7 @@ internal fun DownloadState.withResponse(headers: Headers, stream: InputStream?):
return copy(
fileName = if (fileName.isNullOrBlank()) {
DownloadUtils.guessFileName(contentDisposition, url, contentType)
DownloadUtils.guessFileName(contentDisposition, destinationDirectory, url, contentType)
} else {
fileName
},
......
......@@ -36,7 +36,7 @@ typealias SystemRequest = android.app.DownloadManager.Request
*/
class AndroidDownloadManager(
private val applicationContext: Context,
override var onDownloadCompleted: OnDownloadCompleted = noop
override var onDownloadStopped: onDownloadStopped = noop
) : BroadcastReceiver(), DownloadManager {
private val queuedDownloads = LongSparseArray<DownloadStateWithRequest>()
......@@ -111,7 +111,7 @@ class AndroidDownloadManager(
}
/**
* Invoked when a download is complete. Calls [onDownloadCompleted] and unregisters the
* Invoked when a download is complete. Calls [onDownloadStopped] and unregisters the
* broadcast receiver if there are no more queued downloads.
*/
override fun onReceive(context: Context, intent: Intent) {
......@@ -121,7 +121,7 @@ class AndroidDownloadManager(
as AbstractFetchDownloadService.DownloadJobStatus
if (download != null) {
onDownloadCompleted(download.state, downloadID, downloadStatus)
onDownloadStopped(download.state, downloadID, downloadStatus)
}
}
}
......@@ -141,7 +141,7 @@ private fun DownloadState.toAndroidRequest(cookie: String): SystemRequest {
}
val fileName = if (fileName.isNullOrBlank()) {
DownloadUtils.guessFileName(null, url, contentType)
DownloadUtils.guessFileName(null, destinationDirectory, url, contentType)
} else {
fileName
}
......
......@@ -9,13 +9,13 @@ import mozilla.components.browser.state.state.content.DownloadState
import mozilla.components.feature.downloads.AbstractFetchDownloadService
import mozilla.components.support.ktx.android.content.isPermissionGranted
typealias OnDownloadCompleted = (DownloadState, Long, AbstractFetchDownloadService.DownloadJobStatus) -> Unit
typealias onDownloadStopped = (DownloadState, Long, AbstractFetchDownloadService.DownloadJobStatus) -> Unit
interface DownloadManager {
val permissions: Array<String>
var onDownloadCompleted: OnDownloadCompleted
var onDownloadStopped: onDownloadStopped
/**
* Schedules a download through the [DownloadManager].
......@@ -45,4 +45,4 @@ fun DownloadManager.validatePermissionGranted(context: Context) {
}
}
internal val noop: OnDownloadCompleted = { _, _, _ -> }
internal val noop: onDownloadStopped = { _, _, _ -> }
......@@ -35,7 +35,7 @@ class FetchDownloadManager<T : AbstractFetchDownloadService>(
private val applicationContext: Context,
private val service: KClass<T>,
private val broadcastManager: LocalBroadcastManager = LocalBroadcastManager.getInstance(applicationContext),
override var onDownloadCompleted: OnDownloadCompleted = noop
override var onDownloadStopped: onDownloadStopped = noop
) : BroadcastReceiver(), DownloadManager {
private val queuedDownloads = LongSparseArray<DownloadState>()
......@@ -104,7 +104,7 @@ class FetchDownloadManager<T : AbstractFetchDownloadService>(
}
/**
* Invoked when a download is complete. Calls [onDownloadCompleted] and unregisters the
* Invoked when a download is complete. Calls [onDownloadStopped] and unregisters the
* broadcast receiver if there are no more queued downloads.
*/
override fun onReceive(context: Context, intent: Intent) {
......@@ -114,7 +114,7 @@ class FetchDownloadManager<T : AbstractFetchDownloadService>(
val download = queuedDownloads[downloadID]
if (download != null) {
onDownloadCompleted(download, downloadID, downloadStatus)
onDownloadStopped(download, downloadID, downloadStatus)
}
}
}
......@@ -51,7 +51,7 @@ class AndroidDownloadManagerTest {
fun `calling download must download the file`() {
var downloadCompleted = false
downloadManager.onDownloadCompleted = { _, _, _ -> downloadCompleted = true }
downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true }
grantPermissions()
......@@ -66,7 +66,7 @@ class AndroidDownloadManagerTest {
fun `calling tryAgain starts the download again`() {
var downloadCompleted = false
downloadManager.onDownloadCompleted = { _, _, _ -> downloadCompleted = true }
downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true }
grantPermissions()
......@@ -97,7 +97,7 @@ class AndroidDownloadManagerTest {
}
@Test
fun `sendBroadcast with valid downloadID must call onDownloadCompleted after download`() {
fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() {
var downloadCompleted = false
var downloadStatus: DownloadJobStatus? = null
val downloadWithFileName = download.copy(fileName = "5MB.zip")
......@@ -109,7 +109,7 @@ class AndroidDownloadManagerTest {
cookie = "yummy_cookie=choco"
)!!
downloadManager.onDownloadCompleted = { _, _, status ->
downloadManager.onDownloadStopped = { _, _, status ->
downloadStatus = status
downloadCompleted = true
}
......
......@@ -62,7 +62,7 @@ class FetchDownloadManagerTest {
downloadManager = FetchDownloadManager(context, MockDownloadService::class, broadcastManager)
var downloadCompleted = false
downloadManager.onDownloadCompleted = { _, _, _ -> downloadCompleted = true }
downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true }
grantPermissions()
......@@ -81,7 +81,7 @@ class FetchDownloadManagerTest {
downloadManager = FetchDownloadManager(context, MockDownloadService::class, broadcastManager)
var downloadCompleted = false
downloadManager.onDownloadCompleted = { _, _, _ -> downloadCompleted = true }
downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true }
grantPermissions()
......@@ -113,14 +113,14 @@ class FetchDownloadManagerTest {
}
@Test
fun `sendBroadcast with valid downloadID must call onDownloadCompleted after download`() {
fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() {
var downloadCompleted = false
var downloadStatus: DownloadJobStatus? = null
val downloadWithFileName = download.copy(fileName = "5MB.zip")
grantPermissions()
downloadManager.onDownloadCompleted = { _, _, status ->
downloadManager.onDownloadStopped = { _, _, status ->
downloadStatus = status
downloadCompleted = true
}
......
......@@ -5,8 +5,10 @@
package mozilla.components.support.utils
import android.net.Uri
import android.os.Environment
import android.webkit.MimeTypeMap
import java.io.ByteArrayOutputStream
import java.io.File
import java.io.UnsupportedEncodingException
import java.util.regex.Pattern
......@@ -40,17 +42,50 @@ object DownloadUtils {
* This method is largely identical to [android.webkit.URLUtil.guessFileName]
* which unfortunately does not implement RFC 5987.
*/
@JvmStatic
fun guessFileName(contentDisposition: String?, url: String?, mimeType: String?): String {
val filename = extractFileNameFromUrl(contentDisposition, url)
// Split filename between base and extension
@Suppress("Deprecation")
@JvmStatic
fun guessFileName(
contentDisposition: String?,
destinationDirectory: String?,
url: String?,
mimeType: String?
): String {
// Split fileName between base and extension
// Add an extension if filename does not have one
return if (filename.contains('.')) {
changeExtension(filename, mimeType)
val extractedFileName = extractFileNameFromUrl(contentDisposition, url)
val fileName = if (extractedFileName.contains('.')) {
changeExtension(extractedFileName, mimeType)
} else {
filename + createExtension(mimeType)
extractedFileName + createExtension(mimeType)
}
return destinationDirectory?.let {
uniqueFileName(Environment.getExternalStoragePublicDirectory(destinationDirectory), fileName)
} ?: fileName
}
/**
* Checks if the file exists so as not to overwrite one already in the destination directory
*/
fun uniqueFileName(directory: File, fileName: String): String {
var fileExtension = ".${fileName.substringAfterLast(".")}"
// Check if an extension was found or not
if (fileExtension == ".$fileName") { fileExtension = "" }
val baseFileName = fileName.replace(fileExtension, "")
var potentialFileName = File(directory, fileName)
var copyVersionNumber = 1
while (potentialFileName.exists()) {
potentialFileName = File(directory, "$baseFileName($copyVersionNumber)$fileExtension")
copyVersionNumber += 1
}
return potentialFileName.name
}
private fun extractFileNameFromUrl(contentDisposition: String?, url: String?): String {
......
......@@ -7,15 +7,20 @@ package mozilla.components.support.utils
import android.webkit.MimeTypeMap
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.Assert.assertEquals
import org.junit.Rule
import org.junit.Test
import org.junit.rules.TemporaryFolder
import org.junit.runner.RunWith
import org.robolectric.Shadows
@RunWith(AndroidJUnit4::class)
class DownloadUtilsTest {
@Rule @JvmField
val folder = TemporaryFolder()
private fun assertContentDisposition(expected: String, contentDisposition: String) {
assertEquals(expected, DownloadUtils.guessFileName(contentDisposition, null, null))
assertEquals(expected, DownloadUtils.guessFileName(contentDisposition, null, null, null))
}
@Test
......@@ -55,6 +60,22 @@ class DownloadUtilsTest {
}
}
@Test
fun uniqueFilenameNoExtension() {
assertEquals("test", DownloadUtils.uniqueFileName(folder.root, "test"))
folder.newFile("test")
assertEquals("test(1)", DownloadUtils.uniqueFileName(folder.root, "test"))
}
@Test
fun uniqueFilename() {
assertEquals("test.zip", DownloadUtils.uniqueFileName(folder.root, "test.zip"))
folder.newFile("test.zip")
assertEquals("test(1).zip", DownloadUtils.uniqueFileName(folder.root, "test.zip"))
}
@Test
fun guessFileName_url() {
assertUrl("downloadfile.bin", "http://example.com/")
......@@ -69,15 +90,15 @@ class DownloadUtilsTest {
Shadows.shadowOf(MimeTypeMap.getSingleton()).addExtensionMimeTypMapping("zip", "application/zip")
Shadows.shadowOf(MimeTypeMap.getSingleton()).addExtensionMimeTypMapping("tar.gz", "application/gzip")
assertEquals("file.jpg", DownloadUtils.guessFileName(null, "http://example.com/file.jpg", "image/jpeg"))
assertEquals("file.jpg", DownloadUtils.guessFileName(null, "http://example.com/file.bin", "image/jpeg"))
assertEquals("file.jpg", DownloadUtils.guessFileName(null, null, "http://example.com/file.jpg", "image/jpeg"))
assertEquals("file.jpg", DownloadUtils.guessFileName(null, null, "http://example.com/file.bin", "image/jpeg"))
assertEquals(
"Caesium-wahoo-v3.6-b792615ced1b.zip",
DownloadUtils.guessFileName(null, "https://download.msfjarvis.website/caesium/wahoo/beta/Caesium-wahoo-v3.6-b792615ced1b.zip", "application/zip")
DownloadUtils.guessFileName(null, null, "https://download.msfjarvis.website/caesium/wahoo/beta/Caesium-wahoo-v3.6-b792615ced1b.zip", "application/zip")
)
assertEquals(
"compressed.TAR.GZ",
DownloadUtils.guessFileName(null, "http://example.com/compressed.TAR.GZ", "application/gzip")
DownloadUtils.guessFileName(null, null, "http://example.com/compressed.TAR.GZ", "application/gzip")
)
}
......@@ -85,7 +106,7 @@ class DownloadUtilsTest {
private val CONTENT_DISPOSITION_TYPES = listOf("attachment", "inline")
private fun assertUrl(expected: String, url: String) {
assertEquals(expected, DownloadUtils.guessFileName(null, url, null))
assertEquals(expected, DownloadUtils.guessFileName(null, null, url, null))
}
}
}
......@@ -12,6 +12,10 @@ permalink: /changelog/
* [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt)
* [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt)
* **feature-downloads**
* ⚠️ **This is a breaking change**:
* Renamed to `OnDownloadCompleted` to `OnDownloadStopped` for increased clarity on when it's triggered
* **browser-state**
* ⚠️ **This is a breaking change**: `DownloadState` doesn't include the property `filePath` in its constructor anymore, now it is a computed property. As the previous behavior caused some situations where `fileName` was initially null and after assigned a value to produce `filePath` values like "/storage/emulated/0/Download/null" [for more info](https://sentry.prod.mozaws.net/operations/reference-browser/issues/6609701/).
......
......@@ -109,7 +109,7 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler {
store = components.store,
useCases = components.downloadsUseCases,
fragmentManager = childFragmentManager,
onDownloadCompleted = { download, id, status ->
onDownloadStopped = { download, id, status ->
Logger.debug("Download done. ID#$id $download with status $status")
},
downloadManager = FetchDownloadManager(
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment