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

Sets default maximum font size for emoji to 112sp #206

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Nov 22, 2019

Fixes #205

Builds on top of #204.

Inspired by TextView's source code here and here:

    // Default maximum size for auto-sizing text in scaled pixels.
    private static final int DEFAULT_AUTO_SIZE_MAX_TEXT_SIZE_IN_SP = 112;

this PR sets a default maximum fontSize of 112sp as well for emojis, given otherwise at some point these can't be rendered anymore with the error described in issue #205.

Given I haven't found a way to obtain the Font Family's maximum supported font size, or even calculate this on runtime (*), I think going with TextView's own defaults is conservative enough as to avoid having other problems.

To test:

  1. capture an image
  2. tap on the stickers option, top-right of the screen
  3. choose any emoji
  4. pinch to resize (zoom) and try to make the view as large as possible, observe the emoji stops resizing at some point but it never disappears on this branch.

Note: have in mind the current listener on this branch doens't support rotation or view scaling (it only changes the view's width/height to provoke fontSize to change accordingly). This means, rotating will not work on this branch, and that's expected.

(*)EDIT also note that I've tried other approaches for calculating the maximum renderable font size, for example using the approach described here but it still fails given the error looks to be a runtime problem related to rendering on the screen rather than figuring out supported font sizes for a given font. In fact, for the algorithm run there I also get 256.0 as per the maximum renderable fontSize. Code changes with this test can be seen here on branch issue/205-emoji-max-text-size-calculation

@mzorz mzorz requested a review from aforcier November 22, 2019 16:21
@peril-automattic
Copy link

peril-automattic bot commented Nov 22, 2019

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

@@ -413,11 +413,10 @@ class PhotoEditor private constructor(builder: Builder) :
}
txtTextEmoji.gravity = Gravity.CENTER
txtTextEmoji.setLayerType(View.LAYER_TYPE_SOFTWARE, null)
val maxHeight = parentView.height
rootView.tvPhotoEditorText.textSize =
txtTextEmoji.textSize =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A note on this: we're setting the maximum on the TextView's textSize given internally the maximum is also set there. To double check, see here:

override fun setTextSize(size: Float) {
        maxTextSize = size
        adjustTextSize()
    }

and here (init block):

    init {
        // using the minimal recommended font size
        minTextSize = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_SP, 12f, resources.displayMetrics)
        maxTextSize = textSize

For some other reasons, I'm considering probably forking the source code and reusing it, if I we end up doing that then it's probably better to make the intent explicit (with a setMaxSize() kind of method).

@aforcier
Copy link
Collaborator

@mzorz maybe I'm misunderstanding what should be working by this PR, but on my Pixel 3 (physical) and a API29 Pixel 3XL emulator, the emoji disappears once zoomed into a little. It's still there and you can make it come back by zooming out at the right place, but it seems that was supposed to have been fixed by this PR. Could you check on a 3XL emulator and confirm? Can get you a video too.

@aforcier aforcier self-assigned this Nov 25, 2019
@mzorz
Copy link
Contributor Author

mzorz commented Nov 25, 2019

Following our conversation over Slack, moving salient points here to keep track of it. From empirical testing on various devices and emulators with different maximum values for fontSize it seems emoji invariably take some memory from the cache devices have dedicated for font rendering so, we’ll hit the mark in one way or the other.

So I think all we can hope for is, in case this doesn't work to have a smaller maximum and then use scaling just as we were doing, but this way we should be gaining some quality regardless.

In the meantime, as per my tests and following TextView's sourcecode it looks like having a maximum 112sp without setting the layer type to software on the view (as per defaults) should work 🤞 .

Also worth noting, the plan is to enable View's scale scaling right after reaching the maximum on this kind of TextView (as per part 4 in #207), hopefully giving us better quality results overall for TextViews (which include text as well as emoji).

Let's test a few more and then lower the maximum if needed 👍

@aforcier aforcier changed the base branch from issue/153-textview-fontsize-cleanup to feature/adapt-gesture-listeners-fontsize November 27, 2019 14:16
@aforcier
Copy link
Collaborator

Per our conversation, this works okay ignoring the issue with the low maximum size and the strange movement behavior at max size that we already talked about and will get resolved in another PR.

I did notice the default size for text was also really small now, but from testing later branches this seems to get resolved later.

device-2019-11-27-114745

:shipit:

@aforcier aforcier merged commit d221295 into feature/adapt-gesture-listeners-fontsize Nov 27, 2019
@aforcier aforcier deleted the issue/205-emoji-max-text-size branch November 27, 2019 17:35
@mzorz
Copy link
Contributor Author

mzorz commented Nov 27, 2019

Thanks for the review! 🙇

I did notice the default size for text was also really small now, but from testing later branches this seems to get resolved later.

That's right, handled in #210

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