Part of Emoji Reaction final design #19140
Conversation
jrainville
requested review from
a team,
alexjba,
caybro,
micieslak and
noeliaSD
as code owners
| visible: index < d.recentEmojisModel.count | ||
| emojiId: visible ? emojiReaction.emoji.unicode : "" | ||
| // TODO not implemented yet. We'll need to pass this info | ||
| // reactedByUser: model.didIReactWithThisEmoji |
There was a problem hiding this comment.
Will be implemented in a follow up PR
| } | ||
|
|
||
| StatusMenuSeparator { | ||
| // FIXME there is a padding on each side |
There was a problem hiding this comment.
This exists in master and 2.35 too. It's not the end of the world, but it doesn't match the design and I don't know how to fix
Jenkins BuildsClick to see older builds (79)
|
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
177463b to
5d2f2ca
Compare
|
|
||
| Repeater { | ||
| model: root.defaultEmojiReactionsModel | ||
| model: 5 // Only show up to 5 recent emojis |
There was a problem hiding this comment.
You could have an IndexFilter on the above SFPM, to return only [0,5[ items
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
2 times, most recently
from
0236b03 to
f76f22e
Compare
There was a problem hiding this comment.
Looks good overall!
-
I’m just thinking about how we manage the recent emojis, right now they’re duplicated both in
SQUtils.Emoji.emojisModeland inappMainLocalSettings.recentEmojis, and we’re manually syncing one with the other. That feels a bit error-prone.
Maybe we could simplify by having a single source of truth, for example, letemojisModelown the recents completely, loading them fromappMainLocalSettingson startup and then keeping the two bound so updates automatically persist. That way we avoid having two live copies and reduce the chance of desync issues. WDYT? -
One more architectural thought: why is the
emojisModelliving underSQUtils.Emoji?
What I'm thinking is:
- SQUtils.Emoji → source of truth for static emoji data + stateless utilities (catalog, metadata, search/indexing).
- App store (e.g., EmojiStore) → owns stateful, app-specific features like recents, favorites, custom ordering, write-through persistence, etc.
Concretely, we could:
- Keep
SQUtils.Emojias a data provider (expose allEmojisModel / search API, no app state). - Introduce an
EmojiStorethat wraps the provider and exposes:
- recentModel, favoritesModel (read-only models)
- methods: toggleFavorite(emoji), clearRecents()..
- persistence (load on startup from settings, boounded on change)
- UI binds only to EmojiStore.XXModel ; SQUtils.Emoji never leaks app state.
Benefits: clearer layering, easier testing/mocking, and we avoid coupling utils to product decisions (like which are the recents we keep).
WDYT?
| StatusEmoji { | ||
| id: statusEmoji | ||
| anchors.centerIn: parent | ||
| width: 20 |
There was a problem hiding this comment.
Do we want a fixed size or something dynamic for the times coming? ;)
There was a problem hiding this comment.
I followed the design, but you're right, the design was for Desktop only. What do you suggest to make it more dynamic?
| id: root | ||
|
|
||
| property var defaultEmojiReactionsModel | ||
| required property StatusEmojiModel emojiModel |
There was a problem hiding this comment.
It would be nice to describe which are the properties expected for this specific model
| if (!root.recentEmojis) { | ||
| return | ||
| } | ||
| root.emojiModel.recentEmojis = root.recentEmojis |
There was a problem hiding this comment.
I'm wondering why both recentEmojis and emojiModel are required properties, especially since recentEmojis is later assigned to the model itself. Wouldn't it be cleaner to rely directly on the main model's recentEmojis data? Alternatively, if we just need temporary state, we could handle it internally with a private d.recentEmojis property instead.
There was a problem hiding this comment.
Good point. I did realize that too when I fixed the "final" PR.
So in that other PR, I did basically what you're asking here, ie simplifying so that the Emoji handling is only done in one place, which is the EmojiPopup. Now we only pass the emojiModel and never the skinTone or the recentEmojis.
You can check it out here: #19160 (review)
I should have done that clean up in this first PR, but I thought the issue with recent emojis not updating was because of the model, so I did the simplification in the second one "by accident"
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
f76f22e to
b417274
Compare
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
b417274 to
89c5350
Compare
| skinColor: appMainLocalSettings.skinColor | ||
| emojiModel: SQUtils.Emoji.emojiModel | ||
| onSetSkinColor: color => appMainLocalSettings.skinColor = color | ||
| onSetRecentEmojis: recentEmojis => appMainLocalSettings.recentEmojis = recentEmojis |
There was a problem hiding this comment.
I think it'd be cleaner to actually have a small Settings {} inside the popup and encapsulate the skinColor and recentEmojis entirely inside the popup. I'd consider that way cleaner than it is now; some of those settigns are handled inside the popup (in the opened/closed handlers), and some outside here
| sourceModel: root.emojiModel | ||
|
|
||
| filters: [ | ||
| IndexFilter { |
There was a problem hiding this comment.
Wanna keep the IndexFilter or not? :)
The issue was that we set the property, which just overwrote the property and removed the link to the Setting.
jrainville
force-pushed
the
feat/emoji-reaction-final-design
branch
from
89c5350 to
82125e0
Compare
|
Hurray, I fixed the e2e test 👏 I still need one review please @noeliaSD @micieslak |
4 participants
What does the PR do
Part of #18935
Contains multiple commits:
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
emoji-reaction-new-design-part-1.webm
Impact on end user
Nicer experience when using the emoji reactions in the message context menu + fixes the emoji settings
How to test
Risk
Low