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

Edit screen: add emoji compat #49

Merged
merged 14 commits into from
Oct 15, 2019

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 23, 2019

This PR implements EmojiCompat so to be able to have emojis on devices that may not have support for them on their installed Fonts.

I explored an alternative PR in #55, but I think this one is a mergeable candidate given it uses the latest EmojiCompat library which is supposed to encapsulate the problems #55 tackles separately (downloading the fonts, then using them).
Let's keep #55 as an exploration, can probably close it as well if this one proves to be working fine.

Resources:

Didn't know which the needed replacement for the 28.0.0 support library was, but using jetpack it was enough to declare EmojiCompat in MainActivity and then let the IDE add the correct dependencies.

Basically followed the sample app https://github.com/googlesamples/android-EmojiCompat and implemented the core parts here.

Notes:

  • the certificates used for FontRequest are borrowed from Google's EmojiCompat sample app, I haven't found anywhere in the docs that these should (or should not) be the ones to be used, but judging from the package names it looks like these certs should correspond to Google's, as this is how the call looks like:

Screen Shot 2019-08-23 at 08 40 03

Note on this: these are indeed the same ones that Android Studio adds to your project when following the steps described in #55, so these are 👍

ToDo:

@mzorz mzorz changed the title Issue/13 edit screen controls add emoji compat Edit screen: add emoji compat Aug 23, 2019
@mzorz
Copy link
Contributor Author

mzorz commented Aug 23, 2019

Note: while using EmojiCompat is the "right" (official) way to go (that is, just download the Font and use it), this is an interesting approach to checking availability as well

@mzorz mzorz marked this pull request as ready for review August 23, 2019 17:21
@mzorz mzorz requested a review from aforcier August 23, 2019 17:25
@mzorz mzorz changed the base branch from issue/13-edit-screen-controls-add-emoji to issue/13-edit-screen-controls August 23, 2019 17:29
@aforcier
Copy link
Collaborator

aforcier commented Oct 4, 2019

@mzorz I tested this one and #55 and I have a bunch of questions 😀

IIRC we discussed these PRs and you mentioned that they're not adding support for missing emojis (since we're still reliant on row_emoji.xml, and we're tracking this improvement in #110). But I'm not sure what the testing steps are.

Testing develop and these two PRs on my Pixel 3, I notice a few emoji that were B&W before are using Google fonts when running #55:

emojifonts

However, when running this branch nothing seems changed.

I was also curious what would happen on a device with no Google APIs so I tried on an emulator. Neither branch shows the modified emoji - I guess this is expected. Something we may need to consider for markets without Google Play.

@mzorz
Copy link
Contributor Author

mzorz commented Oct 13, 2019

Testing develop and these two PRs on my Pixel 3, I notice a few emoji that were B&W before are using Google fonts when running #55:

Oh that's an interesting observation, I think I didn't notice that before

But I'm not sure what the testing steps are.

You're totally right I forgot to add steps to test here. I tested by adding breakpoints in the callbacks to make sure the font downloader callbacks were being called, and the .process() call was being made to run spans through it on the desired TextViews, but we need to make sure one or both approaches are working correctly before merging.

Currently looking into this, will loop back 👍

@mzorz mzorz changed the base branch from issue/13-edit-screen-controls to develop October 13, 2019 12:40
@peril-automattic
Copy link

peril-automattic bot commented Oct 13, 2019

You can test the changes on this Pull Request by downloading the APK here.

@mzorz
Copy link
Contributor Author

mzorz commented Oct 13, 2019

Tried by using this in the EmojiCompat configuration:

        config.setReplaceAll(true)
        config.setEmojiSpanIndicatorEnabled(true)

The second line paints the background for the EmojiSpan in green. This shows the emoji that didn't have color are not recognized as emoji, and so these are not replaced with an EmojiSpan by the EmojiProcessor (https://developer.android.com/reference/android/support/text/emoji/EmojiSpan) and thus don't have the green background.

Screenshot_20191013-114226

Why aren't these recognized as Emoji? Let's see.

Looking further down into EmojiCompat's sourcecode realized this is because it will only replace one-byte unicode values as emoji if setUseEmojiAsDefaultStyle() is true (look in this line in EmojiProcessor, then isInFlushableState makes this check here ).

Made this change in commit fe49ad2 and now it works as expected 👍

Screenshot_20191013-113258

So, this is how it looks now:

Screenshot_20191013-114433

I think we can merge this PR if it looks good to you @aforcier , and close #55 (which helped me a lot in understanding how all of this works but this current PR is preferred given it's the recommended way to handle things today). Let me know!

@aforcier aforcier self-assigned this Oct 15, 2019
@aforcier
Copy link
Collaborator

Confirming that it's working correctly now 👍 your explanation makes sense, let's merge this and close #55, we can revisit it later if it comes up for some reason.

:shipit:

@aforcier aforcier merged commit e1dc4fa into develop Oct 15, 2019
@aforcier aforcier deleted the issue/13-edit-screen-controls-add-emoji-compat branch October 15, 2019 12:33
@mzorz mzorz mentioned this pull request Nov 8, 2019
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.

2 participants