fix(Wallet): Hiding assets from wallet's main view doesn't work #15811

caybro · 2024-07-25T14:02:31Z

What does the PR do

partially reuse the already available Global/Popups methods to hide assets (which also emit proper notifications); those are needed as another shared modal from Popups uses it too (may come from outside of Wallet)
some warnings cleanup in the controller

Fixes #15777

Affected areas

Wallet

Screenshot of functionality (including design for comparison)

I've checked the design and this PR matches it

Zaznam.obrazovky.z.2024-07-25.12-47-15.webm

status-im-auto · 2024-07-25T14:09:52Z

Jenkins Builds

Click to see older builds (23)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`874fc4c`	#1	2024-07-25 14:09:51	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`874fc4c`	#1	2024-07-25 14:09:57	~7 min	`tests/nim`	📄`log`
✔️	`874fc4c`	#1	2024-07-25 14:13:53	~10 min	`macos/x86_64`	🍎`dmg`
✔️	`874fc4c`	#1	2024-07-25 14:16:22	~13 min	`tests/ui`	📄`log`
✔️	`874fc4c`	#1	2024-07-25 14:18:37	~15 min	`linux-nix/x86_64`	📦`tgz`
✔️	`874fc4c`	#1	2024-07-25 14:20:27	~17 min	`linux/x86_64`	📦`tgz`
❌	`874fc4c`	#1	2024-07-25 14:24:23	~21 min	`windows/x86_64`	📄`log`
✔️	`874fc4c`	#2	2024-07-25 14:54:23	~25 min	`windows/x86_64`	💿`exe`

✔️	`87d6fb0`	#2	2024-07-26 14:55:24	~6 min	`macos/aarch64`	🍎`dmg`
✔️	`87d6fb0`	#2	2024-07-26 14:58:13	~9 min	`tests/nim`	📄`log`
✔️	`87d6fb0`	#2	2024-07-26 15:03:28	~14 min	`tests/ui`	📄`log`
✔️	`87d6fb0`	#2	2024-07-26 15:04:58	~16 min	`linux-nix/x86_64`	📦`tgz`
✔️	`87d6fb0`	#2	2024-07-26 15:06:45	~18 min	`linux/x86_64`	📦`tgz`
✔️	`87d6fb0`	#2	2024-07-26 15:07:30	~18 min	`macos/x86_64`	🍎`dmg`
✔️	`87d6fb0`	#3	2024-07-26 15:13:32	~24 min	`windows/x86_64`	💿`exe`

✔️	`56dc286`	#3	2024-07-26 15:23:54	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`56dc286`	#3	2024-07-26 15:28:52	~9 min	`tests/nim`	📄`log`
✔️	`56dc286`	#3	2024-07-26 15:32:51	~13 min	`macos/x86_64`	🍎`dmg`
✔️	`56dc286`	#3	2024-07-26 15:35:44	~16 min	`linux/x86_64`	📦`tgz`
❌	`56dc286`	#3	2024-07-26 15:36:13	~16 min	`tests/ui`	📄`log`
✔️	`56dc286`	#3	2024-07-26 15:37:16	~17 min	`linux-nix/x86_64`	📦`tgz`
✔️	`56dc286`	#4	2024-07-26 15:45:57	~26 min	`windows/x86_64`	💿`exe`
✔️	`56dc286`	#4	2024-07-26 15:55:58	~12 min	`tests/ui`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`8a6553f`	#4	2024-07-30 16:04:11	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`8a6553f`	#4	2024-07-30 16:08:12	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`8a6553f`	#4	2024-07-30 16:08:44	~9 min	`tests/nim`	📄`log`
✔️	`8a6553f`	#4	2024-07-30 16:16:11	~16 min	`linux/x86_64`	📦`tgz`
✔️	`8a6553f`	#4	2024-07-30 16:17:27	~18 min	`linux-nix/x86_64`	📦`tgz`
✔️	`8a6553f`	#5	2024-07-30 16:18:31	~19 min	`tests/ui`	📄`log`
✔️	`8a6553f`	#5	2024-07-30 16:32:20	~32 min	`windows/x86_64`	💿`exe`

✔️	`36e6630`	#5	2024-07-30 22:14:26	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`36e6630`	#5	2024-07-30 22:16:24	~6 min	`tests/nim`	📄`log`
✔️	`36e6630`	#5	2024-07-30 22:18:39	~8 min	`macos/x86_64`	🍎`dmg`
✔️	`36e6630`	#6	2024-07-30 22:22:50	~12 min	`tests/ui`	📄`log`
✔️	`36e6630`	#5	2024-07-30 22:22:51	~13 min	`linux-nix/x86_64`	📦`tgz`
✔️	`36e6630`	#5	2024-07-30 22:25:00	~15 min	`linux/x86_64`	📦`tgz`
✔️	`36e6630`	#6	2024-07-30 22:34:16	~24 min	`windows/x86_64`	💿`exe`

status-im-auto · 2024-07-25T14:10:00Z

✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15811#1 🔹 ~7 min 11 sec 🔹 `874fc4c` 🔹 📦 tests/nim package

micieslak

What we should do here is quite opposite - instead of altering AssetsView which has currently quite well defined and clear API, we can remove those popups from Popups and handle externally just the actual action, hiding asset in this case.

That handling can be done in RightTabView, where AssetsView is defined or propagated up in the hierarchy to be handled by some other parent.

With the popup handling via Global the API of AssetsView becomes less clear. Via reading API we don't know that this component is going to request some operation (as it was, there are unused signals like hideCommunityAssets left) or even that, like now, it's going to request some helper popup, because the dependency is hidden via using singleton.

The proposed approach also delegates handling logic to Popups which becomes a huge aggregator of logic of any kind, coming from various places in the app, taking a lot of dependencies. Handling closer to the source, e.g. in parent of AssetsView is more localized and doesn't add additional dependencies there.

Also splitting into separate files is better than growing one big file with not only popups but also business logic triggered by them.

Let's imagine that requirements change and instead of using popup we handle actions from menu directly. In the structure we had so far it's only internal change in AssetsView, API is not even touched. Everything else stays untouched, everything works as before (hideRequested is emitted).
In the new approach many things needs to be changed: AssetsView including API, parent component, and also Popups potentially as some things may be not needed there then.

Btw, there is one thing that could be improved in AssetsView: model should not be passed in one piece to the popup. When window is resized and delegate goes out of view and is removed, model object is also gone. Then popup may be left in invalid state. Oppositely, passing values separately by value is rock solid.

ui/imports/shared/views/AssetsView.qml

caybro · 2024-07-26T12:49:43Z

What we should do here is quite opposite - instead of altering AssetsView which has currently quite well defined and clear API, we can remove those popups from Popups and handle externally just the actual action, hiding asset in this case.

Hmm I don't get this; I am not altering AssetsView API whatsover, and I can't remove those confirmation modals from the global Popups either; the very same codepath is being used from Settings/Manage tokens

That handling can be done in RightTabView, where AssetsView is defined or propagated up in the hierarchy to be handled by some other parent.

That would be just duplicating the same code we already have in Popups

With the popup handling via Global the API of AssetsView becomes less clear. Via reading API we don't know that this component is going to request some operation (as it was, there are unused signals like hideCommunityAssets left) or even that, like now, it's going to request some helper popup, because the dependency is hidden via using singleton.

OK, I can move the calls up to RightTabView.qml

The proposed approach also delegates handling logic to Popups which becomes a huge aggregator of logic of any kind, coming from various places in the app, taking a lot of dependencies. Handling closer to the source, e.g. in parent of AssetsView is more localized and doesn't add additional dependencies there.

Yup, I can do this; but we really want to keep the shared code in Popups; that was the initial purpose of introducing it in the first place

Also splitting into separate files is better than growing one big file with not only popups but also business logic triggered by them.

Let's imagine that requirements change and instead of using popup we handle actions from menu directly. In the structure we had so far it's only internal change in AssetsView, API is not even touched. Everything else stays untouched, everything works as before (hideRequested is emitted). In the new approach many things needs to be changed: AssetsView including API, parent component, and also Popups potentially as some things may be not needed there then.

That's not true, we'd likely want to do the same thing in the other place too (Manage tokens) which would only lead to the same code duplication

Btw, there is one thing that could be improved in AssetsView: model should not be passed in one piece to the popup. When window is resized and delegate goes out of view and is removed, model object is also gone. Then popup may be left in invalid state. Oppositely, passing values separately by value is rock solid.

I'm not passing the whole model?

caybro · 2024-07-26T13:28:29Z

On a second look, the confirmation hide asset dialog in Global/Popups is used only from one place; I can partially remove that and keep one in AssetsView.qml 👍

caybro · 2024-07-26T14:50:04Z

Btw, there is one thing that could be improved in AssetsView: model should not be passed in one piece to the popup. When window is resized and delegate goes out of view and is removed, model object is also gone. Then popup may be left in invalid state. Oppositely, passing values separately by value is rock solid.

Fixed too

ui/app/mainui/Popups.qml

caybro · 2024-07-26T14:55:41Z

ui/app/mainui/Popups.qml

@@ -1168,35 +1169,24 @@ QtObject {
                destroyOnClose: true
                communitiesStore: root.communitiesStore

-                onHideClicked: (tokenSymbol, tokenName, tokenImage, isAsset) => isAsset ? root.openConfirmHideAssetPopup(tokenSymbol, tokenName, tokenImage)
-                                                                                        : root.openConfirmHideCollectiblePopup(tokenSymbol, tokenName, tokenImage)
+                onHideClicked: (tokenSymbol, tokenName, tokenImage, isAsset) => isAsset ? root.openConfirmHideAssetPopup(tokenSymbol, tokenName, tokenImage, true)


Here we need the (shared) confirmHideAssetPopup

micieslak

Hmm I don't get this; I am not altering AssetsView API whatsover, and I can't remove those confirmation modals from the global Popups either; the very same codepath is being used from Settings/Manage tokens

Using Global.* in AssetsView was changing the API of that component indeed and it was my key concern - hideRequested and hideCommunityAssetsRequested would be never called and therefore effectively removed. So thank you for removing those calls and keeping that interaction explicit by having signals exposed via public API.

I'm almost fine with the current version, modulo the issue I raised inline. It should be straightforward to adjust either way.

Ideally, those 2 popups when not reachable now via AssetsView, they should have their own SB pages, but maybe we can do just when needed.

ui/imports/shared/views/AssetsView.qml

micieslak

Thank you for the alignments regarding code structure!

But... have you run the app after your last changes? The app is not starting at all because improper placing Component object in RightTabView.

ui/app/AppLayouts/Wallet/views/RightTabView.qml

caybro · 2024-07-30T21:39:58Z

Thank you for the alignments regarding code structure!

But... have you run the app after your last changes? The app is not starting at all because improper placing Component object in RightTabView.

Ouch, will fix

- partially reuse the already available Global/Popups methods to hide assets (which also emit proper notifications); those are needed as anotehr shared modal from Popups uses it too (may come from outside of Wallet) - some warnings cleanup in the controller Fixes #15777

caybro · 2024-07-30T22:16:35Z

Thank you for the alignments regarding code structure!
But... have you run the app after your last changes? The app is not starting at all because improper placing Component object in RightTabView.

Ouch, will fix

Should run and work correctly now @micieslak

micieslak

Works fine now, thanks!

alexjba

LGTM! Tested 👍

caybro linked an issue Jul 25, 2024 that may be closed by this pull request

Wallet: Hiding assets from wallet's main view doesn't work #15777

Closed

caybro requested review from micieslak, Khushboo-dev-cpp, Seitseman and noeliaSD July 25, 2024 14:02

micieslak requested changes Jul 25, 2024

View reviewed changes

ui/imports/shared/views/AssetsView.qml Outdated Show resolved Hide resolved

caybro force-pushed the 15777-wallet-hiding-assets-from-wallets-main-view-doesnt-work branch from 874fc4c to 87d6fb0 Compare July 26, 2024 14:48

caybro commented Jul 26, 2024

View reviewed changes

ui/app/mainui/Popups.qml Show resolved Hide resolved

caybro commented Jul 26, 2024

View reviewed changes

caybro requested review from micieslak and alexjba July 26, 2024 15:09

micieslak requested changes Jul 29, 2024

View reviewed changes

ui/imports/shared/views/AssetsView.qml Outdated Show resolved Hide resolved

caybro force-pushed the 15777-wallet-hiding-assets-from-wallets-main-view-doesnt-work branch from 56dc286 to 8a6553f Compare July 30, 2024 15:59

caybro requested a review from micieslak July 30, 2024 15:59

micieslak requested changes Jul 30, 2024

View reviewed changes

ui/app/AppLayouts/Wallet/views/RightTabView.qml Outdated Show resolved Hide resolved

caybro force-pushed the 15777-wallet-hiding-assets-from-wallets-main-view-doesnt-work branch from 8a6553f to 36e6630 Compare July 30, 2024 22:09

caybro requested a review from micieslak July 30, 2024 22:09

micieslak approved these changes Jul 31, 2024

View reviewed changes

alexjba approved these changes Aug 1, 2024

View reviewed changes

caybro merged commit 24dd67e into master Aug 1, 2024
9 checks passed

caybro deleted the 15777-wallet-hiding-assets-from-wallets-main-view-doesnt-work branch August 1, 2024 10:12

caybro mentioned this pull request Aug 1, 2024

[CHERRY-PICK] fix(Wallet): Hiding assets from wallet's main view doesn't work #15960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Wallet): Hiding assets from wallet's main view doesn't work #15811

fix(Wallet): Hiding assets from wallet's main view doesn't work #15811

caybro commented Jul 25, 2024 •

edited

Loading

status-im-auto commented Jul 25, 2024 •

edited

Loading

status-im-auto commented Jul 25, 2024

micieslak left a comment

caybro commented Jul 26, 2024

caybro commented Jul 26, 2024 •

edited

Loading

caybro commented Jul 26, 2024

caybro Jul 26, 2024

micieslak left a comment

micieslak left a comment

caybro commented Jul 30, 2024

caybro commented Jul 30, 2024

micieslak left a comment

alexjba left a comment

fix(Wallet): Hiding assets from wallet's main view doesn't work #15811

fix(Wallet): Hiding assets from wallet's main view doesn't work #15811

Conversation

caybro commented Jul 25, 2024 • edited Loading

What does the PR do

Affected areas

Screenshot of functionality (including design for comparison)

status-im-auto commented Jul 25, 2024 • edited Loading

Jenkins Builds

status-im-auto commented Jul 25, 2024

✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15811#1 🔹 ~7 min 11 sec 🔹 874fc4c 🔹 📦 tests/nim package

micieslak left a comment

Choose a reason for hiding this comment

caybro commented Jul 26, 2024

caybro commented Jul 26, 2024 • edited Loading

caybro commented Jul 26, 2024

caybro Jul 26, 2024

Choose a reason for hiding this comment

micieslak left a comment

Choose a reason for hiding this comment

micieslak left a comment

Choose a reason for hiding this comment

caybro commented Jul 30, 2024

caybro commented Jul 30, 2024

micieslak left a comment

Choose a reason for hiding this comment

alexjba left a comment

Choose a reason for hiding this comment

caybro commented Jul 25, 2024 •

edited

Loading

status-im-auto commented Jul 25, 2024 •

edited

Loading

✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15811#1 🔹 ~7 min 11 sec 🔹 `874fc4c` 🔹 📦 tests/nim package

caybro commented Jul 26, 2024 •

edited

Loading