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

EmojiPicker: autosize #199

Merged
merged 14 commits into from
Nov 19, 2019
Merged

EmojiPicker: autosize #199

merged 14 commits into from
Nov 19, 2019

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Nov 7, 2019

Fix #161

This PR has a couple of steps. First I cleaned out the layouts we were using. Then the actual diagnosis and proposed solutions come.

The previous implementation had many unneeded views that were set to visibility:gone long ago. Removed them and then just added the simplest form for both the dialog layout and the row_emoji layout.

BEFORE

Nexus S emulator
Screen Shot 2019-11-07 at 12 27 54

AFTER

Nexus S emulator (API 22)
Screen Shot 2019-11-07 at 12 52 16

Then realized, if I add more columns to the grid, or if we set a fontSize larger than 35sp on emoji_row.xml's TextView, the problem appears again.

Diagnosis

It has to do with the fontSize and the emoji span width.
Setting a fontSize smaller than 35sp as is currently set to now (or even removing the parameter) avoids the cut, but obviously makes the emoji smaller.
For this to work everywhere, we'd need to calculate on runtime and then adjust the fontSize / emoji picker column amount to give there enough space to avoid the TextViews to cut the span's view.
I thought we could proxy this with a hardcoded table and make different sized screens show a different amount of columns depending on their informed width. I think this is probably a good proxy and doesn't hurt the UX while minimizing the effort (that is, it's a lightweight solution).

But then when looking closer into this, was reading the documentation to make sure I got all things in place, and stopped on this for a second:

When defining text sizes, however, you should instead use scalable pixels (sp) as your units (but never use sp for layout sizes). The sp unit is the same size as dp, by default, but it resizes based on the user's preferred text size.

I realized we're using TextView for Text and emoji and, this essentially means... the user would be able to change the sizes on the fly, and this would modify our UI. Effectively tried this on bigger screens and the cut is observed there as well:

  1. set the font size to as big as possible

Screen Shot 2019-11-07 at 13 39 28

  1. the emoji appear larger and cut

Screen Shot 2019-11-07 at 13 39 21

So, that leaves us no choice but to take this into account and adjust the sizes on the fly accordingly.

Proposed solution

So, what we essentially need is a way to set a fontSize for which emoji spans size occupy only as much as their containing view's size (the TextView). In principle (and thankfully!), this is handled by AutoSizeable TextViews and should be backwards compatible with the support library.

Unfortunately, and after a few tests I came to realize what we have in row_emoji is the recently added androidx.emoji.widget.EmojiTextView which is not an AutoSizeableTextView (see the following jetpack code that makes this check before applying the behavior):

    @SuppressWarnings("RedundantCast") // Intentionally invoking interface method.
    public static void setAutoSizeTextTypeWithDefaults(@NonNull TextView textView,
            int autoSizeTextType) {
        if (Build.VERSION.SDK_INT >= 27) {
            textView.setAutoSizeTextTypeWithDefaults(autoSizeTextType);
        } else if (textView instanceof AutoSizeableTextView) {
            ((AutoSizeableTextView) textView).setAutoSizeTextTypeWithDefaults(autoSizeTextType);
        }
    }

We can workaround that by making this view a regular TextView and processing EmojiSpan with a callback, just the way we are doing things for addEmoji in https://github.com/Automattic/portkey-android/pull/49/files#diff-1dfe56dc06fa5c09f0d3ac32efea40d9R375-R394

Changed back to TextView in e6df0f3.
Added EmojiCompat processing on it in 21430d8.

Also worth noting, we need to give the view a concrete size, given relativeindicators such as wrap_content / match_parent don't work when using autoSizeTextType=uniform.
But, if we give it a concrete size, then it may end up being too small for the default 6 columns.
For this, we're calculating the maximum width for each view depending on the columns needed, in 000e8c2.

To test

On API 26 or higher:

  1. open portkey
  2. tap the + button to access the camera preview screen
  3. take a picture
  4. tap on the "Stickers" button and observe the emoji list appears and emoji don't get cut.
  5. try changing in code the number of columns (see bottom of EmojiPickerFragment.kt):
    companion object {
        const val COLUMNS = 8
    }
  1. observe the emoji items get resized accordingly, without losing quality given the font size changes as per AutoSizeableTextView

Repeat the same on pre-API 26 and verify the same behavior holds.
Also, check the emoji represented on API 22 are the same as in > API 26 (if you comment out the EmojiCompat code, you'll see the old emoji representations appear).

@peril-automattic
Copy link

peril-automattic bot commented Nov 7, 2019

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

@mzorz mzorz changed the title WIP - EmojiPicker: remove unneeded layouts and constraints EmojiPicker: autosize Nov 8, 2019
@mzorz mzorz requested a review from aforcier November 8, 2019 16:13
@mzorz
Copy link
Contributor Author

mzorz commented Nov 8, 2019

This one is ready for review now @aforcier 🙇

Copy link
Collaborator

@aforcier aforcier left a comment

Choose a reason for hiding this comment

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

Looks good @mzorz!

  • Always looks right as you shift column numbers
  • Emoji representation is the same (verified it all changes when you revert the emoji compat usage)

Tested on:

  • Pixel 3 API29
  • Pixel 5X API23 (emulator)
  • Nexus One API22 (emulator)

Just a couple minor code suggestions and this is good to go.

EmojiCompat.get().unregisterInitCallback(this)

val regularTextView = holder.txtEmojiRef
if (regularTextView != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary null check here - regularTextView is non-null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 addressed in 78ffcf3

@@ -71,11 +93,36 @@ class EmojiPickerFragment : BottomSheetDialogFragment() {
inner class EmojiAdapter(internal val emojiList: List<String>) : RecyclerView.Adapter<EmojiAdapter.ViewHolder>() {
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ViewHolder {
val view = LayoutInflater.from(parent.context).inflate(R.layout.row_emoji, parent, false)
emojiViewWidth?.let {
val params = view.getLayoutParams()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use property access syntax here and silence an IDE warning:

val params = view.layoutParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 ba15ab1

// Changes the height and width to the specified dp
params.height = it
params.width = it
view.setLayoutParams(params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, can use property access syntax:

view.layoutParams = params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done in ba15ab1

@mzorz mzorz merged commit bc423fb into develop Nov 19, 2019
@mzorz mzorz deleted the issue/161-picker-shows-cut-emoji branch November 19, 2019 14:12
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.

Emoji picker shows cut emoji
2 participants