fix(profile_image): show the contact icon when no display name is set #17866
Conversation
jrainville
requested review from
micieslak,
caybro,
alexjba and
a team
as code owners
| } else if (mouse.button === Qt.LeftButton) { | ||
| Global.openProfilePopup(model.pubKey) | ||
| onRightClicked: function () { | ||
| // FIXME: onClicked is called at the same time as onRightClicked (could be a QT5 only issue) |
There was a problem hiding this comment.
This is a fix because the clicks didn't work anymore, but now the right-click is also broken.
It probably got introduced during the QT6 migration cc @micieslak @caybro
There was a problem hiding this comment.
Were you testing on Qt5 or Qt6? Anyway, feel free to report a separate issue for this component, I'll have a look
| @@ -46,6 +46,8 @@ SQUtils.QObject { | |||
| preferredDisplayName: "everyone" | |||
| icon: "" | |||
| colorId: 0 | |||
| colorHash: [] | |||
There was a problem hiding this comment.
This fixes the issue with the color ring not showing in the suggestion box
Jenkins BuildsClick to see older builds (27)
|
jrainville
force-pushed
the
fix/profile-icon-with-no-displayname
branch
2 times, most recently
from
f24c834 to
779d843
Compare
There was a problem hiding this comment.
The rules of display name resolution are complicated and have been changed multiple times. I think it would be beneficial to summarize the rules in a single place as a documentation somewhere in the code (or external doc if needed). Maybe it will unveil need to do some another round of cleanup here.
| @@ -175,6 +177,8 @@ QtObject: | |||
| result = newQVariant(item.airdropAddress) | |||
| of ModelRole.MembershipRequestState: | |||
| result = newQVariant(item.membershipRequestState.int) | |||
| of ModelRole.UsesDefaultName: | |||
| result = newQVariant(resolveUsesDefaultName(item.localNickname, item.ensName, item.displayName)) | |||
There was a problem hiding this comment.
IMO it's preffered for the data call to return a member value as is and not to do any computation. The reasoning would be that data needs to support heavy traffic.
We've seen before some apparently harmless nim computations in data calls that proved to be very expensive.
This seems harmless indeed, but I don't see any benefit when compared to the approach you've used in src/app/modules/shared_models/message_model.nim.
There was a problem hiding this comment.
also, would be nice to have more granular dataChanged. We're emitting UsesDefaultName changed event even when it's not actually changing.
There was a problem hiding this comment.
Good idea. I updated so that the model just passes the Item's value and I also refactored a bit so that we have an Item creator function in the model to reduce duplication
| property string image | ||
| property bool showRing: !ensVerified && !root.isBridgedAccount | ||
| property bool interactive: true | ||
| property bool disabled: false | ||
| property bool ensVerified: false | ||
| property bool loading: false | ||
| property bool isBridgedAccount: false | ||
| property int onlineStatus: Constants.onlineStatus.unknown | ||
| // TODO replace this with booleans since we do not have access to Constants |
There was a problem hiding this comment.
A local enum would also work
jrainville
force-pushed
the
fix/profile-icon-with-no-displayname
branch
from
779d843 to
78f74bf
Compare
| } | ||
| return "" | ||
| } | ||
| asset.width: isContactIcon ? iconWidth * 0.9 : iconWidth |
There was a problem hiding this comment.
Why is this scaling the icon down? 🤔
There was a problem hiding this comment.
The icon was too big when I was working on it and I thought this is what fixed it, but turns out it's not, I just tried removing it and it doesn't change anything.
I think I need to find a way to fix it though, because it's still too big compared to the design.
Design:
Implementation:
| @@ -418,7 +418,7 @@ Item { | |||
| icon.name: model.icon | |||
| icon.color: Theme.palette.userCustomizationColors[root.colorIdForPubkeyGetter(model.pubKey)] | |||
| status: model.onlineStatus | |||
| ringSettings.ringSpecModel: root.ringSpecModelGetter(model.pubKey) | |||
There was a problem hiding this comment.
You might now remove the toplevel property var ringSpecModelGetter and adjust the usages then
| isImage: true | ||
| readonly property bool isContactIcon: !root.image && root.usesDefaultName | ||
|
|
||
| width: isContactIcon ? Math.floor(root.imageWidth * 0.9) : root.imageWidth |
| } else if (mouse.button === Qt.LeftButton) { | ||
| Global.openProfilePopup(model.pubKey) | ||
| onRightClicked: function () { | ||
| // FIXME: onClicked is called at the same time as onRightClicked (could be a QT5 only issue) |
There was a problem hiding this comment.
Were you testing on Qt5 or Qt6? Anyway, feel free to report a separate issue for this component, I'll have a look
jrainville
force-pushed
the
fix/profile-icon-with-no-displayname
branch
from
78f74bf to
40c8c13
Compare
Fixes #17487 When no displayName, nickname, ens name or user image are set, we show the contact icon instead, in accordance to the design. Also fixes an issue with the SuggestionBox where the colorHash was not set correctly. Then fixes an issue where right-click on contact items didn't work Finally, this refactors some parts of the code to use UserImage instead of SmartIdenticon, as it simplifies the code and its maintenance
jrainville
force-pushed
the
fix/profile-icon-with-no-displayname
branch
from
40c8c13 to
0c18ab2
Compare
What does the PR do
Fixes #17487
When no displayName, nickname, ens name or user image are set, we show the contact icon instead, in accordance to the design.
Also fixes an issue with the SuggestionBox where the colorHash was not set correctly. Then fixes an issue where right-click on contact items didn't work
Finally, this refactors some parts of the code to use UserImage instead of SmartIdenticon, as it simplifies the code and its maintenance
Affected areas
Any place where the User image/icon is shown
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
contacts-icon.webm
Impact on end user
Small impact. Only changes the icon used when the user doesn't have a displayName yet.
Also fixes small issues.
How to test
Risk
More of a medium risk since in my opinion, the worst case scenario is that the icon will just use the old Letters wrongly.
But I did refactor some small parts, so it's safer to do some good testing.
Tick one: