Improve search logic and UI (#703)

* Don't list the current directory in search results

* Scroll to top result when search term is changed

* Match relative path in StrictDomain filter mode

* Improve and document DirectoryStructure null handling

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

4 files changed, 94 insertions, 26 deletions

diff --git a/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt b/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
index f5d9b3ab..e4551f1c 100644
--- a/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/PasswordFragment.kt
@@ -124,10 +124,18 @@ class PasswordFragment : Fragment() {
             // and not on folder navigations since the latter leads to too many removal animations.
             (recyclerView.itemAnimator as OnOffItemAnimator).isEnabled = result.isFiltered
             recyclerAdapter.submitList(result.passwordItems) {
-                recyclerViewStateToRestore?.let {
-                    recyclerView.layoutManager!!.onRestoreInstanceState(it)
+                if (result.isFiltered) {
+                    // When the result is filtered, we always scroll to the top since that is where
+                    // the best fuzzy match appears.
+                    recyclerView.scrollToPosition(0)
+                } else {
+                    // When the result is not filtered and there is a saved scroll position for it,
+                    // we try to restore it.
+                    recyclerViewStateToRestore?.let {
+                        recyclerView.layoutManager!!.onRestoreInstanceState(it)
+                    }
+                    recyclerViewStateToRestore = null
                 }
-                recyclerViewStateToRestore = null
             }
         }
     }
diff --git a/app/src/main/java/com/zeapo/pwdstore/SearchableRepositoryViewModel.kt b/app/src/main/java/com/zeapo/pwdstore/SearchableRepositoryViewModel.kt
index 3e0dddd6..992a5f79 100644
--- a/app/src/main/java/com/zeapo/pwdstore/SearchableRepositoryViewModel.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/SearchableRepositoryViewModel.kt
@@ -39,6 +39,7 @@ import kotlinx.coroutines.FlowPreview
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.asFlow
 import kotlinx.coroutines.flow.distinctUntilChanged
+import kotlinx.coroutines.flow.drop
 import kotlinx.coroutines.flow.emptyFlow
 import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.map
@@ -94,11 +95,11 @@ private fun PasswordItem.Companion.makeComparator(
         PasswordRepository.PasswordSortOrder.FILE_FIRST -> compareByDescending { it.type }
     }
         .then(compareBy(nullsLast(CaseInsensitiveComparator)) {
-            directoryStructure.getIdentifierFor(
-                it.file
-            )
+            directoryStructure.getIdentifierFor(it.file)
+        })
+        .then(compareBy(CaseInsensitiveComparator) {
+            directoryStructure.getUsernameFor(it.file)
         })
-        .then(compareBy(CaseInsensitiveComparator) { directoryStructure.getUsernameFor(it.file) })
 }
 
 val PasswordItem.stableId: String
@@ -219,7 +220,8 @@ class SearchableRepositoryViewModel(application: Application) : AndroidViewModel
                 FilterMode.StrictDomain -> {
                     check(searchAction.listMode == ListMode.FilesOnly) { "Searches with StrictDomain search mode can only list files" }
                     prefilteredResultFlow
-                        .filter { file ->
+                        .filter { absoluteFile ->
+                            val file = absoluteFile.relativeTo(root)
                             val toMatch =
                                 directoryStructure.getIdentifierFor(file) ?: return@filter false
                             // In strict domain mode, we match
@@ -268,6 +270,8 @@ class SearchableRepositoryViewModel(application: Application) : AndroidViewModel
         return dir
             .walkTopDown().onEnter { file -> shouldTake(file) }
             .asFlow()
+            // Skip the root directory
+            .drop(1)
             .map {
                 yield()
                 it
diff --git a/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/AutofillPreferences.kt b/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/AutofillPreferences.kt
index bedb551d..39e1dbb5 100644
--- a/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/AutofillPreferences.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/AutofillPreferences.kt
@@ -19,32 +19,72 @@ enum class DirectoryStructure(val value: String) {
     FileBased("file"),
     DirectoryBased("directory");
 
-    fun getUsernameFor(file: File) = when (this) {
+    /**
+     * Returns the username associated to [file], following the convention of the current
+     * [DirectoryStructure].
+     *
+     * Examples:
+     *   - work/example.org/john@doe.org.gpg --> john@doe.org (FileBased)
+     *   - work/example.org/john@doe.org/password.gpg --> john@doe.org (DirectoryBased)
+     *   - Temporary PIN.gpg --> Temporary PIN (DirectoryBased, fallback)
+     */
+    fun getUsernameFor(file: File): String = when (this) {
         FileBased -> file.nameWithoutExtension
         DirectoryBased -> file.parentFile?.name ?: file.nameWithoutExtension
     }
 
-    fun getIdentifierFor(file: File) = when (this) {
-        FileBased -> file.parentFile.name
-        DirectoryBased -> file.parentFile.parentFile?.name
+    /**
+     * Returns the origin identifier associated to [file], following the convention of the current
+     * [DirectoryStructure].
+     *
+     * At least one of [DirectoryStructure.getIdentifierFor] and
+     * [DirectoryStructure.getAccountPartFor] will always return a non-null result.
+     *
+     * Examples:
+     *   - work/example.org/john@doe.org.gpg --> example.org (FileBased)
+     *   - example.org.gpg --> example.org (FileBased, fallback)
+     *   - work/example.org/john@doe.org/password.gpg --> example.org (DirectoryBased)
+     *   - Temporary PIN.gpg --> null (DirectoryBased)
+     */
+    fun getIdentifierFor(file: File): String? = when (this) {
+        FileBased -> file.parentFile?.name ?: file.nameWithoutExtension
+        DirectoryBased -> file.parentFile?.parent
     }
 
     /**
      * Returns the path components of [file] until right before the component that contains the
      * origin identifier according to the current [DirectoryStructure].
      *
+     * At least one of [DirectoryStructure.getIdentifierFor] and
+     * [DirectoryStructure.getAccountPartFor] will always return a non-null result.
+     *
      * Examples:
-     *   - /work/example.org/john@doe.org --> /work (FileBased)
-     *   - /work/example.org/john@doe.org/password --> /work (DirectoryBased)
+     *   - work/example.org/john@doe.org.gpg --> work (FileBased)
+     *   - example.org/john@doe.org.gpg --> null (FileBased)
+     *   - john@doe.org.gpg --> null (FileBased)
+     *   - work/example.org/john@doe.org/password.gpg --> work (DirectoryBased)
+     *   - example.org/john@doe.org/password.gpg --> null (DirectoryBased)
      */
-    fun getPathToIdentifierFor(file: File) = when (this) {
-        FileBased -> file.parentFile.parent
-        DirectoryBased -> file.parentFile.parentFile?.parent
+    fun getPathToIdentifierFor(file: File): String? = when (this) {
+        FileBased -> file.parentFile?.parent
+        DirectoryBased -> file.parentFile?.parentFile?.parent
     }
 
-    fun getAccountPartFor(file: File) = when (this) {
-        FileBased -> file.nameWithoutExtension
-        DirectoryBased -> "${file.parentFile.name}/${file.nameWithoutExtension}"
+    /**
+     * Returns the path component of [file] following the origin identifier according to the current
+     * [DirectoryStructure] (without file extension).
+     *
+     * Examples:
+     *   - work/example.org/john@doe.org.gpg --> john@doe.org (FileBased)
+     *   - example.org.gpg --> null (FileBased, fallback)
+     *   - work/example.org/john@doe.org/password.gpg --> john@doe.org/password (DirectoryBased)
+     *   - Temporary PIN.gpg --> Temporary PIN (DirectoryBased, fallback)
+     */
+    fun getAccountPartFor(file: File): String? = when (this) {
+        FileBased -> file.nameWithoutExtension.takeIf { file.parentFile != null }
+        DirectoryBased -> file.parentFile?.let { parentFile ->
+            "${parentFile.name}/${file.nameWithoutExtension}"
+        } ?: file.nameWithoutExtension
     }
 
     @RequiresApi(Build.VERSION_CODES.O)
diff --git a/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/ui/AutofillFilterActivity.kt b/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/ui/AutofillFilterActivity.kt
index aee7f1a9..0901f8d9 100644
--- a/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/ui/AutofillFilterActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/autofill/oreo/ui/AutofillFilterActivity.kt
@@ -11,6 +11,7 @@ import android.content.Intent
 import android.content.IntentSender
 import android.os.Build
 import android.os.Bundle
+import android.view.View
 import android.view.autofill.AutofillManager
 import android.widget.TextView
 import androidx.activity.viewModels
@@ -119,13 +120,26 @@ class AutofillFilterView : AppCompatActivity() {
             ::PasswordViewHolder) { item ->
             val file = item.file.relativeTo(item.rootDir)
             val pathToIdentifier = directoryStructure.getPathToIdentifierFor(file)
-            val identifier = directoryStructure.getIdentifierFor(file) ?: "INVALID"
+            val identifier = directoryStructure.getIdentifierFor(file)
             val accountPart = directoryStructure.getAccountPartFor(file)
-            title.text = buildSpannedString {
-                pathToIdentifier?.let { append("$it/") }
-                bold { underline { append(identifier) } }
+            check(identifier != null || accountPart != null) { "At least one of identifier and accountPart should always be non-null" }
+            title.text = if (identifier != null) {
+                buildSpannedString {
+                    if (pathToIdentifier != null)
+                        append("$pathToIdentifier/")
+                    bold { underline { append(identifier) } }
+                }
+            } else {
+                accountPart
+            }
+            subtitle.apply {
+                if (identifier != null && accountPart != null) {
+                    text = accountPart
+                    visibility = View.VISIBLE
+                } else {
+                    visibility = View.GONE
+                }
             }
-            subtitle.text = accountPart
         }.onItemClicked { _, item ->
             decryptAndFill(item)
         }
@@ -154,7 +168,9 @@ class AutofillFilterView : AppCompatActivity() {
         }
         model.searchResult.observe(this) { result ->
             val list = result.passwordItems
-            recyclerAdapter.submitList(list)
+            recyclerAdapter.submitList(list) {
+                binding.rvPassword.scrollToPosition(0)
+            }
             // Switch RecyclerView out for a "no results" message if the new list is empty and
             // the message is not yet shown (and vice versa).
             if ((list.isEmpty() && binding.rvPasswordSwitcher.nextView.id == binding.rvPasswordEmpty.id) ||