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

New iOS 9.1 emojis, emoji modifiers (skintones, etc) #96

Merged
merged 4 commits into from
Dec 17, 2015

Conversation

pfives
Copy link
Contributor

@pfives pfives commented Dec 9, 2015

Addresses issues #82 and #85.

I did this:

String resourceName = "emoji_" + hexUnicode + "_" + hexFollowUnicode;

int resourceId = context.getResources().getIdentifier(resourceName, "drawable", context.getApplicationContext().getPackageName());

for handling emoji modifiers rather than adding (64x5) more elements to a map. I'm not totally sure if this will work everywhere but if it does it seems like we could do something like this for all the emojis rather than managing those huge maps since all of the drawables are appropriately named.

…ag emojis, add single unicode emojis to sEmojisMap for easy retrieval, refactor code to correctly handle keycap emojis when a non-spacing combining mark is used, handle modifier emojis (skin tones, flags, etc.) by getting drawable resourceId from unicode name rather than adding them all individually to another map
@daniele-athome
Copy link
Contributor

I think the reason for using the hash map is because Google discourages use of getIdentifier because of performance reasons (I'm guessing it uses reflection).

Note: use of this function is discouraged. It is much more efficient to retrieve resources by identifier than by name.

Source: http://developer.android.com/reference/android/content/res/Resources.html#getIdentifier%28java.lang.String,%20java.lang.String,%20java.lang.String%29

@pfives
Copy link
Contributor Author

pfives commented Dec 9, 2015

Hm yea makes sense. Seems like using actual reflection is way faster (5x) than using getIdentifier()

http://daniel-codes.blogspot.com/2009/12/dynamically-retrieving-resources-in.html

Still not as fast as the map but probably a negligible difference. And significantly easier to maintain.

@rockerhieu if you want to make a decision on this I'll update the PR if necessary

@rockerhieu
Copy link
Owner

I like your solution, it's clean, and easier to maintain. But given the library already had a poor performance when rendering a lot of emoji at a time, I think we should not make it slower. How about this:

  • Try to use reflection as described in the above link.
  • Use a map as the cache layer

The first time an emoji is rendered, it will take a little more time, but it should be fast from the second time. What do you guys think?

@pfives
Copy link
Contributor Author

pfives commented Dec 11, 2015

So after digging into this it seems like the pure reflection method is faster because you only have to do 1 reflection lookup whereas getIdentifier does 2: one to find the drawable class and one to find the resource. Unfortunately since this is in a library we need to do that lookup anyways (and I couldn't find a consistent way to do it from the library project). getIdentifier is also more reliable than our own reflection lookup would be since it will stay consistent as the APIs change.
I don't think it's a big difference either way, haven't noticed any performance hits on my lower-end devices.
Also added a cache layer so we only do this lookup once per new emoji encountered.

@pfives
Copy link
Contributor Author

pfives commented Dec 14, 2015

@rockerhieu gentle nudge...how are we feeling about this?

rockerhieu added a commit that referenced this pull request Dec 17, 2015
New iOS 9.1 emojis, emoji modifiers (skintones, etc)
@rockerhieu rockerhieu merged commit 48eafe8 into rockerhieu:master Dec 17, 2015
@rockerhieu
Copy link
Owner

Looks good 👍

@daniele-athome
Copy link
Contributor

Thanks! But I think it's missing the new emojis in the UI (I mean they are parsed but they can't be typed in).

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.

3 participants