Update emojis and use sprite sheet instead of individual images #252

rubengees · 2018-02-04T12:11:21Z

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

More emojis! 🥇
Size reduction. Not too great but notable. 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 of Emoji 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 destroy Emoji instances. Currently I'm thinking of making the destroy method of the EmojiManager public and adding a destroy method to Emoji which subclasses can override.

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.

vanniktech

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 · 2018-02-04T12:28:02Z

Codecov Report

Merging #252 into master will increase coverage by 0.43%.
The diff coverage is n/a.

@@            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

Impacted Files	Coverage Δ
...rc/main/java/com/vanniktech/emoji/emoji/Emoji.java	`88.88% <0%> (+0.31%)`	⬆️
...c/main/java/com/vanniktech/emoji/EmojiManager.java	`86.58% <0%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f7f35c...1304865. Read the comment docs.

rubengees · 2018-02-04T12:56:04Z

The images are already heavily optimized (pngquant and zopfli). That is the point of having one large image I think - You can optimize that pretty well.
Yeah thought about that as well. We now have no Provider which uses the old logic from the Emoji class. I could imagine though that somebody is using that for a custom Provider. But I would be fine with converting to an interface.
Sorry, totally forgot about that, here they are:

Click to expand

Yes I actually have! In one app I work on, I constantly fight with OEMs and other problems related to low memory since that app is really image-heavvy. I would rather not have around 4MB loaded at all time but reclaim that memory when the user leaves the Activity with the emoji picker. I can imagine there are more people with similar problems...

vanniktech · 2018-02-04T13:51:27Z

The images are already heavily optimized (pngquant and zopfli). That is the point of having one large image I think - You can optimize that pretty well.

Nice.

Yeah thought about that as well. We now have no Provider which uses the old logic from the Emoji class. I could imagine though that somebody is using that for a custom Provider. But I would be fine with converting to an interface.

Then leave things as is in this PR to minimize the changes.

Sorry, totally forgot about that, here they are:

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.

Yes I actually have! In one app I work on, I constantly fight with OEMs and other problems related to low memory since that app is really image-heavvy. I would rather not have around 4MB loaded at all time but reclaim that memory when the user leaves the Activity with the emoji picker. I can imagine there are more people with similar problems...

Then let's do it.

rubengees · 2018-02-05T09:48:26Z

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.

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.

vanniktech · 2018-02-05T10:21:46Z

Is this PR then out of question?

No not at all. Let's ditch the order then and possibly try to tweak it. The benefits here outweigh that entirely.

rubengees · 2018-02-05T21:12:16Z

I have added a machanism to reclaim memory. Here is a screenshot of it in action (Android Profiler):

possibly try to tweak it

I can't think of a non-manual approach, but will keep looking around.

vanniktech

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?

vanniktech · 2018-02-05T23:15:52Z

emoji/src/main/java/com/vanniktech/emoji/EmojiManager.java

@@ -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()) {


Let's use fori here too

We can't; values() returns a Collection and not a List.

rubengees · 2018-02-06T13:41:11Z

Adjusted a few things (the first two commits) and regenerated!

vanniktech · 2018-02-06T13:56:51Z

Thanks. I'll take a look later.

vanniktech

Sorry. I completely forgot about this. It looks awesome 👍

Update emojis and use sprite sheet instead of individual images

eb3cbde

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.

vanniktech reviewed Feb 4, 2018

View reviewed changes

Add mechanism for reclaiming memory of bitmaps allocated by emojis

de3c922

vanniktech reviewed Feb 5, 2018

View reviewed changes

rubengees added 3 commits February 6, 2018 14:34

Fix code style issues and improve performance a bit

800075d

Adjust sample for latest changes

30d7860

Regenerate providers

1304865

rubengees changed the title ~~[WIP] Update emojis and use sprite sheet instead of individual images~~ Update emojis and use sprite sheet instead of individual images Feb 7, 2018

vanniktech approved these changes Feb 21, 2018

View reviewed changes

vanniktech merged commit 9991a48 into vanniktech:master Feb 21, 2018

vanniktech mentioned this pull request Feb 21, 2018

Investigate how to bring the library size own #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update emojis and use sprite sheet instead of individual images #252

Update emojis and use sprite sheet instead of individual images #252

rubengees commented Feb 4, 2018 •

edited

Loading

vanniktech left a comment

codecov bot commented Feb 4, 2018 •

edited

Loading

rubengees commented Feb 4, 2018

vanniktech commented Feb 4, 2018

rubengees commented Feb 5, 2018

vanniktech commented Feb 5, 2018

rubengees commented Feb 5, 2018

vanniktech left a comment

vanniktech Feb 5, 2018

rubengees Feb 6, 2018

rubengees commented Feb 6, 2018

vanniktech commented Feb 6, 2018

vanniktech left a comment

Update emojis and use sprite sheet instead of individual images #252

Update emojis and use sprite sheet instead of individual images #252

Conversation

rubengees commented Feb 4, 2018 • edited Loading

Wins

Important code changes

Points of discussion

vanniktech left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2018 • edited Loading

Codecov Report

rubengees commented Feb 4, 2018

vanniktech commented Feb 4, 2018

rubengees commented Feb 5, 2018

vanniktech commented Feb 5, 2018

rubengees commented Feb 5, 2018

vanniktech left a comment

Choose a reason for hiding this comment

vanniktech Feb 5, 2018

Choose a reason for hiding this comment

rubengees Feb 6, 2018

Choose a reason for hiding this comment

rubengees commented Feb 6, 2018

vanniktech commented Feb 6, 2018

vanniktech left a comment

Choose a reason for hiding this comment

rubengees commented Feb 4, 2018 •

edited

Loading

codecov bot commented Feb 4, 2018 •

edited

Loading