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

Kotlin bindings, tests, swift/kotlin CI validation and release script #2

Merged
merged 12 commits into from
Aug 10, 2023

Conversation

langleyd
Copy link
Collaborator

@langleyd langleyd commented Aug 9, 2023

  • Kotlin bindings
  • Kotlin tests
  • Kotlin/Swift CI checks
  • Release script

@langleyd langleyd marked this pull request as ready for review August 10, 2023 10:51
@langleyd langleyd requested a review from t3chguy August 10, 2023 10:51
package.json Outdated Show resolved Hide resolved
platforms/android/library/src/main/assets/emojibase.json Outdated Show resolved Hide resolved
scripts/generateJson.sh Show resolved Hide resolved
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

My review still stands, I don't see any changes since my last review.

@langleyd
Copy link
Collaborator Author

I just added Jorge as a reviewer, not sure why, it requested again from you.

Copy link
Collaborator

@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.

The Android bindings LGTM.

My only issue with this implementation is that it'll probably be less performant than what we're using at the moment, since we'll need to parse the JSON instead of just having the emojis available as part of the code, but at least they'll be consistent in all platforms.

@jmartinesp
Copy link
Collaborator

Could we measure how long it takes for load the data from the JSON file in the tests?

@t3chguy
Copy link
Member

t3chguy commented Aug 10, 2023

@jmartinesp could replace the JSON with some form of codegen, like we do for https://github.com/matrix-org/matrix-analytics-events

@jmartinesp
Copy link
Collaborator

@jmartinesp could replace the JSON with some form of codegen, like we do for matrix-org/matrix-analytics-events

That would be great since this parsing is done when the app boots and could potentially affect startup time, although we're using an initializer which should make it happen on background. However, I'm not sure how complex that could be and I don't want to block this release for a long time.

@langleyd
Copy link
Collaborator Author

@jmartinesp That could be a lot of code/symbols generated in this case? Not to say we shouldn't just something to be weighed up?

I suppose lets see how long it takes and I'll report back. On the iOS side it was loading json in the last version so was a like for like.

@t3chguy
Copy link
Member

t3chguy commented Aug 10, 2023

Could we measure how long it takes for load the data from the JSON file in the tests?

Not reasonably, the free Github Actions runners are massively variable in their performance, we saw an insane variance in attempted performance testing for Element Web in GHA.

@jmartinesp
Copy link
Collaborator

@jmartinesp That could be a lot of code/symbols generated in this case? Not to say we shouldn't just something to be weighed up?

I think that's why they're using a chunk strategy + using static variables, so they're only loaded when needed: https://github.com/vanniktech/Emoji/blob/master/emoji-twitter/src/commonMain/kotlin/com/vanniktech/emoji/twitter/category/FlagsCategory.kt#L32

@langleyd
Copy link
Collaborator Author

On my iPhone 12 it takes between 0.09 and 0.13 seconds to load.

On EIX the current implementation, the emoji data is tied to the user session, so it stays in memory vs being loaded on demand when the user loads the picker.

@jmartinesp
Copy link
Collaborator

jmartinesp commented Aug 10, 2023

I run the instrumentation tests too with some measurements on a 5 year old phone and it took ~140ms, which seems reasonable.

In fact, I tested it properly with several iterations and it takes ~35ms, which is a lot better than expected.

- Remove emojibase.json from source and generate at buildtime.
- Fix version so we and conform to typical semver rules
@langleyd langleyd requested a review from t3chguy August 10, 2023 16:58
@langleyd langleyd merged commit b6e934e into main Aug 10, 2023
3 checks passed
@langleyd langleyd deleted the langleyd/kotlin_bindings_and_release_scripts branch August 10, 2023 17:21
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