Improve bulk deletion and password move flow (#855)

Co-authored-by: Fabian Henneke <FabianHenneke@users.noreply.github.com>

CHANGELOG.md

@@ -20,6 +20,8 @@ All notable changes to this project will be documented in this file.
 - Reduced app size
 - Improve IME experience with server config screen
 - Removed edit password option from long-press menu.
+- Batch deletion now does not require manually confirming for each password
+- Better commit messages on password deletion
 
 ## [1.8.1] - 2020-05-24
 

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

@@ -34,7 +34,6 @@ import com.zeapo.pwdstore.utils.PasswordRepository
 import com.zeapo.pwdstore.utils.viewBinding
 import me.zhanghai.android.fastscroll.FastScrollerBuilder
 import java.io.File
-import java.util.Stack
 
 class PasswordFragment : Fragment(R.layout.password_recycler_view) {
     private lateinit var recyclerAdapter: PasswordItemRecyclerAdapter
@@ -160,21 +159,18 @@ class PasswordFragment : Fragment(R.layout.password_recycler_view) {
 
         // Called when the user selects a contextual menu item
         override fun onActionItemClicked(mode: ActionMode, item: MenuItem): Boolean {
-            when (item.itemId) {
+            return when (item.itemId) {
                 R.id.menu_delete_password -> {
-                    requireStore().deletePasswords(
-                        Stack<PasswordItem>().apply {
-                            recyclerAdapter.getSelectedItems(requireContext()).forEach { push(it) }
-                        }
-                    )
-                    mode.finish() // Action picked, so close the CAB
-                    return true
+                    requireStore().deletePasswords(recyclerAdapter.getSelectedItems(requireContext()))
+                    // Action picked, so close the CAB
+                    mode.finish()
+                    true
                 }
                 R.id.menu_move_password -> {
                     requireStore().movePasswords(recyclerAdapter.getSelectedItems(requireContext()))
-                    return false
+                    false
                 }
-                else -> return false
+                else -> false
             }
         }
 

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

@@ -17,11 +17,11 @@ import android.net.Uri
 import android.os.Build
 import android.os.Bundle
 import android.provider.Settings
-import android.text.TextUtils
 import android.view.KeyEvent
 import android.view.Menu
 import android.view.MenuItem
 import android.view.MenuItem.OnActionExpandListener
+import androidx.activity.result.contract.ActivityResultContracts.StartActivityForResult
 import androidx.activity.viewModels
 import androidx.appcompat.app.AppCompatActivity
 import androidx.appcompat.widget.AppCompatTextView
@@ -34,9 +34,9 @@ import androidx.core.content.getSystemService
 import androidx.fragment.app.FragmentManager
 import androidx.fragment.app.commit
 import androidx.lifecycle.ViewModelProvider
+import androidx.lifecycle.lifecycleScope
 import androidx.lifecycle.observe
 import androidx.preference.PreferenceManager
-import com.github.ajalt.timberkt.Timber.tag
 import com.github.ajalt.timberkt.d
 import com.github.ajalt.timberkt.e
 import com.github.ajalt.timberkt.i
@@ -66,12 +66,14 @@ import com.zeapo.pwdstore.utils.PasswordRepository.Companion.isInitialized
 import com.zeapo.pwdstore.utils.PasswordRepository.PasswordSortOrder.Companion.getSortOrder
 import com.zeapo.pwdstore.utils.commitChange
 import com.zeapo.pwdstore.utils.listFilesRecursively
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.api.errors.GitAPIException
 import org.eclipse.jgit.revwalk.RevCommit
 import java.io.File
 import java.lang.Character.UnicodeBlock
-import java.util.Stack
 
 class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
 
@@ -343,7 +345,7 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         } catch (e: Exception) {
             e.printStackTrace()
             if (!localDir.delete()) {
-                tag(TAG).d { "Failed to delete local repository" }
+                d { "Failed to delete local repository" }
             }
             return
         }
@@ -422,7 +424,7 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
 
     private fun checkLocalRepository(localDir: File?) {
         if (localDir != null && settings.getBoolean("repository_initialized", false)) {
-            tag(TAG).d { "Check, dir: ${localDir.absolutePath}" }
+            d { "Check, dir: ${localDir.absolutePath}" }
             // do not push the fragment if we already have it
             if (supportFragmentManager.findFragmentByTag("PasswordsList") == null ||
                 settings.getBoolean("repo_changed", false)) {
@@ -466,7 +468,7 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         val repoPath = getRepositoryDirectory(this)
         val repository = getRepository(repoPath)
         if (repository == null) {
-            tag(TAG).d { "getLastChangedTimestamp: No git repository" }
+            d { "getLastChangedTimestamp: No git repository" }
             return File(fullPath).lastModified()
         }
         val git = Git(repository)
@@ -475,11 +477,11 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         iterator = try {
             git.log().addPath(relativePath).call().iterator()
         } catch (e: GitAPIException) {
-            tag(TAG).e(e) { "getLastChangedTimestamp: GITAPIException" }
+            e(e) { "getLastChangedTimestamp: GITAPIException" }
             return -1
         }
         if (!iterator.hasNext()) {
-            tag(TAG).w { "getLastChangedTimestamp: No commits for file: $relativePath" }
+            w { "getLastChangedTimestamp: No commits for file: $relativePath" }
             return -1
         }
         return iterator.next().commitTime.toLong() * 1000
@@ -542,7 +544,7 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
     fun createPassword() {
         if (!validateState()) return
         val currentDir = currentDir
-        tag(TAG).i { "Adding file to : ${currentDir.absolutePath}" }
+        i { "Adding file to : ${currentDir.absolutePath}" }
         val intent = Intent(this, PasswordCreationActivity::class.java)
         intent.putExtra("FILE_PATH", currentDir.absolutePath)
         intent.putExtra("REPO_PATH", getRepositoryDirectory(applicationContext).absolutePath)
@@ -554,41 +556,112 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
         FolderCreationDialogFragment.newInstance(currentDir.path).show(supportFragmentManager, null)
     }
 
-    // deletes passwords in order from top to bottom
-    fun deletePasswords(selectedItems: Stack<PasswordItem>) {
-        if (selectedItems.isEmpty()) {
-            refreshPasswordList()
-            return
+    fun deletePasswords(selectedItems: List<PasswordItem>) {
+        var size = 0
+        selectedItems.forEach {
+            if (it.file.isFile)
+                size++
+            else
+                size += it.file.listFilesRecursively().size
         }
-        val item = selectedItems.pop()
         MaterialAlertDialogBuilder(this)
-            .setMessage(resources.getString(R.string.delete_dialog_text, item.longName))
+            .setMessage(resources.getQuantityString(R.plurals.delete_dialog_text, size, size))
             .setPositiveButton(resources.getString(R.string.dialog_yes)) { _, _ ->
-                val filesToDelete = if (item.file.isDirectory) {
-                    item.file.listFilesRecursively()
-                } else {
-                    listOf(item.file)
+                val filesToDelete = arrayListOf<File>()
+                selectedItems.forEach { item ->
+                    if (item.file.isDirectory)
+                        filesToDelete.addAll(item.file.listFilesRecursively())
+                    else
+                        filesToDelete.add(item.file)
                 }
+                selectedItems.map { item -> item.file.deleteRecursively() }
                 AutofillMatcher.updateMatches(applicationContext, delete = filesToDelete)
-                item.file.deleteRecursively()
-                commitChange(resources.getString(R.string.git_commit_remove_text, item.longName))
-                deletePasswords(selectedItems)
-            }
-            .setNegativeButton(resources.getString(R.string.dialog_no)) { _, _ ->
-                deletePasswords(selectedItems)
+                commitChange(resources.getString(R.string.git_commit_remove_text,
+                    selectedItems.joinToString(separator = ", ") { item ->
+                        item.file.toRelativeString(getRepositoryDirectory(this))
+                    }
+                ))
+                refreshPasswordList()
             }
+            .setNegativeButton(resources.getString(R.string.dialog_no), null)
             .show()
     }
 
     fun movePasswords(values: List<PasswordItem>) {
         val intent = Intent(this, SelectFolderActivity::class.java)
-        val fileLocations = ArrayList<String>()
-        for ((_, _, _, file) in values) {
-            fileLocations.add(file.absolutePath)
-        }
+        val fileLocations = values.map { it.file.absolutePath }.toTypedArray()
         intent.putExtra("Files", fileLocations)
         intent.putExtra(BaseGitActivity.REQUEST_ARG_OP, "SELECTFOLDER")
-        startActivityForResult(intent, REQUEST_CODE_SELECT_FOLDER)
+        registerForActivityResult(StartActivityForResult()) { result ->
+            val intentData = result.data ?: return@registerForActivityResult
+            val filesToMove = requireNotNull(intentData.getStringArrayExtra("Files"))
+            val target = File(requireNotNull(intentData.getStringExtra("SELECTED_FOLDER_PATH")))
+            val repositoryPath = getRepositoryDirectory(applicationContext).absolutePath
+            if (!target.isDirectory) {
+                e { "Tried moving passwords to a non-existing folder." }
+                return@registerForActivityResult
+            }
+
+            d { "Moving passwords to ${intentData.getStringExtra("SELECTED_FOLDER_PATH")}" }
+            d { filesToMove.joinToString(", ") }
+
+            lifecycleScope.launch(Dispatchers.IO) {
+                for (file in filesToMove) {
+                    val source = File(file)
+                    if (!source.exists()) {
+                        e { "Tried moving something that appears non-existent." }
+                        continue
+                    }
+                    val destinationFile = File(target.absolutePath + "/" + source.name)
+                    val basename = source.nameWithoutExtension
+                    val sourceLongName = getLongName(requireNotNull(source.parent), repositoryPath, basename)
+                    val destinationLongName = getLongName(target.absolutePath, repositoryPath, basename)
+                    if (destinationFile.exists()) {
+                        e { "Trying to move a file that already exists." }
+                        withContext(Dispatchers.Main) {
+                            MaterialAlertDialogBuilder(this@PasswordStore)
+                                .setTitle(resources.getString(R.string.password_exists_title))
+                                .setMessage(resources.getString(
+                                    R.string.password_exists_message,
+                                    destinationLongName,
+                                    sourceLongName)
+                                )
+                                .setPositiveButton(R.string.dialog_ok) { _, _ ->
+                                    launch(Dispatchers.IO) {
+                                        movePassword(source, destinationFile)
+                                    }
+                                }
+                                .setNegativeButton(R.string.dialog_cancel, null)
+                                .show()
+                        }
+                    } else {
+                        launch(Dispatchers.IO) {
+                            movePassword(source, destinationFile)
+                        }
+                    }
+                }
+                when (filesToMove.size) {
+                    1 -> {
+                        val source = File(filesToMove[0])
+                        val basename = source.nameWithoutExtension
+                        val sourceLongName = getLongName(requireNotNull(source.parent), repositoryPath, basename)
+                        val destinationLongName = getLongName(target.absolutePath, repositoryPath, basename)
+                        withContext(Dispatchers.Main) {
+                            commitChange(resources.getString(R.string.git_commit_move_text, sourceLongName, destinationLongName))
+                        }
+                    }
+                    else -> {
+                        withContext(Dispatchers.Main) {
+                            commitChange(resources.getString(R.string.git_commit_move_multiple_text,
+                                getRelativePath("${target.absolutePath}/", getRepositoryDirectory(applicationContext).absolutePath)
+                            ))
+                        }
+                    }
+                }
+            }
+            resetPasswordList()
+            plist?.dismissActionMode()
+        }.launch(intent)
     }
 
     /**
@@ -658,81 +731,32 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
                     intent.putExtra(BaseGitActivity.REQUEST_ARG_OP, BaseGitActivity.REQUEST_CLONE)
                     startActivityForResult(intent, BaseGitActivity.REQUEST_CLONE)
                 }
-                REQUEST_CODE_SELECT_FOLDER -> {
-                    val intentData = data ?: return
-                    tag(TAG).d {
-                        "Moving passwords to ${intentData.getStringExtra("SELECTED_FOLDER_PATH")}"
-                    }
-                    tag(TAG).d {
-                        TextUtils.join(", ", requireNotNull(intentData.getStringArrayListExtra("Files")))
-                    }
-
-                    val target = File(requireNotNull(intentData.getStringExtra("SELECTED_FOLDER_PATH")))
-                    val repositoryPath = getRepositoryDirectory(applicationContext).absolutePath
-                    if (!target.isDirectory) {
-                        tag(TAG).e { "Tried moving passwords to a non-existing folder." }
-                        return
-                    }
-
-                    // TODO move this to an async task
-                    for (fileString in requireNotNull(intentData.getStringArrayListExtra("Files"))) {
-                        val source = File(fileString)
-                        if (!source.exists()) {
-                            tag(TAG).e { "Tried moving something that appears non-existent." }
-                            continue
-                        }
-                        val destinationFile = File(target.absolutePath + "/" + source.name)
-                        val basename = source.nameWithoutExtension
-                        val sourceLongName = getLongName(requireNotNull(source.parent), repositoryPath, basename)
-                        val destinationLongName = getLongName(target.absolutePath, repositoryPath, basename)
-                        if (destinationFile.exists()) {
-                            e { "Trying to move a file that already exists." }
-                            MaterialAlertDialogBuilder(this)
-                                .setTitle(resources.getString(R.string.password_exists_title))
-                                .setMessage(resources.getString(
-                                    R.string.password_exists_message,
-                                    destinationLongName,
-                                    sourceLongName)
-                                )
-                                .setPositiveButton(R.string.dialog_ok) { _, _ ->
-                                    movePasswords(source, destinationFile, sourceLongName, destinationLongName)
-                                }
-                                .setNegativeButton(R.string.dialog_cancel, null)
-                                .show()
-                        } else {
-                            movePasswords(source, destinationFile, sourceLongName, destinationLongName)
-                        }
-                    }
-                    resetPasswordList()
-                    if (plist != null) {
-                        plist!!.dismissActionMode()
-                    }
-                }
             }
         }
         super.onActivityResult(requestCode, resultCode, data)
     }
 
-    private fun movePasswords(source: File, destinationFile: File, sourceLongName: String, destinationLongName: String) {
+    private suspend fun movePassword(source: File, destinationFile: File) {
         val sourceDestinationMap = if (source.isDirectory) {
             // Recursively list all files (not directories) below `source`, then
             // obtain the corresponding target file by resolving the relative path
             // starting at the destination folder.
-            val sourceFiles = source.listFilesRecursively()
-            sourceFiles.associateWith { destinationFile.resolve(it.relativeTo(source)) }
+            source.listFilesRecursively().associateWith { destinationFile.resolve(it.relativeTo(source)) }
         } else {
             mapOf(source to destinationFile)
         }
         if (!source.renameTo(destinationFile)) {
-            // TODO this should show a warning to the user
             e { "Something went wrong while moving." }
+            withContext(Dispatchers.Main) {
+                MaterialAlertDialogBuilder(this@PasswordStore)
+                    .setTitle(R.string.password_move_error_title)
+                    .setMessage(getString(R.string.password_move_error_message, source, destinationFile))
+                    .setCancelable(true)
+                    .setPositiveButton(android.R.string.ok, null)
+                    .show()
+            }
         } else {
             AutofillMatcher.updateMatches(this, sourceDestinationMap)
-            commitChange(resources
-                .getString(
-                    R.string.git_commit_move_text,
-                    sourceLongName,
-                    destinationLongName))
         }
     }
 
@@ -801,7 +825,6 @@ class PasswordStore : AppCompatActivity(R.layout.activity_pwdstore) {
     companion object {
         const val REQUEST_CODE_ENCRYPT = 9911
         const val REQUEST_CODE_DECRYPT_AND_VERIFY = 9913
-        const val REQUEST_CODE_SELECT_FOLDER = 9917
         const val REQUEST_ARG_PATH = "PATH"
         private val TAG = PasswordStore::class.java.name
         const val CLONE_REPO_BUTTON = 401

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

@@ -15,6 +15,7 @@ import android.view.View
 import android.view.autofill.AutofillManager
 import android.view.inputmethod.InputMethodManager
 import androidx.annotation.IdRes
+import androidx.annotation.MainThread
 import androidx.annotation.RequiresApi
 import androidx.appcompat.app.AlertDialog
 import androidx.core.content.getSystemService
@@ -73,6 +74,7 @@ fun Context.getEncryptedPrefs(fileName: String): SharedPreferences {
     )
 }
 
+@MainThread
 fun Activity.commitChange(message: String, finishWithResultOnEnd: Intent? = null) {
     if (!PasswordRepository.isGitRepo()) {
         if (finishWithResultOnEnd != null) {
@@ -99,6 +101,7 @@ fun Activity.commitChange(message: String, finishWithResultOnEnd: Intent? = null
  * view whose id is [id]. Solution based on a StackOverflow
  * answer: https://stackoverflow.com/a/13056259/297261
  */
+@MainThread
 fun <T : View> AlertDialog.requestInputFocusOnView(@IdRes id: Int) {
     setOnShowListener {
         findViewById<T>(id)?.apply {

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

@@ -10,7 +10,6 @@
     <string name="dialog_delete">حذف المجلد</string>
     <string name="dialog_do_not_delete">إلغاء</string>
     <string name="title_activity_git_clone">معلومات حول المستودع</string>
-    <string name="delete_dialog_text">هل تود حقًا حذف كلمة السر %1$s ؟</string>
     <string name="move">نقل</string>
     <string name="edit">تعديل</string>
     <string name="delete">حذف</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Informace repozitáře</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">Naklonujte nebo vytvořte nový repozitář před pokusem přidat heslo nebo spustit synchronizaci.</string>
-    <string name="delete_dialog_text">Opravdu chcete smazat heslo %1$s?</string>
     <string name="move">Přesunout</string>
     <string name="edit">Editovat</string>
     <string name="delete">Smazat</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Repository Informationen</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">Bitte klone oder erstelle ein neues Repository, bevor du versuchst ein Passwort hinzuzufügen oder jegliche Synchronisation-Operation durchführst.</string>
-    <string name="delete_dialog_text">Bist du dir sicher, dass du das Passwort löschen möchtest %1$s?</string>
     <string name="move">Verschieben</string>
     <string name="edit">Bearbeiten</string>
     <string name="delete">Löschen</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Información de repositorio</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">Por favor clona o crea un nuevo repositorio antes de añadir una contraseña o ejecutar una operación de sincronización.</string>
-    <string name="delete_dialog_text">Confirma que deseas eliminar la contraseña %1$s</string>
     <string name="move">Mover</string>
     <string name="edit">Editar</string>
     <string name="delete">Eliminar</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Information sur le dépôt Git</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">Clonez ou créez un dépôt suivant avant d\'essayer d\'ajouter un mot de pass ou d\'effectuer une opération de synchornisation.</string>
-    <string name="delete_dialog_text">Êtes-vous sûr de vouloir supprimer le mot de passe %1$s?</string>
     <string name="move">Déplacer</string>
     <string name="edit">Éditer</string>
     <string name="delete">Supprimer</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">リポジトリ情報</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">パスワードや同期操作を追加する前に、以下の新しいリポジトリをクローンまたは作成してください。</string>
-    <string name="delete_dialog_text">パスワードを削除してもよろしいですか %1$s</string>
     <string name="delete">削除</string>
     <!-- git commits -->
     <string name="git_commit_add_text">追加 %1$s ストアから。</string>

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

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Информация о репозитории</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">Пожалуйста, клонируйте или создайте новый репозиторий перед тем, как добавлять пароль или выполнять синхронизацию.</string>
-    <string name="delete_dialog_text">Вы уверены что хотите удалить пароль %1$s</string>
     <string name="move">Переместить</string>
     <string name="edit">Редактировать</string>
     <string name="delete">Удалить</string>

app/src/main/res/values-zh-rCN/strings.xml

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Repo 信息</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">在尝试添加密码或任何同步操作前请在下方克隆或添加一个新的 Repo</string>
-    <string name="delete_dialog_text">你确定要删除密码 %1$s</string>
     <string name="delete">删除</string>
     <!-- git commits -->
     <string name="git_commit_add_text">使用Android Password Store来添加 %1$s</string>

app/src/main/res/values-zh-rTW/strings.xml

@@ -13,7 +13,6 @@
     <string name="title_activity_git_clone">Repo 訊息</string>
     <!-- Password Store -->
     <string name="creation_dialog_text">在嘗試新增密碼或任何同步操作之前請在下方 clone 或新增一個新的 Repo</string>
-    <string name="delete_dialog_text">你確定要刪除密碼 %1$s</string>
     <string name="delete">刪除</string>
     <!-- PGPHandler -->
     <string name="provider_toast_text">未選擇提供 OpenPGP 的 app</string>

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

@@ -24,7 +24,10 @@
     <!-- Password Store -->
     <string name="creation_dialog_text">Please clone or create a new repository below before trying to add a password or running any synchronization operation.</string>
     <string name="key_dialog_text">A valid PGP key must be selected in Settings before initializing the repository</string>
-    <string name="delete_dialog_text">Are you sure you want to delete the password %1$s?</string>
+    <plurals name="delete_dialog_text">
+        <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="move">Move</string>
     <string name="edit">Edit</string>
     <string name="delete">Delete</string>
@@ -36,12 +39,15 @@
     <string name="no_key_selected_dialog_text">We will redirect you to settings. Please select your OpenPGP Key.</string>
     <string name="password_exists_title">Password already exists!</string>
     <string name="password_exists_message">This will overwrite %1$s with %2$s.</string>
+    <string name="password_move_error_title">Error while moving passwords</string>
+    <string name="password_move_error_message">Failed to move %1$s to %2$s</string>
 
     <!-- git commits -->
     <string name="git_commit_add_text">Add generated password for %1$s using Android Password Store.</string>
     <string name="git_commit_edit_text">Edit password for %1$s using Android Password Store.</string>
     <string name="git_commit_remove_text">Remove %1$s from store.</string>
     <string name="git_commit_move_text">Rename %1$s to %2$s.</string>
+    <string name="git_commit_move_multiple_text">Move multiple passwords to %1$s.</string>
 
     <!-- PGPHandler -->
     <string name="provider_toast_text">No OpenPGP provider selected!</string>