Refactor/16286 refactor profile context menu #16334
Conversation
Jenkins BuildsClick to see older builds (42)
|
iurimatias
force-pushed
the
refactor/16286_refactor_profile_context_menu
branch
from
4391fe6
to
88d1f1f
Compare
There was a problem hiding this comment.
Nice work! Looks promising in general but there are several things to make it better:
-
Please use one Storybook page per component. Then it's clear that provided controls are for that given component. Ideally the whole API of the component should be accessible in the page via controls like e.g. check boxes for flags or text fields for strings.
-
For the popups/menus it's handy to instantiate the component in a following way:
MessageContextMenuView { id: menu anchors.centerIn: parent hideDisabledItems: false visible: true closePolicy: Popup.NoAutoClose } Button { text: "reopen" anchors.centerIn: parent onClicked: menu.open() }
Than it's not necessary to re-open the menu after every change via the provided controls. It's also not necessary to create it dynamically via
createObject(). -
Do not pass stores to low-level like those menus, especially that only tiny flag from the store is used (
isDebugEnabled). It's better to expose bool propertyisDebugEnabledand take just a singleboolinstead of the whole store object. -
As mentioned above, store should not be part of the API. But whenever stores are part of the api, they should be mocked in a storybook page in a following way:
MessageContextMenuView { id: menu store: ChatStores.RootStore { readonly property bool isDebugEnabled: true } }
-
Please keep only necessary imports, in components and in SB pages as well.
-
Separate components for
BridgeProfileContextMenu,RegularProfileContextMenu,SelfProfileContextMenu,BlockedProfileContextMenuwhich are then aggregated inProfileContentMenuare not good idea for serveral reasons:- huge code duplication, e.g.
ProfileHeader {part is almost the same in all variants - need of using
Loaderand complicated imperative code inonLoadedhandler doing assignments to type-erasureditemobject - overgrown api of
ProfileContextMenutaking stuff necessary for all 4 variants. E.g. it's not clear without reading the impl details that forselfversiononlineStatusis irrelevant. ProfileContextMenuisItemso we loose access toPopupapi which may be needed on the caller side
Probably the better idea would be to create two top-level variants for own profile and another for contacts.
- huge code duplication, e.g.
|
wrt to 6, this a common side effect of this pattern, which is sort of the exception to the rule for DRY, in this case although it introduces some duplication it makes everything quite simple, joining them on the otherhand will introduce (like before) a lot overcomplicated if/elseif/else conditions which is easy to get lost in
meaning the caller chooses which one to instantiate/open? |
|
also why would profile and "contacts" be the variants? why not say, contacts and blocked etc.. |
|
wrt 4) I'm not sure what you're reffering to, did this PR introduce stores anywhere? (that were not already there at least) I do see some components still have the store thing which is not used, I just forgot to remove it |
|
wrt
|
|
wrt 2) is this something that was already there or was introduced in this PR? if the former I think it would be helpful to do it in a different stage/PR as this is already a big improvement and lots of changes to be tested too. furthermore this PR helps with other actual issues to do after this. |
|
wrt 3) What is this refering to? I don't see any |
|
|
||
| StatusMenu { | ||
| Item { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
There was a problem hiding this comment.
this is not in use actually, it should be removed from here and the sub components
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
There was a problem hiding this comment.
should be removed, not in use
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
There was a problem hiding this comment.
should be removed, not in use
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
There was a problem hiding this comment.
should be removed, not in use
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
There was a problem hiding this comment.
should be removed, not in use
|
Hey @iurimatias, some points are listed are probably not directly about your changes but about the general current state indeed. Like about storybook page - I haven't noticed it was like that before. It should be improved and split into two pages (
I think that the biggest benefit you gained is achieved by simplifying API of Something that we probably should reconsider is location of |
There was a problem hiding this comment.
Nice work! 🍻 I'm glad that these components are getting some attention.
I've done some initial testing and found no regressions.
Added a few findings below. Some points have already been touched by @micieslak, but I think it's worth reiterating them as it brings some long term value.
ui/imports/utils/Utils.qml
Outdated
| @@ -455,6 +455,44 @@ QtObject { | |||
| } | |||
| } | |||
|
|
|||
| function getProfileContext(publicKey, myPublicKey, isBridgedAccount = false) { | |||
| const contactDetails = getContactDetailsAsJson(publicKey, true, true); | |||
There was a problem hiding this comment.
There's this effort of removing the nim access from singletons. Adding this function here will add more work for later.
I'd move this to ContactsStore.qml if it's really needed.
| @@ -85,6 +86,46 @@ SplitView { | |||
| ProfileContextMenu { | |||
| anchors.centerIn: parent | |||
| hideDisabledItems: false | |||
| profileType: profileTypeSelector.currentText | |||
There was a problem hiding this comment.
There's a warning herefile:///Users/alexjbanca/Repos/status-desktop/storybook/pages/MessageContextMenuPage.qml:89:21: Unable to assign QString to int
There was a problem hiding this comment.
This could become the base component for the other 3.
Then we'd have a ProfileMenuBase containing the ProfileHeader. In this case other components won't have to duplicate the header. WDYT?
The advantage here would be that the other components will be much cleaner with this logic extracted in a base component. We'd probably end-up with simple sets of actions for each variation.
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
| StatusMenu { | ||
| id: root | ||
|
|
||
| property ChatStores.RootStore store |
| signal removeContact(string publicKey) | ||
| signal blockContact(string publicKey) | ||
|
|
||
| onClosed: { |
There was a problem hiding this comment.
Please remove the onClosed handler from all menu components. The issue with this handler is that it's breaking the bindings set from the outside. A component should not break the bindings on its public API.
If we need to reset the data here it's better to do it from the outside. Also, the reset should cover all the input properties. But I think it's fine without the reset, right?
| topPadding: root.topPadding | ||
| visible: !root.isBridgedAccount | ||
| } | ||
| onLoaded: { |
There was a problem hiding this comment.
I'd do it the other way: Each component to bind its properties and signals to the root properties and signals.
The are a few advantages:
- we won't need this imperative approach and all the bindings would work as expected.
- No need to create most of the the
ifbranches - No need for the manual connections
- Failing fast in case of type errors at qml compilation time
And probably more.
In general it's a good approach to choose the declarative approach as it brings many advantages.
| Global.openProfilePopup(publicKey, null) | ||
| } | ||
| onCreateOneToOneChat: (communityId, chatId, ensName) => { | ||
| Global.changeAppSectionBySectionType(Constants.appSection.chat) | ||
| root.rootStore.chatCommunitySectionModule.createOneToOneChat("", chatId, ensName) | ||
| } | ||
| onReviewContactRequest: (publicKey) => { | ||
| const contactDetails = publicKey === "" ? {} : Utils.getContactDetailsAsJson(publicKey, true, true) |
There was a problem hiding this comment.
WDYT of using the ContactDetails component as a helper for the ProfileContextMenu instead of Utils.getContactDetailsAsJson usages. I think it would simplify our life quite a bit.
We'd end-up with something like this:
Global.openMenu(profileContextMenuComponent, sender, { selectedUserPublicKey: publicKey })
...
ProfileContextMenu {
ContactDetails {
id: contactDetails
publicKey: selectedUserPublicKey
contactsStore: root.contactsStore
profileStore: root.profileStore
}
selectedUserDisplayName: contactDetails.displayName
selectedUserIcon: contactDetails.icon
profileType: Utils.getProfileType(publicKey, myPublicKey, isBridgedAccount, isBlocked);
contactType: Utils.getContactType(contactDetails.contactRequestState, contactDetails.isContact);
trustStatus: contactDetails.trustStatus
ensVerified: contactDetails.ensVerified
onlineStatus: contactDetails.onlineStatus
hasLocalNickname: !!contactDetails.localNickname
}
Would be a step in the right direction in the sense that we won't need to revisit this code when removing usages of Utils.getContactDetailsAsJson. Another big advantage IMO is that it completely removes the need of adding more dependencies to the nim layer in the Utils singleton (Utils.getProfileContext).
There was a problem hiding this comment.
The same could be done for ContactsView, UsersPanel. My hope is that the code could eventually be extracted in a single place if we only need to specify the publicKey to the ProfileContextMenu.
There was a problem hiding this comment.
not sure I understood what you want here, may I suggest in a different PR though?
There was a problem hiding this comment.
sounds good to me! The idea here is to use ContactDetails instead of getContactDetailsAsJson where possible.
| onOpenProfileClicked: (publicKey) => { | ||
| console.log("MessageView.qml:1: open profile clicked", publicKey) |
| @@ -170,13 +173,48 @@ Item { | |||
| ProfileContextMenu { | |||
| store: root.store | |||
| margins: 8 | |||
| onOpenProfileClicked: { | |||
| onOpenProfileClicked: (publicKey) => { | |||
| console.log("UserListPanel.qml:1: open profile clicked", publicKey) | |||
There was a problem hiding this comment.
I'm glad ProfileContextMenu is back to a StatusMenu 👍 Looks great
| @@ -85,6 +86,46 @@ SplitView { | |||
| ProfileContextMenu { | |||
| anchors.centerIn: parent | |||
| hideDisabledItems: false | |||
| profileType: profileTypeSelector.currentText | |||
There was a problem hiding this comment.
| profileType: profileTypeSelector.currentText | |
| profileType: profileTypeSelector.currentValue |
| onTriggered: { | ||
| root.createOneToOneChat("", root.selectedUserPublicKey, "") | ||
| root.close() | ||
| } |
There was a problem hiding this comment.
Can be removed. The menu closes by itself after triggering the action
| onTriggered: { | |
| root.createOneToOneChat("", root.selectedUserPublicKey, "") | |
| root.close() | |
| } | |
| onTriggered: root.createOneToOneChat("", root.selectedUserPublicKey, "") |
| onTriggered: { | ||
| root.openProfileClicked(root.selectedUserPublicKey) | ||
| root.close() | ||
| } |
There was a problem hiding this comment.
| onTriggered: { | |
| root.openProfileClicked(root.selectedUserPublicKey) | |
| root.close() | |
| } | |
| onTriggered: root.openProfileClicked(root.selectedUserPublicKey) |
| signal markAsUntrusted(string publicKey) | ||
| signal removeTrustStatus(string publicKey) | ||
| signal removeContact(string publicKey) | ||
| signal blockContact(string publicKey) | ||
|
|
||
| onClosed: { |
There was a problem hiding this comment.
It's a good chance to remove this onClosed handler. It's not needed and potentially a source of bugs because it's breaking the bindings. We're currently destroying the menu on close anyways.
iurimatias
force-pushed
the
refactor/16286_refactor_profile_context_menu
branch
2 times, most recently
from
2349473
to
93f17b2
Compare
There was a problem hiding this comment.
Looks nice already, much simpler than initially.
Couple minor suggestions left.
What I'd love to see here is splitting storybook pages. Now the page is MessageContextMenuPage.qml, for two components: MessageContextMenuView and ProfileContextMenu.
The convention, respected almost in all pages is the rule 1 page per 1 component, with naming convention: MyComponent -> MyComponentPage.
So we should have
MessageContextMenuViewPage and ProfileContextMenuPage instead of the single one used now. Also the previously left suggestions how to instantiate menus/popups would improve readability/usability of the page as well.
| property bool ensVerified: false | ||
| property int onlineStatus: Constants.onlineStatus.unknown | ||
| property bool hasLocalNickname: false | ||
| property int profileType: Constants.profileType.regular | ||
|
|
||
| signal openProfileClicked(string publicKey) |
There was a problem hiding this comment.
Signals could be simplified because publicKey is always known at the caller side. Providing some kind of identifier is necessary only when the component maintains more than one item of given type, e.g. in a list.
signal openProfileClicked
signal createOneToOneChat
signal reviewContactRequest
signal sendContactRequest
signal editNickname
signal removeNickname
signal unblockContact
signal markAsUntrusted
signal removeTrustStatus
signal removeContact
signal blockContact
Especially confusing is
signal createOneToOneChat(string communityId, string chatId, string ensName) because is triggered only in one place in the following way:
root.createOneToOneChat("", root.selectedUserPublicKey, "")
So communityId and ensName are always empty strings and chatId is the same publicKey as in other signals.
| property string selectedUserPublicKey: "" | ||
| property string selectedUserDisplayName: "" | ||
| property string selectedUserIcon: "" |
There was a problem hiding this comment.
Minor thing: those names could be simplified to just publicKey, displayNameandicon`. The prefix doesn't disambiguate anything, so probably can be removed.
| @@ -178,4 +178,42 @@ QtObject { | |||
| function getLinkToProfile(publicKey) { | |||
| return root.contactsModule.shareUserUrlWithData(publicKey) | |||
| } | |||
|
|
|||
| function getProfileContext(publicKey, myPublicKey, isBridgedAccount = false) { | |||
| const contactDetails = Utils.getContactDetailsAsJson(publicKey, true, true); | |||
There was a problem hiding this comment.
We are not using ; in qml code. Not a big deal, but for consistency it would be nice to keep the no ; convention :)
| const onlineStatus = contactDetails ? contactDetails.onlineStatus : Constants.onlineStatus.unknown; | ||
| const hasLocalNickname = contactDetails ? !!contactDetails.localNickname : false; | ||
|
|
||
| return { profileType, trustStatus, contactType, ensVerified, onlineStatus, hasLocalNickname }; |
There was a problem hiding this comment.
I would consider early return to increase readability of that function:
function getProfileContext(publicKey, myPublicKey, isBridgedAccount = false) {
const contactDetails = Utils.getContactDetailsAsJson(publicKey, true, true);
if (!contactDetails) {
return {
profileType: getProfileType(publicKey, myPublicKey, isBridgedAccount, false),
trustStatus: Constants.trustStatus.unknown,
contactType: getContactType(Constants.ContactRequestState.None, false),
ensVerified: false,
onlineStatus: Constants.onlineStatus.unknown,
hasLocalNickname: false
}
}
const isBlocked = contactDetails.isBlocked
const profileType = getProfileType(publicKey, myPublicKey, isBridgedAccount, isBlocked)
const contactType = getContactType(contactDetails.contactRequestState, contactDetails.isContact)
const trustStatus = contactDetails.trustStatus
const ensVerified = contactDetails.ensVerified
const onlineStatus = contactDetails.onlineStatus
const hasLocalNickname = !!contactDetails.localNickname
return { profileType, trustStatus, contactType, ensVerified, onlineStatus, hasLocalNickname }
}There was a problem hiding this comment.
or
function getProfileContext(publicKey, myPublicKey, isBridgedAccount = false) {
const contactDetails = Utils.getContactDetailsAsJson(publicKey, true, true);
if (!contactDetails) {
return {
profileType: getProfileType(publicKey, myPublicKey, isBridgedAccount, false),
trustStatus: Constants.trustStatus.unknown,
contactType: getContactType(Constants.ContactRequestState.None, false),
ensVerified: false,
onlineStatus: Constants.onlineStatus.unknown,
hasLocalNickname: false
}
}
return {
profileType: getProfileType(publicKey, myPublicKey, isBridgedAccount, contactDetails.isBlocked),
trustStatus: contactDetails.trustStatus,
contactType: getContactType(contactDetails.contactRequestState, contactDetails.isContact),
ensVerified: contactDetails.ensVerified,
onlineStatus: contactDetails.onlineStatus,
hasLocalNickname: !!contactDetails.localNickname
}
}| } | ||
|
|
||
| function getProfileType(publicKey, myPublicKey, isBridgedAccount, isBlocked) { | ||
| if (publicKey === myPublicKey) { |
There was a problem hiding this comment.
we usually skip {} for one liners
There was a problem hiding this comment.
Related issues found to be raised against master:
- ENS names are not being shown
- Context menu in community members list does not display profile image or name
--
Completed:
SD-343: Profile context menu for a stranger
SD-344: Profile context menu for a contact
SD-347: Profile context menu for discord bridge user
SD-346: Profile context menu for self
TR: https://ethstatus.testrail.net/index.php?/plans/view/17989
Both are definitely working in master: So it's a regression here |
|
ok we should not merge this then if there is a regression, I'll look into it |
|
that said, I just tested it and it's not happening to me 🤔 |
iurimatias
force-pushed
the
refactor/16286_refactor_profile_context_menu
branch
from
93f17b2
to
8e0b619
Compare
|
I cannot replicate the regressions, or perhaps I'm looking at the wrong place, I need more info. |
Reproduced the issues mentioned on master.
|
iurimatias
force-pushed
the
refactor/16286_refactor_profile_context_menu
branch
from
8e0b619
to
e423081
Compare
There was a problem hiding this comment.
@iurimatias , noted a couple of regressions compared to master:
-
There is no longer a line above the red items at the bottom of lists
-
Remove nickname is not moved to the lower part of the list with the other red items
-
In community settings > members, "add nickname" and "send contact request" fail to open the dialogs to perform those actions
refactor ProfileContextMenu to make it a functional component refactor ProfileContextMenu to make it a functional component refactor ProfileContextMenu to make it a functional component This refactor ProfileContextMenu to make it a functional component by: refactored out direct calls to backend, and passing backend data structures and moved this logic to the callers, also refactored common calls between the callers common types of context menus have been extracted to their sub components which removes a lot of logic too and makes the behaviour very clear user verification workflow (which was already disabled) has been removed refactor: use signals and call singletons on the parent instead remove unused code for now from profile context menu refactor profile context menu into two components; add property to storybook extract blocked profile context menu and self profile context menu use profileType instead of individual bools refactor to pass trustStatus as an argument make contact type a parameter remove unnecessary method from RegularProfileContextMenu add ensVerified property to ProfileContextMenu components add onlineStatus property to ProfileContextMenu components move ProfileContextMenu storybook controls to the right sidebar move contactDetails logic up from the view add local nickname property to ProfileContextMenu components fix issue with missing signal; fix logs in storybook use constant for profileType instead of string refactor common code into a single method refactor getProfileContext remove references to contactDetails which are not longer needed remove unnecessary comments fix bridged constant refactor into a single ProfileContextMenu component refactor into a single ProfileContextMenu component refactor into a single ProfileContextMenu component simplify imports remove unused store field move methods from utils to contacts store remove onClosed signal remove unused param rename ProfileContextMenu variables simplify signals in ProfileContextMenu remove ; refactor: do early return simplify ifs move ProfileContextMenu to its own storybook page fix wrong params fix profile context menu separator add missing signals to profile context menu on the members tab panel
iurimatias
force-pushed
the
refactor/16286_refactor_profile_context_menu
branch
from
0d80878
to
a09b773
Compare
closes #16286
What does the PR do
This refactor ProfileContextMenu to make it a functional component by:
Affected areas
Any where in which one can right click or click on
...menu to see the profile context menu, to my knoweldge this includes:...menu in contacts list in settingsImpact on end user
The before/after behaviour should be the same for the end user
How to test
note: The goal here is to refactor under the hood while keeping the same behaviour as before. For example, if a profile is in a blocked state, the trustworthy/untrustworthy mark doesn't show because it didn't show before either, if that behaviour should change is beyond the scope of this PR.
Risk
Needs further testing before merging: