Skip to content
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

Rewrite for more Emojis, modularity and performance #77

Merged
merged 68 commits into from
Feb 4, 2017
Merged

Rewrite for more Emojis, modularity and performance #77

merged 68 commits into from
Feb 4, 2017

Conversation

rubengees
Copy link
Collaborator

@rubengees rubengees commented Jan 10, 2017

As discussed in #75.

Contents of this PR

Generator

The folder generator contains a NodeJS script for updating the images and generating relevant java classes.
This is done like follows: Download the file containing the emoji and the mapping file -> Index all the emojis and categories -> Copy the images to the drawable folders -> Generate the java files and copy them directly into the respective folders.

Emojis are downloaded from here and the mapping file is from here.

I may add a README with instructions on how to run the script later.

Emojis

This adds the latest iOS and EmojiOne Emojis. There are different modules for different variants.

The benefits are:

  • Complete collection of all emojis (even with skin tones and even more than WhatsApp Beta currently has!)
  • Better image quality
  • Choice of Emoji collection

Modularization

There is now a core module named "emoji". For each emoji variant there is a "emoji-<variant>" module.
As of this PR this are "emoji-ios" and "emoji-one".
These modules have a dependency on "emoji", so the user can just write compile com.vanniktech.emoji-ios:version for instance.
Furthermore, the user can pass an own instance of the new interface EmojiProvider. It is very easy to implement (just one method).

The EmojiProvider is then (and has to be) registrered like this (In an Application subclass for example):

EmojiManager.install(new IOSEmojiProvider()); // Or EmojiOneProvider or 
                                              // custom implementation

Categories

Categories are now also generated and thus need to be more abstract. For this purpose, a new interface EmojiCategory which holds the icon for the category and all the emojis in the category. The EmojiView was changed to work with this new approach.
The icons of the categories are from EmojiOne (including the recent icon), except the backspace icon, which I generated with the Android Studio Asset Studio. All of these icons are now vectors.

Architecture and internals

Parsing
  • I rewrote large parts of the emoji parsing to be much more flexible. I added a new data structure EmojiTree which holds paths to each emoji (each child is one character, if the child is the end of an emoji, it holds the respective resource). When parsing, the tree is traversed and the longest fitting path is used. This idea is from emoji-java, which does it quite similar.

  • A new singleton class EmojiManager holds the EmojiTree (and the categories), which is filled upon construction (Like the EmojiHandler before).

  • This also greatly simplifies the EmojiHandler, which now only iterates the String and looks up the emojis.

Category adapter

I noticed that the emoji picker is a bit laggy. To help with that, the EmojiArrayAdapter was changed to asynchronously load the images. This is done with a plain AsyncTask. Moreover the current approach with TextViews were replaced by ImageViews as this further improves performance.
Another problem was, that the GridViews were hold in memory even when the PagerAdapter called the destroy-method. This could lead to OOMs, especially since the images know have better quality and thus are larger. This is know changed so that the GridView is inflated when requested and also properly destroyed.

Emoji class

I changed the Emoji class a bit to better work with the new architecture. It know holds the associated drawable resource and has new constructors. This required the RecentEmojiManager implementation to change a bit. Upon loading from disk, it searches the EmojiManager for the existing unicode. If not found, the emoji is discarded (This also improves compability between modules. If the user changes for example from ios to emojione and the latter does not contain an Emoji, the App will not crash).
Furthermore it is also used in the new EmojiTree, reusing existing instances and thus minimizing unnecessary object allocation like before (Unicode Strings were allocated both for categories and the EmojiHandler).

New Views

I added a new View: SquareImageView`. As the name suggests, it draws the image in a square, with the width set for the height. This is used in the emoji picker.

Minor other changes
  • I altered the value for the EmojiGridView item width a bit to make it look good with the new images.

Just to clarify again: These changes are not directly backwards compatible

This is what it looks like

iOS EmojiOne
device-2017-01-29-165256 device-2017-01-29-165515

Problems I noticed

This is not directly related to this PR, but I noticed it when I worked on it:
When adding many emojis to the EmojiEditText it becomes quite slow (I measured once and managed to get a freeze of 500ms). This is due to the EmojiHandler adding a span for each emoji, which is very unperformant. I looked into that, but didn't found a better solution... Maybe you now of a different way to approach this?

I am almost certain that I forgot something so please have a look yourself (Especially at the Gradle config, I'm not sure if it is right and have no way of testing)!

Moreover I do apologize for the awfully large PR, which makes GitHub freeze 😁.

@codecov-io
Copy link

codecov-io commented Jan 10, 2017

Codecov Report

Merging #77 into master will increase coverage by -19.18%.

@@            Coverage Diff             @@
##           master     #77       +/-   ##
==========================================
- Coverage   47.28%   28.1%   -19.18%     
==========================================
  Files          21      18        -3     
  Lines        2464     523     -1941     
  Branches       66      50       -16     
==========================================
- Hits         1165     147     -1018     
+ Misses       1291     370      -921     
+ Partials        8       6        -2

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 557e81e...918e6ff. Read the comment docs.

@vanniktech
Copy link
Owner

This will replace the current emojis right? If so that's not really doable / acceptable since a lot of the library consumers rely on that particular style.

@rubengees
Copy link
Collaborator Author

rubengees commented Jan 15, 2017

Yes, this would replace the current ones. I wanted to modularize it in another PR, as this one is already very large, but I can work on it in this one if you prefer.

@vanniktech
Copy link
Owner

I've been mulling over this and I'd accept it if it's in a separate module and the current version isn't touched. So that you can either use:

compile 'com.vanniktech:emoji:0.3.0'

or

compile 'com.vanniktech:emoji-one:0.3.0'

and they have similiar APIs / usage.

Of course I can also understand it if you want to fork this library and maintain your version for emojis with EmojiOne.

@rubengees
Copy link
Collaborator Author

rubengees commented Jan 21, 2017

I thought about it and would do it like this:

One core module, which has the EmojiTextView, EmojiEditText and managing functions. This would then be compile 'com.vanniktech:emoji:0.5.0' (Or whatever version it is then).
The core module would need some entry point for installing the chosen emojis, maybe in the Application or for each EmojiTextView and EmojiEditText.
I could imagine the first looking like this:

Emoji.install(new EmojiOneProvider()); // Or new IOsProvider()

The second would look like this:

emojiEditText.setEmojiProvider(new EmojiOneProvider());

Personally I like the first one better, as we would have a singleton then, so we don't hold duplicate data in the memory.
What do you think?

We would then have modules for each variant.
This would be compile 'com.vanniktech:emoji-one:0.3.0' or compile 'com.vanniktech:emoji-ios:0.3.0' for example. Each of these modules would have a transitive dependency on the core module so you can just write what variant you would like to have.
Yes, this requires the user to change it's dependency once and adjust usage a bit, but I think that is better for modularity and clearnes in the long run. Moreover it has the benefit, that the user could provide its own EmojiProvider or even something entirely different (The new architecture allows to replace any text by images).

On a side note: I am currently not actively working on it, but will be in a week or so.

@vanniktech
Copy link
Owner

vanniktech commented Feb 3, 2017

Getting:

/Users/nik/tmp/Emoji/emoji-ios/src/main/java/com/vanniktech/emoji/ios/category/ActivityCategory.java:112: error: cannot find symbol
            new Emoji(new int[]{0x1f6b5, 0x200d, 0x2640, 0xfe0f}, R.drawable.emoji_ios_1f6b5_200d_2640_fe0f),

(sorry for the misclick)

@vanniktech vanniktech closed this Feb 3, 2017
@vanniktech vanniktech reopened this Feb 3, 2017
@rubengees
Copy link
Collaborator Author

rubengees commented Feb 3, 2017

Ah! Sorry, I forgot to actually add the renamed images 😅

@vanniktech
Copy link
Owner

Nice looking better and better, I also updated the issue

Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better and better. Only two more things missing:

  • Exclude skin emojis in keyboard for now but display them in TextView
  • Old category icons for ios and the good looking ones for the new emoji one

@rubengees
Copy link
Collaborator Author

rubengees commented Feb 4, 2017

Just pushed the change for the first point.
The second point may take some time as I want to search for better quality ones (Hopefully even vectors). If I do not find some, I'll include the old ones.

@vanniktech
Copy link
Owner

@rubengees why not simply use the old icons for now? There's no harm in that we can change them later if needed.

@rubengees
Copy link
Collaborator Author

rubengees commented Feb 4, 2017

I have added new ones. They are PNGs and don't have the best quality, but better than before.
They are also used for EmojiOne now as it looks also good.

@vanniktech
Copy link
Owner

Awesome. Thank you so much for all of your work. Tomorrow I'm on a train and I'll do some more adjustments locally to the code and prepare some more things. It really looks fabulous. Either I'll release 0.4.0 tomorrow or once I'm back from vacation (~8 days). SNAPSHOT version will be built automatically and should be available soon after the merge.

@vanniktech vanniktech merged commit e735c06 into vanniktech:master Feb 4, 2017
This was referenced Feb 4, 2017
@rubengees
Copy link
Collaborator Author

Great, we did it 👏
Looking forward to 0.4!

@vanniktech
Copy link
Owner

All of the new ios emojis are right now adding an extra of 3MB :/ on top of the 2 or 3 MB it was before.

I also tried compressing them further but had no luck. I guess that's just the drawback of supporting more emojis. Another option might be vector drawables although I'm not sure you can get emojis in that format. Do you know of anything @rubengees ?

@rubengees rubengees deleted the emojione branch February 5, 2017 22:57
@rubengees
Copy link
Collaborator Author

rubengees commented Feb 5, 2017

@vanniktech Yes, thats the price of having them all...

Only EmojiOne has vectors as far as I know. I haven't found them for other variants at least.

@vanniktech
Copy link
Owner

Tracking in #88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants