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

Big emoji-only messages #3295

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Big emoji-only messages #3295

merged 3 commits into from
Sep 4, 2024

Conversation

frebib
Copy link
Contributor

@frebib frebib commented Aug 11, 2024

Content

Render emoji-only messages in a larger text

Adapted from SpiritCroc's SchildNext implementation from SchildiChat/schildichat-android-next@7eba87f

Authored-by: SpiritCroc [email protected]
Submitted-by: Joe Groocock [email protected]

Motivation and context

Parity with Element-X iOS, Element Web, and pretty much any other chat client ever.

Fixes #1438

Screenshots / GIFs

Before After
Screenshot_20240811-175500 Screenshot_20240811-175137

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 15

Checklist

  • Changes have been tested on an Android device or Android emulator with API 23
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off
  • You've made a self review of your PR

@frebib frebib requested a review from a team as a code owner August 11, 2024 16:58
@frebib frebib requested review from ganfra and removed request for a team August 11, 2024 16:58
Copy link
Contributor

Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:

  • Your branch should be based on origin/develop, at least when it was created.
  • The title of the PR will be used for release notes, so it needs to describe the change visible to the user.
  • The test pass locally running ./gradlew test.
  • The code quality check suite pass locally running ./gradlew runQualityChecks.
  • If you modified anything related to the UI, including previews, you'll have to run the Record screenshots GH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.

@frebib
Copy link
Contributor Author

frebib commented Aug 11, 2024

cc @SpiritCroc

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@SpiritCroc
Copy link
Contributor

Thanks!

Sign-off

Signed-off-by: Tobias Büttner [email protected]

@frebib
Copy link
Contributor Author

frebib commented Aug 11, 2024

> Task :app:compileGplayDebugKotlin FAILED
e: warnings found and -Werror specified
w: file:///home/runner/work/element-x-android/element-x-android/app/src/main/kotlin/io/element/android/x/ElementXApplication.kt:42:26 'constructor BundledEmojiCompatConfig(Context)' is deprecated. Deprecated in Java

(big) 😑

Yet, not a single other match for this on github passes the second argument: https://github.com/search?q=BundledEmojiCompatConfig&type=code

This library is becoming more effort than it's worth. We don't even need the font compat rubbish 💩

@frebib frebib marked this pull request as draft August 11, 2024 20:35
@frebib frebib force-pushed the feat/big-emoji branch 3 times, most recently from 7095631 to 3d793ee Compare August 12, 2024 22:07
@frebib
Copy link
Contributor Author

frebib commented Aug 12, 2024

I contemplated the options for a while, and opted to stick with androidx-emoji2 for a couple of reasons

  • This library is the most "native" (being an androidx library), and it also gives the most correct results out of the options I've tested.
  • Implementing emoji detection is hard.. EX iOS does it, but it's not perfect (and they even opted for a simpler implementation for the sake of maintainability element-hq/element-x-ios@2019984)
  • I tried to integrate https://github.com/vanniktech/Emoji but that has the same problems/questionable implementation, plus it would require either androidx-emoji2 or yet another vanniktech library for emoji data. I also note that Benoit calls out the vanniktech library as being "3rd party" here Add a isOnlyEmojis() function matrix-org/emojibase-bindings#16 which seems to me like a negative.
  • Using a regex, which seems like the most straightforward (and honestly, probably performant too) option is actually very complex due to the questionable nature of the design of the unicode emoji system. It'd be a burden to maintain in the long run, even if it would work now.
  • The "is this string an emoji-only string" implementation can be swapped out in future if a better option comes along, like Java 21's String.isEmoji()

@frebib frebib marked this pull request as ready for review August 12, 2024 22:08
Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'm a bit torn about this: we already have the emojibase library for using emojis, getting the categories, etc., so I don't really like the idea of adding another one with mostly the same functionality - even though it's the official one. It also probably adds some small overhead on the app init time.

On the other side, the implementation is quite simple and works well, and duplicating the logic of EmojiCompat seems like an overkill.

Also, the logic isn't exactly the same as iOS: they only use a large text style if the message contains a single emoji and nothing else, so maybe we should do the same.

@bmarty any thoughts about this?

import androidx.emoji2.text.EmojiCompat
import timber.log.Timber

fun String.containsOnlyEmojis(maxEmojis: Int = Integer.MAX_VALUE): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to remind myself how to write JavaDoc KDoc(?), but done

@frebib
Copy link
Contributor Author

frebib commented Aug 13, 2024

we already have the emojibase library for using emojis, getting the categories, etc.

Fully agree here. In fact, I'm quite frustrated that we need a library for this at all. It should just be pattern matching really. If the spec wasn't so awful, I'd do it with a regex.
I'm happy to drop this lib and find another way to do it- I'm not totally satisfied with it either tbqh

they only use a large text style if the message contains a single emoji and nothing else, so maybe we should do the same.

But then how will I get my large chefs kiss?
😗🤌

@SpiritCroc
Copy link
Contributor

I'm not a huge fan of the emoji2 lib either. Docs say:

The initializer creates a background thread to load the emoji font, and font download can take up to 10 seconds before timing out. After the font is downloaded, it takes approximately 150 milliseconds on a background thread to initialize EmojiCompat.
After loading, EmojiCompat uses about 300 KB of RAM to hold the emoji metadata.

Sounds annoying to add just for emoji detection. But in the end for my needs it was still the best solution, regarding detecting all emojis properly, and keeping maintenance effort low. Also I thought I saw some dependency pulling in emoji2 anyway back when I implemented this, but maybe my memory fails me with that since I don't find it anymore.

@frebib
Copy link
Contributor Author

frebib commented Aug 13, 2024

Based on that excerpt from the docs, it makes using this library a non-starter. It's dead weight. I'll go back to the drawing board yet again. Maybe I'll try to understand the unicode spec a bit more and write some lightweight logic to detect emojis.

@frebib frebib marked this pull request as draft August 13, 2024 10:04
@bmarty
Copy link
Member

bmarty commented Aug 13, 2024

We probably need a product decision about rendering messages with only Emojis with a bigger font. This very old feature has been introduced before the Matrix protocol supports reaction. So now that we have reaction, maybe we do not need this feature.

Also, ideally, the "only emoji" detection could be done by the Rust SDK, so that the algorithm is shared with iOS.

(note: tracked by #1438)

CC @mxandreas

@frebib
Copy link
Contributor Author

frebib commented Aug 13, 2024

I took some more time trying to implement this with a regex, but after reading into the unicode spec, I realised that it would be a pretty monumental feat to implement something that correctly catches all various of the cases and variants that can apply to specific emojis (of which there are a LOT). I guess it sorta explains why many of these libraries ship huge tables- it's so they can account for all of the variants. For example here is just one set of variants for one of the many ranges of emoji graphemes: https://en.wikipedia.org/wiki/Miscellaneous_Symbols_and_Pictographs#Table_of_emojis_with_modifiers

I settled on implementing with https://github.com/sigpwned/emoji4j as it seems relatively well maintained and simple/lightweight. The implementation has emoji-specific grapheme selection which is what we want. There are a few edge cases that it doesn't account for (but I think they're all from unicode 15.1.. it only supports up to 15), otherwise it seems to work great.

@frebib frebib marked this pull request as ready for review August 13, 2024 22:06
@frebib frebib force-pushed the feat/big-emoji branch 3 times, most recently from a10f8a0 to 6a307a2 Compare August 13, 2024 22:50
@mxandreas
Copy link

We probably need a product decision about rendering messages with only Emojis with a bigger font. This very old feature has been introduced before the Matrix protocol supports reaction. So now that we have reaction, maybe we do not need this feature.

I can see @amshakal 's comment on the original ticket that this is nice to have (but I understand it is still something to have), she is probably better to comment what were the arguments for it.

@frebib
Copy link
Contributor Author

frebib commented Aug 20, 2024

Updates since the last push:

From my testing now, this seems functionally correct with every emoji that I've tested and parity with EW/EA/EXI

@frebib
Copy link
Contributor Author

frebib commented Aug 20, 2024

CI OOMed (again). It's pretty flaky.
Can someone re-run it please?

@jmartinesp
Copy link
Member

CI OOMed (again). It's pretty flaky. Can someone re-run it please?

Done, but I think it also needs a screenshot update.

@frebib frebib force-pushed the feat/big-emoji branch 3 times, most recently from 6f081eb to 571359c Compare August 21, 2024 18:31
@jmartinesp
Copy link
Member

jmartinesp commented Aug 22, 2024

Ok, the screenshot test failure looks like this:

image

I think it's because in the mocked data to display we forgot to enforce a locale. If you go here and add Locale.US to the time formatter it'll probably fix the issue we're seeing after recording the screenshots again.

@frebib
Copy link
Contributor Author

frebib commented Aug 22, 2024

Perfect, thanks for the help! I'd have never found that 😆
I pushed an extra commit to fix those screenshots 🤞🏻

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.74%. Comparing base (1b7c0db) to head (23001d6).
Report is 132 commits behind head on develop.

Files with missing lines Patch % Lines
.../timeline/components/event/TimelineItemTextView.kt 83.33% 0 Missing and 1 partial ⚠️
...ment/android/features/messages/impl/utils/Emoji.kt 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3295      +/-   ##
===========================================
- Coverage    82.75%   82.74%   -0.01%     
===========================================
  Files         1680     1681       +1     
  Lines        39382    39399      +17     
  Branches      4777     4784       +7     
===========================================
+ Hits         32591    32602      +11     
- Misses        5117     5118       +1     
- Partials      1674     1679       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amshakal
Copy link

We probably need a product decision about rendering messages with only Emojis with a bigger font. This very old feature has been introduced before the Matrix protocol supports reaction. So now that we have reaction, maybe we do not need this feature.

I can see @amshakal 's comment on the original ticket that this is nice to have (but I understand it is still something to have), she is probably better to comment what were the arguments for it.

I recommend we implement large-sized emojis in our messaging app because this aligns with user expectations based on the behavior of standard messaging apps. Most apps render a single or a few emojis larger because they have the space, and this small detail can enhance the user experience by adding a sense of delight and improving visual accessibility.

While this isn’t a core functional feature, it’s a subtle enhancement that users have come to appreciate. As a baseline, we could start by making a single emoji larger, which is the minimum implementation seen across popular messaging platforms. For example, WhatsApp uses this for up to three emojis, Signal for up to four, Slack doesn’t limit emoji size, and Telegram reduces emoji size incrementally after six.

Ultimately, I support adding this feature, even if it's just for single emojis. However, I’m also open to allowing multiple large emojis, similar to other platforms, if we want to provide a more flexible user experience.

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM then!

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Some remarks, then I guess the PR can be merged, thanks!

Different system locales can generate different screenshots, causing CI
to be unhappy. Hardcoding a locale ensures the same date format is
always used.

Signed-off-by: Joe Groocock <[email protected]>
Adapted from SpiritCroc's SchildNext implementation from
SchildiChat/schildichat-android-next@7eba87f

Fixes: element-hq#1438
Signed-off-by: Tobias Büttner <[email protected]>
Signed-off-by: Joe Groocock <[email protected]>
@frebib
Copy link
Contributor Author

frebib commented Aug 23, 2024

One final remark- it seems that maybe this isn't a unanimously desired feature. Perhaps additionally to this, we could add a toggle for it, like Element Web has?

@bmarty
Copy link
Member

bmarty commented Aug 23, 2024

One final remark- it seems that maybe this isn't a unanimously desired feature. Perhaps additionally to this, we could add a toggle for it, like Element Web has?

We try to limit the number of settings EX has, so I guess it's fine if this behavior cannot be disabled. Thanks!

@bmarty bmarty merged commit e8d1598 into element-hq:develop Sep 4, 2024
18 checks passed
@frebib frebib deleted the feat/big-emoji branch September 4, 2024 16:40
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.

Implement large emoji
6 participants