Tweak portfolio UI and add option to show hidden NFTs #19990

simoarpe · 2023-09-05T09:12:26Z

Resolves brave/brave-browser#32545

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Open Brave Wallet and navigate to "NFTs" section by tapping on the tab at the top
Observe NFTs and Hidden sections are shown
Add a new NFT
Observe the NFT is shown in the NFTs section
Tap on "Edit Visible NFTs"
Deselect the NFT and tap on "Save account watchlist"
Observe the NFT is shown in the Hidden section

simoarpe · 2023-09-05T11:22:41Z

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/NftGridFragment.java

@@ -384,7 +445,7 @@ private void recordP3AView() {
        if (activity instanceof BraveWalletActivity) {
            BraveWalletActivity walletActivity = (BraveWalletActivity) activity;
            BraveWalletP3a walletP3A = walletActivity.getBraveWalletP3A();
-            if (walletP3A != null) {
+            if (walletP3A != null && mNftDataModels != null) {


Improved check as the wallet was crashing here.

Pavneet-Sing · 2023-09-05T12:32:37Z

android/java/org/chromium/chrome/browser/crypto_wallet/util/PortfolioHelper.java

+            TokenUtils.getUserOrAllTokensFiltered(braveWalletService, blockchainRegistry,
+                    networkInfo, networkInfo.coin, TokenUtils.TokenType.NFTS, true, userAssets -> {
+                        mUserAssets.addAll(Arrays.asList(userAssets));
+
+                        if (userAssetsCount.incrementAndGet() == totalNetworks) {
+                            mUserAssets.sort(Comparator.comparing(
+                                    token -> mAssertSortPriorityPerCoinIndex.get(token.chainId)));
+                            AtomicInteger allAssets = new AtomicInteger();
+                            final List<BlockchainToken> assets = new ArrayList<>();
+                            for (NetworkInfo selectedNetwork : mSelectedNetworks) {
+                                TokenUtils.getUserOrAllTokensFiltered(braveWalletService,
+                                        blockchainRegistry, selectedNetwork, selectedNetwork.coin,
+                                        TokenUtils.TokenType.NFTS, false, tokens -> {


TokenUtils.getUserOrAllTokensFiltered with true and false is internally calling braveWalletService.getUserAssets for each networks twice. The result of braveWalletService.getUserAssets can be persisted during the first call and can be reused with with getAllTokensFiltered to reduce latency.

Pavneet-Sing · 2023-09-05T12:42:34Z

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/NftGridFragment.java

+                    LiveDataUtil.observeOnce(getNetworkModel().mCryptoNetworks,
+                            networkList


PortfolioModel has the mAllNetworkInfos. Add a getter and reuse it than LiveDataUtil.observeOnce(getNetworkModel().mCryptoNetworks,

Oh, right! Nice catch, thank you!

Pavneet-Sing · 2023-09-05T12:47:48Z

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/NftGridFragment.java

+        mPortfolioModel.fetchNfts(
+                mSelectedNetworkList, (BraveWalletBaseActivity) activity, (portfolioHelper) -> {
                    if (!AndroidUtils.canUpdateFragmentUi(this)) return;
                    mPortfolioHelper = portfolioHelper;
                    List<BlockchainToken> nfts = mPortfolioHelper.getUserAssets();
-                    LiveDataUtil.observeOnce(getNetworkModel().mCryptoNetworks, networkInfos -> {
-                        mPortfolioModel.prepareNftListMetaData(
-                                nfts, networkInfos, mPortfolioHelper);
-                    });
+                    List<BlockchainToken> hiddenNfts = mPortfolioHelper.getHiddenAssets();
+                    LiveDataUtil.observeOnce(getNetworkModel().mCryptoNetworks,
+                            networkList
+                            -> mPortfolioModel.prepareNftListMetaData(
+                                    nfts, hiddenNfts, networkList, mPortfolioHelper));


nit: We always gonna be needing NFT meta data (can add a new method if not) so fetchNfts can internally call prepareNftListMetaData directly for further processing to remove the back and forth data passing/calls.

mkarolin

strings++

Pavneet-Sing

LGTM, added some comments to improve performance. (can be addressed later if suitable).

SergeyZhukovsky

++

simoarpe added 10 commits September 4, 2023 14:32

Remove indicator from bottom navigation view

36d1402

Tweak bottom navigation view colors

230bbfd

Tweak chip tab layout

420da52

Fix null point exception

f401f93

Improve colors in night mode

3aa88d7

Rename style

6f03388

Improve tab layout margin

525039d

Tweak NFT grid layout

95990f9

Add hidden section

fc42af0

Add hidden NFTs adapter

d7eecb3

simoarpe added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x86 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Sep 5, 2023

simoarpe requested a review from a team as a code owner September 5, 2023 09:12

simoarpe self-assigned this Sep 5, 2023

github-actions bot added the feature/web3/wallet label Sep 5, 2023

simoarpe added 3 commits September 5, 2023 11:13

Add logic to show hide NFT sections

33272f9

Improve layout

0bd2480

Apply code formatting

adede55

simoarpe commented Sep 5, 2023

View reviewed changes

simoarpe requested review from Pavneet-Sing and SergeyZhukovsky September 5, 2023 11:37

Pavneet-Sing reviewed Sep 5, 2023

View reviewed changes

mkarolin approved these changes Sep 5, 2023

View reviewed changes

Pavneet-Sing approved these changes Sep 5, 2023

View reviewed changes

SergeyZhukovsky approved these changes Sep 5, 2023

View reviewed changes

simoarpe added 4 commits September 5, 2023 19:41

Improve NFT fetching logic

c80611b

Optimize nested calls

a44df2d

Faster fetch based on review feedback

d067bbc

Perform minor refactoring

f383bed

simoarpe merged commit 6f55919 into master Sep 6, 2023

simoarpe deleted the simone/portfolio-ui-tweak branch September 6, 2023 11:43

github-actions bot added this to the 1.59.x - Nightly milestone Sep 6, 2023

srirambv mentioned this pull request Oct 9, 2023

Tweak portfolio UI and add option to show hidden NFTs brave/brave-browser#32545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak portfolio UI and add option to show hidden NFTs #19990

Tweak portfolio UI and add option to show hidden NFTs #19990

simoarpe commented Sep 5, 2023 •

edited

Loading

simoarpe Sep 5, 2023

Pavneet-Sing Sep 5, 2023

simoarpe Sep 6, 2023

Pavneet-Sing Sep 5, 2023

simoarpe Sep 5, 2023

Pavneet-Sing Sep 5, 2023 •

edited

Loading

simoarpe Sep 6, 2023

mkarolin left a comment

Pavneet-Sing left a comment

SergeyZhukovsky left a comment

		LiveDataUtil.observeOnce(getNetworkModel().mCryptoNetworks,
		networkList

Tweak portfolio UI and add option to show hidden NFTs #19990

Tweak portfolio UI and add option to show hidden NFTs #19990

Conversation

simoarpe commented Sep 5, 2023 • edited Loading

Resolves brave/brave-browser#32545

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simoarpe Sep 5, 2023

Choose a reason for hiding this comment

Pavneet-Sing Sep 5, 2023

Choose a reason for hiding this comment

simoarpe Sep 6, 2023

Choose a reason for hiding this comment

Pavneet-Sing Sep 5, 2023

Choose a reason for hiding this comment

simoarpe Sep 5, 2023

Choose a reason for hiding this comment

Pavneet-Sing Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

simoarpe Sep 6, 2023

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

Pavneet-Sing left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

simoarpe commented Sep 5, 2023 •

edited

Loading

Pavneet-Sing Sep 5, 2023 •

edited

Loading