Fix external storage UX (#1022)

* build: update to Kotlin 1.4

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* UserPreference: finish if directory selection was triggered from an intent

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* PasswordStore: switch permission request to ActivityResultContracts

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* PasswordStore: fix activity reference

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* GitOperationActivity: make invalid values more obvious

Would have caught this issue much sooner if I had just done this

Fixes: 3d8cea5966 ("Improve permission handling logic (#732)")
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* Assorted collection of hackery to make external storage use palatable

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* Update changelog

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

CHANGELOG.md

@@ -22,6 +22,8 @@ All notable changes to this project will be documented in this file.
 - I keep saying this but for real: error message for wrong SSH/HTTPS password is properly fixed now
 - Fix crash when OpenKeychain is not installed
 - Clone operation won't leave user on an empty password list upon failure
+- Cloning a new repository to external storage wouldn't work
+- UI froze for some people when deleting existing files from the external directory
 
 ## [1.10.3] - 2020-07-30
 

app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt

@@ -19,13 +19,13 @@ import android.view.KeyEvent
 import android.view.Menu
 import android.view.MenuItem
 import android.view.MenuItem.OnActionExpandListener
+import androidx.activity.result.contract.ActivityResultContracts.RequestPermission
 import androidx.activity.result.contract.ActivityResultContracts.StartActivityForResult
 import androidx.activity.viewModels
 import androidx.appcompat.app.AppCompatActivity
 import androidx.appcompat.widget.AppCompatTextView
 import androidx.appcompat.widget.SearchView
 import androidx.appcompat.widget.SearchView.OnQueryTextListener
-import androidx.core.app.ActivityCompat
 import androidx.core.content.ContextCompat
 import androidx.core.content.edit
 import androidx.core.content.getSystemService
@@ -127,7 +127,13 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
                     return@registerForActivityResult
                 }
             }
-            val intent = Intent(activity, GitOperationActivity::class.java)
+            checkPermissionsAndCloneAction.launch(Manifest.permission.WRITE_EXTERNAL_STORAGE)
+        }
+    }
+
+    private val checkPermissionsAndCloneAction = registerForActivityResult(RequestPermission()) { granted ->
+        if (granted) {
+            val intent = Intent(activity, GitServerConfigActivity::class.java)
             intent.putExtra(BaseGitActivity.REQUEST_ARG_OP, BaseGitActivity.REQUEST_CLONE)
             cloneAction.launch(intent)
         }
@@ -220,14 +226,13 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         }
     }
 
-    public override fun onStart() {
+    override fun onStart() {
         super.onStart()
         refreshPasswordList()
     }
 
-    public override fun onResume() {
+    override fun onResume() {
         super.onResume()
-        // do not attempt to checkLocalRepository() if no storage permission: immediate crash
         if (settings.getBoolean(PreferenceKeys.GIT_EXTERNAL, false)) {
             hasRequiredStoragePermissions(true)
         } else {
@@ -240,16 +245,6 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         }
     }
 
-    override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<String>, grantResults: IntArray) {
-        super.onRequestPermissionsResult(requestCode, permissions, grantResults)
-        // If request is cancelled, the result arrays are empty.
-        if (requestCode == REQUEST_EXTERNAL_STORAGE) {
-            if (grantResults.isNotEmpty() && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
-                checkLocalRepository()
-            }
-        }
-    }
-
     override fun onCreateOptionsMenu(menu: Menu?): Boolean {
         val menuRes = when {
             GitSettings.connectionMode == ConnectionMode.None -> R.menu.main_menu_no_auth
@@ -423,19 +418,18 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
      * is true if the permission has been granted.
      */
     private fun hasRequiredStoragePermissions(checkLocalRepo: Boolean = false): Boolean {
+        val cloning = supportFragmentManager.findFragmentByTag("ToCloneOrNot") != null
         return if (ContextCompat.checkSelfPermission(activity, Manifest.permission.WRITE_EXTERNAL_STORAGE)
-            != PackageManager.PERMISSION_GRANTED) {
+            != PackageManager.PERMISSION_GRANTED && !cloning) {
             Snackbar.make(
                 findViewById(R.id.main_layout),
                 getString(R.string.access_sdcard_text),
                 Snackbar.LENGTH_INDEFINITE
             ).run {
                 setAction(getString(R.string.snackbar_action_grant)) {
-                    ActivityCompat.requestPermissions(
+                    registerForActivityResult(RequestPermission()) { granted ->
-                        activity,
+                        if (granted) checkLocalRepository()
-                        arrayOf(Manifest.permission.WRITE_EXTERNAL_STORAGE),
+                    }.launch(Manifest.permission.WRITE_EXTERNAL_STORAGE)
-                        REQUEST_EXTERNAL_STORAGE
-                    )
                     dismiss()
                 }
                 show()
@@ -491,7 +485,7 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
             supportActionBar!!.hide()
             supportFragmentManager.popBackStack(null, FragmentManager.POP_BACK_STACK_INCLUSIVE)
             supportFragmentManager.commit {
-                replace(R.id.main_layout, ToCloneOrNot())
+                replace(R.id.main_layout, ToCloneOrNot(), "ToCloneOrNot")
             }
         }
     }
@@ -903,7 +897,6 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         const val REQUEST_ARG_PATH = "PATH"
         const val CLONE_REPO_BUTTON = 401
         const val NEW_REPO_BUTTON = 402
-        private const val REQUEST_EXTERNAL_STORAGE = 50
         private fun isPrintable(c: Char): Boolean {
             val block = UnicodeBlock.of(c)
             return (!Character.isISOControl(c) &&

app/src/main/java/com/zeapo/pwdstore/UserPreference.kt

@@ -464,7 +464,7 @@ class UserPreference : AppCompatActivity() {
         when (intent?.getStringExtra("operation")) {
             "get_ssh_key" -> getSshKey()
             "make_ssh_key" -> makeSshKey(false)
-            "git_external" -> selectExternalGitRepository()
+            "git_external" -> selectExternalGitRepository(fromIntent = true)
         }
         prefsFragment = PrefsFragment()
 
@@ -477,7 +477,7 @@ class UserPreference : AppCompatActivity() {
     }
 
     @Suppress("Deprecation") // for Environment.getExternalStorageDirectory()
-    fun selectExternalGitRepository() {
+    fun selectExternalGitRepository(fromIntent: Boolean = false) {
         MaterialAlertDialogBuilder(this)
             .setTitle(this.resources.getString(R.string.external_repository_dialog_title))
             .setMessage(this.resources.getString(R.string.external_repository_dialog_text))
@@ -506,6 +506,10 @@ class UserPreference : AppCompatActivity() {
                             .show()
                     }
                     prefs.edit { putString(PreferenceKeys.GIT_EXTERNAL_REPO, repoPath) }
+                    if (fromIntent) {
+                        setResult(RESULT_OK)
+                        finish()
+                    }
 
                 }.launch(null)
             }

app/src/main/java/com/zeapo/pwdstore/git/GitOperationActivity.kt

@@ -20,13 +20,12 @@ open class GitOperationActivity : BaseGitActivity() {
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
-        when (intent.extras?.getInt(REQUEST_ARG_OP)) {
+        when (val reqCode = intent.extras?.getInt(REQUEST_ARG_OP)) {
             REQUEST_PULL -> lifecycleScope.launch { syncRepository(REQUEST_PULL) }
             REQUEST_PUSH -> lifecycleScope.launch { syncRepository(REQUEST_PUSH) }
             REQUEST_SYNC -> lifecycleScope.launch { syncRepository(REQUEST_SYNC) }
             else -> {
-                setResult(RESULT_CANCELED)
+                throw IllegalArgumentException("Invalid request code: $reqCode")
-                finish()
             }
         }
     }

app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt

@@ -17,9 +17,12 @@ import com.zeapo.pwdstore.git.config.ConnectionMode
 import com.zeapo.pwdstore.git.config.GitSettings
 import com.zeapo.pwdstore.git.config.Protocol
 import com.zeapo.pwdstore.utils.PasswordRepository
+import com.zeapo.pwdstore.utils.snackbar
 import com.zeapo.pwdstore.utils.viewBinding
 import java.io.IOException
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 
 /**
  * Activity that encompasses both the initial clone as well as editing the server config for future
@@ -125,8 +128,14 @@ class GitServerConfigActivity : BaseGitActivity() {
                 .setCancelable(false)
                 .setPositiveButton(R.string.dialog_delete) { dialog, _ ->
                     try {
-                        localDir.deleteRecursively()
+                        lifecycleScope.launch {
-                        lifecycleScope.launch { launchGitOperation(REQUEST_CLONE) }
+                            val snackbar = snackbar(message = getString(R.string.delete_directory_progress_text), length = Snackbar.LENGTH_INDEFINITE)
+                            withContext(Dispatchers.IO) {
+                                localDir.deleteRecursively()
+                            }
+                            snackbar.dismiss()
+                            launchGitOperation(REQUEST_CLONE)
+                        }
                     } catch (e: IOException) {
                         // TODO Handle the exception correctly if we are unable to delete the directory...
                         e.printStackTrace()

app/src/main/java/com/zeapo/pwdstore/utils/PasswordRepository.kt

@@ -169,7 +169,7 @@ open class PasswordRepository protected constructor() {
             val dir = getRepositoryDirectory()
             // uninitialize the repo if the dir does not exist or is absolutely empty
             settings.edit {
-                if (!dir.exists() || !dir.isDirectory || dir.listFiles()!!.isEmpty()) {
+                if (!dir.exists() || !dir.isDirectory || dir.listFiles()?.isEmpty() == true) {
                     putBoolean(PreferenceKeys.REPOSITORY_INITIALIZED, false)
                 } else {
                     putBoolean(PreferenceKeys.REPOSITORY_INITIALIZED, true)

app/src/main/res/values/strings.xml

@@ -27,6 +27,7 @@
         <item quantity="one">Are you sure you want to delete the password?</item>
         <item quantity="other">Are you sure you want to delete %d passwords?</item>
     </plurals>
+    <string name="delete_directory_progress_text">Deleting…</string>
     <string name="move">Move</string>
     <string name="edit">Edit</string>
     <string name="delete">Delete</string>