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

refactor: update Android bridge #171

Merged
merged 9 commits into from
Feb 28, 2024
Merged

Conversation

Mercy811
Copy link
Contributor

@Mercy811 Mercy811 commented Feb 23, 2024

  • Update Android bridge file at android/src/main/kotlin/com/amplitude/amplitude_flutter/AmplitudeFlutterPlugin.kt
  • Add Android native side tests at android/src/test/kotlin/com/amplitude/amplitude_flutter/AmplitudeFlutterPluginTest.kt
    • Simulate method call from Flutter
    • Verify corresponding Android APIs are called

To run Android native unit tests cd example/android && ./gradlew testDebugUnitTest
Unfortunately I cannot make Android unit test CI running. Will just keep the tests to local for now.
image

@Mercy811 Mercy811 force-pushed the AMP-89312-update-android-bridge branch from a8962f9 to dbc2f0b Compare February 23, 2024 21:24
@Mercy811 Mercy811 marked this pull request as ready for review February 23, 2024 21:27
@@ -2,14 +2,14 @@ group 'com.amplitude.amplitude_flutter'
version '1.0-SNAPSHOT'

buildscript {
ext.kotlin_version = '1.7.10'
ext.kotlin_version = '1.5.20'
Copy link

Choose a reason for hiding this comment

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

Are we going to an earlier version of kotlin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I lowered it for some compatibility issues. I don't see any harms to customers to lower the version. Do you any concern? I'm open to discussion and can try to see if I can revert it if it's necessary.

@@ -147,7 +147,7 @@ class Amplitude extends _Amplitude {
event.mergeEventOptions(options);
}

return await _channel.invokeMethod("track", jsonEncode(event.toMap()));
Copy link

Choose a reason for hiding this comment

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

Why does this change? Was the old version calling the wrong method or did the interface change in the new Android SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to fix the error of my last commit. Note this PR is compared with branch 4.x not main. The interface setGroup() remains unchanged.

android/build.gradle Outdated Show resolved Hide resolved
android/build.gradle Outdated Show resolved Hide resolved
android/build.gradle Outdated Show resolved Hide resolved
example/android/build.gradle Outdated Show resolved Hide resolved
@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-6.7.1-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5-bin.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.2.2-bin.zip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 1a3d02c.

apply plugin: 'com.android.application'
apply plugin: 'kotlin-android'
apply from: "$flutterRoot/packages/flutter_tools/gradle/flutter.gradle"

android {
compileSdkVersion 28
compileSdkVersion 33
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this 34 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 1a3d02c.

@Mercy811 Mercy811 merged commit 6351902 into v4.x Feb 28, 2024
2 checks passed
@Mercy811 Mercy811 deleted the AMP-89312-update-android-bridge branch February 28, 2024 17:57
Copy link

🎉 This PR is included in version 4.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants