Minor fixes and cleanups

app/src/main/java/com/topjohnwu/magisk/Config.kt

@@ -2,6 +2,7 @@ package com.topjohnwu.magisk
 
 import android.content.Context
 import android.content.SharedPreferences
+import android.os.Environment
 import android.util.Xml
 import androidx.core.content.edit
 import com.topjohnwu.magisk.data.database.SettingsDao
@@ -97,10 +98,8 @@ object Config : PreferenceModel, DBConfig {
         if (Utils.isCanary) Value.CANARY_DEBUG_CHANNEL
         else Value.DEFAULT_CHANNEL
 
-    private val defaultDownloadPath get() = Const.EXTERNAL_PATH.toRelativeString(Const.EXTERNAL_PATH.parentFile)
-
     var isDownloadCacheEnabled by preference(Key.DOWNLOAD_CACHE, true)
-    var downloadPath by preference(Key.DOWNLOAD_PATH, defaultDownloadPath)
+    var downloadPath by preference(Key.DOWNLOAD_PATH, Environment.DIRECTORY_DOWNLOADS)
     var repoOrder by preference(Key.REPO_ORDER, Value.ORDER_DATE)
 
     var suDefaultTimeout by preferenceStrInt(Key.SU_REQUEST_TIMEOUT, 10)
@@ -128,14 +127,9 @@ object Config : PreferenceModel, DBConfig {
     @JvmStatic
     var suManager by dbStrings(Key.SU_MANAGER, "")
 
-    fun downloadsFile(path: String = downloadPath) =
-        File(Const.EXTERNAL_PATH.parentFile, path).run {
-            if (exists()) {
-                if (isDirectory) this else null
-            } else {
-                if (mkdirs()) this else null
-            }
-        }
+    // Always return a path in external storage where we can write
+    val downloadDirectory get() =
+        Utils.ensureDownloadPath(downloadPath) ?: get<Context>().getExternalFilesDir(null)!!
 
     fun initialize() = prefs.edit {
         parsePrefs(this)

app/src/main/java/com/topjohnwu/magisk/model/download/DownloadService.kt

@@ -6,13 +6,13 @@ import android.app.PendingIntent
 import android.content.Context
 import android.content.Intent
 import android.os.Build
+import android.os.Environment
 import android.webkit.MimeTypeMap
 import android.widget.Toast
 import androidx.annotation.RequiresPermission
 import androidx.core.app.NotificationCompat
 import com.topjohnwu.magisk.ClassMap
 import com.topjohnwu.magisk.Config
-import com.topjohnwu.magisk.Const
 import com.topjohnwu.magisk.R
 import com.topjohnwu.magisk.model.entity.internal.Configuration.*
 import com.topjohnwu.magisk.model.entity.internal.Configuration.Flash.Secondary
@@ -31,7 +31,7 @@ import kotlin.random.Random.Default.nextInt
 open class DownloadService : RemoteFileService() {
 
     private val context get() = this
-    private val String.downloadsFile get() = Config.downloadsFile()?.let { File(it, this) }
+    private val String.downloadsFile get() = File(Config.downloadDirectory, this)
     private val File.type
         get() = MimeTypeMap.getSingleton()
             .getMimeTypeFromExtension(extension)
@@ -40,7 +40,6 @@ open class DownloadService : RemoteFileService() {
     override fun onFinished(file: File, subject: DownloadSubject) = when (subject) {
         is Magisk -> onFinishedInternal(file, subject)
         is Module -> onFinishedInternal(file, subject)
-        else -> Unit
     }
 
     private fun onFinishedInternal(
@@ -71,7 +70,6 @@ open class DownloadService : RemoteFileService() {
     ) = when (subject) {
         is Magisk -> addActionsInternal(file, subject)
         is Module -> addActionsInternal(file, subject)
-        else -> this
     }
 
     private fun NotificationCompat.Builder.addActionsInternal(
@@ -109,13 +107,7 @@ open class DownloadService : RemoteFileService() {
     // ---
 
     private fun moveToDownloads(file: File) {
-        val destination = file.name.downloadsFile ?: let {
-            Utils.toast(
-                getString(R.string.download_file_folder_error),
-                Toast.LENGTH_LONG
-            )
-            return
-        }
+        val destination = file.name.downloadsFile
 
         if (file != destination) {
             destination.deleteRecursively()
@@ -125,32 +117,18 @@ open class DownloadService : RemoteFileService() {
         Utils.toast(
             getString(
                 R.string.internal_storage,
-                "/" + destination.toRelativeString(Const.EXTERNAL_PATH.parentFile)
+                "/" + destination.toRelativeString(Environment.getExternalStorageDirectory())
             ),
             Toast.LENGTH_LONG
         )
     }
 
     private fun fileIntent(fileName: String): Intent {
-        val file = fileName.downloadsFile ?: let {
-            Utils.toast(
-                getString(R.string.download_file_folder_error),
-                Toast.LENGTH_LONG
-            )
-            return Intent()
-        }
-        return fileIntent(file)
+        return fileIntent(fileName.downloadsFile)
     }
 
     private fun fileParentIntent(fileName: String): Intent {
-        val file = fileName.downloadsFile?.parentFile ?: let {
-            Utils.toast(
-                getString(R.string.download_file_folder_error),
-                Toast.LENGTH_LONG
-            )
-            return Intent()
-        }
-        return fileIntent(file)
+        return fileIntent(fileName.downloadsFile.parentFile!!)
     }
 
     private fun fileIntent(file: File): Intent {

app/src/main/java/com/topjohnwu/magisk/model/download/RemoteFileService.kt

@@ -8,7 +8,8 @@ import com.topjohnwu.magisk.Const
 import com.topjohnwu.magisk.R
 import com.topjohnwu.magisk.data.repository.FileRepository
 import com.topjohnwu.magisk.model.entity.internal.DownloadSubject
-import com.topjohnwu.magisk.model.entity.internal.DownloadSubject.*
+import com.topjohnwu.magisk.model.entity.internal.DownloadSubject.Magisk
+import com.topjohnwu.magisk.model.entity.internal.DownloadSubject.Module
 import com.topjohnwu.magisk.utils.ProgInputStream
 import com.topjohnwu.magisk.utils.cachedFile
 import com.topjohnwu.magisk.utils.firstMap
@@ -30,8 +31,7 @@ abstract class RemoteFileService : NotificationService() {
     private val supportedFolders
         get() = listOfNotNull(
             cacheDir,
-            Config.downloadsFile(),
-            Const.EXTERNAL_PATH
+            Config.downloadDirectory
         )
 
     override val defaultNotification: NotificationCompat.Builder
@@ -46,31 +46,27 @@ abstract class RemoteFileService : NotificationService() {
 
     // ---
 
-    private fun startInternal(subject: DownloadSubject): Single<File> = search(subject)
+    private fun start(subject: DownloadSubject) = search(subject)
         .onErrorResumeNext(download(subject))
         .doOnSubscribe { update(subject.hashCode()) { it.setContentTitle(subject.fileName) } }
         .observeOn(AndroidSchedulers.mainThread())
         .doOnSuccess {
             runCatching { onFinished(it, subject) }.onFailure { Timber.e(it) }
             finish(it, subject)
-        }
-
-    private fun start(subject: DownloadSubject) = startInternal(subject).subscribeK()
+        }.subscribeK()
 
     private fun search(subject: DownloadSubject) = Single.fromCallable {
         if (!Config.isDownloadCacheEnabled) {
             throw IllegalStateException("The download cache is disabled")
         }
 
-        val file = supportedFolders.firstMap { it.find(subject.fileName) }
-
-        if (subject is Magisk) {
-            if (!ShellUtils.checkSum("MD5", file, subject.magisk.hash)) {
-                throw IllegalStateException("The given file doesn't match the hash")
+        supportedFolders.firstMap { it.find(subject.fileName) }.also {
+            if (subject is Magisk) {
+                if (!ShellUtils.checkSum("MD5", it, subject.magisk.hash)) {
+                    throw IllegalStateException("The given file doesn't match the hash")
+                }
             }
         }
-
-        file
     }
 
     private fun download(subject: DownloadSubject) = repo.downloadFile(subject.url)
@@ -107,8 +103,6 @@ abstract class RemoteFileService : NotificationService() {
     }
 
     private fun finish(file: File, subject: DownloadSubject) = finishWork(subject.hashCode()) {
-        if (subject is Installer) return@finishWork null
-
         it.addActions(file, subject)
             .setContentText(getString(R.string.download_complete))
             .setSmallIcon(android.R.drawable.stat_sys_download_done)

app/src/main/java/com/topjohnwu/magisk/model/entity/internal/Configuration.kt

@@ -14,10 +14,6 @@ sealed class Configuration : Parcelable {
         @Parcelize
         object Secondary : Flash()
 
-        companion object {
-            operator fun invoke(): Flash = Primary
-        }
-
     }
 
     @Parcelize

app/src/main/java/com/topjohnwu/magisk/model/entity/internal/DownloadDialogData.kt

@@ -1,12 +0,0 @@
-package com.topjohnwu.magisk.model.entity.internal
-
-import com.skoumal.teanity.util.KObservableField
-import com.topjohnwu.magisk.Config
-import com.topjohnwu.magisk.model.observer.Observer
-
-class DownloadDialogData(initialValue: String) {
-
-    val text = KObservableField(initialValue)
-    val path = Observer(text) { Config.downloadsFile(text.value)?.absolutePath.orEmpty() }
-
-}

app/src/main/java/com/topjohnwu/magisk/model/entity/internal/DownloadSubject.kt

@@ -1,8 +1,6 @@
 package com.topjohnwu.magisk.model.entity.internal
 
 import android.os.Parcelable
-import com.topjohnwu.magisk.BuildConfig
-import com.topjohnwu.magisk.Const
 import com.topjohnwu.magisk.Info
 import com.topjohnwu.magisk.model.entity.MagiskJson
 import com.topjohnwu.magisk.model.entity.Repo
@@ -39,10 +37,4 @@ sealed class DownloadSubject : Parcelable {
 
     }
 
-    @Parcelize
-    object Installer : DownloadSubject() {
-        override val fileName: String get() = "module_installer(${BuildConfig.VERSION_CODE}).sh"
-        override val url: String get() = Const.Url.MODULE_INSTALLER
-    }
-
 }

app/src/main/java/com/topjohnwu/magisk/ui/flash/FlashActivity.kt

@@ -18,7 +18,7 @@ open class FlashActivity : MagiskActivity<FlashViewModel, ActivityFlashBinding>(
     override val layoutRes: Int = R.layout.activity_flash
     override val viewModel: FlashViewModel by viewModel {
         val uri = intent.data ?: let { finish(); Uri.EMPTY }
-        val additionalUri = intent.getParcelableExtra<Uri>(Const.Key.FLASH_DATA) ?: uri
+        val additionalUri = intent.getParcelableExtra(Const.Key.FLASH_DATA) ?: uri
         val action = intent.getStringExtra(Const.Key.FLASH_ACTION) ?: let { finish();"" }
         parametersOf(action, uri, additionalUri)
     }

app/src/main/java/com/topjohnwu/magisk/ui/module/ReposFragment.kt

@@ -99,7 +99,7 @@ class ReposFragment : MagiskFragment<ModuleViewModel, FragmentReposBinding>(),
         fun download(install: Boolean) = context.withExternalRW {
             onSuccess {
                 DownloadService(context) {
-                    val config = if (install) Configuration.Flash() else Configuration.Download
+                    val config = if (install) Configuration.Flash.Primary else Configuration.Download
                     subject = DownloadSubject.Module(item, config)
                 }
             }

app/src/main/java/com/topjohnwu/magisk/ui/settings/SettingsFragment.kt

@@ -3,6 +3,7 @@ package com.topjohnwu.magisk.ui.settings
 import android.content.SharedPreferences
 import android.os.Build
 import android.os.Bundle
+import android.os.Environment
 import android.view.LayoutInflater
 import android.widget.EditText
 import android.widget.Toast
@@ -14,19 +15,21 @@ import androidx.preference.Preference
 import androidx.preference.PreferenceCategory
 import androidx.preference.SwitchPreferenceCompat
 import com.skoumal.teanity.extensions.subscribeK
+import com.skoumal.teanity.util.KObservableField
 import com.topjohnwu.magisk.BuildConfig
 import com.topjohnwu.magisk.Config
 import com.topjohnwu.magisk.Const
 import com.topjohnwu.magisk.R
 import com.topjohnwu.magisk.data.database.RepoDatabaseHelper
 import com.topjohnwu.magisk.databinding.CustomDownloadDialogBinding
-import com.topjohnwu.magisk.model.entity.internal.DownloadDialogData
+import com.topjohnwu.magisk.model.observer.Observer
 import com.topjohnwu.magisk.ui.base.BasePreferenceFragment
 import com.topjohnwu.magisk.utils.*
 import com.topjohnwu.magisk.view.dialogs.FingerprintAuthDialog
 import com.topjohnwu.net.Networking
 import com.topjohnwu.superuser.Shell
 import org.koin.android.ext.android.inject
+import java.io.File
 
 class SettingsFragment : BasePreferenceFragment() {
 
@@ -297,6 +300,13 @@ class SettingsFragment : BasePreferenceFragment() {
             .show()
     }
 
+    inner class DownloadDialogData(initialValue: String) {
+        val text = KObservableField(initialValue)
+        val path = Observer(text) {
+            File(Environment.getExternalStorageDirectory(), text.value).absolutePath
+        }
+    }
+
     private inline fun showDownloadDialog(
         initialValue: String = Config.downloadPath,
         crossinline onSuccess: (String) -> Unit
@@ -310,10 +320,10 @@ class SettingsFragment : BasePreferenceFragment() {
             .setTitle(R.string.settings_download_path_title)
             .setView(binding.root)
             .setPositiveButton(R.string.ok) { _, _ ->
-                Config.downloadsFile(data.text.value)?.let { onSuccess(data.text.value) }
+                Utils.ensureDownloadPath(data.text.value)?.let { onSuccess(data.text.value) }
                     ?: Utils.toast(R.string.settings_download_path_error, Toast.LENGTH_SHORT)
             }
             .setNegativeButton(R.string.close, null)
             .show()
     }
-}
+}

app/src/main/java/com/topjohnwu/magisk/utils/Utils.kt

@@ -8,6 +8,7 @@ import android.content.pm.PackageManager
 import android.content.res.Configuration
 import android.content.res.Resources
 import android.net.Uri
+import android.os.Environment
 import android.widget.Toast
 import androidx.annotation.WorkerThread
 import androidx.work.*
@@ -19,6 +20,7 @@ import com.topjohnwu.net.Networking
 import com.topjohnwu.superuser.Shell
 import com.topjohnwu.superuser.internal.UiThreadHandler
 import com.topjohnwu.superuser.io.SuFile
+import java.io.File
 import java.util.*
 import java.util.concurrent.TimeUnit
 
@@ -120,4 +122,9 @@ object Utils {
         }
     }
 
+    fun ensureDownloadPath(path : String) =
+        File(Environment.getExternalStorageDirectory(), path).run {
+            if ((exists() && isDirectory) || mkdirs()) this else null
+        }
+
 }

app/src/main/res/layout/custom_download_dialog.xml

@@ -7,7 +7,7 @@
 
         <variable
             name="data"
-            type="com.topjohnwu.magisk.model.entity.internal.DownloadDialogData" />
+            type="com.topjohnwu.magisk.ui.settings.SettingsFragment.DownloadDialogData" />
 
     </data>
 

shared/src/main/res/xml/file_paths.xml

@@ -3,4 +3,6 @@
     <files-path name="internal_files" path="."/>
     <cache-path name="cache_files" path="." />
     <external-path name="external_files" path="."/>
-</paths>
+    <external-cache-path name="external_cache_files" path="."/>
+    <external-files-path name="private_external_files" path="."/>
+</paths>