Final design of Emoji Reactions part 2/2 #19160
Conversation
jrainville
requested review from
a team,
alexjba,
caybro,
micieslak and
noeliaSD
as code owners
Jenkins BuildsClick to see older builds (105)
|
|
There is one weird bug I'm running into that might be a QT bug. If the limit is reached and I remove an emoji, sometimes, not always, it's random, the recent emojis in the hover menu just disappear and the menu has a big blank. I have no idea why it does that. I'd appreciate some help on that. Worst case, it's a pretty edge case, since it requires that the limit is reached and you remove one of the reactions, plus it's random. reactions-bug.webm |
|
|
||
| StatusIcon { | ||
| id: addEmojiButton | ||
| Item { |
There was a problem hiding this comment.
This whole thing could be a small StatusButton with enabled: !addEmojiButton.isHovered
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
9cf2f15 to
20cec2f
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
5d2f2ca to
0236b03
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
20cec2f to
1bb48ed
Compare
There was a problem hiding this comment.
Great job! Tested it and in general it works fine.
I left some suggestions / comments / questions here:
Design and Implementation Questions:
- If I understand it correctly the emojis list when hovering should contain the most recent ones, right? So they are dynamically changing? I'm not seing this when testing, the hover emojis list is fixed on my end.
Screen.Recording.2025-10-30.at.12.26.43.mov
- Looking at the design, there is this subcategory of
RECENTSin the popup that is clearly separated by the subtitle. I cannot see it on the app here:
Screen.Recording.2025-10-30.at.12.28.46.mov
The design I'm referring to:
- Also I see in our implementation we order the emojis by name. Is it intended? I'd expect to be ordered by the time it was set, like in other apps.
| numberOfReactions: 2, | ||
| numberOfReactions: 232, | ||
| jsonArrayOfUsersReactedWithThisEmoji: "[\"Bob\", \"John\"]" | ||
| }, |
There was a problem hiding this comment.
Maybe we could create a specific file with the reactions model so that it's reusable and doesn't add a lot of noise to this specific page?
| anchors.rightMargin: 20 | ||
| anchors.top: parent.top | ||
| anchors.topMargin: -8 | ||
| anchors.topMargin: -4 |
There was a problem hiding this comment.
Could we use some Theme.padding to make it more dynamic?
There was a problem hiding this comment.
Same here. No standard 4.
| QtObject { | ||
| id: d | ||
|
|
||
| readonly property string addReactionButtonName: "addReactionButton" |
| anchors.verticalCenter: parent.verticalCenter | ||
| text: model.emoji | ||
| } | ||
| topPadding: 3 |
There was a problem hiding this comment.
Could be nice to use our standard paddings
There was a problem hiding this comment.
We don't have a padding of 3. Should I just add it?
There was a problem hiding this comment.
Maybe we should slightly change the design and just use the standard ones?
There was a problem hiding this comment.
The other option is to take some other padding, maybe just Theme.padding and multiply it by some factor to land with what you need when the defaultPadding padding is used. Thanks to that it will be scaling along the whole app when paddings are changed globally.
There was a problem hiding this comment.
Yeah that's roughly what I did 👍
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
0236b03 to
f76f22e
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
1bb48ed to
81bcdea
Compare
jrainville
left a comment
jrainville
left a comment
There was a problem hiding this comment.
Thanks @noeliaSD for the tests and review.
I answered your questions below. I also simplified the code by making EmojiPopup the owner of all the emojis and it now acts like a Singleton (which is already kinda acted as).
If I understand it correctly the emojis list when hovering should contain the most recent ones, right? So they are dynamically changing? I'm not seing this when testing, the hover emojis list is fixed on my end.
Good find. I fixed in the latest commit. Because I was using a Repeater, the model was parsed only once.
I added a Connections to force a model re-eval. It's not the cleanest, but it works.
I tried using a ListView instead, but it didn't behave as well and it has more complexity as a Repeater.
Looking at the design, there is this subcategory of RECENTS in the popup that is clearly separated by the subtitle. I cannot see it on the app here:
I think @caybro implemented it as you saw because it's simpler and doing the categories for real had a big performance impact.
We can try re-visiting that later, but I wouldn't make it part of this issue/PR
Also I see in our implementation we order the emojis by name. Is it intended? I'd expect to be ordered by the time it was set, like in other apps.
I didn't touch the ordering, but I agree it would be better to order by the time they were set, but we currently don't keep track of that. We could do it later, but it's more effort.
| anchors.verticalCenter: parent.verticalCenter | ||
| text: model.emoji | ||
| } | ||
| topPadding: 3 |
There was a problem hiding this comment.
We don't have a padding of 3. Should I just add it?
| anchors.rightMargin: 20 | ||
| anchors.top: parent.top | ||
| anchors.topMargin: -8 | ||
| anchors.topMargin: -4 |
There was a problem hiding this comment.
Same here. No standard 4.
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
2 times, most recently
from
b417274 to
89c5350
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
81bcdea to
e99156d
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
89c5350 to
82125e0
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
e99156d to
3fa891a
Compare
micieslak
left a comment
micieslak
left a comment
There was a problem hiding this comment.
Lgtm in general, however there are some solutions looking like workarounds. I think we could try to solve it and make cleaner.
| required property StatusEmojiModel emojiModel | ||
| required property var recentEmojis | ||
| required property string skinColor | ||
| required property SortFilterProxyModel emojiModel |
There was a problem hiding this comment.
Hmm, this is very unusual to require specifically SortFilterProxyModel in the public API. Could it be used with just ListModel containing expected roles? The plain var is expected type for models.
|
|
||
| Repeater { | ||
| id: recentEmojisRepeater | ||
| model: 5 // Only show up to 5 recent emojis |
There was a problem hiding this comment.
Why we cannot use the model directly? Especially it's already limited to 5 items 🤔
There was a problem hiding this comment.
I don't know why, but using the model directly slows things down to a crawl and also if I do model.unicode it doesn't have anything in it. So clearly it doesn't work as expected or I'm not using it right.
Anyway, I dropped the IndexFilter for now since it seems to only add unwanted complexity. Feel free to try yourself, maybe you'll use it correctly.
There was a problem hiding this comment.
--- a/ui/imports/shared/controls/chat/MessageReactionsRow.qml
+++ b/ui/imports/shared/controls/chat/MessageReactionsRow.qml
@@ -30,19 +30,25 @@ Row {
Repeater {
id: recentEmojisRepeater
- model: 5 // Only show up to 5 recent emojis
+
+ model: SortFilterProxyModel {
+ sourceModel: root.emojiModel
+ filters: IndexFilter {
+ maximumIndex: 4
+ }
+ }
+
delegate: EmojiReaction {
id: emojiReaction
+ required property string unicode
+ required property string emoji
- required property int index
- property var emoji: root.emojiModel.get(index)
-
- emojiId: emojiReaction.emoji.unicode
+ emojiId: unicode
anchors.verticalCenter: parent.verticalCenter
// TODO not implemented yet. We'll need to pass this info
// reactedByUser: model.didIReactWithThisEmoji
onToggleReaction: {
- root.toggleReaction(emojiReaction.emoji.emoji)
+ root.toggleReaction(emoji)
}
}
}Checked, works like a charm for me. No slowness observed.
There was a problem hiding this comment.
It works! Thanks! I think the problem was that I was using model.unicode
| return | ||
| Connections { | ||
| target: root.emojiModel | ||
| onRecentEmojisUpdated: { |
There was a problem hiding this comment.
This is kind of hidden dependency. Reading only the public API of that component, developer cannot know that some extra signal emitted from SFPM is assumed to be handled there. The question is why it's needed at all?
There was a problem hiding this comment.
I need a signal to know that I need to update the MessageReactinsRow.
Since it uses a Repeater, if I add a new recent emoji, the Row doesn't update, because it was already generated.
I use the signal to re-do the Repeater.
I know it's not very clean. Maybe instead I could run the Repeater only when the Row is visible? So when you hover off and on again, it will be updated, so no need to have that signal?
There was a problem hiding this comment.
Ok it works. I'll remove the signal
|
|
||
| implicitHeight: 22 | ||
| implicitWidth: childrenRect.width | ||
| spacing: 4 |
There was a problem hiding this comment.
Should be derived from some padding probably.
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
82125e0 to
8a3ee65
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
8a3ee65 to
50fe136
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
3fa891a to
8e22047
Compare
|
@micieslak thanks for the review. I addressed your comments |
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
8e22047 to
2acc6bc
Compare
micieslak
left a comment
micieslak
left a comment
There was a problem hiding this comment.
When testing the repeater thing I noticed something wrong:
Screencast.from.14.11.2025.16.53.35.webm
- Different icon that the selected is added
- When selecting custom one, the box is bigger
|
@micieslak I fixed it in the last commit. The problem was that I was using the raw emoji as the thing we send and parse. That caused the problem with the different emojis selected, but also didn't help for the height. The one downside is that if you used the previous version and put emojis, they will not appear correctly, but since only devs used them, I think it's ok. Also, it's just emoji reactions. |
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
2acc6bc to
26c8987
Compare
micieslak
left a comment
micieslak
left a comment
There was a problem hiding this comment.
Almost there!
But there is other problem now - the old reactions are not visible and not interactive.
Other a bit annoying problem is with the popup: it cannot be dismissed by clicking outside but within the current message.
Another issue is order of reactions - as a user I would expect them to be in the order of addition. Now newly added reactions jump somewhere in the middle sometimes.
Yeah, as noted above, it's because of the API change, but that problem will only affect devs, so it should be fine.
Agreed it's annoying. I think there is already an issue for that. I think the problem appeared during the QT6 migration
Indeed, it's not intuitive. We don't have a clock on the reactions. I'll see if I can just try to make it use the order of added |
jrainville
force-pushed
the
feat/emoji-reaction-final-design-p2
branch
from
26c8987 to
0eb11f0
Compare
Ok that was fixed. I didn't realize we had a custom ordering code for new reactions. I guess it was to make sure the old reactions were in the same order as the menu |
5 participants
What does the PR do
Fixes #18935
Based on top of #19140
Adds the remaining commits that implements the final design for emoji reactions.
Commits:
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
emojis-reactions-design-p2.webm
Impact on end user
Makes the reactions look nice and easier to use
How to test
Risk
Low