Feat/update launch send modal mechanism #16718
Conversation
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
379561d
to
db6dafa
Compare
Jenkins BuildsClick to see older builds (54)
|
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
db6dafa
to
9845809
Compare
Khushboo-dev-cpp
requested review from
micieslak,
a team,
caybro and
alexjba
as code owners
Khushboo-dev-cpp
requested review from
Cuteivist
and removed request for
a team
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
9845809
to
ab5fe7f
Compare
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
ab5fe7f
to
5c1e2e7
Compare
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
5c1e2e7
to
d389808
Compare
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
d389808
to
dce2237
Compare
There was a problem hiding this comment.
I'm glad we are going to sort out launching SendModal from multiple places. It's about specific modal but the way we do it should be general and as good as possible, example for another similar cases. After initial review I have several thoughts/propositions regarding architecture I'd like to share. Please wait with merging, will provide details asap.
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
dce2237
to
943ab34
Compare
I dont think it will matter cause we dont test launch of send modal from the different places in the storybook right? also I thought the UI tests would complain had I missed something. |
There was a problem hiding this comment.
I will try to explain what are the problems with the current approach used here (and already used in other places of the app), and also propose some solutions for further discussion.
Let's imagine that we want to add some additional parameter to our send modal which is relevant for only one location requesting the modal (e.g. WalletLayout). For other locations this parameter is irrelevant, default, fixed value can be always used.
How we could introduce that change now? We need to add that parameter to the Global.launchSendWithParams in order to use it the mentioned place. Unfortunately we need to search through the whole app and refactor all calls to Global.launchSendWithParams by providing default argument there. It's necessary to keep the signal emit working, even thought all those places don't care about our new parameter.
It's typical example of OCP (open-closed principle) violation. We want to extend in one place, but due to used approach, we need to modify multiple logically unrelated places.
Wrapping the signal invocation into a function to maintain default arguments there, as proposed by @caybro reduces the problem somewhat but does not eliminate it completely. When we want to provide the last argument, we need to provide all earlier arguments even though they are irrelevant for us.
Ok, how we could change it?
Let's take a look at call to Global.launchSendWithParams in MessageView.qml:
Global.launchSendWithParams(Constants.SendType.Transfer, //sendType
"", //senderAddress
"", //tokenId
Constants.TokenType.Unknown, //tokenType
"", //tokenAmount
0, //chainId
addressOrEns //recipientAddress
)the only param that is relevant for here is recipientAddress. But we are forced to provide all parameters up to the last one that we really need.
From MessageView perspective we just want to do some send to a given address. This component shouldn't care how it will be done, especially that it can be done in various ways depending on the context. But here we provide some other parameters only because they are used in some other parts of the app. And we configure the component handling the action by providing specific send type, used by SendModal.
So how it should be done to avoid those problems?
The simplest options seems to be applicable here. Instead of the call above, we could just emit `sendRequested(addressOrEns).
where the signal is defined as
signal sendRequested(string addressOrEns)The consequence is necessity to propagate the signal to the place where it's finally handled. E.g. in case of the MessageView the path may be relatively long:
ChatMessagesView -> ChatMessagesView -> ChatContentView -> ChatColumnView -> ChatView -> ChatLayout -> AppMain
However it shouldn't be considered as a drawback because those components indeed request send and it's better to do it in an explicit way.
The signature may change on that way. If there is some other component in the hierarchy requesting send with other parameters e.g. with custom amount, the parent (containing both that component and MessageView), will emit signal of following signature then: signal sendRequested(string addressOrEns, string amount) as both params are needed here to propagate sendRequested from both sub-components.
Even more interesting situation we have for components like OverviewSettingsPanel, where we call Global.openTransferOwnershipPopup:
Global.openTransferOwnershipPopup(root.communityId,
root.name,
root.logoImageData,
{
key: "0",
privilegesLevel: root.ownerToken.privilegesLevel,
chainId: root.ownerToken.chainId,
name: root.ownerToken.name,
artworkSource: root.ownerToken.image,
accountAddress: root.ownerToken.accountAddress.toLowerCase(),
tokenAddress: root.ownerToken.tokenAddress.toLowerCase()
})this whole invocation could be replaced just by: ownershipRequestedRequested() signal. No parameters are needed because all of them are known to the entity instantiating OverviewSettingsPanel. There is no need to bypass all those parameters back to the caller via a signal. Moreover obtaining arguments may be completely detached from this component and it's parent, because only community id is needed to fetch rest of the info from proper model, higher in the hierarchy.
Thanks to that if we change the way of handling transfer, the OverviewSettingsPanel will stay intact because requesting side (OverviewSettingsPanel) is not tightly coupled with the handling side (TransferOwnershipPopup), which may be modified, e.g. to require more params, or. .e.g. completely replaced by other solution.
I think we should study this case deliberately because we need such mechanism designed carefully for this and other modals, potentially also menus, notifications and others. And loose coupling should be treated seriously to keep the app organized and easily maintainable when growing.
Reorganizing this part may cost some time but careful design should pay-off quickly.
@Khushboo-dev-cpp @alexjba @caybro please share your opinions :)
Hey @micieslak definitely something I thought of too, about the params list increasing and a call to launch send modal become more and more complex with a chance of missing something and in turn causing errors too. The idea of emitting signals relative to the place it is called from and passing it down the qml hierarchy, is something which at first glance I feel like its an overhead. Also in case MessageVIew and some other component in the same chain want to launch send modal with different params but same type for example openSend(var recipient) I dont think its possible to have 2 signals like this right? |
Nope, it's not even possible to have a signal with different params and same name |
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
943ab34
to
139c6ea
Compare
|
Nice analysis @micieslak! Glad you've raised it and I totally agree that it would be great to have a discussion on this. I'm not convienced though that reorganizing the code to favor signal-handler chains from the leaf to appMain over I'll favor the approach proposed by @caybro. Looks to me like it will reduce the boilerplate code needed to forward the signal from one leaf component all the way to AppMain. It's reducing the bug surface, implementation/debugging time and we'll also have loosely coupled components. We already have the There are some caveats though, well described by @micieslak. But maybe we can solve it in the I see two main issues:
One good example would be the call below. Here we have 2 issues. The first one is that the modal itself becomes a time bomb and could crash the app on a community tokens model change. The second one is that the |
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
139c6ea
to
0638cf0
Compare
Hey @alexjba yesterday I had a discussion with Michal and thought that the pasing signal down to Appmain is perhaps the best solution thus far. I have tried to show it here in this commit 0638cf0 so we can discuss if we like this approach or not and move forward accordingly. I like the idea of multiple global signals too, however, s Michal pointed out, this introduces the need for a smaller component to know what should be done for the action it wants, and maybe thats best abstracted and handled elsewhere instead. |
Yeah, that's the approach I'd prefer...
I'm not a big fan of those long call chains, it's very error prone, esp. when you want to refactor the components in the middle, or add a new chain. That said, I think the SendModal itself should go to Popups.qml (just like any other shared modal), there's no need to instantiate it AppMain.qml. Then, combined with the above purpose-driven functions/signals, we can avoid the clutter of having a huge signal that contains every possible parameter. One objection that @micieslak had to this approach was that Popups.qml is already too big -> I proposed to split it into two: WalletPopups.qml (wallet related stuff) and Popups.qml (everything else), the usage would remain the same. Wdyt? :) |
I don't mean to block your work here by extending the discussion! 😄 Glad you've reached an agreement to move forward! I'm all for it! @micieslak has a good point and IMO it's not really about this PR here, but about how do we want to structure the "global" signals. It's more of a general issue with the |
Hey @caybro I have moved SendModal to Popups.qml for now. Discussed with Michal and maybe we still need to thin about how we wish to split this further. I also like the approach of having multi signals better than passing signal along the whole chain. But I am open to whichever we think is best. @micieslak any objections on having multi functions defined as per scope instead? |
Not at all. @alexjba I think its important we have this discussion so that we implement things the right way. I also prefer the multi function handling instead. Lets see what MIchal has to say. |
| @@ -40,6 +40,9 @@ Item { | |||
| signal changePubKey(ensUsername: string) | |||
| signal goToWelcome(); | |||
| signal goToList(); | |||
| signal connectUsername(string publicKey, string ensName) | |||
There was a problem hiding this comment.
I'd strongly recommend to follow convention with *Requested at the end, connectUsernameRequested. For consistency and to underline it's just a request at this stage, not the actual action.
There was a problem hiding this comment.
Whatever is known externally, should be skipped in the signal. So publicKey is not needed as it's always own pubkey and the caller "knows" that.
| onConnectUsername: root.launchSendModal(Constants.SendType.ENSSetPubKey /*sendType*/, | ||
| "" /*senderAddress*/, | ||
| Constants.ethToken /*tokenId*/, | ||
| Constants.TokenType.ERC20 /*tokenType*/, | ||
| LocaleUtils.numberToLocaleString(0) /*tokenAmount*/, | ||
| selectedChainId /*chainId*/, | ||
| publicKey, | ||
| ensName) | ||
| onRegisterUsername: root.launchSendModal(Constants.SendType.ENSRegister /*sendType*/, | ||
| "" /*senderAddress*/, | ||
| !!d.sntToken && !!d.sntToken.symbol ? d.sntToken.symbol: "" /*tokenId*/, | ||
| Constants.TokenType.ERC20 /*tokenType*/, | ||
| LocaleUtils.numberToLocaleString(10) /*tokenAmount*/, | ||
| selectedChainId /*chainId*/, | ||
| publicKey, | ||
| ensName) | ||
| onReleaseUsernameRequested: root.launchSendModal(Constants.SendType.ENSRelease /*sendType*/, | ||
| senderAddress, | ||
| Constants.ethToken /*tokenId*/, | ||
| Constants.TokenType.Native /*tokenType*/, | ||
| LocaleUtils.numberToLocaleString(0) /*tokenAmount*/, | ||
| selectedChainId /*chainId*/, | ||
| publicKey, | ||
| ensName) |
There was a problem hiding this comment.
It would be more pragmatic to just re-emit those signals there:
So, in public api:
signal connectUsernameRequested(string ensName)
signal registerUsernameRequested(string ensName)
signal releaseUsernameRequested(string senderAddress, string ensName)and here just:
onConnectUsernameRequested: root.connectUsernameRequested(ensName)
// same for other two
ui/app/mainui/AppMain.qml
Outdated
| onLaunchSendModal: { | ||
| let ensUsernamesStore = appMain.rootStore.profileSectionStore.ensUsernamesStore | ||
| popups.openSendModal(sendType, senderAddress, tokenId, tokenType, tokenAmount, chainId, | ||
| ensUsernamesStore.getEnsRegisteredAddress() /*recipientAddress*/, | ||
| SendPopups.Helpers.RecipientAddressObjectType.Address /*recipientType*/, | ||
| false /*onlyAssets*/, | ||
| false /*interactive*/, | ||
| publicKey, ensName) | ||
| } |
There was a problem hiding this comment.
Assuming that ProfileLayout has:
signal connectUsernameRequested(string ensName)
signal registerUsernameRequested(string ensName)
signal releaseUsernameRequested(string senderAddress, string ensName)we could have sth like that here:
onConnectUsernameRequested: popups.connectUsername(ensName)Then all the final calls to openSendModal (the one with the full list of parameters for send modal) can be done only in Popups (and at some point later maybe in dedicated component controlling all use cases of send modal). openSendModal may be kept as a private method of Popups.
It also allows to encapsulate any additional actions in Popups, like obtaining addtional info from some stores or models (like let ensUsernamesStore = appMain.rootStore.profileSectionStore.ensUsernamesStore).
There was a problem hiding this comment.
So, especially when switching from Popups to dedicated component, we will have one place handling all operations that are intended to be done via send modal. Only here we obtain additional data needed for send modal, only here we configure the actual send modal. Whenever we need to refactor usages of send modal, everything is basically in one place then.
There was a problem hiding this comment.
hmmm so we do end up having multiple function on the Popups.qml file for launching send. How do we feel about those being calls to specific Global function instead so that we dont have to pass the information along the whole chain? which is then inline with with Alex and Lukas also mentioned?
There was a problem hiding this comment.
Functions are more flexible, because you can default the arguments, whereas for signals, you must spell all of them out. Otherwise connecting a signal to another signal is fine for sure
|
Thank you @Khushboo-dev-cpp @alexjba and @caybro for this in-depth discussion. I'd like to summarize what we concluded so far and go bit deeper into details from perspective of compliance with SOLID. We have two approaches on the table:
The latter one, at least at first glance seems to be concise and elegant. However there are some caveats that we should keep in mind, especially if we want to introduce app-wise mechanism reusable in various scenarios, respecting all SOLID rules. Let's consider very simple scenario: // A.qml
Item {
Button {
onClicked: Global.doSomething(someArg)
}
}Assume that we have that component And now, design for new functionality arrived and it turns out that we need to reuse our component How we can do that? Probably the only option is to do it as follows: // A.qml
Item {
property bool isDoSomethingSupported
Button {
onClicked: Global.doSomething(someArg, isDoSomethingSupported)
}
}So in order to change behavior external for component From So if our target is to decompose the app into set of isolated, independent and reusable component, this example is contradiction of all 3 (and In comparison, none of these problems occur in the first approach (emitting signals notifying intent). Let's go back to the moment where we need to use our exemplary component
Ideally 1 and 2 should be enough to learn everything is needed to use that component (example: we don't need to read source code of In comparison in the first approach the user of So once again, if the target is to have well-defined components, first approach supports it much better because enforces APIs with all interactions listed explicitly. I'm not sure about the argument that this solution (1st one) is more error-prone because of signals propagation. By reducing number of parameters (the handling side is responsible to fetch whatever is needed from proper models) we already reduce the need of frequent changes. If the signal in low-level component needs to be changed, we need to refactor the whole chain up, to the location where it's handled. But those components are actually really affected and that change should be explicitly visible in their API. In the case in this PR the chain is long indeed because it goes to the Yeah, in the current codebase, where many top-level components are super messy, adding new signals in order to re-emit sth from sub-components in order to improve architecture may seem counter-intuitive. But I think explicit APIs are always better than some shortcuts done somewhere in internals, and in long term it will result in much better code structure. @alexjba @caybro @Khushboo-dev-cpp please consider those arguments. My choice is clear, I'm voting for solution in spirit of SOLID because it usually pays off quickly. I provided example against solution 2 which is possible in practice, but this is not exhaustive, I could describe several other problems. The verbosity of propagation is not a problem for me because it's needed for well-formed api. On the other hand I may be missing something similarly problematic but in solution 1. If you see sth please share! |
|
@micieslak @caybro @Khushboo-dev-cpp WDYT if we move the discussion in a meeting? Would nice be for the discussion to result in a clear path forward, with some tasks and an action plan. We all agree there are some issues to address, but I think it's no longer in the PR scope! My suggestion is to unblock @Khushboo-dev-cpp with this PR until we'll get an agreement. We can always revisit if needed! |
|
|
||
| signal cancelClicked | ||
| signal sendRequested(var sendType, var senderAddress, var tokenId, var tokenType) |
There was a problem hiding this comment.
| signal sendRequested(var sendType, var senderAddress, var tokenId, var tokenType) | |
| signal sendRequested(int sendType, string senderAddress, string tokenId, int tokenType) |
| property EnsUsernamesStore ensUsernamesStore | ||
| property string username: "" | ||
|
|
||
| signal backBtnClicked() | ||
| signal registerUsername() | ||
|
|
||
| QtObject { | ||
| id: d | ||
| readonly property var sntToken: ModelUtils.getByKey(root.accountAssetsModel, "tokensKey", root.ensUsernamesStore.getStatusTokenKey()) |
There was a problem hiding this comment.
This is dangerous. In case of a model reset the app could crash.
ModelEntry would help here. Same in ui/app/AppLayouts/Profile/ProfileLayout.qml
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
0638cf0
to
061393c
Compare
… architecture guidelines. After this change there is not need to pass sendModal instance from AppMain to other parts of app. Then SendModal should be launched simply by calling Global.openSendModal(....)
Khushboo-dev-cpp
force-pushed
the
feat/UpdateLaunchSendModalMechanism
branch
from
061393c
to
34ae4ae
Compare
What does the PR do
Changes the SendModal launching mechanish.
Update signals in Global for openSendModal to be parameterised, instead of having to pass sendModal instance across the app.
Affected areas
SendModal Invocations points from-
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
SendVia1-1Chat.mov
TransferOwnership.mov
TransactionsHistorySend.mov
Screen.Recording.2024-11-05.at.9.49.43.PM.mov
profileShowcaseMenu.mov
BuyStickers.mov
MainWalletSavedAddresses.mov
MainWalletViewSend.mov
RegisterEns.mov