-
-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update emojis and use sprite sheet instead of individual images #252
Conversation
This comes with a full rewrite of the generator. Also the data source of the emojis is changed to https://github.com/iamcal/emoji-data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super interesting.
- Can we still crunch the sprite sheet and save some mbs?
- Do we still need the Emoji class then? Maybe it should be an interface instead of an actual class?
- Do you have a screenshot regarding the ordering?
- Do you have a personal use case for reclaiming the memory? I can't think of one and maybe we should just provide that functionality someone wants it. We should just be cautious about memory leaks. Those should never occur.
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 28.04% 28.48% +0.43%
==========================================
Files 22 22
Lines 820 825 +5
Branches 90 90
==========================================
+ Hits 230 235 +5
Misses 571 571
Partials 19 19
Continue to review full report at Codecov.
|
|
Nice.
Then leave things as is in this PR to minimize the changes.
Oh wow things are really sorted differently. Is there any way to get a sorting that's close to Whatsapp ones? From the user perspective it's best if we use the same ordering otherwise you always need to end up looking for emojis which is the worst thing ever. Also we don't have any search functionality yet so this becomes even more important.
Then let's do it. |
I agree! Sadly I don't know of a way to get the WhatsApp ordering with the data we have. We require a metadata file with categories and ordering (like that in WhatsApp) that currently only that file from EmojiOne has (at least I have not found another one). And that one does not get updated sadly. Is this PR then out of question? I have looked around and there are at least two apps that have a somewhat similar ordering: Slack and the Telegram X app. |
No not at all. Let's ditch the order then and possibly try to tweak it. The benefits here outweigh that entirely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small minor change. Otherwise I think this is ready for pulling in all of the other files and start generating the new ones right?
@@ -134,11 +134,16 @@ public static void install(@NonNull final EmojiProvider provider) { | |||
INSTANCE.emojiRepetitivePattern = Pattern.compile('(' + regex + ")+"); | |||
} | |||
|
|||
static void destroy() { | |||
public static void destroy() { | |||
for (final Emoji emoji : INSTANCE.emojiMap.values()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use fori here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't; values() returns a Collection
and not a List
.
Adjusted a few things (the first two commits) and regenerated! |
Thanks. I'll take a look later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I completely forgot about this. It looks awesome 👍
This is a rewrite of the generator for updating the emojis and switching to the sprite sheet approach discussed in #88.
We now use this data source instead of the EmojiOne metadata file, which does not seem to get updated anytime soon.
I have marked this as WIP, as I'm not sure if this is the right way to go. I also have not regenerated yet to make this PR reviewable.
Wins
Ios
went from 4.5MB to 3.7MB.Important code changes
A
<Name>Emoji
class is now generated for each emoji type (e.g.IosEmoji
). This is a subclass ofEmoji
to make the sprite sheet approach work. It works by loading the big sheet into a static field and then cuts the individual emojis from that when requested.The generator does not download any files anymore but depends on the aforementioned library through
npm
now.Points of discussion
The ordering is different. It makes sense but is different from WhatsApp (which we always tried to copy in previous versions). It is now based on the official unicode recommendations. Slack uses this ordering now also for example.
Once the big sheet is loaded, there is no way to reclaim that memory. Should we introduce a way of uninstalling a
Provider
? We then would also need a mechanism to destroyEmoji
instances. Currently I'm thinking of making thedestroy
method of theEmojiManager
public and adding adestroy
method toEmoji
which subclasses can override.