Add tentative workaround for dialog crashes and refactor Git-related code (#1100)

* Ensure we're creating dialogs on the main thread

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

* Remove unused operation type

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

* Refactor launchGitOperation to use an enum

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

7 files changed, 58 insertions, 63 deletions

diff --git a/app/src/main/java/com/zeapo/pwdstore/OnboardingActivity.kt b/app/src/main/java/com/zeapo/pwdstore/OnboardingActivity.kt
index 61bba1cd..45bf1d1c 100644
--- a/app/src/main/java/com/zeapo/pwdstore/OnboardingActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/OnboardingActivity.kt
@@ -17,7 +17,6 @@ import com.github.michaelbull.result.onFailure
 import com.github.michaelbull.result.runCatching
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import com.zeapo.pwdstore.databinding.ActivityOnboardingBinding
-import com.zeapo.pwdstore.git.BaseGitActivity
 import com.zeapo.pwdstore.git.GitServerConfigActivity
 import com.zeapo.pwdstore.utils.PasswordRepository
 import com.zeapo.pwdstore.utils.PreferenceKeys
@@ -92,9 +91,7 @@ class OnboardingActivity : AppCompatActivity() {
      */
     private fun cloneToHiddenDir() {
         settings.edit { putBoolean(PreferenceKeys.GIT_EXTERNAL, false) }
-        cloneAction.launch(Intent(this, GitServerConfigActivity::class.java).apply {
-            putExtra(BaseGitActivity.REQUEST_ARG_OP, BaseGitActivity.REQUEST_CLONE)
-        })
+        cloneAction.launch(GitServerConfigActivity.createCloneIntent(this))
     }
 
     /**
diff --git a/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt b/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
index 013a922d..7c1dae9b 100644
--- a/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
@@ -5,7 +5,6 @@
 package com.zeapo.pwdstore
 
 import android.content.Context
-import android.content.Intent
 import android.content.SharedPreferences
 import android.os.Bundle
 import android.os.Parcelable
@@ -90,9 +89,7 @@ class PasswordFragment : Fragment(R.layout.password_recycler_view) {
             } else if (!PasswordRepository.isGitRepo()) {
                 Snackbar.make(binding.root, getString(R.string.clone_git_repo), Snackbar.LENGTH_INDEFINITE)
                     .setAction(R.string.clone_button) {
-                        val intent = Intent(context, GitServerConfigActivity::class.java)
-                        intent.putExtra(BaseGitActivity.REQUEST_ARG_OP, BaseGitActivity.REQUEST_CLONE)
-                        swipeResult.launch(intent)
+                        swipeResult.launch(GitServerConfigActivity.createCloneIntent(requireContext()))
                     }
                     .show()
                 binding.swipeRefresher.isRefreshing = false
@@ -100,8 +97,8 @@ class PasswordFragment : Fragment(R.layout.password_recycler_view) {
                 // When authentication is set to AuthMode.None then the only git operation we can
                 // run is a pull, so automatically fallback to that.
                 val operationId = when (GitSettings.authMode) {
-                    AuthMode.None -> BaseGitActivity.REQUEST_PULL
-                    else -> BaseGitActivity.REQUEST_SYNC
+                    AuthMode.None -> BaseGitActivity.GitOp.PULL
+                    else -> BaseGitActivity.GitOp.SYNC
                 }
                 requireStore().apply {
                     lifecycleScope.launch {
diff --git a/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt b/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt
index 7501146a..93dfad56 100644
--- a/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/PasswordStore.kt
@@ -272,7 +272,7 @@ class PasswordStore : BaseGitActivity() {
                     initBefore.show()
                     return false
                 }
-                runGitOperation(REQUEST_PUSH)
+                runGitOperation(GitOp.PUSH)
                 return true
             }
             R.id.git_pull -> {
@@ -280,7 +280,7 @@ class PasswordStore : BaseGitActivity() {
                     initBefore.show()
                     return false
                 }
-                runGitOperation(REQUEST_PULL)
+                runGitOperation(GitOp.PULL)
                 return true
             }
             R.id.git_sync -> {
@@ -288,7 +288,7 @@ class PasswordStore : BaseGitActivity() {
                     initBefore.show()
                     return false
                 }
-                runGitOperation(REQUEST_SYNC)
+                runGitOperation(GitOp.SYNC)
                 return true
             }
             R.id.refresh -> {
@@ -312,10 +312,10 @@ class PasswordStore : BaseGitActivity() {
             searchItem.collapseActionView()
     }
 
-    private fun runGitOperation(operation: Int) = lifecycleScope.launch {
+    private fun runGitOperation(operation: GitOp) = lifecycleScope.launch {
         launchGitOperation(operation).fold(
             success = { refreshPasswordList() },
-            failure = ::promptOnErrorHandler
+            failure = { promptOnErrorHandler(it) },
         )
     }
 
@@ -520,7 +520,6 @@ class PasswordStore : BaseGitActivity() {
         val intent = Intent(this, SelectFolderActivity::class.java)
         val fileLocations = values.map { it.file.absolutePath }.toTypedArray()
         intent.putExtra("Files", fileLocations)
-        intent.putExtra(REQUEST_ARG_OP, "SELECTFOLDER")
         registerForActivityResult(StartActivityForResult()) { result ->
             val intentData = result.data ?: return@registerForActivityResult
             val filesToMove = requireNotNull(intentData.getStringArrayExtra("Files"))
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/BaseGitActivity.kt b/app/src/main/java/com/zeapo/pwdstore/git/BaseGitActivity.kt
index 19413efd..6a975f6c 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/BaseGitActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/BaseGitActivity.kt
@@ -6,9 +6,7 @@ package com.zeapo.pwdstore.git
 
 import androidx.appcompat.app.AppCompatActivity
 import androidx.core.content.edit
-import com.github.ajalt.timberkt.Timber.tag
 import com.github.ajalt.timberkt.d
-import com.github.ajalt.timberkt.e
 import com.github.michaelbull.result.Err
 import com.github.michaelbull.result.Result
 import com.github.michaelbull.result.andThen
@@ -18,7 +16,6 @@ import com.zeapo.pwdstore.R
 import com.zeapo.pwdstore.git.config.GitSettings
 import com.zeapo.pwdstore.git.operation.BreakOutOfDetached
 import com.zeapo.pwdstore.git.operation.CloneOperation
-import com.zeapo.pwdstore.git.operation.GitOperation
 import com.zeapo.pwdstore.git.operation.PullOperation
 import com.zeapo.pwdstore.git.operation.PushOperation
 import com.zeapo.pwdstore.git.operation.ResetToRemoteOperation
@@ -26,6 +23,8 @@ import com.zeapo.pwdstore.git.operation.SyncOperation
 import com.zeapo.pwdstore.utils.PreferenceKeys
 import com.zeapo.pwdstore.utils.getEncryptedPrefs
 import com.zeapo.pwdstore.utils.sharedPrefs
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.withContext
 import net.schmizz.sshj.common.DisconnectReason
 import net.schmizz.sshj.common.SSHException
 import net.schmizz.sshj.userauth.UserAuthException
@@ -37,29 +36,37 @@ import net.schmizz.sshj.userauth.UserAuthException
 abstract class BaseGitActivity : AppCompatActivity() {
 
     /**
+     * Enum of possible Git operations than can be run through [launchGitOperation].
+     */
+    enum class GitOp {
+        BREAK_OUT_OF_DETACHED,
+        CLONE,
+        PULL,
+        PUSH,
+        RESET,
+        SYNC,
+    }
+
+    /**
      * Attempt to launch the requested Git operation.
      * @param operation The type of git operation to launch
      */
-    suspend fun launchGitOperation(operation: Int): Result<Unit, Throwable> {
+    suspend fun launchGitOperation(operation: GitOp): Result<Unit, Throwable> {
         if (GitSettings.url == null) {
             return Err(IllegalStateException("Git url is not set!"))
         }
-        if (operation == REQUEST_SYNC && !GitSettings.useMultiplexing) {
+        if (operation == GitOp.SYNC && !GitSettings.useMultiplexing) {
             // If the server does not support multiple SSH channels per connection, we cannot run
             // a sync operation without reconnecting and thus break sync into its two parts.
-            return launchGitOperation(REQUEST_PULL).andThen { launchGitOperation(REQUEST_PUSH) }
+            return launchGitOperation(GitOp.PULL).andThen { launchGitOperation(GitOp.PUSH) }
         }
         val op = when (operation) {
-            REQUEST_CLONE, GitOperation.GET_SSH_KEY_FROM_CLONE -> CloneOperation(this, GitSettings.url!!)
-            REQUEST_PULL -> PullOperation(this)
-            REQUEST_PUSH -> PushOperation(this)
-            REQUEST_SYNC -> SyncOperation(this)
-            BREAK_OUT_OF_DETACHED -> BreakOutOfDetached(this)
-            REQUEST_RESET -> ResetToRemoteOperation(this)
-            else -> {
-                tag(TAG).e { "Operation not recognized : $operation" }
-                return Err(IllegalArgumentException("$operation is not a valid Git operation"))
-            }
+            GitOp.CLONE -> CloneOperation(this, GitSettings.url!!)
+            GitOp.PULL -> PullOperation(this)
+            GitOp.PUSH -> PushOperation(this)
+            GitOp.SYNC -> SyncOperation(this)
+            GitOp.BREAK_OUT_OF_DETACHED -> BreakOutOfDetached(this)
+            GitOp.RESET -> ResetToRemoteOperation(this)
         }
         return op.executeAfterAuthentication(GitSettings.authMode).mapError { throwable ->
             val err = rootCauseException(throwable)
@@ -76,7 +83,7 @@ abstract class BaseGitActivity : AppCompatActivity() {
         finish()
     }
 
-    fun promptOnErrorHandler(err: Throwable, onPromptDone: () -> Unit = {}) {
+    suspend fun promptOnErrorHandler(err: Throwable, onPromptDone: () -> Unit = {}) {
         val error = rootCauseException(err)
         if (!isExplicitlyUserInitiatedError(error)) {
             getEncryptedPrefs("git_operation").edit {
@@ -84,14 +91,16 @@ abstract class BaseGitActivity : AppCompatActivity() {
             }
             sharedPrefs.edit { remove(PreferenceKeys.SSH_OPENKEYSTORE_KEYID) }
             d(error)
-            MaterialAlertDialogBuilder(this).run {
-                setTitle(resources.getString(R.string.jgit_error_dialog_title))
-                setMessage(ErrorMessages[error])
-                setPositiveButton(resources.getString(R.string.dialog_ok)) { _, _ -> }
-                setOnDismissListener {
-                    onPromptDone()
+            withContext(Dispatchers.Main) {
+                MaterialAlertDialogBuilder(this@BaseGitActivity).run {
+                    setTitle(resources.getString(R.string.jgit_error_dialog_title))
+                    setMessage(ErrorMessages[error])
+                    setPositiveButton(resources.getString(R.string.dialog_ok)) { _, _ -> }
+                    setOnDismissListener {
+                        onPromptDone()
+                    }
+                    show()
                 }
-                show()
             }
         } else {
             onPromptDone()
@@ -130,16 +139,4 @@ abstract class BaseGitActivity : AppCompatActivity() {
         }
         return rootCause
     }
-
-    companion object {
-
-        const val REQUEST_ARG_OP = "OPERATION"
-        const val REQUEST_PULL = 101
-        const val REQUEST_PUSH = 102
-        const val REQUEST_CLONE = 103
-        const val REQUEST_SYNC = 104
-        const val BREAK_OUT_OF_DETACHED = 105
-        const val REQUEST_RESET = 106
-        const val TAG = "AbstractGitActivity"
-    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/GitConfigActivity.kt b/app/src/main/java/com/zeapo/pwdstore/git/GitConfigActivity.kt
index 49abc92d..752a6b7a 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/GitConfigActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/GitConfigActivity.kt
@@ -91,7 +91,7 @@ class GitConfigActivity : BaseGitActivity() {
         }
         binding.gitAbortRebase.setOnClickListener {
             lifecycleScope.launch {
-                launchGitOperation(BREAK_OUT_OF_DETACHED).fold(
+                launchGitOperation(GitOp.BREAK_OUT_OF_DETACHED).fold(
                   success = {
                       MaterialAlertDialogBuilder(this@GitConfigActivity).run {
                           setTitle(resources.getString(R.string.git_abort_and_push_title))
@@ -115,7 +115,7 @@ class GitConfigActivity : BaseGitActivity() {
         }
         binding.gitResetToRemote.setOnClickListener {
             lifecycleScope.launch {
-                launchGitOperation(REQUEST_RESET).fold(
+                launchGitOperation(GitOp.RESET).fold(
                   success = ::finishOnSuccessHandler,
                   failure = { err ->
                       promptOnErrorHandler(err) {
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt b/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
index 4bbc5965..92381384 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
@@ -4,6 +4,8 @@
  */
 package com.zeapo.pwdstore.git
 
+import android.content.Context
+import android.content.Intent
 import android.os.Bundle
 import android.os.Handler
 import android.view.MenuItem
@@ -42,7 +44,7 @@ class GitServerConfigActivity : BaseGitActivity() {
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
-        val isClone = intent?.extras?.getInt(REQUEST_ARG_OP) ?: -1 == REQUEST_CLONE
+        val isClone = intent?.extras?.getBoolean("cloning") ?: false
         if (isClone) {
             binding.saveButton.text = getString(R.string.clone_button)
         }
@@ -151,7 +153,7 @@ class GitServerConfigActivity : BaseGitActivity() {
                                 localDir.deleteRecursively()
                             }
                             snackbar.dismiss()
-                            launchGitOperation(REQUEST_CLONE).fold(
+                            launchGitOperation(GitOp.CLONE).fold(
                                 success = {
                                     setResult(RESULT_OK)
                                     finish()
@@ -185,14 +187,22 @@ class GitServerConfigActivity : BaseGitActivity() {
                 MaterialAlertDialogBuilder(this).setMessage(e.message).show()
             }
             lifecycleScope.launch {
-                launchGitOperation(REQUEST_CLONE).fold(
+                launchGitOperation(GitOp.CLONE).fold(
                     success = {
                         setResult(RESULT_OK)
                         finish()
                     },
-                    failure = ::promptOnErrorHandler,
+                    failure = { promptOnErrorHandler(it) },
                 )
             }
         }
     }
+
+    companion object {
+        fun createCloneIntent(context: Context): Intent {
+            return Intent(context, GitServerConfigActivity::class.java).apply {
+                putExtra("cloning", true)
+            }
+        }
+    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
index 85c38c6a..d7ff7c4c 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
@@ -202,9 +202,4 @@ abstract class GitOperation(protected val callingActivity: FragmentActivity) {
             sshSessionFactory?.close()
         }
     }
-
-    companion object {
-
-        const val GET_SSH_KEY_FROM_CLONE = 201
-    }
 }