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

feat: add android context plugin #14

Merged
merged 4 commits into from
Mar 22, 2022
Merged

Conversation

qingzhuozhen
Copy link
Contributor

@qingzhuozhen qingzhuozhen commented Mar 20, 2022

Summary

Update Android Context Plugin

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

companion object {
const val PLATFORM = "Android"
const val SDK_LIBRARY = "amplitude-android-kotlin"
const val SDK_VERSION = "0.0.1"

Choose a reason for hiding this comment

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

is this going to be hard-coded or can we dynamically read from project metadata?

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 temporary now, going to be updated once get to the release setup.

}

override fun onActivityResumed(p0: Activity) {
(amplitude as com.amplitude.android.Amplitude).onEnterForeground(getCurrentTimeMillis())

Choose a reason for hiding this comment

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

did you need as com.amplitude.android.Amplitude because it's not defined in constructor?

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 the tricky part when I work more on android. Since we are extending the com.amplitude.core.Amplitude, any other custom field inside android, would need to cast first to com.amplitude.android.Amplitude. Same for configuration... Will rethink about this and discuss if we want to adjust the inheritance way, that could be a separate PR if decided.
cc @justin-fiedler I know we discussed this before. This is one thing I found not so convenient when extend more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this is tricky in Kotlin/Java. The types must match exactly in the method signatures for extension/inheritance checks to pass.

I tired a few things in the past but none of them worked great.

  1. interface<A extends Amplitude> Plugin this is annoying as you will always need to set a type to create an instance.
  2. Convenience getter fun amplitudeAndroid(): com.amplitude.android.Amplitude

Maybe we could make a AndroidPlugin base class to help more with the typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the plugin is tricky, and I also have configuration cast inside Amplitude class. If we do AndroidPlugin, should we also have event plugin and destination plugin type also have an android version?... I wonder if we adjust this to an extension way can solve this.
cc @bgiori if any thoughts on this.

@@ -5,15 +5,15 @@ import com.amplitude.core.platform.ObservePlugin
class State {
var userId: String? = null
set(value: String?) {
userId = value
field = value
Copy link
Contributor

Choose a reason for hiding this comment

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

It's interesting how Kotlin defines the setter.

val event = BaseEvent()
event.eventType = eventType
event.eventProperties = eventProperties
process(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

where do we validate the eventProperties? in the process function?

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 would be added later in our destination plugin. Planning to validate after we run enrichment plugins on event

Copy link
Contributor

@bohan-amplitude bohan-amplitude left a comment

Choose a reason for hiding this comment

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

LGTM

@qingzhuozhen qingzhuozhen merged commit 25b6da3 into main Mar 22, 2022
@qingzhuozhen qingzhuozhen deleted the android-context-plugin branch March 31, 2022 22:58
github-actions bot pushed a commit that referenced this pull request Jun 28, 2022
# 1.0.0 (2022-06-28)

### Bug Fixes

* fix android aar issue ([#32](#32)) ([69f5fe7](69f5fe7))
* fix group call ([#42](#42)) ([f84d143](f84d143))

### Features

* add amplitude destination plugin draft ([#3](#3)) ([7ee9408](7ee9408))
* add android context plugin ([#14](#14)) ([25b6da3](25b6da3))
* add basic test action and issue templates ([#9](#9)) ([e72c608](e72c608))
* add button to kotlin example to send custom event ([#41](#41)) ([5c1e573](5c1e573))
* add configuration java support ([#35](#35)) ([a4e0801](a4e0801))
* add error handling and retry ([#13](#13)) ([689b114](689b114))
* add eu and batch support ([#17](#17)) ([6402b2a](6402b2a))
* add event bridge module ([#12](#12)) ([49229ac](49229ac))
* add event id support ([#25](#25)) ([94aaa6b](94aaa6b))
* add github action about docs and release ([#19](#19)) ([3aa868e](3aa868e))
* add identify support ([#18](#18)) ([2f86adc](2f86adc))
* add identity module and file storage ([#8](#8)) ([8b7bada](8b7bada))
* add Java android example ([#24](#24)) ([2670a2b](2670a2b))
* add Kotlin android sample ([#23](#23)) ([3b948c8](3b948c8))
* add parner_id ([#16](#16)) ([5e46cb9](5e46cb9))
* add plan in configuration and export ([#34](#34)) ([d84a84e](d84a84e))
* add plan versionId support ([#22](#22)) ([ae84619](ae84619))
* add revenue helper class ([#6](#6)) ([341fcf1](341fcf1))
* add unit tests for common jvm module ([#29](#29)) ([dcc0e9d](dcc0e9d))
* add unit tests in core module ([#30](#30)) ([4283148](4283148))
* add unit tests in event bridge module ([#28](#28)) ([8d63c28](8d63c28))
* add unit tests in id module ([#27](#27)) ([dc78bc6](dc78bc6))
* android local sync update ([#20](#20)) ([e25ecd4](e25ecd4))
* enable to pass the event options for all track call ([#36](#36)) ([8dd1ab6](8dd1ab6))
* expose more api for support ([#33](#33)) ([312fd85](312fd85))
* make configuration open ([#38](#38)) ([d0dfb15](d0dfb15))
* remove json object from public interface ([#26](#26)) ([0840b5d](0840b5d))
* set the client userId and deviceId in identify call ([#37](#37)) ([2c8b3f1](2c8b3f1))
* set up android module bare bone ([#7](#7)) ([a8a8dc9](a8a8dc9))
* Set up jvm example ([#4](#4)) ([ae825d6](ae825d6))
* setup basic barebone ([#2](#2)) ([8e4b635](8e4b635))
* setup basic doc and workflow ([#1](#1)) ([31bd223](31bd223))
* setup common modules ([#11](#11)) ([4a4ff66](4a4ff66))
* update android storage ([#15](#15)) ([56c1d6a](56c1d6a))
* update library name ([#21](#21)) ([309976d](309976d))
@github-actions
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants