Skip to content

Conversation

@Vladyslav-Soldatenko
Copy link

Hi guys.
This is my proposal for #347 issue. Currently the two ways are supported: alphabetical and by CloudType enum. Since this action is kinda destructive (since the user could have their own sorting via DnD) there is confirmation modal with proper text. We can, of course, add asc/desc option, but I didn't want to complicate things initially as even such sorting is better than none. Feel free to leave comments
image
image

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2025

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link

coderabbitai bot commented Nov 1, 2025

Walkthrough

This pull request adds a vault sorting feature to the presentation layer. It introduces a new enum VaultListSortOption with NAME and LOCATION. VaultListPresenter gains a cachedVaults state, locale-aware nameComparator() and locationComparator(), and a public onSortOverrideConfirmed() to reorder, reindex, persist (via SaveVaultsUseCase), and refresh the view. A new SortOverrideConfirmationDialog presents radio options and a Callback; VaultListActivity implements the callback and delegates selection to the presenter. UI resources added: menu item, dialog layout, and strings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Presenter sorting logic and the cachedVaults lifecycle (population, refresh, potential staleness).
  • Integration of SortOverrideConfirmationDialog.Callback through VaultListActivity to the presenter (wiring and lifecycle).
  • Persistence flow in onSortOverrideConfirmed() (reindexing and SaveVaultsUseCase usage).
  • Comparator implementations for locale-aware name sorting and location sorting, including null/edge-case handling.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Option to sort vaults by vault name or cloud" directly aligns with the changeset's primary objective. The changes introduce a sorting feature that allows users to sort vaults by two criteria: vault name (alphabetically) and cloud location (by CloudType), which is exactly what the title conveys. The title is concise, specific, and not vague or generic, making it clear to reviewers scanning the history what the main change entails.
Description Check ✅ Passed The pull request description is clearly related to the changeset. It references issue #347, explicitly mentions the two supported sorting methods (alphabetical and by CloudType enum), explains the inclusion of a confirmation modal due to the destructive nature of the action, and provides design rationale for not including ascending/descending options initially. The description also includes screenshots demonstrating the UI implementation, which support understanding of the changes made throughout the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/SortOverrideConfirmationDialog.kt (1)

19-19: Remove redundant public modifier.

The public visibility modifier is redundant in Kotlin since it's the default for override methods.

Apply this diff:

-	public override fun setupDialog(builder: AlertDialog.Builder): android.app.Dialog {
+	override fun setupDialog(builder: AlertDialog.Builder): android.app.Dialog {
-	public override fun setupView() {
+	override fun setupView() {

Also applies to: 26-26

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49d6705 and e97d5c9.

📒 Files selected for processing (7)
  • presentation/src/main/java/org/cryptomator/presentation/model/VaultListSortOption.kt (1 hunks)
  • presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt (6 hunks)
  • presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt (4 hunks)
  • presentation/src/main/java/org/cryptomator/presentation/ui/dialog/SortOverrideConfirmationDialog.kt (1 hunks)
  • presentation/src/main/res/layout/dialog_sort_override_confirmation.xml (1 hunks)
  • presentation/src/main/res/menu/menu_vault_list.xml (1 hunks)
  • presentation/src/main/res/values/strings.xml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • presentation/src/main/java/org/cryptomator/presentation/ui/activity/VaultListActivity.kt
  • presentation/src/main/java/org/cryptomator/presentation/presenter/VaultListPresenter.kt
  • presentation/src/main/res/values/strings.xml
  • presentation/src/main/res/layout/dialog_sort_override_confirmation.xml
  • presentation/src/main/java/org/cryptomator/presentation/model/VaultListSortOption.kt
  • presentation/src/main/res/menu/menu_vault_list.xml
🔇 Additional comments (2)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/SortOverrideConfirmationDialog.kt (2)

1-17: LGTM! Clean dialog setup.

The dialog structure, imports, and callback interface are well-organized. The use of view binding and the callback pattern is idiomatic for Android dialogs.


31-41: LGTM! Robust selection handling.

The selectedSortOption() method provides a safe fallback to NAME, and the companion factory method follows standard DialogFragment patterns. Combined with the default selection in setupView(), this ensures a valid sort option is always available.

@JaniruTEC
Copy link
Contributor

Hey,
thank you for your PR! 😁
I can't say much about the code at this point, but I'd like to put two notes up for discussion:

  1. I think it'd be easier (as in less complexity, less dialogs and less destructive operations) to apply the sorting dynamically while populating the list view. The user could then pick between location/name/custom-sorting. Location/Name each would employ a comparator and disable DnD behavior while Custom would retain the current behavior.
  2. Building on that we could then switch to a less intrusive component such as menu.

@Vladyslav-Soldatenko
Copy link
Author

Hi, @JaniruTEC. Could you clarify one thing: when user selects "custom", do you want to just unlock DnD or you want to keep previous "custom" sorting saved somewhere when user selects "Location/Name" and restore it when users moves back to custom?

  • If we decide NOT to keep previous custom sorting when user moves to non-custom sort, than the operation is quite destructive still (and possible some warning won't hurt).
  • If we decide to keep previous custom sorting order, then it raises quite a lot of questions like "what if user has some custom sorting, moved to non-custom, deleted X vaults and added Y, where should they be placed when user moves back to custom order"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants