feat: shortcuts customization #1385
Conversation
WalkthroughThis update introduces a comprehensive refactor of the wallet's shortcut system. The legacy imperative Android View-based shortcut buttons and pane have been replaced with a modern, composable, and customizable shortcut bar using Jetpack Compose. The new system supports dynamic shortcut management via a dedicated view model and provider, adds new shortcut options, enables shortcut replacement, and updates related resources, icons, and navigation. Several unused or redundant files and resources were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletFragment
participant ShortcutsViewModel
participant ShortcutProvider
participant ComposeUI
User->>WalletFragment: Tap or long-tap shortcut
WalletFragment->>ShortcutsViewModel: onShortcutTap/onShortcutLongTap
ShortcutsViewModel->>ShortcutProvider: (if needed) setCustomShortcuts/getFilteredShortcuts
ShortcutsViewModel->>ComposeUI: Update shortcut list state
ComposeUI->>User: Display updated shortcuts / Show replacement dialog
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (17)
wallet/res/values/strings.xml (1)
423-426: Ensure new “select_option” string is localized and key is descriptiveThe
select_optionstring supports the new shortcuts UI but isn’t present in other locale folders, so it will fall back to English. Verify that translations are added to allvalues-xxresource directories. Additionally, consider renaming this key toshortcuts_select_optionto clearly scope it for the shortcuts feature and avoid potential naming conflicts.wallet/res/layout/dialog_compose_container.xml (1)
21-26: Add contentDescription for collapse button
TheImageButtonwith IDcollapse_buttonlacks anandroid:contentDescription, which is required for accessibility. Consider adding a description, for example:<ImageButton android:id="@+id/collapse_button" style="@style/DialogCloseButton" + android:contentDescription="@string/dialog_close_description" />Then define
dialog_close_descriptionin yourstrings.xml.common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt (1)
40-46: Add documentation for clarity
Consider adding a KDoc comment aboveOverlineto describe its intended use (e.g., for shortcut labels) and distinguish it fromOverlineSemibold.common/src/main/java/org/dash/wallet/common/data/WalletUIConfig.kt (1)
68-68: Add preference key for customized shortcutsIntroducing
CUSTOMIZED_SHORTCUTSaligns perfectly with the newShortcutProvider. Ensure the code that reads this key handles an unset value (e.g. falls back to default shortcuts), and consider adding unit tests for get/set operations on this preference key.wallet/res/drawable/ic_shortcut_secure_now.xml (1)
2-5: Ensure icon sizing consistency across shortcuts.
This icon uses 37×36 dp while others are 24×24 dp. Consider standardizing all shortcut icons to the same artboard size (e.g. 24×24 dp) to avoid manual scaling in Compose.wallet/res/drawable/ic_where_to_spend.xml (1)
7-11: Consider enabling dynamic theming via tinting.
Currently the fill is hard‐coded to#008DE4. If you want the icon to adapt to light/dark or theme colors, removeandroid:fillColorhere and applytintfrom Compose or useandroid:tint="?attr/colorControlNormal".wallet/res/drawable/ic_shortcut_buy_sell_dash.xml (1)
2-5: Align icon dimensions with design system.
This vector is 24×20 dp—other shortcuts use 24×24 dp. Consider standardizing to 24×24 dp (with appropriate viewBox) for consistent layout in the shortcuts pane.wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsPane.kt (1)
59-60: Consider handling edge cases for empty shortcuts list.The weight calculation assumes the shortcuts list is non-empty. If the list is empty, this would lead to a division by zero.
-val itemWidth = 1f / shortcuts.size +val itemWidth = if (shortcuts.isEmpty()) 1f else 1f / shortcuts.sizewallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsList.kt (1)
84-93: Preview composable aids in development.The preview function is a great addition for visualizing the component during development. However, you might want to add different preview variants (e.g., with fewer items, dark theme, etc.) to better test the component's appearance under various conditions.
@Preview(showBackground = true) @Composable fun PreviewShortcutsList() { val shortcuts = ShortcutOption.entries ShortcutsList( shortcuts = shortcuts, onClick = {} ) } +@Preview(showBackground = true) +@Composable +fun PreviewShortcutsListFewItems() { + val shortcuts = ShortcutOption.entries.take(3) + + ShortcutsList( + shortcuts = shortcuts, + onClick = {} + ) +}wallet/src/de/schildbach/wallet/ui/main/shortcuts/Shortcut.kt (1)
64-70: Good inclusion of a preview composable.The preview will help with UI development and testing. Consider adding a dark theme preview variant to ensure the component works well with both light and dark themes.
+@Preview(showBackground = true, name = "Light Theme") +@Preview(showBackground = true, uiMode = Configuration.UI_MODE_NIGHT_YES, name = "Dark Theme") @Composable fun PreviewShortcutPaneItem() { Shortcut( shortcutOption = ShortcutOption.RECEIVE ) }wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutListItem.kt (1)
78-85: Add dark theme preview for comprehensive testing.Like with the Shortcut component, consider adding a dark theme preview to ensure proper theming.
+@Preview(showBackground = true, name = "Light Theme") +@Preview(showBackground = true, uiMode = Configuration.UI_MODE_NIGHT_YES, name = "Dark Theme") @Composable fun PreviewShortcutListItem() { ShortcutListItem( shortcutOption = ShortcutOption.RECEIVE, onClick = {} ) }wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt (2)
24-24: Remove unused import to keep the file lint-clean
collectis never referenced in this file. Keeping unused imports will trigger detekt/ktlint warnings and hinders readability.-import kotlinx.coroutines.flow.collect
98-103: De-duplicate IDs when parsingIf the stored string accidentally contains duplicates (
1,2,2,3), the current logic preserves them, producing a list longer thanMINIMUM_SHORTCUTSand breaking the UI order in subtle ways.-return shortcutString.split(",").mapNotNull { … } +return shortcutString + .split(",") + .mapNotNull { it.toIntOrNull()?.let(ShortcutOption::fromId) } + .distinct()wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt (2)
47-51:hasCustomShortcutsis not reactive – consider backing it with aStateFlow
hasCustomShortcutsreads the currentvalueof the provider’s state-flow, but it does not update when the flow emits later.
Consequently, if the user customises shortcuts afterShortcutsViewModelis created, the setters foruserHasBalance/userHasContactswill still behave as if no custom list exists.Simplest fix: convert it to
val hasCustomShortcuts: Boolean get() = shortcutProvider.customShortcuts.value.isNotEmpty()OR expose
shortcutProvider.customShortcutsdirectly and usecombineto derive state.
57-61: Setter recalculates shortcuts on every balance tick – debounce or check equality
userHasBalanceis assigned on everytotalBalanceemission inWalletFragment.
When the balance fluctuates (e.g. syncing headers), this setter is triggered many times, causing redundantgetPresetShortcuts()and recompositions.Consider guarding with
if (_userHasBalance != value) { … }or moving the balance flow directly into the ViewModel and calling
distinctUntilChanged().wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt (2)
449-453: Add basic error handling for the Topper URL fetchIf the network call fails (or Topper is unavailable) the coroutine will throw, and the lambda will silently cancel without user feedback.
lifecycleScope.launch { runCatching { shortcutViewModel.getTopperUrl(getString(R.string.dash_wallet_name)) }.onSuccess { uri -> requireActivity().openCustomTab(uri) }.onFailure { err -> log.warn("Topper URL fetch failed", err) Snackbar.make(binding.root, R.string.generic_network_error, LENGTH_LONG).show() } }
391-465:whenblock is becoming long – consider extracting into dedicated handlersThe 60-line
whencascades mix logging, navigation, and I/O.
Extracting each case into a private method (or usingMap<ShortcutOption, suspend () -> Unit>) will:
- improve readability,
- make it easier to unit-test individual actions,
- keep WalletFragment focused on UI orchestration.
This is optional but will reduce future merge conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
wallet/testNet3/res/drawable-xhdpi/currency_symbol_mdash.pngis excluded by!**/*.pngwallet/testNet3/res/drawable-xhdpi/currency_symbol_udash.pngis excluded by!**/*.png
📒 Files selected for processing (75)
common/src/main/java/org/dash/wallet/common/data/WalletUIConfig.kt(1 hunks)common/src/main/java/org/dash/wallet/common/services/analytics/AnalyticsConstants.kt(1 hunks)common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt(1 hunks)common/src/main/java/org/dash/wallet/common/ui/receive/ReceiveInfoView.kt(1 hunks)common/src/main/java/org/dash/wallet/common/util/GenericUtils.java(0 hunks)wallet/res/drawable/ic_arrow_drop_down_blue.xml(0 hunks)wallet/res/drawable/ic_arrow_drop_up_blue.xml(0 hunks)wallet/res/drawable/ic_arrow_icon.xml(0 hunks)wallet/res/drawable/ic_avatar_blue.xml(0 hunks)wallet/res/drawable/ic_balance_warning.xml(0 hunks)wallet/res/drawable/ic_blue_clock.xml(0 hunks)wallet/res/drawable/ic_contact.xml(1 hunks)wallet/res/drawable/ic_explore.xml(1 hunks)wallet/res/drawable/ic_receive_payments.xml(0 hunks)wallet/res/drawable/ic_send_to_address.xml(1 hunks)wallet/res/drawable/ic_shortcut_atm.xml(1 hunks)wallet/res/drawable/ic_shortcut_buy_sell_dash.xml(1 hunks)wallet/res/drawable/ic_shortcut_receive.xml(0 hunks)wallet/res/drawable/ic_shortcut_secure_now.xml(1 hunks)wallet/res/drawable/ic_shortcut_staking.xml(1 hunks)wallet/res/drawable/ic_where_to_spend.xml(1 hunks)wallet/res/layout/dialog_compose_container.xml(1 hunks)wallet/res/layout/home_content.xml(1 hunks)wallet/res/layout/shortcut_button.xml(1 hunks)wallet/res/navigation/nav_home.xml(1 hunks)wallet/res/values-ar/strings-extra.xml(1 hunks)wallet/res/values-bg/strings-extra.xml(1 hunks)wallet/res/values-cs/strings-extra.xml(1 hunks)wallet/res/values-de/strings-extra.xml(1 hunks)wallet/res/values-el/strings-extra.xml(1 hunks)wallet/res/values-es/strings-extra.xml(1 hunks)wallet/res/values-fa/strings-extra.xml(1 hunks)wallet/res/values-fil/strings-extra.xml(1 hunks)wallet/res/values-fr/strings-extra.xml(1 hunks)wallet/res/values-he/strings.xml(0 hunks)wallet/res/values-id/strings-extra.xml(1 hunks)wallet/res/values-it/strings-extra.xml(1 hunks)wallet/res/values-iw/strings.xml(0 hunks)wallet/res/values-ja/strings-extra.xml(1 hunks)wallet/res/values-ko/strings-extra.xml(1 hunks)wallet/res/values-nl/strings-extra.xml(1 hunks)wallet/res/values-pl/strings-extra.xml(1 hunks)wallet/res/values-pt/strings-extra.xml(1 hunks)wallet/res/values-ro/strings-extra.xml(1 hunks)wallet/res/values-ru/strings-extra.xml(2 hunks)wallet/res/values-sk/strings-extra.xml(1 hunks)wallet/res/values-sr/strings-extra.xml(1 hunks)wallet/res/values-sw/strings.xml(0 hunks)wallet/res/values-th/strings-extra.xml(1 hunks)wallet/res/values-tr/strings-extra.xml(1 hunks)wallet/res/values-uk/strings-extra.xml(1 hunks)wallet/res/values-vi/strings-extra.xml(1 hunks)wallet/res/values-zh-rTW/strings-extra.xml(1 hunks)wallet/res/values-zh/strings-extra.xml(1 hunks)wallet/res/values/strings-extra.xml(2 hunks)wallet/res/values/strings.xml(1 hunks)wallet/schnapps/res/values/values.xml(1 hunks)wallet/src/de/schildbach/wallet/di/AppModule.kt(1 hunks)wallet/src/de/schildbach/wallet/service/DeviceInfoProvider.kt(1 hunks)wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/compose_views/ComposeBottomSheet.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/FrequentContactViewHolder.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt(10 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/Shortcut.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutListItem.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutOption.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsList.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsPane.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/widget/ShortcutButton.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/widget/ShortcutsPane.kt(0 hunks)wallet/staging/res/values/values.xml(1 hunks)wallet/testNet3/res/values/values.xml(1 hunks)
💤 Files with no reviewable changes (16)
- wallet/res/drawable/ic_arrow_icon.xml
- wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt
- wallet/src/de/schildbach/wallet/ui/dashpay/FrequentContactViewHolder.kt
- wallet/res/drawable/ic_arrow_drop_down_blue.xml
- wallet/res/drawable/ic_receive_payments.xml
- wallet/res/drawable/ic_arrow_drop_up_blue.xml
- wallet/res/drawable/ic_balance_warning.xml
- wallet/res/drawable/ic_shortcut_receive.xml
- wallet/res/values-sw/strings.xml
- wallet/res/drawable/ic_blue_clock.xml
- wallet/res/drawable/ic_avatar_blue.xml
- wallet/res/values-he/strings.xml
- wallet/res/values-iw/strings.xml
- wallet/src/de/schildbach/wallet/ui/widget/ShortcutButton.kt
- wallet/src/de/schildbach/wallet/ui/widget/ShortcutsPane.kt
- common/src/main/java/org/dash/wallet/common/util/GenericUtils.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsPane.kt (1)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/Shortcut.kt (1)
Shortcut(35-62)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsList.kt (1)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutListItem.kt (1)
ShortcutListItem(40-76)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (54)
wallet/res/values-cs/strings-extra.xml (1)
32-32: Rename translation key for send shortcutThe Czech translation key was correctly updated from
shortcut_pay_to_addresstoshortcut_send, matching the Compose UI changes. The translated value “Poslat” remains accurate and consistent.wallet/res/values-tr/strings-extra.xml (1)
34-34: Consistent string key rename toshortcut_send
The new<string name="shortcut_send">Gönder</string>correctly replaces the oldshortcut_pay_to_addressidentifier, and the Turkish translation remains accurate.wallet/res/values-el/strings-extra.xml (1)
36-36: Consistent string key rename toshortcut_send
The<string name="shortcut_send">Αποστολή</string>entry replaces the old key and preserves the intended Greek label.wallet/res/values-ro/strings-extra.xml (1)
11-11: Consistent string key rename toshortcut_send
The Romanian translation<string name="shortcut_send">Trimite</string>correctly adopts the new key without altering the user-visible text.wallet/res/values-es/strings-extra.xml (1)
36-36: Consistent string key rename toshortcut_send
The Spanish entry<string name="shortcut_send">Enviar</string>aligns with the unified rename and keeps the translation intact.wallet/res/values-sr/strings-extra.xml (1)
11-11: Consistent string key rename toshortcut_send
The Serbian string<string name="shortcut_send">Pošalji</string>correctly updates the key while retaining the original label.wallet/res/values-pl/strings-extra.xml (1)
36-36: Polish locale: renameshortcut_sendis correct
The key was updated fromshortcut_pay_to_addresstoshortcut_sendwhile preserving the translation. This aligns with the new Compose-based shortcut system.wallet/res/values-fr/strings-extra.xml (1)
36-36: French locale: renameshortcut_sendis correct
The resource key change preserves the intended label "Envoyer". Confirm that the two-line, ≤9-character layout constraint still holds in the Compose UI.wallet/res/values-nl/strings-extra.xml (1)
36-36: Dutch locale: renameshortcut_sendis correct
The key rename toshortcut_sendkeeps translation “Verzenden” intact and within UI limits.wallet/res/values-id/strings-extra.xml (1)
36-36: Indonesian locale: renameshortcut_sendis correct
The key has been updated while retaining the translation “Kirim.” This is consistent with other locales.wallet/res/values-it/strings-extra.xml (2)
36-36: Approve renaming key toshortcut_sendin Italian strings
The resource key has been correctly updated toshortcut_sendand the Italian translation remains unchanged.
36-36:✅ Verification successful
Verify no leftover references to old key
shortcut_pay_to_address
Ensure that all occurrences of the deprecated resource key have been removed or updated.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old shortcut resource key. rg "shortcut_pay_to_address"Length of output: 28
All references to
shortcut_pay_to_addresshave been removed
Search across the entire codebase returned no occurrences of the deprecated key.wallet/res/values-pt/strings-extra.xml (1)
36-36: Approve renaming key toshortcut_sendin Portuguese strings
The resource key has been correctly updated toshortcut_sendand the Portuguese translation remains unchanged.wallet/res/values-sk/strings-extra.xml (1)
36-36: Approve renaming key toshortcut_sendin Slovak strings
The resource key has been correctly updated toshortcut_sendand the Slovak translation remains unchanged.wallet/res/values-bg/strings-extra.xml (1)
27-27: Approve renaming key toshortcut_sendin Bulgarian strings
The resource key has been correctly updated toshortcut_sendand the Bulgarian translation remains unchanged.wallet/res/values-zh-rTW/strings-extra.xml (1)
36-36: Approve renaming key toshortcut_sendin Traditional Chinese (TW) strings
The resource key has been correctly updated toshortcut_sendand the Chinese translation remains unchanged.wallet/res/values-vi/strings-extra.xml (1)
28-28: String key rename for send shortcut
Renamedshortcut_pay_to_addresstoshortcut_sendaligns with the new Compose-based shortcut implementation. Ensure there are no remaining references to the old key and thatShortcutOption.SENDuses this resource.wallet/res/values-fil/strings-extra.xml (1)
36-36: String key rename for send shortcut
Renamedshortcut_pay_to_addresstoshortcut_sendto match the updated shortcut identifiers. Verify that all code paths and Compose UI components reference this key.wallet/res/values-zh/strings-extra.xml (1)
36-36: String key rename for send shortcut
Updatedshortcut_pay_to_address→shortcut_sendfor the Chinese locale. This matches the Compose-based shortcut system; confirm thatShortcutOption.SENDand related ViewModel reference this string.wallet/res/values-ko/strings-extra.xml (1)
36-36: String key rename for send shortcut
Renamed toshortcut_sendto align with the new shortcut customization feature. Ensure the analytics constantSHORTCUT_SENDand any default configuration inShortcutProvideruse this key.wallet/res/layout/shortcut_button.xml (1)
19-19: Icon update is correct
Theandroid:srcchange to@drawable/ic_transaction_receivedaligns with the new transaction-received shortcut and is the proper attribute for an ImageView in this merge layout.wallet/res/values-ar/strings-extra.xml (1)
33-33: Consistent string key rename
Renaming the Arabic string key toshortcut_sendcorrectly aligns with the new Compose-based shortcut identifiers and matches other locales.wallet/res/values-uk/strings-extra.xml (1)
36-36: Consistent string key rename
The Ukrainian locale now usesshortcut_send, ensuring uniformity across all translations and the Compose shortcut enum.wallet/res/values-th/strings-extra.xml (1)
34-34: Consistent string key rename
Updating the Thai translation toshortcut_sendkeeps the resource key consistent with the default and other localized files.common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt (1)
40-46: NewOverlinetext style added
The Overline style (12sp font, 16sp line height, medium weight, centered) correctly supports the Compose shortcut labels as designed.wallet/res/values-de/strings-extra.xml (1)
36-36:✅ Verification successful
Unified resource key for the "Send" shortcut
Renaming
shortcut_pay_to_address→shortcut_sendkeeps the key consistent across all Compose-based shortcut code and localizations.To ensure no stale references remain, run:
🏁 Script executed:
#!/bin/bash # Verify no occurrences of the old key rg -n "shortcut_pay_to_address"Length of output: 31
Unified resource key verified, no stale references remain
Ran
rg -n "shortcut_pay_to_address"with no matches—all references have been updated toshortcut_send. Approving these changes.wallet/res/values-ja/strings-extra.xml (1)
36-36:✅ Verification successful
Consistent rename of the shortcut key to
shortcut_sendThe Japanese translation now uses
shortcut_sendin line with other locales and the newShortcutOptionenum.
🏁 Script executed:
#!/bin/bash # Search for any leftover `shortcut_pay_to_address` usages rg -n "shortcut_pay_to_address"Length of output: 31
No leftover
shortcut_pay_to_addressreferences; rename toshortcut_sendis consistent.
Ranrg -n "shortcut_pay_to_address"and found no occurrences.wallet/res/values-fa/strings-extra.xml (1)
36-36:✅ Verification successful
Standardize the "Send" shortcut key across Persian resources
Great—
shortcut_pay_to_addresshas been renamed toshortcut_sendto match the new Compose shortcuts.
🏁 Script executed:
#!/bin/bash # Confirm removal of the old key rg -n "shortcut_pay_to_address"Length of output: 31
Standardize Persian “Send” shortcut
Confirmed thatshortcut_pay_to_addressno longer exists andshortcut_sendis correctly used inwallet/res/values-fa/strings-extra.xml.wallet/res/drawable/ic_contact.xml (1)
1-14: LGTM: Clean vector drawable implementation for the contact iconThe vector drawable is well-structured with appropriate dimensions and color scheme. The icon effectively represents a user silhouette with the Dash brand color (#008DE4), which will provide a clear visual cue for the "Send to Contact" shortcut.
wallet/res/drawable/ic_send_to_address.xml (1)
1-9: LGTM: Well-implemented vector drawable for Send to Address functionalityThe icon is cleanly implemented with the appropriate Dash brand blue color (#008DE4) and properly defined vector path. The icon will provide clear visual representation for the "Send to Address" shortcut option.
common/src/main/java/org/dash/wallet/common/ui/receive/ReceiveInfoView.kt (1)
83-83: Verify if sharing just the address is sufficientThis change modifies the share functionality to share only the raw base58 address instead of the full payment request URI that might include additional information like amount.
- address?.let { handleShare(paymentRequestUri) } + address?.let { handleShare(it.toBase58()) }Ensure this simpler sharing format meets user expectations and requirements across all use cases.
Are there scenarios where users would need to share the full payment request URI with amount information?
wallet/res/layout/home_content.xml (1)
130-138: LGTM: Compose migration for shortcuts paneThis change appropriately replaces the custom ShortcutsPane view with a ComposeView, supporting the PR's objective of migrating the shortcuts feature to Jetpack Compose. The change to
wrap_contentfor height allows the component to determine its own size based on content, which is more flexible than the previous fixed height.wallet/res/drawable/ic_explore.xml (2)
2-5: Approve updated artboard dimensions.
Updating the vector to 24×24 dp with a matching 24×24 viewport simplifies alignment in Compose.
7-8: Approve path consolidation and solid color fill.
The complex multi‐path gradient was replaced by a single path with#78C4F5, which reduces APK size and rendering cost.wallet/res/drawable/ic_shortcut_secure_now.xml (1)
7-8: Approve path simplification and flat fill.
Switching to a singlepathwith solid#78C4F5is correct and aligns with the new flat‐style icons.wallet/res/drawable/ic_where_to_spend.xml (1)
1-5: Approve new vector addition.
The 24×24 dp artboard and viewport are correct, and the new icon follows the same style as other shortcut assets.wallet/res/values-ru/strings-extra.xml (1)
36-36: Approve renaming of shortcut key.
Changingshortcut_pay_to_address→shortcut_sendaligns with the updated enum and Compose code. The displayed text remains correct.wallet/res/drawable/ic_shortcut_buy_sell_dash.xml (1)
7-11: Approve simplified paths and color.
The removal of gradients and consolidation into two solid‐color paths (#008DE4) is correct and in line with the Compose UI’s flat iconography.wallet/src/de/schildbach/wallet/service/DeviceInfoProvider.kt (3)
3-3: Appropriate imports for screen density detection.The added imports for Resources and DisplayMetrics support the new screen size detection functionality.
Also applies to: 5-5
9-10: Good constructor parameter addition.Adding the resources parameter to the constructor enables access to device metrics while maintaining dependency injection principles.
16-17: Well-implemented screen size detection.The
isSmallScreenproperty provides a clean way to detect smaller screens using standard Android density constants. This will help support the dynamic shortcut customization based on screen size.wallet/res/drawable/ic_shortcut_atm.xml (1)
1-12: Well-structured vector drawable for ATM shortcut.The vector drawable follows Android best practices with appropriate sizing (24dp) and uses the Dash brand color (#008DE4). The paths are well-defined to create a clear ATM icon.
wallet/res/drawable/ic_shortcut_staking.xml (1)
1-24: Well-designed staking icon with appropriate color scheme.This vector drawable uses a consistent visual language with the other icons and employs two complementary blue shades (#78C4F5 and #008DE4) to create visual hierarchy. The slightly non-standard dimensions (22x24dp) appear intentional for visual balance.
wallet/res/values/strings-extra.xml (3)
34-34: Improved clarity with more descriptive button text.Changing from "Scan" to "Scan QR" makes the shortcut's purpose more explicit to users.
36-36: Consistent shortcut naming convention.Renaming the string resource key from
shortcut_pay_to_addresstoshortcut_sendimproves consistency while maintaining the same user-facing text. This aligns with the overall shortcuts customization effort.
220-220: Fixed XML escape character.Properly escaped the apostrophe in the string resource to prevent XML parsing issues.
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
345-347:❓ Verification inconclusive
Change in threading context could impact behavior.
The switch from
postValue()to direct.valueassignment changes the threading behavior of these updates. WhilepostValue()is thread-safe and asynchronous, direct.valueassignment must be done on the main thread. Since these are inviewModelScope(likely on the main dispatcher), it should work, but consider the threading implications if these flows emit on background threads.
🏁 Script executed:
#!/bin/bash # Checking if the observeTotalBalance/observeMixedBalance flows might emit on background threads rg -A 5 -B 5 "observeTotalBalance|observeMixedBalance" --include="*.kt"Length of output: 515
🏁 Script executed:
#!/bin/bash # Correctly searching for observeTotalBalance and observeMixedBalance definitions and usages in Kotlin files rg -n "fun observeTotalBalance" -g '*.kt' rg -n "fun observeMixedBalance" -g '*.kt' rg -n "observeTotalBalance" -g '*.kt' rg -n "observeMixedBalance" -g '*.kt'Length of output: 1536
Verify thread context for LiveData updates
Ensure that
_totalBalance.value = it(and similarly_mixedBalance.value = it) always happens on the main thread. By default,
viewModelScope.launchIn(...)runs onDispatchers.Main, so this is safe provided that the source flows do not switch to a background dispatcher. IfobserveTotalBalance()orobserveMixedBalance()ever emit on IO/Default (for example viaflowOn), you will need to either:
- Revert to
postValue()to safely bridge from background threads, or- Explicitly switch back to the main dispatcher (e.g.
withContext(Dispatchers.Main)) before assigning.value.Locations to review:
- wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt → lines ~345–347 (
.onEach { _totalBalance.value = it })- wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt → lines ~350–352 (
.onEach { _mixedBalance.value = it })Please verify that your flow implementations do not emit off the main thread.
wallet/src/de/schildbach/wallet/di/AppModule.kt (1)
97-98: Good enhancement to support screen-size awareness.Adding the resources parameter to the DeviceInfoProvider constructor enables screen size detection, which is essential for adaptive UI in the new shortcut customization feature. This aligns well with the Compose migration which needs to adapt to different screen sizes.
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsPane.kt (1)
39-76: Well-structured Compose UI component with proper interaction handling.This new
ShortcutsPanecomposable efficiently replaces the previous view-based implementation with a modern Compose UI. The code demonstrates good practices:
- Proper use of modifiers for styling and layout
- Dynamic weight calculation to evenly distribute shortcuts
- Combined click handling for both normal and long-press interactions
- Clean separation between UI structure and business logic
The ripple effect enhances the user experience with appropriate visual feedback.
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsList.kt (1)
47-82: Clean implementation of shortcuts selection UI.This composable provides a well-structured vertical list for shortcut selection, complementing the horizontal shortcuts pane. The code follows good Compose practices with proper styling and layout structure.
wallet/src/de/schildbach/wallet/ui/main/shortcuts/Shortcut.kt (1)
35-62: Clean implementation of the Shortcut composable!The Shortcut composable is well-structured with a clear vertical layout, appropriate padding, and proper resource handling for both icon and text. The component follows Compose best practices and maintains a single responsibility.
wallet/res/navigation/nav_home.xml (2)
60-71: Navigation action for Explore search looks good.The action properly includes the required argument for the ExploreTopic type with a default value, and uses consistent animation parameters.
73-92: New navigation actions for external services are correctly implemented.The actions for Uphold, Buy/Sell Overview, and Coinbase maintain consistent animation patterns with the rest of the navigation graph.
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutOption.kt (1)
24-99: Well-organized shortcut options enum with stable IDs.The ShortcutOption enum is well-structured with a clear comment about the importance of maintaining stable IDs when reordering. The comprehensive set of shortcuts with appropriate icons and text resources provides a solid foundation for the shortcut customization feature.
common/src/main/java/org/dash/wallet/common/services/analytics/AnalyticsConstants.kt
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt
Outdated
Show resolved
Hide resolved
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt (1)
102-121: Validate indices and avoid duplicate shortcuts on replacement.The method correctly checks if the index is within bounds, but it doesn't check if the new shortcut already exists in the list, which could lead to duplicates.
fun replaceShortcut(oldIndex: Int, new: ShortcutOption) { if (oldIndex !in shortcuts.indices) { return } + if (new in shortcuts) { + return // prevents duplicates + } + val currentShortcuts = shortcuts.toMutableList() if (currentShortcuts[oldIndex] == ShortcutOption.SECURE_NOW) { // Don't allow replacing SECURE_NOW shortcut return }
🧹 Nitpick comments (4)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt (1)
48-61: Consider using proper state synchronization for dependent properties.The
userHasBalancesetter synchronously updates the shortcuts when no custom shortcuts exist. This could lead to unexpected behavior if called from different threads as the shortcuts property might be modified from multiple sources concurrently.Consider using a more reactive approach:
- private var _userHasBalance = true - var userHasBalance: Boolean - get() = _userHasBalance - set(value) { - _userHasBalance = value - - if (!hasCustomShortcuts) { - shortcuts = getPresetShortcuts().take(maxShortcuts) - } - } + private val _userHasBalance = mutableStateOf(true) + var userHasBalance: Boolean + get() = _userHasBalance.value + set(value) { + _userHasBalance.value = value + }Then update the initialization block to observe these states:
init { // Existing code snapshotFlow { _userHasBalance.value } .combine(shortcutProvider.customShortcuts) { hasBalance, customShortcuts -> Pair(hasBalance, customShortcuts) } .onEach { (_, customShortcuts) -> if (customShortcuts.isEmpty()) { shortcuts = getPresetShortcuts().take(maxShortcuts) } } .launchIn(viewModelScope) }wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt (3)
396-470: Consider using a more maintainable approach for shortcut handling.The current implementation uses a large when statement with many cases. As you add more shortcuts, this approach will become harder to maintain.
Consider using a map-based approach or a strategy pattern:
private val shortcutHandlers = mapOf( ShortcutOption.SECURE_NOW to { viewModel.logEvent(AnalyticsConstants.Home.SHORTCUT_SECURE_WALLET) handleVerifySeed() }, ShortcutOption.SCAN_QR to { viewModel.logEvent(AnalyticsConstants.Home.SHORTCUT_SCAN_TO_PAY) handleScan() }, // Add other handlers here ) private fun onShortcutTap(shortcut: ShortcutOption) { shortcutHandlers[shortcut]?.invoke() }
485-504: Potential race condition in handleStakingNavigation.The method checks
viewModel.isBlockchainSynced.valuedirectly, which could lead to race conditions if the value changes between the check and the usage.Consider using the collect flow pattern instead:
private fun handleStakingNavigation() { lifecycleScope.launch { - if (viewModel.isBlockchainSynced.value == true) { + viewModel.isBlockchainSynced.collect { isSynced -> + if (isSynced == true) { stakingLauncher.launch(Intent(requireContext(), StakingActivity::class.java)) - } else { + return@collect + } else { val openWebsite = AdaptiveDialog.create( null, getString(R.string.chain_syncing), getString(R.string.crowdnode_wait_for_sync), getString(R.string.button_close), getString(R.string.crowdnode_open_website) ).showAsync(requireActivity()) if (openWebsite == true) { val browserIntent = Intent(Intent.ACTION_VIEW, getString(R.string.crowdnode_website).toUri()) startActivity(browserIntent) } + } } } }
453-458: Add loading state when fetching Topper URL.The code makes a network request to get the Topper URL without showing any loading indicator to the user. This could lead to a poor user experience if the request takes time.
ShortcutOption.TOPPER -> { lifecycleScope.launch { + // Show loading state + val loadingDialog = createLoadingDialog() + loadingDialog.show() + + try { val uri = shortcutViewModel.getTopperUrl(getString(R.string.dash_wallet_name)) + loadingDialog.dismiss() requireActivity().openCustomTab(uri) + } catch (e: Exception) { + loadingDialog.dismiss() + // Show error message + showErrorDialog(getString(R.string.error_loading_topper)) + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wallet/schnapps/res/values/values.xml(1 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt(10 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutListItem.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt(1 hunks)wallet/staging/res/values/values.xml(1 hunks)wallet/testNet3/res/values/values.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- wallet/testNet3/res/values/values.xml
- wallet/schnapps/res/values/values.xml
- wallet/staging/res/values/values.xml
- wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutListItem.kt
- wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutProvider.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
wallet/src/de/schildbach/wallet/ui/main/shortcuts/ShortcutsViewModel.kt (1)
1-2:❌ Incorrect review comment
Verify the copyright year.
The copyright year is set to 2025, but the current year is 2024. Consider updating it to reflect the correct year.
🏁 Script executed:
#!/bin/bash # Check copyright year in other files for consistency grep -r "Copyright 2024" wallet/src/de/schildbach/wallet/ui/Length of output: 1498
No Action Needed: License Header Year Is Correct
The project’s current year is 2025, and this file was just added in 2025. The header already reflects the correct year—no update required.
Likely an incorrect or invalid review comment.
wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt (2)
275-278: LGTM! View composition strategy is correctly implemented.The code now properly disposes of the Compose hierarchy with the Fragment view lifecycle to avoid memory leaks, addressing a previous review comment.
111-117: LGTM! Proper activity result handling for staking.The implementation correctly handles the activity result from staking, navigating to buy/sell screen when needed.
| private fun removeSecureNowShortcut() { | ||
| val currentShortcuts = shortcuts.toMutableList() | ||
| val index = currentShortcuts.indexOf(ShortcutOption.SECURE_NOW) | ||
| currentShortcuts.removeAt(index) | ||
| val shortcutIds = currentShortcuts.map { it.id } | ||
| .take(maxShortcuts) | ||
| .toIntArray() | ||
| viewModelScope.launch { | ||
| shortcutProvider.setCustomShortcuts(shortcutIds) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential IndexOutOfBoundsException in removeSecureNowShortcut.
If ShortcutOption.SECURE_NOW is not found in the shortcuts list, indexOf will return -1, and removeAt(-1) will throw an IndexOutOfBoundsException.
private fun removeSecureNowShortcut() {
val currentShortcuts = shortcuts.toMutableList()
val index = currentShortcuts.indexOf(ShortcutOption.SECURE_NOW)
+ if (index == -1) {
+ return
+ }
currentShortcuts.removeAt(index)
val shortcutIds = currentShortcuts.map { it.id }
.take(maxShortcuts)
.toIntArray()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun removeSecureNowShortcut() { | |
| val currentShortcuts = shortcuts.toMutableList() | |
| val index = currentShortcuts.indexOf(ShortcutOption.SECURE_NOW) | |
| currentShortcuts.removeAt(index) | |
| val shortcutIds = currentShortcuts.map { it.id } | |
| .take(maxShortcuts) | |
| .toIntArray() | |
| viewModelScope.launch { | |
| shortcutProvider.setCustomShortcuts(shortcutIds) | |
| } | |
| } | |
| private fun removeSecureNowShortcut() { | |
| val currentShortcuts = shortcuts.toMutableList() | |
| val index = currentShortcuts.indexOf(ShortcutOption.SECURE_NOW) | |
| if (index == -1) { | |
| return | |
| } | |
| currentShortcuts.removeAt(index) | |
| val shortcutIds = currentShortcuts.map { it.id } | |
| .take(maxShortcuts) | |
| .toIntArray() | |
| viewModelScope.launch { | |
| shortcutProvider.setCustomShortcuts(shortcutIds) | |
| } | |
| } |
| suspend fun getTopperUrl(walletName: String): String { | ||
| return topperClient.getOnRampUrl( | ||
| walletUIConfig.getExchangeCurrencyCode(), | ||
| walletData.freshReceiveAddress(), | ||
| walletName | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing error handling for the TopperClient API call.
The getTopperUrl method doesn't include error handling for network or API failures. This could lead to crashes or unhandled exceptions if the API request fails.
suspend fun getTopperUrl(walletName: String): String {
- return topperClient.getOnRampUrl(
- walletUIConfig.getExchangeCurrencyCode(),
- walletData.freshReceiveAddress(),
- walletName
- )
+ return try {
+ topperClient.getOnRampUrl(
+ walletUIConfig.getExchangeCurrencyCode(),
+ walletData.freshReceiveAddress(),
+ walletName
+ )
+ } catch (e: Exception) {
+ // Log the error
+ // Return a fallback URL or rethrow as a specific exception
+ throw e
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suspend fun getTopperUrl(walletName: String): String { | |
| return topperClient.getOnRampUrl( | |
| walletUIConfig.getExchangeCurrencyCode(), | |
| walletData.freshReceiveAddress(), | |
| walletName | |
| ) | |
| } | |
| suspend fun getTopperUrl(walletName: String): String { | |
| return try { | |
| topperClient.getOnRampUrl( | |
| walletUIConfig.getExchangeCurrencyCode(), | |
| walletData.freshReceiveAddress(), | |
| walletName | |
| ) | |
| } catch (e: Exception) { | |
| // Log the error | |
| // Return a fallback URL or rethrow as a specific exception | |
| throw e | |
| } | |
| } |
| private fun onShortcutLongTap(shortcut: ShortcutOption, index: Int) { | ||
| if (shortcut == ShortcutOption.SECURE_NOW) { | ||
| return | ||
| } | ||
|
|
||
| ComposeBottomSheet(R.style.SecondaryBackground, forceExpand = true) { dialog -> | ||
| ShortcutsList(shortcutViewModel.getAllShortcutOptions(shortcut)) { newShortcut -> | ||
| shortcutViewModel.replaceShortcut(index, newShortcut) | ||
| dialog.dismiss() | ||
| } | ||
| }.show(requireActivity()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential memory leaks in ComposeBottomSheet.
The ComposeBottomSheet is created without proper lifecycle handling. If the fragment is destroyed while the dialog is shown, it could lead to memory leaks.
private fun onShortcutLongTap(shortcut: ShortcutOption, index: Int) {
if (shortcut == ShortcutOption.SECURE_NOW) {
return
}
- ComposeBottomSheet(R.style.SecondaryBackground, forceExpand = true) { dialog ->
+ val dialog = ComposeBottomSheet(R.style.SecondaryBackground, forceExpand = true) { dialog ->
ShortcutsList(shortcutViewModel.getAllShortcutOptions(shortcut)) { newShortcut ->
shortcutViewModel.replaceShortcut(index, newShortcut)
dialog.dismiss()
}
- }.show(requireActivity())
+ }
+
+ // Use viewLifecycleOwner to ensure dialog is dismissed when fragment view is destroyed
+ viewLifecycleOwner.lifecycle.addObserver(object : DefaultLifecycleObserver {
+ override fun onDestroy(owner: LifecycleOwner) {
+ dialog.dismiss()
+ }
+ })
+
+ dialog.show(requireActivity())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun onShortcutLongTap(shortcut: ShortcutOption, index: Int) { | |
| if (shortcut == ShortcutOption.SECURE_NOW) { | |
| return | |
| } | |
| ComposeBottomSheet(R.style.SecondaryBackground, forceExpand = true) { dialog -> | |
| ShortcutsList(shortcutViewModel.getAllShortcutOptions(shortcut)) { newShortcut -> | |
| shortcutViewModel.replaceShortcut(index, newShortcut) | |
| dialog.dismiss() | |
| } | |
| }.show(requireActivity()) | |
| } | |
| private fun onShortcutLongTap(shortcut: ShortcutOption, index: Int) { | |
| if (shortcut == ShortcutOption.SECURE_NOW) { | |
| return | |
| } | |
| // Capture the dialog instance so we can dismiss it on view destroy | |
| val dialog = ComposeBottomSheet(R.style.SecondaryBackground, forceExpand = true) { dialog -> | |
| ShortcutsList(shortcutViewModel.getAllShortcutOptions(shortcut)) { newShortcut -> | |
| shortcutViewModel.replaceShortcut(index, newShortcut) | |
| dialog.dismiss() | |
| } | |
| } | |
| // Ensure the dialog is dismissed when the fragment's view is destroyed | |
| viewLifecycleOwner.lifecycle.addObserver(object : DefaultLifecycleObserver { | |
| override fun onDestroy(owner: LifecycleOwner) { | |
| dialog.dismiss() | |
| } | |
| }) | |
| dialog.show(requireActivity()) | |
| } |
We want to allow users to customize shortcuts on the shortcut panel.
Issue being fixed or feature implemented
Related PR's and Dependencies
Should be merged after #1392
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Refactor